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

        1. screenshot-1.jpg
          105 kB
        2. screenshot-2.jpg
          84 kB

          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