Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37397

formautosubmit.js does not check whether select is valid before acting on it

    Details

    • Testing Instructions:
      Hide
      • Open a course
      • Open JS console
      • Turn editing on
        • Confirm no errors or warnings were shown
      • Turn off the activity chooser
      • Try using one of the 'Add a new...' dropdowns to make a selection
        • Confirm no errors or warnings were shown
        • Confirm that the page redirected

      For Moodle 2.3 only:

      • Create a scorm activity using the attached scorm file.
      • Make sure the display course structure in player setting is set to "To the side".
      • Enter the scorm
        • Confirm no errors or warnings were shown
      Show
      Open a course Open JS console Turn editing on Confirm no errors or warnings were shown Turn off the activity chooser Try using one of the 'Add a new...' dropdowns to make a selection Confirm no errors or warnings were shown Confirm that the page redirected For Moodle 2.3 only: Create a scorm activity using the attached scorm file. Make sure the display course structure in player setting is set to "To the side". Enter the scorm Confirm no errors or warnings were shown
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:

      Description

      Launches previously working in Moodle 2.3.2 fail in Moodle 2.3.3+ (Build: 20121230)

      Issue appears to be in the TOC generation logic, but actually manifests in the use of the new moodle-core-formautosubmit javascript in http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/yui/formautosubmit/formautosubmit.js;h=01d666879c801dcddc4c9377eb97189b1679134d;hb=MOODLE_23_STABLE#l43

      The TOC code creates a single select with the outputrender code that registers the moodle-core-formautosubmit to add itself to the "dropdown".
      http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/outputrenderers.php;h=bc89a754f8d7464f37efba460a8dd021ad868333;hb=MOODLE_23_STABLE#l1359

      However the dropdown is not always displayed and so the javascript code will not detect it and blocks the operation continuing, a javascript error is generated.
      http://git.moodle.org/gw?p=moodle.git;a=blob;f=mod/scorm/player.php;h=570543d06c9e35be02f89dfea02763899dc0cb0c;hb=MOODLE_23_STABLE#l214

      NOTE: $result->tocmenu is the single select

      A quick fix is editing the moodle-core-formautosubmit and having it check that the "select" it is attempting to bind to exists.

      http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/yui/formautosubmit/formautosubmit.js;h=01d666879c801dcddc4c9377eb97189b1679134d;hb=MOODLE_23_STABLE#l43

      //martin_holden@skillsoft.com 20130106 - Add a check that the select exists before we try anything
      //as issues with SCORM/AICC launches
      if (thisselect != null)

      { thisselect.setData('nothing', this.get('nothing')); thisselect.setData('startindex', thisselect.get('selectedIndex')); }

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              martin_holden@skillsoft.com Martin Holden added a comment -

              SHows JavaScript error when loading "view.php" for SCORM asset

              Show
              martin_holden@skillsoft.com Martin Holden added a comment - SHows JavaScript error when loading "view.php" for SCORM asset
              Hide
              dobedobedoh Andrew Nicols added a comment -

              We never actually bind to the select, but we do call setData on it.

              Show
              dobedobedoh Andrew Nicols added a comment - We never actually bind to the select, but we do call setData on it.
              Hide
              martin_holden@skillsoft.com Martin Holden added a comment -

              If we ignore first error and then "launch" the content in normal mode

              Show
              martin_holden@skillsoft.com Martin Holden added a comment - If we ignore first error and then "launch" the content in normal mode
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Have you got a SCORM package I can test with?
              I have an idea which would help with this.

              Show
              dobedobedoh Andrew Nicols added a comment - Have you got a SCORM package I can test with? I have an idea which would help with this.
              Hide
              martin_holden@skillsoft.com Martin Holden added a comment -

              See MDL-37393 for a sample AICC package you can use

              Show
              martin_holden@skillsoft.com Martin Holden added a comment - See MDL-37393 for a sample AICC package you can use
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Also:

              if (thisselect) {
                thisselect.setData('nothing', this.get('nothing'))
                          .setData('startindex', thisselect.get('selectedIndex'));
              }

              should suffice.

              Show
              dobedobedoh Andrew Nicols added a comment - Also: if (thisselect) { thisselect.setData('nothing', this.get('nothing')) .setData('startindex', thisselect.get('selectedIndex')); } should suffice.
              Hide
              martin_holden@skillsoft.com Martin Holden added a comment - - edited

              Andy so I guess this will hopefully make it into next 2.3 build and 2.4 - bizarrely JS code and display code looks to be same in 2.4 for the SCORM display yet this error does not occur - just the others I logged for Dan

              Show
              martin_holden@skillsoft.com Martin Holden added a comment - - edited Andy so I guess this will hopefully make it into next 2.3 build and 2.4 - bizarrely JS code and display code looks to be same in 2.4 for the SCORM display yet this error does not occur - just the others I logged for Dan
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Even after applying my patch, I'm still hitting issues with AICC content on master - issues relating to the SCORM module.js code rather than the formautosubmit.js

              Show
              dobedobedoh Andrew Nicols added a comment - Even after applying my patch, I'm still hitting issues with AICC content on master - issues relating to the SCORM module.js code rather than the formautosubmit.js
              Hide
              matteo Matteo Scaramuccia added a comment -

              Hi Andrew,
              I'm coming from MDL-37424: do you still require a working package for this issue? Here is a sample taken from the Tracker: MDL-33053. Let me know.

              HTH,
              Matteo

              Show
              matteo Matteo Scaramuccia added a comment - Hi Andrew, I'm coming from MDL-37424 : do you still require a working package for this issue? Here is a sample taken from the Tracker: MDL-33053 . Let me know. HTH, Matteo
              Hide
              danmarsden Dan Marsden added a comment -

              yeah - there's a regression in scorm_get_toc in master/2.4 - esp for AICC packages - probably best to test this particular patch with a standard SCORM package until someone (me?) traces the AICC issues...

              Show
              danmarsden Dan Marsden added a comment - yeah - there's a regression in scorm_get_toc in master/2.4 - esp for AICC packages - probably best to test this particular patch with a standard SCORM package until someone (me?) traces the AICC issues...
              Hide
              martin_holden@skillsoft.com Martin Holden added a comment -

              The AICC package supplied in MDL-37393 (the regression Dan mentions) will allow this to be tested in 2.3.3+ - unless the 2.4 TOC code was back ported

              Show
              martin_holden@skillsoft.com Martin Holden added a comment - The AICC package supplied in MDL-37393 (the regression Dan mentions) will allow this to be tested in 2.3.3+ - unless the 2.4 TOC code was back ported
              Hide
              damyon Damyon Wiese added a comment -

              This patch looks good - thanks!

              I'll grab that scorm and try and add some testing instructions for it.

              Show
              damyon Damyon Wiese added a comment - This patch looks good - thanks! I'll grab that scorm and try and add some testing instructions for it.
              Hide
              schach Heiko Schach added a comment -

              There is a good description of this issue here:
              https://moodle.org/mod/forum/discuss.php?d=219341

              Show
              schach Heiko Schach added a comment - There is a good description of this issue here: https://moodle.org/mod/forum/discuss.php?d=219341
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Damyon - much appreciated.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Damyon - much appreciated.
              Hide
              martin_holden@skillsoft.com Martin Holden added a comment -

              I have tested the scenario in which I discovered this using AICC content and the issue for me is resolved. Moodle Version 2.3.3+ (Build: 20121230), with this patch file. Thanks Andrew

              Show
              martin_holden@skillsoft.com Martin Holden added a comment - I have tested the scenario in which I discovered this using AICC content and the issue for me is resolved. Moodle Version 2.3.3+ (Build: 20121230), with this patch file. Thanks Andrew
              Hide
              schach Heiko Schach added a comment -

              I can confirm this works for Moodle 2.3.
              Applied the patch on 2.3.3+ (Build: 20121230) and SCORM packages are working again.
              Thank you very much.

              Show
              schach Heiko Schach added a comment - I can confirm this works for Moodle 2.3. Applied the patch on 2.3.3+ (Build: 20121230) and SCORM packages are working again. Thank you very much.
              Hide
              damyon Damyon Wiese added a comment - - edited

              Hi Andrew,

              I had a look at the scorm module to see why it was triggering this bug - and the reason is that it renders a single_select (for the toc menu) but does not always output it to the page. I think that the formautosubmit js should be robust enough to handle this - but I don't think that we need the warning printed. If you still want the warning maybe we could change the text to more accurately describe the issue.

              Suggestion: "Warning: A single_select element was rendered but the output was not printed to the page."

              Regards, Damyon

              Show
              damyon Damyon Wiese added a comment - - edited Hi Andrew, I had a look at the scorm module to see why it was triggering this bug - and the reason is that it renders a single_select (for the toc menu) but does not always output it to the page. I think that the formautosubmit js should be robust enough to handle this - but I don't think that we need the warning printed. If you still want the warning maybe we could change the text to more accurately describe the issue. Suggestion: "Warning: A single_select element was rendered but the output was not printed to the page." Regards, Damyon
              Hide
              damyon Damyon Wiese added a comment -

              Example scorm file for 2.3 testing.

              Show
              damyon Damyon Wiese added a comment - Example scorm file for 2.3 testing.
              Hide
              damyon Damyon Wiese added a comment -

              Note: mod_scorm in 24 and 25 does not trigger this bug as it only renders the single_select when it is actually used in the page. So the scorm test only needs to be done on 2.3.

              Show
              damyon Damyon Wiese added a comment - Note: mod_scorm in 24 and 25 does not trigger this bug as it only renders the single_select when it is actually used in the page. So the scorm test only needs to be done on 2.3.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Damyon,

              I've amended the text as per your suggestion - I agree that there may be cases where this can happen legitimately.

              I'm planning to make some changes to master to reduce the potential for this further still.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Damyon, I've amended the text as per your suggestion - I agree that there may be cases where this can happen legitimately. I'm planning to make some changes to master to reduce the potential for this further still.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Andrew, this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
              Hide
              andyjdavis Andrew Davis added a comment -

              It all seems to be working as described. Passing.

              Show
              andyjdavis Andrew Davis added a comment - It all seems to be working as described. Passing.
              Hide
              poltawski Dan Poltawski added a comment -

              Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

              Show
              poltawski Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  12 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Mar/13