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

Rewrite DND Upload to YUI module (performance)

    Details

    • Story Points (Obsolete):
      120

      Description

      This is just something that would be really nice to do, given much of the course ajax JS has now been converted into YUI modules this really should as well to keep things clean and organised.

      Cheers
      Sam

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            samhemelryk Sam Hemelryk added a comment -

            While it would be nice to get this done before 2.3 as this is a new feature I think it probably can wait until after.
            Would have to consider it in regards to code freeze but I imagine this would be a pretty straight forward easy conversion to make.

            Show
            samhemelryk Sam Hemelryk added a comment - While it would be nice to get this done before 2.3 as this is a new feature I think it probably can wait until after. Would have to consider it in regards to code freeze but I imagine this would be a pretty straight forward easy conversion to make.
            Hide
            davosmith Davo Smith added a comment -

            I'll look at doing this, but with the other issues that have come up, I may not have time before 2.3 is frozen.

            Show
            davosmith Davo Smith added a comment - I'll look at doing this, but with the other issues that have come up, I may not have time before 2.3 is frozen.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            It's probably wise to wait until shifter lands before considering this. We should then be able to benefit from reduce load times.

            Show
            dobedobedoh Andrew Nicols added a comment - It's probably wise to wait until shifter lands before considering this. We should then be able to benefit from reduce load times.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've made a start on doing this but have hit a few issues along the way.
            Now that a few other submodule changes have landed, I'm hoping I can break this code into more logical steps.

            The main issue that I've hit though is that we're doing something really nasty to drag and drop. I think whatever we're doing is in the course/section/block drag/drop code and it's nasty enough that dragging a file around a section causes a number of duplicate events. This in turn means that we can't reliably use the dragenter and dragleave events to determine if we're in a section.

            My changes also implement the concept of a loader so that we only load a minimal module to test whether drag/drop is even supported. However, in further thought we should actually do this with the loader itself by using a condition (see http://yuilibrary.com/yui/docs/api/classes/Loader.html#method_addModule). We may need to modify our yui_combo.php to handle conditions...

            I also came across a number of potential performance concerns in the existing code. These will affect courses with large numbers of sections in particular. The main one is that the 'preview' elements (the 'Add file(s) here' bit) are generated on page load and added to every single section. We also create 4 event listeners per section on page load. in the rewrite, I've changed this to 4 delegated listeners, and to only creating the preview element when a file is actually held over that section (and if it doesn't already exist).

            I've also changed the style of this to be much more obvious and noticeable. I've attached a screenshot to demonstrate.

            So, in summary, this is ongoing and more complicated than first thought, but the new version should lead to some good performance improvements for courses, particularly those with many sections and mobile devices which don't support dndupload at all.

            Show
            dobedobedoh Andrew Nicols added a comment - I've made a start on doing this but have hit a few issues along the way. Now that a few other submodule changes have landed, I'm hoping I can break this code into more logical steps. The main issue that I've hit though is that we're doing something really nasty to drag and drop. I think whatever we're doing is in the course/section/block drag/drop code and it's nasty enough that dragging a file around a section causes a number of duplicate events. This in turn means that we can't reliably use the dragenter and dragleave events to determine if we're in a section. My changes also implement the concept of a loader so that we only load a minimal module to test whether drag/drop is even supported. However, in further thought we should actually do this with the loader itself by using a condition (see http://yuilibrary.com/yui/docs/api/classes/Loader.html#method_addModule ). We may need to modify our yui_combo.php to handle conditions... I also came across a number of potential performance concerns in the existing code. These will affect courses with large numbers of sections in particular. The main one is that the 'preview' elements (the 'Add file(s) here' bit) are generated on page load and added to every single section. We also create 4 event listeners per section on page load. in the rewrite, I've changed this to 4 delegated listeners, and to only creating the preview element when a file is actually held over that section (and if it doesn't already exist). I've also changed the style of this to be much more obvious and noticeable. I've attached a screenshot to demonstrate. So, in summary, this is ongoing and more complicated than first thought, but the new version should lead to some good performance improvements for courses, particularly those with many sections and mobile devices which don't support dndupload at all.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've just created MDL-40812 to look at reducing the issues we've seen regarding themes and course formats using different structures which break the existing code.

            Show
            dobedobedoh Andrew Nicols added a comment - I've just created MDL-40812 to look at reducing the issues we've seen regarding themes and course formats using different structures which break the existing code.
            Hide
            davosmith Davo Smith added a comment -

            Andrew - I wasn't quite sure what you were saying here about the dragenter/dragleave events. The problem with multiple events triggering is an inherent problem of these events, which is why there is some complex logic in there to try (and in 95% of cases succeed) in counting whether we are actually over a section at the current moment. The drag/drop reordering of sections came later.

            If you can find some way of improving the logic for determining when we're dragging over a section, then I'd be happy to take a look at it.

            The minimal loader is probably worthwhile (although given the fairly small size of the drag/drop code, would this not end up with the overhead of downloading an extra module being greater than the saving of not downloading it in the first place?).

            The 'preview' elements are generated at page load on purpose to work around a firefox bug whereby adding them 'on demand' meant that they were not counted as drop targets by the drag and drop code. Firefox only seems to handle 'dragenter/dragleave/drop' events properly for child elements that were present when the event listener was added to the outer container - as a result, dropping a file onto the 'add file(s) here' message causes the file to be displayed, not uploaded. If you have found a better workaround for this bug, then I'd be interested in seeing what it is.

            If delegated listeners work better than attaching listeners to each section, then that sounds like a good improvement. Thinking about it, since the code already searches to find out which section you were in, given the target node that triggered the event, it could probably have been implemented as events on the whole page anyway (note that the current implementation evolved over time, in response to a number of issues that came up whilst developing it).

            The styling of the 'add file(s) here' is deliberately meant to show where the new resource will appear in the section, which I think is clearer than suggesting it might appear somewhere in the section, however if there is a general preference for the 'whole section' styling, then I'm happy to go with that.

            Feel free to contact me via dev chat, if you want to talk through any of these code changes.

            Show
            davosmith Davo Smith added a comment - Andrew - I wasn't quite sure what you were saying here about the dragenter/dragleave events. The problem with multiple events triggering is an inherent problem of these events, which is why there is some complex logic in there to try (and in 95% of cases succeed) in counting whether we are actually over a section at the current moment. The drag/drop reordering of sections came later. If you can find some way of improving the logic for determining when we're dragging over a section, then I'd be happy to take a look at it. The minimal loader is probably worthwhile (although given the fairly small size of the drag/drop code, would this not end up with the overhead of downloading an extra module being greater than the saving of not downloading it in the first place?). The 'preview' elements are generated at page load on purpose to work around a firefox bug whereby adding them 'on demand' meant that they were not counted as drop targets by the drag and drop code. Firefox only seems to handle 'dragenter/dragleave/drop' events properly for child elements that were present when the event listener was added to the outer container - as a result, dropping a file onto the 'add file(s) here' message causes the file to be displayed, not uploaded. If you have found a better workaround for this bug, then I'd be interested in seeing what it is. If delegated listeners work better than attaching listeners to each section, then that sounds like a good improvement. Thinking about it, since the code already searches to find out which section you were in, given the target node that triggered the event, it could probably have been implemented as events on the whole page anyway (note that the current implementation evolved over time, in response to a number of issues that came up whilst developing it). The styling of the 'add file(s) here' is deliberately meant to show where the new resource will appear in the section, which I think is clearer than suggesting it might appear somewhere in the section, however if there is a general preference for the 'whole section' styling, then I'm happy to go with that. Feel free to contact me via dev chat, if you want to talk through any of these code changes.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Davo,

            Thanks for taking the time to respond

            I wasn't aware that the duplicate events were an inherent issue. I'll continue to look into them. I currently still have the same logic in place, but I've been trying to simplify that if possible. If it isn't possible, it isn't possible. I will keep looking, but I suspect that for the moment, I'll be keeping the existing logic.

            The minimal loader currently clocks in at 427 bytes and moves the existing checks from dndupload to the loader. There is a small added overhead for headers etc obviously, but the vast majority of mobile devices do not support drag/drop uploading of files, and nor do a majority of versions of IE. W3schools stats suggest that a majority of IE users are still using older versions (http://www.w3schools.com/browsers/browsers_explorer.asp) which do not support DND, and these are likely to be a relatively large percentage of Moodle users.
            Conversely, the existing dndupload code clocks in at 40KB (40739 bytes) and 16K (16,557 bytes) minified. The new YUI module weighs in at 40K (40,719 bytes) with debugging statements and 13KB (13,776 bytes) minified. 16KB isn't a massive amount, but when combined with all of the dependencies too it can really add up - potentially problematic, especially on mobile devices.

            I've not come across any issues with Firefox and adding the preview element. Which versions of Firefox were affected, and with what browsers? I'm primarily testing on Mac OS with Firefox 21, but I've just confirmed it working on Firefox 4 too. I suspect that the YUI event delegation may actually be the solution in thinking about it. The event should be bubbling down to the child nodes (even after they've been added) which probably addresses this issue.

            WRT the style, I was just playing around. There has been a general move to that kind of style of drag/drop alert recently (see Twitter, Facebook, and Gmail for starters) making it more familiar with users and I (personally) think that it's clearer that the whole section is a drop target. With the existing style, it could be missed, or users may think that the preview node should be moveable up the section. That change would have to go by Martin in any case.

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Davo, Thanks for taking the time to respond I wasn't aware that the duplicate events were an inherent issue. I'll continue to look into them. I currently still have the same logic in place, but I've been trying to simplify that if possible. If it isn't possible, it isn't possible. I will keep looking, but I suspect that for the moment, I'll be keeping the existing logic. The minimal loader currently clocks in at 427 bytes and moves the existing checks from dndupload to the loader. There is a small added overhead for headers etc obviously, but the vast majority of mobile devices do not support drag/drop uploading of files, and nor do a majority of versions of IE. W3schools stats suggest that a majority of IE users are still using older versions ( http://www.w3schools.com/browsers/browsers_explorer.asp ) which do not support DND, and these are likely to be a relatively large percentage of Moodle users. Conversely, the existing dndupload code clocks in at 40KB (40739 bytes) and 16K (16,557 bytes) minified. The new YUI module weighs in at 40K (40,719 bytes) with debugging statements and 13KB (13,776 bytes) minified. 16KB isn't a massive amount, but when combined with all of the dependencies too it can really add up - potentially problematic, especially on mobile devices. I've not come across any issues with Firefox and adding the preview element. Which versions of Firefox were affected, and with what browsers? I'm primarily testing on Mac OS with Firefox 21, but I've just confirmed it working on Firefox 4 too. I suspect that the YUI event delegation may actually be the solution in thinking about it. The event should be bubbling down to the child nodes (even after they've been added) which probably addresses this issue. WRT the style, I was just playing around. There has been a general move to that kind of style of drag/drop alert recently (see Twitter, Facebook, and Gmail for starters) making it more familiar with users and I (personally) think that it's clearer that the whole section is a drop target. With the existing style, it could be missed, or users may think that the preview node should be moveable up the section. That change would have to go by Martin in any case. Cheers, Andrew
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Apologies, with the Firefox question I meant which Operating Systems .

            Cheers

            Show
            dobedobedoh Andrew Nicols added a comment - Apologies, with the Firefox question I meant which Operating Systems . Cheers
            Hide
            davosmith Davo Smith added a comment -

            The duplicate events / Firefox issues have been around since I first developed the code for Moodle 2.0 (and backported to Moodle 1.9).

            I've seen the problem with Firefox on Windows and Linux and it was consistently reproducible at the time I first wrote the code (about 2 years ago) and still at the time when I integrated it into Moodle 2.3 (I looked at stripping out the 'create all elements at the start' fix at that point). The situation may have changed. The problem manifested in that when you dragged over the text of 'Add file(s) here', the mouse pointer would change to indicate that dropping was no longer possible and attempting to drop the file would result in it being displayed, not uploaded.

            I also looked at multiple solutions to the dragenter/dragleave problem and this one seemed to be the most stable, reliable solution I could find (although it is possible to break it, especially on the 'file manager' element - resize your browser so that the filemanager element is partially off the side of the page, drag a file over the page, then off again, leaving through the filemanager element, where it touches the edge of the browser; if you are quick enough the overlays do not always get cleared properly).

            Quite happy to go with your figures for size savings in only downloading the initial tests on unsupported browsers.

            Show
            davosmith Davo Smith added a comment - The duplicate events / Firefox issues have been around since I first developed the code for Moodle 2.0 (and backported to Moodle 1.9). I've seen the problem with Firefox on Windows and Linux and it was consistently reproducible at the time I first wrote the code (about 2 years ago) and still at the time when I integrated it into Moodle 2.3 (I looked at stripping out the 'create all elements at the start' fix at that point). The situation may have changed. The problem manifested in that when you dragged over the text of 'Add file(s) here', the mouse pointer would change to indicate that dropping was no longer possible and attempting to drop the file would result in it being displayed, not uploaded. I also looked at multiple solutions to the dragenter/dragleave problem and this one seemed to be the most stable, reliable solution I could find (although it is possible to break it, especially on the 'file manager' element - resize your browser so that the filemanager element is partially off the side of the page, drag a file over the page, then off again, leaving through the filemanager element, where it touches the edge of the browser; if you are quick enough the overlays do not always get cleared properly). Quite happy to go with your figures for size savings in only downloading the initial tests on unsupported browsers.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Adding links to my (VERY) WIP branch for this.

            Show
            dobedobedoh Andrew Nicols added a comment - Adding links to my (VERY) WIP branch for this.
            Hide
            damyon Damyon Wiese added a comment -

            Just a general comment - the dndupload code is one of the slowest performing JS modules (found by testing with behat and phantomjs patch) - so this would be a good patch to focus on.

            Show
            damyon Damyon Wiese added a comment - Just a general comment - the dndupload code is one of the slowest performing JS modules (found by testing with behat and phantomjs patch) - so this would be a good patch to focus on.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'll add this to my Must fix for 2.7 list. Now if only I had a way of marking things as that in the tracker...

            Show
            dobedobedoh Andrew Nicols added a comment - I'll add this to my Must fix for 2.7 list. Now if only I had a way of marking things as that in the tracker...
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've already started development on this so marking it as such...

            Show
            dobedobedoh Andrew Nicols added a comment - I've already started development on this so marking it as such...
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I am un-assigning myself from this issue as I am not currently working on this and it will give the opportunity for someone else to work on it.

            Show
            dobedobedoh Andrew Nicols added a comment - I am un-assigning myself from this issue as I am not currently working on this and it will give the opportunity for someone else to work on it.

              People

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

                Dates

                • Created:
                  Updated: