Moodle
  1. Moodle
  2. MDL-11892

JavaScript error on SCORM tree list

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.3, 1.9.1, 2.0
    • Fix Version/s: 1.8.7, 1.9.3, 2.0
    • Component/s: SCORM
    • Labels:
      None
    • Environment:
      Firefox / Internet Explorer and probably other browsers.
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE

      Description

      Clicking the +/- icon next to items in the SCORM package list does not expand/contract the items and throws an error.

      This is originates from the "scorm_get_toc" function, within the files scorm_13lib.php and scorm_12lib.php.

      scorm_13lib.php line 117 (creating a link using javascript:expandCollide)
      The second parameter needs to be passed as a string, but isn't surrounded by single quotes which makes the browser think it's a undeclared variable.
      $result->toc .= '<a href="javascript:expandCollide(img'.$sublist.',\'s'.$sublist.'\','.$nextsco->id.');">'.

      Same as above but for scorm_12lib.php line 266.

      Then in both files, the JavaScript function expandCollide is incorrect and badly written (switches between trying to find a suitable method for getting an element by ID, but then just uses document.getElementById anyway for changing the image).

      I updated both to the following (since I didn't know what level of browser Moodle wants to support when it comes to JavaScript).

      function expandCollide(which,list,item) {
      var el = document.ids ? document.ids[list] : document.getElementById ? document.getElementById(list) : document.all[list];
      which = which.substring(0,(which.length));
      var el2 = document.ids ? document.ids[which] : document.getElementById ? document.getElementById(which) : document.all[which];
      if (el.style.display != "none")

      { el2.src = "'.$scormpixdir.'/plus.gif"; el.style.display=\'none\'; new cookie("hide:SCORMitem" + item, 1, 356, "/").set(); }

      else

      { el2.src = "'.$scormpixdir.'/minus.gif"; el.style.display=\'block\'; new cookie("hide:SCORMitem" + item, 1, -1, "/").set(); }

      }

      I've attached two updated files for inspection (updated files from 1.8.3+ 2007-10-24).

        Gliffy Diagrams

        1. MDL-11892-head.patch
          6 kB
          Piers Harding
        2. MDL-11892-moodle16.patch
          6 kB
          Piers Harding
        3. MDL-11892-moodle17.patch
          12 kB
          Piers Harding
        4. MDL-11892-moodle18.patch
          6 kB
          Piers Harding
        5. MDL-11892-moodle19.patch
          6 kB
          Piers Harding
        6. scorm_12lib.php
          17 kB
          David Boyer
        7. scorm_13lib.php
          11 kB
          David Boyer

          Activity

          Hide
          Marco Loche added a comment -

          Also parmeter 1 ( img ) in expandCollide must be surrounded by single quotes.

          $result->toc .= '<a href="javascript:expandCollide(\'img'.$sublist.'\',\'s'.$sublist.'\','.$nextsco->id.');">'.

          tested on FF2.0, IE6 and IE7

          Show
          Marco Loche added a comment - Also parmeter 1 ( img ) in expandCollide must be surrounded by single quotes. $result->toc .= '<a href="javascript:expandCollide(\'img'.$sublist.'\',\'s'.$sublist.'\','.$nextsco->id.');">'. tested on FF2.0, IE6 and IE7
          Hide
          Dan Marsden added a comment -

          assigning to me - will test fix and look at patching it.

          Dan

          Show
          Dan Marsden added a comment - assigning to me - will test fix and look at patching it. Dan
          Hide
          Mike Churchward added a comment -

          I don't think the above is correct, at least not in 'scorm_13lib.php'.

          The item coming in the 'list' parameter needs to be quoted in the link.

          So, around line 122, the code needs to read:
          $result->toc .= "\t\t".'<li><a href="javascript:expandCollide(\'img'.$sublist.'\',\'s'.$sublist.'\','.$nextsco->id.');">'.
          '<img id="img'.$sublist.'" src="'.$scormpixdir.'/'.$icon.'.gif" alt="'.$strexpand.'" title="'.$strexpand.'"/></a>';

          Then the javascript needs to fixed such as:
          <script type="text/javascript">
          //<![CDATA[
          function expandCollide(which,list,item) {
          var nn=document.ids?true:false
          var w3c=document.getElementById?true:false
          var beg=nn?"document.ids.":w3c?"document.getElementById(\'":"document.all.";
          var mid=w3c?"\').style":".style";

          which = which.substring(0,(which.length));
          if (eval(beg+list+mid+".display") != "none")

          { document.getElementById(which).src = "'.$scormpixdir.'/plus.gif"; eval(beg+list+mid+".display=\'none\';"); new cookie("hide:SCORMitem" + item, 1, 356, "/").set(); }

          else

          { document.getElementById(which).src = "'.$scormpixdir.'/minus.gif"; eval(beg+list+mid+".display=\'block\';"); new cookie("hide:SCORMitem" + item, 1, -1, "/").set(); }

          }
          //]]>
          </script>

          Show
          Mike Churchward added a comment - I don't think the above is correct, at least not in 'scorm_13lib.php'. The item coming in the 'list' parameter needs to be quoted in the link. So, around line 122, the code needs to read: $result->toc .= "\t\t".'<li><a href="javascript:expandCollide(\'img'.$sublist.'\',\'s'.$sublist.'\','.$nextsco->id.');">'. '<img id="img'.$sublist.'" src="'.$scormpixdir.'/'.$icon.'.gif" alt="'.$strexpand.'" title="'.$strexpand.'"/></a>'; Then the javascript needs to fixed such as: <script type="text/javascript"> //<![CDATA[ function expandCollide(which,list,item) { var nn=document.ids?true:false var w3c=document.getElementById?true:false var beg=nn?"document.ids.":w3c?"document.getElementById(\'":"document.all."; var mid=w3c?"\').style":".style"; which = which.substring(0,(which.length)); if (eval(beg+list+mid+".display") != "none") { document.getElementById(which).src = "'.$scormpixdir.'/plus.gif"; eval(beg+list+mid+".display=\'none\';"); new cookie("hide:SCORMitem" + item, 1, 356, "/").set(); } else { document.getElementById(which).src = "'.$scormpixdir.'/minus.gif"; eval(beg+list+mid+".display=\'block\';"); new cookie("hide:SCORMitem" + item, 1, -1, "/").set(); } } //]]> </script>
          Hide
          Dan Marsden added a comment -

          Hi Mike,

          feel free to take this bug! - I've got a few Scorm related ones assigned to me that I planned to fix while working @ Lincoln.... I don't have time at the moment to look at these right now....

          Dan

          Show
          Dan Marsden added a comment - Hi Mike, feel free to take this bug! - I've got a few Scorm related ones assigned to me that I planned to fix while working @ Lincoln.... I don't have time at the moment to look at these right now.... Dan
          Hide
          Mike Churchward added a comment -

          I have tested the changes I posted. They seem to work on the browsers I have tested with (FF and IE). I am committing those changes.

          Show
          Mike Churchward added a comment - I have tested the changes I posted. They seem to work on the browsers I have tested with (FF and IE). I am committing those changes.
          Hide
          Dan Marsden added a comment -

          great! - there are several other scorm bugs that you're welcome to, if you feel the inclination!

          Dan

          Show
          Dan Marsden added a comment - great! - there are several other scorm bugs that you're welcome to, if you feel the inclination! Dan
          Hide
          Petr Skoda added a comment -

          thanks!

          I have fixed the version numbers here

          Show
          Petr Skoda added a comment - thanks! I have fixed the version numbers here
          Hide
          Dan Marsden added a comment -

          Hi Mike - did this get fixed for scorm_12 as well?

          Dan

          Show
          Dan Marsden added a comment - Hi Mike - did this get fixed for scorm_12 as well? Dan
          Hide
          Dan Marsden added a comment -

          we need to fix this for scorm12 as well.

          Show
          Dan Marsden added a comment - we need to fix this for scorm12 as well.
          Hide
          Dan Marsden added a comment -

          Hi Piers - can you please check to see if we need to fix this in scorm12 as well?

          thanks,

          Dan

          Show
          Dan Marsden added a comment - Hi Piers - can you please check to see if we need to fix this in scorm12 as well? thanks, Dan
          Hide
          Mike Churchward added a comment -

          Hi Dan. When I did the fix to scorm_1, it had already been done to scorm_12 (I believe). Have you seen otherwise?

          Show
          Mike Churchward added a comment - Hi Dan. When I did the fix to scorm_1, it had already been done to scorm_12 (I believe). Have you seen otherwise?
          Hide
          Dan Marsden added a comment -

          Thanks Mike - hadn't looked, just wanted to make sure! - a lot of SCORM bugs have been patched intermittently - eg in 1.6 but not 1.7, in scorm12 but not scorm13 - just wanted to make sure we don't repeat bad history!

          happy to close again if you are certain it was already fixed in 1.2!

          thanks,

          Dan

          Show
          Dan Marsden added a comment - Thanks Mike - hadn't looked, just wanted to make sure! - a lot of SCORM bugs have been patched intermittently - eg in 1.6 but not 1.7, in scorm12 but not scorm13 - just wanted to make sure we don't repeat bad history! happy to close again if you are certain it was already fixed in 1.2! thanks, Dan
          Hide
          Piers Harding added a comment -

          Patch file for HEAD. patch is relative to ./mod/scorm so cd to this directory and run patch -p0 < MDL-11892-head.patch

          Show
          Piers Harding added a comment - Patch file for HEAD. patch is relative to ./mod/scorm so cd to this directory and run patch -p0 < MDL-11892 -head.patch
          Hide
          Piers Harding added a comment -

          Patch file for stable19. patch is relative to ./mod/scorm so cd to this directory and run patch -p0 < MDL-11892-moodle19.patch

          Show
          Piers Harding added a comment - Patch file for stable19. patch is relative to ./mod/scorm so cd to this directory and run patch -p0 < MDL-11892 -moodle19.patch
          Hide
          Piers Harding added a comment -

          Patch file for stable18. patch is relative to ./mod/scorm so cd to this directory and run patch -p0 < MDL-11892-moodle18.patch

          Show
          Piers Harding added a comment - Patch file for stable18. patch is relative to ./mod/scorm so cd to this directory and run patch -p0 < MDL-11892 -moodle18.patch
          Hide
          Piers Harding added a comment -

          Hi All - I've taken the combined fixes and propagated them through the relevant releases for both scorm 1.2 and 2004. As a result I've come up with patch files for HEAD, 1.9, and 1.8. I've tested these + 1.6 and 1.7 (both don't have the same bug, and are implemented differently) in FF and IE. As I don't have CVS access, could someone review, and apply the patches please.
          Cheers,
          Piers Harding.

          Show
          Piers Harding added a comment - Hi All - I've taken the combined fixes and propagated them through the relevant releases for both scorm 1.2 and 2004. As a result I've come up with patch files for HEAD, 1.9, and 1.8. I've tested these + 1.6 and 1.7 (both don't have the same bug, and are implemented differently) in FF and IE. As I don't have CVS access, could someone review, and apply the patches please. Cheers, Piers Harding.
          Hide
          Martin Dougiamas added a comment -

          Hi Piers,

          I see you've committed the patches to our stable branches already. Were these reviewed by someone first?

          Show
          Martin Dougiamas added a comment - Hi Piers, I see you've committed the patches to our stable branches already. Were these reviewed by someone first?
          Hide
          Piers Harding added a comment -

          Hi Martin -
          Sorry - I understood that it worked the other way around - that I should commit first and then it went it to peer review. Hopefully this isn't too much of a problem in this case, as it is mainly back porting something that has been trunk for a while - what do you suggest that I do.

          Cheers.

          Show
          Piers Harding added a comment - Hi Martin - Sorry - I understood that it worked the other way around - that I should commit first and then it went it to peer review. Hopefully this isn't too much of a problem in this case, as it is mainly back porting something that has been trunk for a while - what do you suggest that I do. Cheers.
          Hide
          Dan Marsden added a comment -

          that's my bad sorry! - Piers - Jonathon or myself should review any change before you commit to Stable.

          thanks!

          Dan

          Show
          Dan Marsden added a comment - that's my bad sorry! - Piers - Jonathon or myself should review any change before you commit to Stable. thanks! Dan
          Hide
          Piers Harding added a comment -

          patch files containing back port of fixes for this issue for 1.6, and 1.7 including fixing up the behaviour of the TOC tree, so that the 3rd level elements behave the same as in HEAD (currently don't expand/collapse). These updates may not be desirable at these releases, but seeing as I was working in the same space I've generated them anyway. I would be grateful if someone reviewed the changes.

          patches applied with patch -p0 in mod/scorm

          Thanks,
          Piers Harding.

          Show
          Piers Harding added a comment - patch files containing back port of fixes for this issue for 1.6, and 1.7 including fixing up the behaviour of the TOC tree, so that the 3rd level elements behave the same as in HEAD (currently don't expand/collapse). These updates may not be desirable at these releases, but seeing as I was working in the same space I've generated them anyway. I would be grateful if someone reviewed the changes. patches applied with patch -p0 in mod/scorm Thanks, Piers Harding.
          Hide
          Dan Marsden added a comment -

          This bug is now fixed for Scorm1.2 and Scorm1.3(2004) in 1.8Stable, 1.9Stable, and HEAD,

          Piers has only Patched HEAD with the AICC fix, as we don't currently have any AICC Packages that allow us to test this correctly - if anyone has any AICC packages that we can use to test, we can put the fix for AICC in 1.8Stable and 1.9Stable as well.

          thanks,

          Dan

          Show
          Dan Marsden added a comment - This bug is now fixed for Scorm1.2 and Scorm1.3(2004) in 1.8Stable, 1.9Stable, and HEAD, Piers has only Patched HEAD with the AICC fix, as we don't currently have any AICC Packages that allow us to test this correctly - if anyone has any AICC packages that we can use to test, we can put the fix for AICC in 1.8Stable and 1.9Stable as well. thanks, Dan
          Hide
          Piers Harding added a comment -

          1.8 and 1.9 have been patched with the AICC fixes.

          This should be all clear now for HEAD, 1.9, 1.8 across SCORM 1.2, 1.3 and AICC.

          Cheers.

          Show
          Piers Harding added a comment - 1.8 and 1.9 have been patched with the AICC fixes. This should be all clear now for HEAD, 1.9, 1.8 across SCORM 1.2, 1.3 and AICC. Cheers.
          Hide
          Dan Marsden added a comment -

          good work Piers!

          Dan

          Show
          Dan Marsden added a comment - good work Piers! Dan
          Hide
          Dan Marsden added a comment -

          looks good to me! - closing.

          Show
          Dan Marsden added a comment - looks good to me! - closing.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: