Moodle
  1. Moodle
  2. MDL-44270

JavaScript error when drag-dropping into a File manager

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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

      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

          Activity

          Hide
          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
          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
          Jonathon Fowler added a comment -

          Hi Andrew

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

          Cheers

          Jonathon

          Show
          Jonathon Fowler added a comment - Hi Andrew Sorry for the delay prepping the stable branches. Done now. Cheers Jonathon
          Hide
          CiBoT added a comment -
          Show
          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
          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
          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
          Jonathon Fowler added a comment -

          I've added a comment to the source.

          Cheers

          Jonathon

          Show
          Jonathon Fowler added a comment - I've added a comment to the source. Cheers Jonathon
          Hide
          CiBoT added a comment -
          Show
          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
          Andrew Nicols added a comment -

          Submitting for IR. Thanks Jonathon.

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

          Thanks Jonathon - this has been integrated now.

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

          Tested during integration review and passed.

          Show
          Sam Hemelryk added a comment - Tested during integration review and passed.
          Hide
          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
          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: