Moodle
  1. Moodle
  2. MDL-35672 META JavaScript performance issues
  3. MDL-35569

Rewrite init_url_select / init_select_autosubmit as YUI module and improve performance

    Details

    • Testing Instructions:
      Hide

      These testing instructions must be carried out in as many browsers as possible. At the very least the following combinations should be tested:

      [ ] Windows IE
      [ ] Windows Chrome
      [ ] Windows Firefox
      [ ] Windows Opera
      [ ] Mac OS Chrome
      [ ] Mac OS Safari
      [ ] Mac OS Firefox
      [ ] Mac OS Opera
      [ ] iOS Safari
      
      • Open a course
      • Turn editing on
      • Turn the Activity chooser off
      • Choose an activity from the dropdown
        • Confirm that the page redirected
      • Return back to the course page in editing mode
      • From the 'Add a block' block's dropdown, choose a block
        • Confirm that the page redirects and that the block is added
      • Return back to the course page in editing mode
      • Add a new Choice activity
      • Submit a choice for one or more users
      • View the submitted responses
      • Select a response and from the 'Choose an action...' dropdown, choose 'Delete'
        • Confirm that the page reloads and performs the action
      • Navigate to Site administration -> Courses -> Add/edit courses
      • Create a new category
      • Navigate to the category with your other courses
      • Select a course using the checkboxes
      • From the 'Move selected courses to...', choose your new category
        • Confirm that the page reloads and performs the action
      • Search for a course (/course/search.php?search=foo)
      • Select a course using the checkboxes
      • From the 'Move selected courses to...', choose your new category
        • Confirm that the page reloads and performs the action

      NOTE: There are other dropdowns in:

      • mod/feedback/analysis_course.php
      • mod/lesson/report.php
      • theme/mymobile/renderers.php

      I've not provided test instructions for these as I don't have test data to use them with.

      Show
      These testing instructions must be carried out in as many browsers as possible. At the very least the following combinations should be tested: [ ] Windows IE [ ] Windows Chrome [ ] Windows Firefox [ ] Windows Opera [ ] Mac OS Chrome [ ] Mac OS Safari [ ] Mac OS Firefox [ ] Mac OS Opera [ ] iOS Safari Open a course Turn editing on Turn the Activity chooser off Choose an activity from the dropdown Confirm that the page redirected Return back to the course page in editing mode From the 'Add a block' block's dropdown, choose a block Confirm that the page redirects and that the block is added Return back to the course page in editing mode Add a new Choice activity Submit a choice for one or more users View the submitted responses Select a response and from the 'Choose an action...' dropdown, choose 'Delete' Confirm that the page reloads and performs the action Navigate to Site administration -> Courses -> Add/edit courses Create a new category Navigate to the category with your other courses Select a course using the checkboxes From the 'Move selected courses to...', choose your new category Confirm that the page reloads and performs the action Search for a course (/course/search.php?search=foo) Select a course using the checkboxes From the 'Move selected courses to...', choose your new category Confirm that the page reloads and performs the action NOTE: There are other dropdowns in: mod/feedback/analysis_course.php mod/lesson/report.php theme/mymobile/renderers.php I've not provided test instructions for these as I don't have test data to use them with.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35569-master
    • Rank:
      44282

      Description

      init_url_select is woefully inefficient, and init_select_autosubmit may have similar issues.
      We should rewrite it, possibly as a YUI module, and improve it's performance.
      We should try not to:

      • use loops of Y.use()
      • use loops of Y.Node.on()

      We also need to bear the accessibility issues in mind

      I believe that the attached branch should do that, but I'm not sure whether it's best to run it as a YUI module though or not.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          This is my initial work on the area. Not complete, but will try and work more on it before 2.4 freeze.

          Show
          Andrew Nicols added a comment - This is my initial work on the area. Not complete, but will try and work more on it before 2.4 freeze.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, Andrew.

          Please continue working on the issue until you are confident with it, then seek out a peer reviewer. Sam Hemelryk may be the best candidate.

          Show
          Michael de Raadt added a comment - Thanks for working on this, Andrew. Please continue working on the issue until you are confident with it, then seek out a peer reviewer. Sam Hemelryk may be the best candidate.
          Hide
          Paul Nicholls added a comment - - edited

          Hi Andrew,
          A quick test (on the Manage Filters page, since it has a whole bunch of autosubmit selects) revealed that you seem to have a slight error in your initializer() function - although your comment suggests that only IE (<=8) fails to support delegated change events, you're forcing all non-IE browsers to also use the applyto.all().on() approach instead of delegation. In non-IE browsers, Y.UA.ie is 0 - which is <= 8. It's probably better to test for Y.UA.ie && Y.UA.ie <= 8, as long as there aren't other browsers which suffer from the same issue (I can confirm that Firefox 15/16 supports the delegation, since that's what I used for my quick test (started with 15, then it updated!), and it worked perfectly after making the slight tweak).

          Edited to add: As I hadn't caught up with the deprecation of init_url_select in favour of a rewritten init_select_autosubmit, I initially thought you'd skipped the add activity/resource dropdowns from this commit - but I see that you did indeed cover those! All seems to be working fine here for me, from the limited testing I've done.

          -Paul

          Show
          Paul Nicholls added a comment - - edited Hi Andrew, A quick test (on the Manage Filters page, since it has a whole bunch of autosubmit selects) revealed that you seem to have a slight error in your initializer() function - although your comment suggests that only IE (<=8) fails to support delegated change events, you're forcing all non-IE browsers to also use the applyto.all().on() approach instead of delegation. In non-IE browsers, Y.UA.ie is 0 - which is <= 8 . It's probably better to test for Y.UA.ie && Y.UA.ie <= 8 , as long as there aren't other browsers which suffer from the same issue (I can confirm that Firefox 15/16 supports the delegation, since that's what I used for my quick test (started with 15, then it updated!), and it worked perfectly after making the slight tweak). Edited to add: As I hadn't caught up with the deprecation of init_url_select in favour of a rewritten init_select_autosubmit, I initially thought you'd skipped the add activity/resource dropdowns from this commit - but I see that you did indeed cover those! All seems to be working fine here for me, from the limited testing I've done. -Paul
          Hide
          Andrew Nicols added a comment -

          Thanks for spotting that Paul.

          I've rebased on latest master and added some notes about the old calls being deprecated.

          Submitting for Peer Review.

          Show
          Andrew Nicols added a comment - Thanks for spotting that Paul. I've rebased on latest master and added some notes about the old calls being deprecated. Submitting for Peer Review.
          Hide
          Adrian Greeve added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [N] Testing
          [Y] Security
          [Y] Documentation
          [N] Git
          [N] Sanity check

          Hi Andrew,

          The code for this looks really good and is a definite improvement over the current mess that is being used. Just a couple of thing:

          1. These changes need to be checked on all browsers and operating systems, so that should be mentioned in the testing instructions.
          2. The commit message doesn't include the component http://docs.moodle.org/dev/Peer_reviewing_checklist#Git
          3. This patch doesn't work on Chrome or Safari on a Mac. Using a mouse is fine, but when using a keyboard, pressing enter doesn't redirect to the page.
          Show
          Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [N] Testing [Y] Security [Y] Documentation [N] Git [N] Sanity check Hi Andrew, The code for this looks really good and is a definite improvement over the current mess that is being used. Just a couple of thing: These changes need to be checked on all browsers and operating systems, so that should be mentioned in the testing instructions. The commit message doesn't include the component http://docs.moodle.org/dev/Peer_reviewing_checklist#Git This patch doesn't work on Chrome or Safari on a Mac. Using a mouse is fine, but when using a keyboard, pressing enter doesn't redirect to the page.
          Hide
          Andrew Nicols added a comment -

          Thanks Adrian for the lovely detailed feedback.

          1: Sorry - I'll add a note about this now
          2: What is the correct component for this kind of change? I guess AJAX?
          3: It works for me on Chrome and Safari - it seems that with these browsers you have to press [enter] to select the item, then [enter] again to submit the form.

          Still need to add instructions for those final dropdowns and then I'll submit for IR.

          Show
          Andrew Nicols added a comment - Thanks Adrian for the lovely detailed feedback. 1: Sorry - I'll add a note about this now 2: What is the correct component for this kind of change? I guess AJAX? 3: It works for me on Chrome and Safari - it seems that with these browsers you have to press [enter] to select the item, then [enter] again to submit the form. Still need to add instructions for those final dropdowns and then I'll submit for IR.
          Hide
          Adrian Greeve added a comment -

          Hi Andrew,

          You're correct. It does only require hitting the enter button twice to submit the form. Not ideal. The current implementation doesn't have this issue, but I understand that trying to implement this small enhancement may not be worth the development time.

          Show
          Adrian Greeve added a comment - Hi Andrew, You're correct. It does only require hitting the enter button twice to submit the form. Not ideal. The current implementation doesn't have this issue, but I understand that trying to implement this small enhancement may not be worth the development time.
          Hide
          Aparup Banerjee 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
          Aparup Banerjee 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
          Sam Hemelryk added a comment -

          Hi Andrew,

          I really like this change, I spotted a couple of minor things and will reopen this just to get them addressed/discussed.

          • Typo: ignorechangevent => ignorechangeevent (double e)
          • Mixed operation; There seems to be one relatively minor glitch here, if the user follows through the following steps:
            1. Tab to the select box
            2. Press the down arrow
            3. Click on the select box
            4. Click to select a category.
            • Expected: redirect and courses moved to category clicked on.
            • Now: Nothing happens, you can click as much as you want and nothing happens.
            • Before: redirect occurs at first click, moving course to category selected using down arrow. This seems wrong anyway.
          • I keep looking back to the user of nothing and lastindex as attributes within the HTML structure of the select. I think these are perhaps better called data-nothing and data-lastindex, this seems to fit more with HTML5. Although to be truthful I am only aware of the custom data attributes, I've never used them or seen them being used so feel free to correct me.
          • Most trivial thing ever, you should probably mention that that init_select_autosubmit was deprecated in Moodle 2.4 as well as mentioning that it will be removed in 2.6. Just helps keep things obvious to others stumbling upon it.

          Really pretty trivial things.
          It will be nice to get rid of the deprecated code from javascript-static!

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I really like this change, I spotted a couple of minor things and will reopen this just to get them addressed/discussed. Typo: ignorechangevent => ignorechangeevent (double e) Mixed operation; There seems to be one relatively minor glitch here, if the user follows through the following steps: Tab to the select box Press the down arrow Click on the select box Click to select a category. Expected: redirect and courses moved to category clicked on. Now: Nothing happens, you can click as much as you want and nothing happens. Before: redirect occurs at first click, moving course to category selected using down arrow. This seems wrong anyway. I keep looking back to the user of nothing and lastindex as attributes within the HTML structure of the select. I think these are perhaps better called data-nothing and data-lastindex, this seems to fit more with HTML5. Although to be truthful I am only aware of the custom data attributes, I've never used them or seen them being used so feel free to correct me. Most trivial thing ever, you should probably mention that that init_select_autosubmit was deprecated in Moodle 2.4 as well as mentioning that it will be removed in 2.6. Just helps keep things obvious to others stumbling upon it. Really pretty trivial things. It will be nice to get rid of the deprecated code from javascript-static! Many thanks Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Nicols added a comment -

          Thanks for that catch Sam,

          I've corrected the typo and changed to using the data- style attribute names. I've also added the deprecated since 2.4 comment as suggested.

          WRT to the mixed operation issues, I've rewritten part of the way that the event handling works. This makes it simpler for the most part and it also seems to be more reliable.

          Unfortunately, Opera still does some really fruity things on some OSs - notably under Macintosh it doesn't submit the form on mouse change, but does on every keyboard change. This isn't a change from the current state of play though and I can't find any way to fix it either - Opera sucks.

          Show
          Andrew Nicols added a comment - Thanks for that catch Sam, I've corrected the typo and changed to using the data- style attribute names. I've also added the deprecated since 2.4 comment as suggested. WRT to the mixed operation issues, I've rewritten part of the way that the event handling works. This makes it simpler for the most part and it also seems to be more reliable. Unfortunately, Opera still does some really fruity things on some OSs - notably under Macintosh it doesn't submit the form on mouse change, but does on every keyboard change. This isn't a change from the current state of play though and I can't find any way to fix it either - Opera sucks.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Andrew Nicols added a comment -

          Rebased and ready to go

          Show
          Andrew Nicols added a comment - Rebased and ready to go
          Hide
          Martin Dougiamas added a comment -

          What kind of performance improvement are we seeing from this?

          Show
          Martin Dougiamas added a comment - What kind of performance improvement are we seeing from this?
          Hide
          Andrew Nicols added a comment -

          Not in a position to give you exact figures now, but from what I recall this gives a pretty reasonable improvement and really helps. MDL-35819 gives better results but I need to write a version suitable for inclusion in 2.3 and 2.4 since we're long past freeze. Shouldn't be too tricky except I won't be able to til Wednesday.

          Andrew

          Show
          Andrew Nicols added a comment - Not in a position to give you exact figures now, but from what I recall this gives a pretty reasonable improvement and really helps. MDL-35819 gives better results but I need to write a version suitable for inclusion in 2.3 and 2.4 since we're long past freeze. Shouldn't be too tricky except I won't be able to til Wednesday. Andrew
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          I've just been looking at this now.
          There was only one thing I spotted, the use of $PAGE within renderers, thats a no-no as each renderer keeps a reference to the page it belongs to ($this->page) and is the only page it should be used with.
          I've attached a patch that can be applied before or during integration that fixes that up, its a very simple change

          I'm leaving this in review presently so that I can discuss its integration with the other integrators and MD, if no one has any issues with it landing I'll go in.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I've just been looking at this now. There was only one thing I spotted, the use of $PAGE within renderers, thats a no-no as each renderer keeps a reference to the page it belongs to ($this->page) and is the only page it should be used with. I've attached a patch that can be applied before or during integration that fixes that up, its a very simple change I'm leaving this in review presently so that I can discuss its integration with the other integrators and MD, if no one has any issues with it landing I'll go in. Many thanks Sam
          Hide
          Andrew Nicols added a comment -

          Thanks Sam - I've applied your patch and re-pushed the branch.

          Show
          Andrew Nicols added a comment - Thanks Sam - I've applied your patch and re-pushed the branch.
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now
          Hide
          Frédéric Massart added a comment -

          Failing the test to open the discussion about the two minor fails in the steps:

          Windows 7 IE 8
          Windows 7 IE 9
          Windows 7 Chrome
          Windows 7 Firefox
          Windows 7 Opera
          Windows 7 Safari
          Mac OS 10.6.2 Chrome
          Mac OS 10.6.2 Safari 4.0.3

          • The dropdown did not trigger any action when deleting a choice answer

          Mac OS 10.6.2 Firefox
          Mac OS 10.6.2 Opera

          • The 'Add resource/activity' dropdown did not trigger any action

          Ubuntu 12.04 Chromium
          Ubuntu 12.04 Firefox
          Ubuntu 12.04 Opera
          iOS 5 Safari

          Show
          Frédéric Massart added a comment - Failing the test to open the discussion about the two minor fails in the steps: Windows 7 IE 8 Windows 7 IE 9 Windows 7 Chrome Windows 7 Firefox Windows 7 Opera Windows 7 Safari Mac OS 10.6.2 Chrome Mac OS 10.6.2 Safari 4.0.3 The dropdown did not trigger any action when deleting a choice answer Mac OS 10.6.2 Firefox Mac OS 10.6.2 Opera The 'Add resource/activity' dropdown did not trigger any action Ubuntu 12.04 Chromium Ubuntu 12.04 Firefox Ubuntu 12.04 Opera iOS 5 Safari
          Hide
          Dan Poltawski added a comment -

          OSX is on 10.8 now, Safari on version 6 and version 4 isn't supported by us:
          http://docs.moodle.org/dev/Moodle_2.4_release_notes

          I tested in Safari 6 and it works.

          Show
          Dan Poltawski added a comment - OSX is on 10.8 now, Safari on version 6 and version 4 isn't supported by us: http://docs.moodle.org/dev/Moodle_2.4_release_notes I tested in Safari 6 and it works.
          Hide
          Andrew Nicols added a comment -

          According to the YUI environment support page, they do not support Opera: http://yuilibrary.com/yui/environments/ and to be honest, I don't blame them.

          As I mentioned above, Opera is doing some pretty frustrating things and doesn't generate consistent events for the same version of Opera across different OSs:

          Unfortunately, Opera still does some really fruity things on some OSs - notably under Macintosh it doesn't submit the form on mouse change, but does on every keyboard change. This isn't a change from the current state of play though and I can't find any way to fix it either - Opera sucks.

          Show
          Andrew Nicols added a comment - According to the YUI environment support page, they do not support Opera: http://yuilibrary.com/yui/environments/ and to be honest, I don't blame them. As I mentioned above, Opera is doing some pretty frustrating things and doesn't generate consistent events for the same version of Opera across different OSs: Unfortunately, Opera still does some really fruity things on some OSs - notably under Macintosh it doesn't submit the form on mouse change, but does on every keyboard change. This isn't a change from the current state of play though and I can't find any way to fix it either - Opera sucks.
          Hide
          Andrew Nicols added a comment -

          Oh, and also the lack of Opera support is no different to the existing functionality AFAIK.

          Show
          Andrew Nicols added a comment - Oh, and also the lack of Opera support is no different to the existing functionality AFAIK.
          Hide
          Dan Poltawski added a comment -

          Given that the opera problems are no difference, and safari works find in supported versions, passing this.

          Thanks!

          Show
          Dan Poltawski added a comment - Given that the opera problems are no difference, and safari works find in supported versions, passing this. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!
          Hide
          Sam Marshall added a comment - - edited

          Just to add a note on this in case it helps others in the same position as us: if you are suffering critical performance problems for IE8 users on the course editing page in Moodle 2.3 (when using the dropdowns), so that the 'Stop running this script?' dialog appears, then this issue is most likely the cause.

          According to IE8's JS profiler, total function call time on a test course with 52 weeks was about 40 seconds before applying this change, and is about 2.5 seconds after it.

          If anyone else is considering doing a backport for their local 2.3, Andrew told me they are intending to backport it to their system soon, so it should probably be safe. (I'm going to do it for our site too.)

          Show
          Sam Marshall added a comment - - edited Just to add a note on this in case it helps others in the same position as us: if you are suffering critical performance problems for IE8 users on the course editing page in Moodle 2.3 (when using the dropdowns), so that the 'Stop running this script?' dialog appears, then this issue is most likely the cause. According to IE8's JS profiler, total function call time on a test course with 52 weeks was about 40 seconds before applying this change, and is about 2.5 seconds after it. If anyone else is considering doing a backport for their local 2.3, Andrew told me they are intending to backport it to their system soon, so it should probably be safe. (I'm going to do it for our site too.)
          Hide
          Sam Marshall added a comment -

          I did a core moodle version of my backport - have only tested the OU derived version of this (which is the same except removing the 'mod' changes) and not exactly tested it a lot at this point.

          Note that I think this is a critical problem only under these circumstances:

          a) If using IE 8 (or probably 7, if that works).
          b) When not using the new activity chooser, so that a course page with say 52 weeks will include more than 104 dropdowns.

          With that in mind, if anybody would like to integrate it, here are the details (branch based on currentish MOODLE_23_MASTER):

          git://github.com/sammarshallou/moodle.git
          MDL-35569-m23
          https://github.com/sammarshallou/moodle/compare/MOODLE_23_STABLE...MDL-35569-m23

          Show
          Sam Marshall added a comment - I did a core moodle version of my backport - have only tested the OU derived version of this (which is the same except removing the 'mod' changes) and not exactly tested it a lot at this point. Note that I think this is a critical problem only under these circumstances: a) If using IE 8 (or probably 7, if that works). b) When not using the new activity chooser, so that a course page with say 52 weeks will include more than 104 dropdowns. With that in mind, if anybody would like to integrate it, here are the details (branch based on currentish MOODLE_23_MASTER): git://github.com/sammarshallou/moodle.git MDL-35569 -m23 https://github.com/sammarshallou/moodle/compare/MOODLE_23_STABLE...MDL-35569-m23

            People

            • Votes:
              16 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: