Moodle

Trailing comma in course/format/weeks/format.js breaks course JS in IE

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.3.1, 2.4
  • Fix Version/s: 2.3.3
  • Component/s: Course
  • Labels:
  • Testing Instructions:
    Hide

    This needs to be tested across browsers, especially IE8.

    1. Open a course which uses the Weekly course format.
    2. Open the error console in your browser.
      • If you are using IE8, press F12 and change the Browser mode to IE8 Compatibility View and click on the Script tab.
    3. Drag and drop some resources/activites.
    4. Drag and drop some sections.
    5. VERIFY no errors appear in the error console.
    Show
    This needs to be tested across browsers, especially IE8. Open a course which uses the Weekly course format. Open the error console in your browser. If you are using IE8, press F12 and change the Browser mode to IE8 Compatibility View and click on the Script tab. Drag and drop some resources/activites. Drag and drop some sections. VERIFY no errors appear in the error console.
  • Affected Branches:
    MOODLE_23_STABLE, MOODLE_24_STABLE
  • Fixed Branches:
    MOODLE_23_STABLE
  • Pull from Repository:
  • Pull Master Branch:
    MDL-35134_master
  • Pull Master Diff URL:

Description

IE doesn't like trailing commas in object (or array) definitions - and one seems to have turned up in 2.3/2.4dev, in course/format/weeks/format.js. This breaks subsequent javascript on courses using the weeks format.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Paul Nicholls added a comment - - edited

I've nudged the priority to "major" since it involves loss of functionality (albeit only in IE), even though the patch itself is incredibly trivial (quite literally just removing a comma).

Show
Paul Nicholls added a comment - - edited I've nudged the priority to "major" since it involves loss of functionality (albeit only in IE), even though the patch itself is incredibly trivial (quite literally just removing a comma).
Hide
Permalink
Michael de Raadt added a comment -

Thanks for submitting that, Paul.

The change looks good. In JS, a trailing comma like that is an error, unlike in PHP.

However I was not able to trigger any errors in IE8 or IE9 by dragging and dropping sections (which is where I think the code is used).

Were you seeing actual errors?

Show
Michael de Raadt added a comment - Thanks for submitting that, Paul. The change looks good. In JS, a trailing comma like that is an error, unlike in PHP. However I was not able to trigger any errors in IE8 or IE9 by dragging and dropping sections (which is where I think the code is used). Were you seeing actual errors?
Hide
Permalink
Paul Nicholls added a comment - - edited

Hi Michael,
The error only shows up in courses using the Weekly course format - and I was seeing it in IE8 (I'm not sure if IE9 still throws the error and aborts processing, as I don't have IE9 available) on page load. In a course using the Weekly format in IE8, the error caused JS to stop executing before the resource dragdrop was initialised - leaving all of the icons as the move up/down icon rather than the four-way drag icon.

-Paul

Show
Paul Nicholls added a comment - - edited Hi Michael, The error only shows up in courses using the Weekly course format - and I was seeing it in IE8 (I'm not sure if IE9 still throws the error and aborts processing, as I don't have IE9 available) on page load. In a course using the Weekly format in IE8, the error caused JS to stop executing before the resource dragdrop was initialised - leaving all of the icons as the move up/down icon rather than the four-way drag icon. -Paul
Hide
Permalink
Michael de Raadt added a comment -

Thanks, Paul.

I was able to see an error when the browser was in compatibility view.

Show
Michael de Raadt added a comment - Thanks, Paul. I was able to see an error when the browser was in compatibility view.
Hide
Permalink
Michael de Raadt added a comment -

Pushing directly to integration.

Show
Michael de Raadt added a comment - Pushing directly to integration.
Hide
Permalink
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
Permalink
Eloy Lafuente (stronk7) added a comment -

Integrated (23 and master), thanks!

Show
Eloy Lafuente (stronk7) added a comment - Integrated (23 and master), thanks!
Hide
Permalink
Tim Barker added a comment -

Tested as per the testing instructions and the test passed. Good work

Show
Tim Barker added a comment - Tested as per the testing instructions and the test passed. Good work
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

Thanks!

Show
Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: