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
    • Rank:
      30022

      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).

      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 Škoda added a comment -

        thanks!

        I have fixed the version numbers here

        Show
        Petr Škoda 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: