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 2.4 Branch:
      MDL-37397-m24
    • Pull Master Branch:
    • Rank:
      47020

      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')); }

        Issue Links

          Activity

          Hide
          Martin Holden added a comment -

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

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

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

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

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

          Show
          Martin Holden added a comment - If we ignore first error and then "launch" the content in normal mode
          Hide
          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
          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 added a comment -

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

          Show
          Martin Holden added a comment - See MDL-37393 for a sample AICC package you can use
          Hide
          Andrew Nicols added a comment -

          Also:

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

          should suffice.

          Show
          Andrew Nicols added a comment - Also: if (thisselect) { thisselect.setData('nothing', this .get('nothing')) .setData('startindex', thisselect.get('selectedIndex')); } should suffice.
          Hide
          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 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
          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
          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 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 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
          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
          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 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 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 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 Wiese added a comment - This patch looks good - thanks! I'll grab that scorm and try and add some testing instructions for it.
          Hide
          Heiko Schach added a comment -

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

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

          Thanks Damyon - much appreciated.

          Show
          Andrew Nicols added a comment - Thanks Damyon - much appreciated.
          Hide
          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 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
          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
          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 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 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 Wiese added a comment -

          Example scorm file for 2.3 testing.

          Show
          Damyon Wiese added a comment - Example scorm file for 2.3 testing.
          Hide
          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 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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now.

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

          It all seems to be working as described. Passing.

          Show
          Andrew Davis added a comment - It all seems to be working as described. Passing.
          Hide
          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
          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: