Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-44270

JavaScript error when drag-dropping into a File manager

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.5.6, 2.6.3
    • Component/s: Filepicker
    • Labels:
    • Environment:
      Safari 6.1 on OSX
    • Testing Instructions:
      Hide

      Drag and drop a file into a File manager component on a form (e.g. add a File resource to a course) and ensure it still works.

      Show
      Drag and drop a file into a File manager component on a form (e.g. add a File resource to a course) and ensure it still works.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      A JavaScript error may be triggered when dragging and dropping a file into a File manager component, which halts JavaScript execution and leaves the page in a faulty state. This has been observed (inconsistently) using OSX Safari 6.1/6.1.1 and is caused by the 'dragover' event having a null value for the dataTransfer.types property. The lib/form/dndupload.js has_files() method assumes the event.dataTransfer.types property is an Array, and when it is not and attempts to read the length property, an error is triggered.

      I have tried to research whether this is specifically a Safari browser bug or a difference in interpretation of the W3C spec, and reproducing it consistently has been difficult as only one system has produced the error under normal use. On other systems with the same browser versions it has only occurred sporadically while stepping within the debugger. The solution is trivial and ensures that an Array is always returned so that
      reading the array length always succeeds.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Jonathon,

            Thanks for taking on this issue and tracking down the cause. This patch looks mostly on the right track and will indeed prevent the error, but I assume since the file types array is empty, we won't actually upload the file which may be confusing to the user.

            Thinking about it more, I don't think that there's much that we can do to alert the user of the error, but perhaps you could add a comment above so that anyone trying to step through in a debugger and work out why files are not uploaded may have a clue.

            On re-reading this I see that it's only in the dragover that this sometimes happens so it should be fine as is.

            Are you able to produce the stable branches for this?

            Thanks again,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Jonathon, Thanks for taking on this issue and tracking down the cause. This patch looks mostly on the right track and will indeed prevent the error, but I assume since the file types array is empty, we won't actually upload the file which may be confusing to the user. Thinking about it more, I don't think that there's much that we can do to alert the user of the error, but perhaps you could add a comment above so that anyone trying to step through in a debugger and work out why files are not uploaded may have a clue. On re-reading this I see that it's only in the dragover that this sometimes happens so it should be fine as is. Are you able to produce the stable branches for this? Thanks again, Andrew
            Hide
            jonof Jonathon Fowler added a comment -

            Hi Andrew

            Sorry for the delay prepping the stable branches. Done now.

            Cheers

            Jonathon

            Show
            jonof Jonathon Fowler added a comment - Hi Andrew Sorry for the delay prepping the stable branches. Done now. Cheers Jonathon
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-44270 Remote repository: git://github.com/jonof/moodle.git Remote branch MDL-44270 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2971 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2971/artifact/work/smurf.html Remote branch MDL-44270 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2972 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2972/artifact/work/smurf.html Remote branch MDL-44270 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2973 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2973/artifact/work/smurf.html
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Jonathon,

            The changes look good. My only thought is that I still think that it is worth adding a comment above the array declaration to explain that some browsers will fire the event without the types being instantiated yet.

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Jonathon, The changes look good. My only thought is that I still think that it is worth adding a comment above the array declaration to explain that some browsers will fire the event without the types being instantiated yet. Cheers, Andrew
            Hide
            jonof Jonathon Fowler added a comment -

            I've added a comment to the source.

            Cheers

            Jonathon

            Show
            jonof Jonathon Fowler added a comment - I've added a comment to the source. Cheers Jonathon
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-44270 Remote repository: git://github.com/jonof/moodle.git Remote branch MDL-44270 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2984 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2984/artifact/work/smurf.html Remote branch MDL-44270 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2985 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2985/artifact/work/smurf.html Remote branch MDL-44270 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2986 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2986/artifact/work/smurf.html
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Submitting for IR. Thanks Jonathon.

            Show
            dobedobedoh Andrew Nicols added a comment - Submitting for IR. Thanks Jonathon.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Jonathon - this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Jonathon - this has been integrated now.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Tested during integration review and passed.

            Show
            samhemelryk Sam Hemelryk added a comment - Tested during integration review and passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Your code is now part of Moodle upstream, many thanks!

            It's a constant, continuous,
            spectacular world we live in,
            and every day you see things that
            just knock you out, if you pay attention.

            Robert Irwin

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Your code is now part of Moodle upstream, many thanks! It's a constant, continuous, spectacular world we live in, and every day you see things that just knock you out, if you pay attention. Robert Irwin

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14