Details

    • Testing Instructions:
      Hide

      Note
      This patch changes javascript libraries, so be sure to purge caches and do a browser shift-reload after applying.

      Performance testing:
      Copy the attached bigform_test.php to the moodle root and visit with a browser.
      It will be very slow to load and handle events. With the patch it's much faster.

      Regression testing:
      This needs to be tested on as many browsers as possible.

      Some good pages to test:

      • attached bigform_test.php (copy to moodle root)
        Each form group contains an Enable checkbox which enables/disables the entire group. Verify that this interacts properly with the datetime control enable boxes.
      • attached samstest.php (copy to moodle root)
        In particular, verify that nested dependencies work properly:
        • In the "Disabled if things change" section:
          • Unchecking "Test checkbox" should disable the Basic and Array groups.
          • Clearing the text in the first textfield should disable the Basic and Array groups.
          • Adding text to the second textfield should disable the Basic and Array groups.
          • Changing the select to "No" should disable the Basic and Array groups.
        • In the Stacked Chain, each item disables/enables all the items below it.
        • For the Date/Duration selectors, the enable box should enable/disable the entire selector.

      Other moodle pages:

      • Backup UI
      • XML Grade upload
      • Calendar event add
      • Forum settings page
      • Assignment settings page
      Show
      Note This patch changes javascript libraries, so be sure to purge caches and do a browser shift-reload after applying. Performance testing: Copy the attached bigform_test.php to the moodle root and visit with a browser. It will be very slow to load and handle events. With the patch it's much faster. Regression testing: This needs to be tested on as many browsers as possible. Some good pages to test: attached bigform_test.php (copy to moodle root) Each form group contains an Enable checkbox which enables/disables the entire group. Verify that this interacts properly with the datetime control enable boxes. attached samstest.php (copy to moodle root) In particular, verify that nested dependencies work properly: In the "Disabled if things change" section: Unchecking "Test checkbox" should disable the Basic and Array groups. Clearing the text in the first textfield should disable the Basic and Array groups. Adding text to the second textfield should disable the Basic and Array groups. Changing the select to "No" should disable the Basic and Array groups. In the Stacked Chain, each item disables/enables all the items below it. For the Date/Duration selectors, the enable box should enable/disable the entire selector. Other moodle pages: Backup UI XML Grade upload Calendar event add Forum settings page Assignment settings page
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-35674-form-dependencies
    • Rank:
      44411

      Description

      This is best seen by going to the backup settings page for a large course in internet explorer. There are lots of checkboxes that all depend on each other.

      lib/form/form.js uses lots of separate node.on event handlers. Using a single delegate event handler would be much kinder to web browsers.

      1. bigform_test.php
        1 kB
        Matt Petro
      2. form.js
        20 kB
        Matt Petro
      3. samstest.php
        5 kB
        Sam Hemelryk
      1. ChromeCheckbox.png
        32 kB

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          I'm going to cowardly back away from this issue for the moment as I don't have time to work on it right now and I'm not all that familiar with this particularly beautiful section of Moodle's heritage.

          Reassigning back to moodle.com. If I get a chance, I'll pick it up again but I'm super busy between now and 2.4 freeze.

          Show
          Andrew Nicols added a comment - I'm going to cowardly back away from this issue for the moment as I don't have time to work on it right now and I'm not all that familiar with this particularly beautiful section of Moodle's heritage. Reassigning back to moodle.com. If I get a chance, I'll pick it up again but I'm super busy between now and 2.4 freeze.
          Hide
          Sam Marshall added a comment -

          Just to add a note - the description might give the impression that this problem is only serious in IE, but in fact the page is also horrifically slow in Firefox or Chrome when using a large course. It will typically show the 'this JavaScript is taking ages, do you want to stop it?' box once or twice.

          There is a workaround: when users here need to backup sites with standard Moodle backup interface, they typically disable JavaScript so they can make it through this page.

          I'm not offering to fix it either, sorry Just thought the workaround might be helpful...

          Show
          Sam Marshall added a comment - Just to add a note - the description might give the impression that this problem is only serious in IE, but in fact the page is also horrifically slow in Firefox or Chrome when using a large course. It will typically show the 'this JavaScript is taking ages, do you want to stop it?' box once or twice. There is a workaround: when users here need to backup sites with standard Moodle backup interface, they typically disable JavaScript so they can make it through this page. I'm not offering to fix it either, sorry Just thought the workaround might be helpful...
          Hide
          Matt Petro added a comment -

          This is a big issue for large courses in the backup UI as well as when using Tim's 'edit dates' contrib report. (The bowser can lockup for literally minutes with the later.)

          Results of profiling lib/form/form.js :

          1. elementsByName() does a full element traversal for every element, so it's quadratic in the number of elements
          2. checkDependencies() updates every dependency on the form for every event. This is really unnecessary as long as we store a bit of state information.
          3. _disableElement() makes an expensive ancestor() call which causes significant overhead on initial page load if many items are disabled.

          I've rewritten form.js to use a more efficient algorithm and to store more state so that fewer elements need to be updates on events. The improvement is enormous for large forms!

          I've tested on recent versions of FF and Chrome, as well as IE 6, 7 and 9

          Show
          Matt Petro added a comment - This is a big issue for large courses in the backup UI as well as when using Tim's 'edit dates' contrib report. (The bowser can lockup for literally minutes with the later.) Results of profiling lib/form/form.js : elementsByName() does a full element traversal for every element, so it's quadratic in the number of elements checkDependencies() updates every dependency on the form for every event. This is really unnecessary as long as we store a bit of state information. _disableElement() makes an expensive ancestor() call which causes significant overhead on initial page load if many items are disabled. I've rewritten form.js to use a more efficient algorithm and to store more state so that fewer elements need to be updates on events. The improvement is enormous for large forms! I've tested on recent versions of FF and Chrome, as well as IE 6, 7 and 9
          Hide
          Matt Petro added a comment -

          Any idea who should peer review this?

          Show
          Matt Petro added a comment - Any idea who should peer review this?
          Hide
          Tim Hunt added a comment -

          I think Andrew Nicols would be a good person to review this if he has the time.

          Andrew, if you are busy, just remove your name from the peer-reviewer field again.

          Show
          Tim Hunt added a comment - I think Andrew Nicols would be a good person to review this if he has the time. Andrew, if you are busy, just remove your name from the peer-reviewer field again.
          Hide
          Andrew Nicols added a comment -

          Cheers Tim,

          I can look at Peer Reviewing this next week, but I'd also appreciate Sam's thoughts.

          I don't know this section of code particularly well or what it is trying to achieve, but I wonder whether we can look at adjusting the HTML to use some more suitable selectors, and to store a mapping of dependent to dependees during instantiation and try to make the JS lazier.

          At present, even after the above changes, there are several (one to three) 'on' event handlers created for each dependent element and there's a lot of additional JS in the initializer for each element.

          If we moved to this model, we could have a single set of delegated event handlers:

          input[type=button|submit|radio|checkbox].hasdependency - click
          input.hasdependency:not[type=button|submit|radio|checkbox] - blur
          select.hasdependency - change
          !(input|select).hasdependency - click, blur, change

          This would obviously need some thought, but it should reduce the overhead - particularly for complex forms such as backup.

          Andrew

          Show
          Andrew Nicols added a comment - Cheers Tim, I can look at Peer Reviewing this next week, but I'd also appreciate Sam's thoughts. I don't know this section of code particularly well or what it is trying to achieve, but I wonder whether we can look at adjusting the HTML to use some more suitable selectors, and to store a mapping of dependent to dependees during instantiation and try to make the JS lazier. At present, even after the above changes, there are several (one to three) 'on' event handlers created for each dependent element and there's a lot of additional JS in the initializer for each element. If we moved to this model, we could have a single set of delegated event handlers: input [type=button|submit|radio|checkbox] .hasdependency - click input.hasdependency:not [type=button|submit|radio|checkbox] - blur select.hasdependency - change !(input|select).hasdependency - click, blur, change This would obviously need some thought, but it should reduce the overhead - particularly for complex forms such as backup. Andrew
          Hide
          Andrew Nicols added a comment -

          I've put a fix together which:

          • adds the hasDependency class to any dependency
          • adds a single delegated listener for .hasdependency on click, blur, and change
          • checks the type of input together with the type of event and proceeds according to the existing state of affairs

          I have however discovered a bug in some part of this approach so I'll keep looking tomorrow.

          Show
          Andrew Nicols added a comment - I've put a fix together which: adds the hasDependency class to any dependency adds a single delegated listener for .hasdependency on click, blur, and change checks the type of input together with the type of event and proceeds according to the existing state of affairs I have however discovered a bug in some part of this approach so I'll keep looking tomorrow.
          Hide
          Andrew Nicols added a comment -

          Tracked the bug down.

          Show
          Andrew Nicols added a comment - Tracked the bug down.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Didn't have time to look at this sorry, ended up spending more time on the shifter idea.
          Will try again on Monday (I'm away for the rest of the week).

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Didn't have time to look at this sorry, ended up spending more time on the shifter idea. Will try again on Monday (I'm away for the rest of the week). Many thanks Sam
          Hide
          Matt Petro added a comment -

          Hi Andrew,

          I like the change, as it cleans up the event handling and cuts down on the javascript on load time. I'd like to point though out that the major inefficiencies I found from profiling were related to the other items that I listed above. Is your plan to merge your changes with the ones that I provided?

          Thanks,
          -matt

          Show
          Matt Petro added a comment - Hi Andrew, I like the change, as it cleans up the event handling and cuts down on the javascript on load time. I'd like to point though out that the major inefficiencies I found from profiling were related to the other items that I listed above. Is your plan to merge your changes with the ones that I provided? Thanks, -matt
          Hide
          Matt Petro added a comment -

          Aha, nevermind. I see that you based the patch off mine. Many thanks!

          Show
          Matt Petro added a comment - Aha, nevermind. I see that you based the patch off mine. Many thanks!
          Hide
          Matt Petro added a comment -

          Hi Andrew,

          Both 'dateselector' and 'duration' form element types need the same special processing that your patch does for the datetimeselector. But for all three, shouldn't disabledIf() already handle adding the hasDependency class? It shouldn't be necessary to add that class in the element types themselves.

          Show
          Matt Petro added a comment - Hi Andrew, Both 'dateselector' and 'duration' form element types need the same special processing that your patch does for the datetimeselector. But for all three, shouldn't disabledIf() already handle adding the hasDependency class? It shouldn't be necessary to add that class in the element types themselves.
          Hide
          Andrew Nicols added a comment -

          Thanks Matt,

          I suspected that other types may need this too. They do still need it because they're sub-components of the form and don't seem to have the same dependency chain - this is of course, arguably a bug in itself.

          I'll get these added as soon as I get a chance.

          Andrew

          Show
          Andrew Nicols added a comment - Thanks Matt, I suspected that other types may need this too. They do still need it because they're sub-components of the form and don't seem to have the same dependency chain - this is of course, arguably a bug in itself. I'll get these added as soon as I get a chance. Andrew
          Hide
          Andrew Nicols added a comment -

          Sam said he'd take a look at this on a peer review front.

          Show
          Andrew Nicols added a comment - Sam said he'd take a look at this on a peer review front.
          Hide
          Matt Petro added a comment -

          In case anyone needs a fix for this now: We're running the original patch I provided

          Pull Master Diff URL: https://github.com/mpetrowi/moodle/compare/master...MDL-35672-form-dependencies
          Pull Master Branch: https://github.com/mpetrowi/moodle/tree/MDL-35672-form-dependencies

          on a large production site with no problems at all. My patch addresses most of the performance problems. The remaining issues in this ticket are related to event delegation, which isn't as critical for performance. The last time I tested there were still issues with the delegation part, see my comments above.

          Show
          Matt Petro added a comment - In case anyone needs a fix for this now: We're running the original patch I provided Pull Master Diff URL: https://github.com/mpetrowi/moodle/compare/master...MDL-35672-form-dependencies Pull Master Branch: https://github.com/mpetrowi/moodle/tree/MDL-35672-form-dependencies on a large production site with no problems at all. My patch addresses most of the performance problems. The remaining issues in this ticket are related to event delegation, which isn't as critical for performance. The last time I tested there were still issues with the delegation part, see my comments above.
          Hide
          Sam Hemelryk added a comment - - edited

          Hi Andrew + Matt,

          I've been looking over this now.
          Changes look real good thanks. What I noted:

          • The duration + dateselector fields using the optional argument do both need updating.
          • Undefined variable (simpletypo) form.js line 132, change => changed.

          Gets a +1 from me as soon as they are addressed.

          Perhaps of interest I created a slightly larger test form for this issue, I'll attach it after this, up to you if you include it in the test instructions etc.
          If you use this form you should add a step to change the values and then compare the before after data that is printed when this form is submit.

          As for where this should land, there is a chance that this change will cause regressions. Either because people have hacked core, or much more likely due to custom field types.
          100% this should land to master, do we consider backporting it or are we all happy with master presently?

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - - edited Hi Andrew + Matt, I've been looking over this now. Changes look real good thanks. What I noted: The duration + dateselector fields using the optional argument do both need updating. Undefined variable (simpletypo) form.js line 132, change => changed. Gets a +1 from me as soon as they are addressed. Perhaps of interest I created a slightly larger test form for this issue, I'll attach it after this, up to you if you include it in the test instructions etc. If you use this form you should add a step to change the values and then compare the before after data that is printed when this form is submit. As for where this should land, there is a chance that this change will cause regressions. Either because people have hacked core, or much more likely due to custom field types. 100% this should land to master, do we consider backporting it or are we all happy with master presently? Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Attached my test form.

          Show
          Sam Hemelryk added a comment - Attached my test form.
          Hide
          Tim Hunt added a comment -

          I think this is a classic case where this should land in master, then after a month or so, if there are no regressions, it should be back-ported. (The performance in IE is on large forms is really painful. We should back-port once we are sure.)

          Show
          Tim Hunt added a comment - I think this is a classic case where this should land in master, then after a month or so, if there are no regressions, it should be back-ported. (The performance in IE is on large forms is really painful. We should back-port once we are sure.)
          Hide
          Matt Petro added a comment -

          Thanks Sam, your test form is great! I'll include it in the testing instructions.
          I've made the changes you noted and tested on firefox, chrome, and IE 6,9.

          I think this is ready for master.

          Show
          Matt Petro added a comment - Thanks Sam, your test form is great! I'll include it in the testing instructions. I've made the changes you noted and tested on firefox, chrome, and IE 6,9. I think this is ready for master.
          Hide
          Sam Hemelryk added a comment -

          Sorry, this fell off my radar for one reason or another.
          I've moved it into integration immediately.

          Show
          Sam Hemelryk added a comment - Sorry, this fell off my radar for one reason or another. I've moved it into integration immediately.
          Hide
          Damyon Wiese added a comment -

          This is currently conflicting on master - showadvanced has been moved to a yui module as part of the shortforms changes.

          Could please resolve these conflicts and provide an updated branch Matt ?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - This is currently conflicting on master - showadvanced has been moved to a yui module as part of the shortforms changes. Could please resolve these conflicts and provide an updated branch Matt ? Thanks, Damyon
          Hide
          Damyon Wiese added a comment -

          Taking this out of integration for this week. Pleas resubmit when the conflicts on master have been resolved.

          Thanks!

          Show
          Damyon Wiese added a comment - Taking this out of integration for this week. Pleas resubmit when the conflicts on master have been resolved. Thanks!
          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
          Matt Petro added a comment -

          Sure, I'll update the code and re-submit.

          Show
          Matt Petro added a comment - Sure, I'll update the code and re-submit.
          Hide
          Matt Petro added a comment -

          Hi Sam,

          Yikes, I didn't mean to sit on this for a month. I think it is ready for integration. The changes since the last time you looked at it are only related to to showadvanced moving out of form.js. Thanks!

          Show
          Matt Petro added a comment - Hi Sam, Yikes, I didn't mean to sit on this for a month. I think it is ready for integration. The changes since the last time you looked at it are only related to to showadvanced moving out of form.js. Thanks!
          Hide
          Matt Petro added a comment -

          This will take a while to be backported to stable branches, so in case anyone is impatient, I've attached the form.js file which I've been running on a large production site for the past 4 months. This is a drop-in replacement for lib/form/form.js and includes some of the more critical performance changes. I'm using it with both 2.3.x and 2.4.x sites.

          Show
          Matt Petro added a comment - This will take a while to be backported to stable branches, so in case anyone is impatient, I've attached the form.js file which I've been running on a large production site for the past 4 months. This is a drop-in replacement for lib/form/form.js and includes some of the more critical performance changes. I'm using it with both 2.3.x and 2.4.x sites.
          Hide
          Matt Petro added a comment -

          Hi Sam, Have you had a chance to look at this? I'd like to get this integrated as the performance difference is huge for large forms. Thanks.

          Show
          Matt Petro added a comment - Hi Sam, Have you had a chance to look at this? I'd like to get this integrated as the performance difference is huge for large forms. Thanks.
          Hide
          Tim Hunt added a comment -

          Any chance of resuscitating this issue? Would be great to get it finished.

          Show
          Tim Hunt added a comment - Any chance of resuscitating this issue? Would be great to get it finished.
          Hide
          Matt Petro added a comment -

          Tim, can you suggest someone to peer review this? This issue has been stalled on peer review for months.

          Show
          Matt Petro added a comment - Tim, can you suggest someone to peer review this? This issue has been stalled on peer review for months.
          Hide
          Andrew Nicols added a comment -

          Hi Matt,

          I'm happy to Peer Review this. However, I think it's probably best if you kill off my commit on there - we can look at integrating those changes separately to this issue.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Matt, I'm happy to Peer Review this. However, I think it's probably best if you kill off my commit on there - we can look at integrating those changes separately to this issue. Cheers, Andrew
          Hide
          Matt Petro added a comment -

          Hi Andrew, I'm not sure I understand as I thought your changes made a lot of sense and relate directly to the Description of the issue. What's the reason for separating them out?

          Show
          Matt Petro added a comment - Hi Andrew, I'm not sure I understand as I thought your changes made a lot of sense and relate directly to the Description of the issue. What's the reason for separating them out?
          Hide
          Andrew Nicols added a comment -

          Hi Matt,

          You suggested in one of your previous comments that the biggest performance issue was addressed by your changes. I agree that it would be good to get the changes I've proposed in too, but I think that due to a combination of the complexity of both issues taken independently, and the different areas that they address, I feel it would be better to address each individually.

          Your change should bring us a quick win that we can commit now and possibly even backport to stable versions; whereas my change is much more complicated as it requires each type of input element to have a different set of event handlers. It may not be possible to backport this change because it may require custom code if there are any custom input types to deal with.

          Does that make sense?

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Matt, You suggested in one of your previous comments that the biggest performance issue was addressed by your changes. I agree that it would be good to get the changes I've proposed in too, but I think that due to a combination of the complexity of both issues taken independently, and the different areas that they address, I feel it would be better to address each individually. Your change should bring us a quick win that we can commit now and possibly even backport to stable versions; whereas my change is much more complicated as it requires each type of input element to have a different set of event handlers. It may not be possible to backport this change because it may require custom code if there are any custom input types to deal with. Does that make sense? Cheers, Andrew
          Hide
          Matt Petro added a comment -

          Hi Andrew, That makes sense. I've killed your commit and put you in as peer reviewer. When you're satisfied, I'd suggest this go into the stable branches as well (I'll do the rebase as there's a trivial change needed.) I've been running the code on 20k+ user 2.3 and 2.4 production sites since December without issues.

          Show
          Matt Petro added a comment - Hi Andrew, That makes sense. I've killed your commit and put you in as peer reviewer. When you're satisfied, I'd suggest this go into the stable branches as well (I'll do the rebase as there's a trivial change needed.) I've been running the code on 20k+ user 2.3 and 2.4 production sites since December without issues.
          Hide
          Andrew Nicols added a comment -

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

          Syntax

          checkDependencies - I'd prefer it if you swapped the parameters to the checkDependencies function such that it is more in keeping with other event handlers.
          Also, the event passed in is an EventFacade.

          Output

          group 1: top 'Enabled' checkbox does not disable the Calendar input button
          This could be an existing issue, and if so we need to raise a new issue for it.

          Whitespace

          A few cases of trailing whitespace.

          Otherwise, this looks good.

          Show
          Andrew Nicols added a comment - [N] Syntax [N] Whitespace [N] Output [-] Language [-] Databases [Y] Testing [Y] Security [-] Documentation [Y] Git [Y] Sanity check Syntax checkDependencies - I'd prefer it if you swapped the parameters to the checkDependencies function such that it is more in keeping with other event handlers. Also, the event passed in is an EventFacade. Output group 1: top 'Enabled' checkbox does not disable the Calendar input button This could be an existing issue, and if so we need to raise a new issue for it. Whitespace A few cases of trailing whitespace. Otherwise, this looks good.
          Hide
          Matt Petro added a comment -

          Hi Andrew,

          I've addressed the syntax and whitespace issues.

          The issue with the calendar popup not getting disabled is an existing problem and the fix is not obvious to me. I think it should be raised as a new issue.

          Thanks,
          -matt

          Show
          Matt Petro added a comment - Hi Andrew, I've addressed the syntax and whitespace issues. The issue with the calendar popup not getting disabled is an existing problem and the fix is not obvious to me. I think it should be raised as a new issue. Thanks, -matt
          Hide
          Andrew Nicols added a comment -

          Thanks Matt,

          Sorry for not doing anything with this sooner. I hadn't realised it was up for PR again.

          So as not to delay things further, I've rebased against this week's master and pushed (this preserves your authorship of course).

          Submitting for integration review now.

          Show
          Andrew Nicols added a comment - Thanks Matt, Sorry for not doing anything with this sooner. I hadn't realised it was up for PR again. So as not to delay things further, I've rebased against this week's master and pushed (this preserves your authorship of course). Submitting for integration review now.
          Hide
          Andrew Nicols added a comment -

          Must create an issue to track the calendar not disabled bug discovered here. Battery about to die otherwise I'd do it now.

          Show
          Andrew Nicols added a comment - Must create an issue to track the calendar not disabled bug discovered here. Battery about to die otherwise I'd do it now.
          Hide
          Dan Poltawski added a comment -

          Hey Andrew - reminder!

          Must create an issue to track the calendar not disabled bug discovered here. Battery about to die otherwise I'd do it now.

          Show
          Dan Poltawski added a comment - Hey Andrew - reminder! Must create an issue to track the calendar not disabled bug discovered here. Battery about to die otherwise I'd do it now.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks Matt!

          Show
          Dan Poltawski added a comment - Integrated to master, thanks Matt!
          Hide
          Andrew Davis added a comment -

          Clearing the text in the first textfield should disable the Basic and Array groups.
          Adding text to the second textfield should disable the Basic and Array groups.

          One oddity with this. It appears that we're only enabling/disabling on loss of focus. Makes it a little bit odd. You type and nothing happens and you have to click away to get anything to happen.

          Show
          Andrew Davis added a comment - Clearing the text in the first textfield should disable the Basic and Array groups. Adding text to the second textfield should disable the Basic and Array groups. One oddity with this. It appears that we're only enabling/disabling on loss of focus. Makes it a little bit odd. You type and nothing happens and you have to click away to get anything to happen.
          Hide
          Andrew Davis added a comment - - edited

          Another oddity I noticed (although on further investigation this was NOT introduced by this issue). Using Clean in Chrome checkboxes are grey the same as disabled controls. They're not actually disabled. They just look like they are. https://tracker.moodle.org/secure/attachment/33638/ChromeCheckbox.png

          Anyhow, passing.

          Show
          Andrew Davis added a comment - - edited Another oddity I noticed (although on further investigation this was NOT introduced by this issue). Using Clean in Chrome checkboxes are grey the same as disabled controls. They're not actually disabled. They just look like they are. https://tracker.moodle.org/secure/attachment/33638/ChromeCheckbox.png Anyhow, passing.
          Hide
          Andrew Davis added a comment -

          Oh, tested in Firefox and Chrome.

          Show
          Andrew Davis added a comment - Oh, tested in Firefox and Chrome.
          Hide
          Damyon Wiese added a comment -

          Thanks again for another week of fixes, improvements and testing. These changes have been released to the world.

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Thanks again for another week of fixes, improvements and testing. These changes have been released to the world. Cheers, Damyon
          Hide
          Ryan Foster added a comment -

          Is there any possibility of backporting this to the 2.4 branch as previously discussed in several comments? I'm working with a 2.4.x site that just encountered this problem. Dropping in the form.js attached here seems to have solved this problem for us. Thanks!

          Show
          Ryan Foster added a comment - Is there any possibility of backporting this to the 2.4 branch as previously discussed in several comments? I'm working with a 2.4.x site that just encountered this problem. Dropping in the form.js attached here seems to have solved this problem for us. Thanks!
          Hide
          Matt Petro added a comment -

          I don't think that will happen as 2.4 is only getting security updates now. Replacing lib/form/form.js with the one attached to this issue should work fine though.

          Show
          Matt Petro added a comment - I don't think that will happen as 2.4 is only getting security updates now. Replacing lib/form/form.js with the one attached to this issue should work fine though.

            People

            • Votes:
              17 Vote for this issue
              Watchers:
              22 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: