Moodle
  1. Moodle
  2. MDL-31315

Warn users before moving away from modified forms

    Details

    • Testing Instructions:
      Hide

      Open the Moodle Login Page

      • Make a change to to the username
      • Hit Refresh
      • The page refreshes

      Log in to Moodle and navigate to a course

      • Choose Edit Settings
      Without any changes
      • Refresh the page (the page refreshes)
      • Hit the back button (you go back in time and space)
      • Go back to the Edit Settings Page
      • Try closing the browser window (it closes)
      • Click Save and Display (the form saves)
      Now make some changes to various fields
      • Go back to the Edit Settings Page
      • Make a change to the 'Course full name'
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • Click Save and Display (the form saves)

      Repeat the above steps using a select instead of 'Course full name'
      Repeat the above steps using a textarea (not TinyMCE) instead of 'Course full name'

      Go back to the course and create an Assignment
      • Choose Edit Settings
      Without any changes
      • Refresh the page to confirm that it refreshes when there have been no changes
      Make some changes to the TinyMCE editor
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • Click Save and Display (the form saves)
      Uploading files
      • As a Student, choose the 'Upload Files' option
      • Refresh the page to confirm that it refreshes when there have been no changes
      • Upload a file using the 'Add' button and the filepicker
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • Refresh the page and this time leave to start afresh
      • Upload a file using the drag/drop functionality (Supported browsers only - Chrome and Firefox)
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • Click 'Save Changes'
      Modify existing files
      • Choose the 'Edit these files' button
      • Refresh the page and this time leave to start afresh
      • From the drop-down on a file, choose the rename option and enter a new name
      • Upload a file using the drag/drop functionality (Supported browsers only - Chrome and Firefox)
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • Refresh the page and this time leave to start afresh
      • From the drop-down menu on a file, choose the Delete option and confirm
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • Refresh the page and this time leave to start afresh
      • Create a new directory
      • From the drop-down menu on a file, choose the Delete option and confirm
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      • Refresh the page and this time leave to start afresh
      • Add an additional files using either option
      • From the drop-down menu on a file, choose the Delete option and confirm
      • Refresh the page (you get a popup) - click to stay
      • Hit the back button (you get a popup) - click to stay
      • Try closing the browser window/tab (you get a popup) - click to stay
      Show
      Open the Moodle Login Page Make a change to to the username Hit Refresh The page refreshes Log in to Moodle and navigate to a course Choose Edit Settings Without any changes Refresh the page (the page refreshes) Hit the back button (you go back in time and space) Go back to the Edit Settings Page Try closing the browser window (it closes) Click Save and Display (the form saves) Now make some changes to various fields Go back to the Edit Settings Page Make a change to the 'Course full name' Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay Click Save and Display (the form saves) Repeat the above steps using a select instead of 'Course full name' Repeat the above steps using a textarea (not TinyMCE) instead of 'Course full name' Go back to the course and create an Assignment Choose Edit Settings Without any changes Refresh the page to confirm that it refreshes when there have been no changes Make some changes to the TinyMCE editor Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay Click Save and Display (the form saves) Uploading files As a Student, choose the 'Upload Files' option Refresh the page to confirm that it refreshes when there have been no changes Upload a file using the 'Add' button and the filepicker Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay Refresh the page and this time leave to start afresh Upload a file using the drag/drop functionality (Supported browsers only - Chrome and Firefox) Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay Click 'Save Changes' Modify existing files Choose the 'Edit these files' button Refresh the page and this time leave to start afresh From the drop-down on a file, choose the rename option and enter a new name Upload a file using the drag/drop functionality (Supported browsers only - Chrome and Firefox) Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay Refresh the page and this time leave to start afresh From the drop-down menu on a file, choose the Delete option and confirm Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay Refresh the page and this time leave to start afresh Create a new directory From the drop-down menu on a file, choose the Delete option and confirm Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay Refresh the page and this time leave to start afresh Add an additional files using either option From the drop-down menu on a file, choose the Delete option and confirm Refresh the page (you get a popup) - click to stay Hit the back button (you get a popup) - click to stay Try closing the browser window/tab (you get a popup) - click to stay
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31315-master-6
    • Rank:
      37784

      Description

      We've just had a spate of users who have uploaded files for an assignment, but not clicked the 'Save Changes' button.

      I see two potential aids to prevent this:

      • automatically save (background ajax?) when uploading; and/or
      • window.onbeforeunload to pop up a warning before closing.

      The possibility of autosave is being investigated elsewhere. This patch introduces a warning popup when navigating away from partially filled in forms.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          You could say this about any moodle form - once you start typing anything or upload a file there could be a JS warning before leaving the form in a non standard way (not clicking submit buttons).

          New forms library does not seem to have enough priority, it might be worth the time to hack all current mforms elements to register a changed state via JS in some global M.forms.changed flag and verify it when there is requested change of page url, the form buttons would be whitelisted. At the same time we could finally add the double click protection to submit buttons as a prevention for those nasty problems caused by interrupted execution of critical code.

          Show
          Petr Škoda added a comment - You could say this about any moodle form - once you start typing anything or upload a file there could be a JS warning before leaving the form in a non standard way (not clicking submit buttons). New forms library does not seem to have enough priority, it might be worth the time to hack all current mforms elements to register a changed state via JS in some global M.forms.changed flag and verify it when there is requested change of page url, the form buttons would be whitelisted. At the same time we could finally add the double click protection to submit buttons as a prevention for those nasty problems caused by interrupted execution of critical code.
          Hide
          Andrew Nicols added a comment -

          YUI3 doesn't support the onbeforeupload event: http://yuilibrary.com/projects/yui3/ticket/2528059

          Show
          Andrew Nicols added a comment - YUI3 doesn't support the onbeforeupload event: http://yuilibrary.com/projects/yui3/ticket/2528059
          Hide
          Andrew Nicols added a comment -

          The provided patch should work on any moodle form for any of the following elements:

          • input
          • select
          • textarea

          By virtue that the files area modifies a hidden element, this also then works for file uploads.

          I haven't yet worked out how to get this to work for TinyMCE editors, but I'd appreciate some thoughts on the way that I've implemented the changes thus far.

          Please note that it's not possible to do this in Opera as it doesn't support the onbeforeupload event, but it should work in other mainstream browsers including:

          • IE
          • Chrome
          • Firefox
          Show
          Andrew Nicols added a comment - The provided patch should work on any moodle form for any of the following elements: input select textarea By virtue that the files area modifies a hidden element, this also then works for file uploads. I haven't yet worked out how to get this to work for TinyMCE editors, but I'd appreciate some thoughts on the way that I've implemented the changes thus far. Please note that it's not possible to do this in Opera as it doesn't support the onbeforeupload event, but it should work in other mainstream browsers including: IE Chrome Firefox
          Hide
          Andrew Nicols added a comment -

          Petr - thanks for the thoughts.
          I hadn't thought about making this generic to every mform, but it actually turned out to be the easiest way to implement.

          Still need to look at the best way of getting TinyMCE to support this though...

          Show
          Andrew Nicols added a comment - Petr - thanks for the thoughts. I hadn't thought about making this generic to every mform, but it actually turned out to be the easiest way to implement. Still need to look at the best way of getting TinyMCE to support this though...
          Hide
          Andrew Nicols added a comment -

          As a note, I've confirmed functionality in:

          • IE9
          • Firefox
          • Chrome

          It's not possible to do this with Opera

          Show
          Andrew Nicols added a comment - As a note, I've confirmed functionality in: IE9 Firefox Chrome It's not possible to do this with Opera
          Hide
          Andrew Nicols added a comment -

          Also tested on:

          • Safari
          Show
          Andrew Nicols added a comment - Also tested on: Safari
          Hide
          Andrew Nicols added a comment -

          I'm aware that this doesn't yet cover all form elements - notably, it's specifically missing support for TinyMCE, but I'd really appreciate some feedback on the way that I've done this so far.

          Show
          Andrew Nicols added a comment - I'm aware that this doesn't yet cover all form elements - notably, it's specifically missing support for TinyMCE, but I'd really appreciate some feedback on the way that I've done this so far.
          Hide
          Andrew Nicols added a comment -

          This is really a change to formslib, not assignment. Updating the components

          Show
          Andrew Nicols added a comment - This is really a change to formslib, not assignment. Updating the components
          Hide
          Andrew Nicols added a comment - - edited

          I've added a second branch which handles TinyMCE:
          MDL-31315-master-2 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=d703595904e4485b0c8f77abc486e327fda7977d

          This commit adds a TinyMCE specific change to check for dirty editors.

          I'd ideally like to make this change editor agnostic but I suspect that it isn't possible. An alternative would be to add some way of getting a JS snippet from the editor instance as in my commit for MDL-31016, but again I suspect that this will be tricky.

          If anyone has some thoughts on this, I'd love to hear them.

          Show
          Andrew Nicols added a comment - - edited I've added a second branch which handles TinyMCE: MDL-31315 -master-2 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=d703595904e4485b0c8f77abc486e327fda7977d This commit adds a TinyMCE specific change to check for dirty editors. I'd ideally like to make this change editor agnostic but I suspect that it isn't possible. An alternative would be to add some way of getting a JS snippet from the editor instance as in my commit for MDL-31016 , but again I suspect that this will be tricky. If anyone has some thoughts on this, I'd love to hear them.
          Hide
          Kevin Wiliarty added a comment -

          Working on something similar for the files management interface I found that iOS browsers do not respond to onbeforeunload. All the same, warnings of this kind would be very welcome at our institution. They can save frustrations every time they succeed.

          Show
          Kevin Wiliarty added a comment - Working on something similar for the files management interface I found that iOS browsers do not respond to onbeforeunload. All the same, warnings of this kind would be very welcome at our institution. They can save frustrations every time they succeed.
          Hide
          Andrew Nicols added a comment -

          I am aware of some bugs with this implementation as it stands, particularly with tiny mce and files. Still looking into them but feedback on the implementation method would be good.

          Kevin, have you found a solution to the issue with iOS (and opera)?

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - I am aware of some bugs with this implementation as it stands, particularly with tiny mce and files. Still looking into them but feedback on the implementation method would be good. Kevin, have you found a solution to the issue with iOS (and opera)? Cheers, Andrew
          Hide
          Andrew Nicols added a comment -

          I found that the filemanager wasn't correctly supported, so I've moved the application logic to the M.util namespace and called the set_changed when making any relevant changes in the filemanager. This now supports any changes to files (file removal, rename, mkdir, and upload).

          This commit also incorporates a fully working tinyMCE isDirty() test to ensure that changes to a tinyMCE element are adequately checked.

          Unfortunately I don't believe that it is possible to get this working on the following platforms:

          • iOS
          • Opera

          I haven't got an Android device at my disposal for testing, but have tested on:

          • IE9
          • Chrome
          • Firefox

          I've also tested that dragging and dropping a file into the filebrowser area also gives the popup on relevant browsers with onbeforeupload support.

          Show
          Andrew Nicols added a comment - I found that the filemanager wasn't correctly supported, so I've moved the application logic to the M.util namespace and called the set_changed when making any relevant changes in the filemanager. This now supports any changes to files (file removal, rename, mkdir, and upload). This commit also incorporates a fully working tinyMCE isDirty() test to ensure that changes to a tinyMCE element are adequately checked. Unfortunately I don't believe that it is possible to get this working on the following platforms: iOS Opera I haven't got an Android device at my disposal for testing, but have tested on: IE9 Chrome Firefox I've also tested that dragging and dropping a file into the filebrowser area also gives the popup on relevant browsers with onbeforeupload support.
          Hide
          Andrew Nicols added a comment -

          Now tested on Android 2.2 (well, using VirtualBox)

          Show
          Andrew Nicols added a comment - Now tested on Android 2.2 (well, using VirtualBox)
          Hide
          Andrew Nicols added a comment -

          Added additional testing procedures for all file actions, and TinyMCE

          Show
          Andrew Nicols added a comment - Added additional testing procedures for all file actions, and TinyMCE
          Hide
          Kevin Wiliarty added a comment -

          I did not find any onbeforeunload solution for iOS and suspect from what I've read that there is unlikely to be one.

          Show
          Kevin Wiliarty added a comment - I did not find any onbeforeunload solution for iOS and suspect from what I've read that there is unlikely to be one.
          Hide
          Kevin Wiliarty added a comment -

          Your recent commit is working in the filemanager for me now.

          Show
          Kevin Wiliarty added a comment - Your recent commit is working in the filemanager for me now.
          Hide
          Andrew Nicols added a comment -

          Rebased patch on latest master

          Show
          Andrew Nicols added a comment - Rebased patch on latest master
          Hide
          Andrew Nicols added a comment -
          Show
          Andrew Nicols added a comment - Discussion on this feature at http://moodle.org/mod/forum/discuss.php?d=195026
          Hide
          Andrew Nicols added a comment -

          Screencast demonstrating how it works at http://goo.gl/Nl3y8

          Show
          Andrew Nicols added a comment - Screencast demonstrating how it works at http://goo.gl/Nl3y8
          Hide
          Sam Marshall added a comment -

          Couple questions regarding the code (which I haven't looked at sorry):

          1) Dynamic form usage - any support?

          For instance, let's say I load a form into a page dynamically and override the submit button so that it doesn't actually submit the form but instead does something fancy with AJAX when it's clicked. Then I might also reuse the same form with different content later (programmatically changing field values). Is there a JS API function like form.resetChangeMonitoring(); or form.assumeNoChange(); that I can call to make it behave as if there are no changes (ie cancel the handler) but look out for changes from that point on?

          2) Multiple forms?

          You can have two (or three, or etc) mforms on the same page, does this work OK with that? I guess there's no reason why it shouldn't, but it's something you might want to test if you haven't already.

          Show
          Sam Marshall added a comment - Couple questions regarding the code (which I haven't looked at sorry): 1) Dynamic form usage - any support? For instance, let's say I load a form into a page dynamically and override the submit button so that it doesn't actually submit the form but instead does something fancy with AJAX when it's clicked. Then I might also reuse the same form with different content later (programmatically changing field values). Is there a JS API function like form.resetChangeMonitoring(); or form.assumeNoChange(); that I can call to make it behave as if there are no changes (ie cancel the handler) but look out for changes from that point on? 2) Multiple forms? You can have two (or three, or etc) mforms on the same page, does this work OK with that? I guess there's no reason why it shouldn't, but it's something you might want to test if you haven't already.
          Hide
          Andrew Nicols added a comment -

          Sam:
          1) Dynamic form usage - any support?

          In essence, I don't see why not. This is implemented in two parts - a YUI module which adds listeners; and a set of functions in lib/javascript-static.js for changing the status (M.util.set_form_changed(), M.util.set_form_submitted()).

          If you add additional elements, you might need to find a way of adding the listener to them, but it should be relatively simple. This is how the initializer adds the listener:

          var formid = 'form#' + this.get('formid');
          Y.all(formid + ' input').on('change', M.util.set_form_changed, this);
          Y.all(formid + ' textarea').on('change', M.util.set_form_changed, this);
          Y.all(formid + ' select').on('change', M.util.set_form_changed, this);
          

          HTML Editors are handled separately in the onbeforeunload handler as they're not guaranteed to have been initalized when this module is initialized.

          2) Multiple forms?

          Yes - the YUI module can be called multiple times with different form IDs. At present this is done in the startForm() function of the form renderer so it's automatic.

          Show
          Andrew Nicols added a comment - Sam: 1) Dynamic form usage - any support? In essence, I don't see why not. This is implemented in two parts - a YUI module which adds listeners; and a set of functions in lib/javascript-static.js for changing the status (M.util.set_form_changed(), M.util.set_form_submitted()). If you add additional elements, you might need to find a way of adding the listener to them, but it should be relatively simple. This is how the initializer adds the listener: var formid = 'form#' + this .get('formid'); Y.all(formid + ' input').on('change', M.util.set_form_changed, this ); Y.all(formid + ' textarea').on('change', M.util.set_form_changed, this ); Y.all(formid + ' select').on('change', M.util.set_form_changed, this ); HTML Editors are handled separately in the onbeforeunload handler as they're not guaranteed to have been initalized when this module is initialized. 2) Multiple forms? Yes - the YUI module can be called multiple times with different form IDs. At present this is done in the startForm() function of the form renderer so it's automatic.
          Hide
          Sam Marshall added a comment -

          Thanks Andrew! To confirm for the record, those two functions sounds like exactly what is needed to make this work dynamically. (You may have guessed that my example wasn't a hypothetical one.

          Show
          Sam Marshall added a comment - Thanks Andrew! To confirm for the record, those two functions sounds like exactly what is needed to make this work dynamically. (You may have guessed that my example wasn't a hypothetical one.
          Hide
          Andrew Davis added a comment -

          This will be a big improvement

          A few little things. This comment in the initialisation function isnt quite right (I believe).

          // We need any submit buttons on the form to clear the dirty flag

          set_form_submitted() sets a second flag. It doesn't clear the dirty flag.

          In get_form_dirty_state() is the empty try catch around the tinyMCE handling to deal with querying an initialised tinyMCE instance or some other sort of error that we want to silently handle? If so, could you add a comment saying that the empty catch block is deliberate and why. Otherwise someone will come along in the future and add error outputting where it shouldn't be. And of course, if the empty catch isn't there for a good reason add error handling

          It is truly trivial but "and thare no dirty editors" in javascript-static.js should be "and there are no dirty editors"

          Is it worth reshuffling get_form_dirty_state() to use the M.cfg.form_changed and M.cfg.form_submitted flags to avoid the bother of checking tinyMCE instances?

          if (M.cfg.form_submitted) {
              return 0;
          }
          
          // Form is dirty and is not being submitted
          if (M.cfg.form_changed) {
              return 1;
          }
          
          // Handle TinyMCE editor instances 

          Maybe this isnt worth fiddling with. Maybe the difference is negligible but we want to make sure that form submission is snappy.

          Show
          Andrew Davis added a comment - This will be a big improvement A few little things. This comment in the initialisation function isnt quite right (I believe). // We need any submit buttons on the form to clear the dirty flag set_form_submitted() sets a second flag. It doesn't clear the dirty flag. In get_form_dirty_state() is the empty try catch around the tinyMCE handling to deal with querying an initialised tinyMCE instance or some other sort of error that we want to silently handle? If so, could you add a comment saying that the empty catch block is deliberate and why. Otherwise someone will come along in the future and add error outputting where it shouldn't be. And of course, if the empty catch isn't there for a good reason add error handling It is truly trivial but "and thare no dirty editors" in javascript-static.js should be "and there are no dirty editors" Is it worth reshuffling get_form_dirty_state() to use the M.cfg.form_changed and M.cfg.form_submitted flags to avoid the bother of checking tinyMCE instances? if (M.cfg.form_submitted) { return 0; } // Form is dirty and is not being submitted if (M.cfg.form_changed) { return 1; } // Handle TinyMCE editor instances Maybe this isnt worth fiddling with. Maybe the difference is negligible but we want to make sure that form submission is snappy.
          Hide
          Andrew Nicols added a comment -

          Hi Andrew,

          Thanks for the feedback. I've made the changes you've suggested:

          • comments corrected - the original workflow meant that the submit button cleared the dirty flag rather than setting it's own
          • the try/catch was there to handle pages without any tinyMCE editors - if there aren't any editors, the tinyMCE variable does not exist. I've changed this to use an if (typeOf tinyMCE != 'undefined') so as to avoid over-catching any tinyMCE errors
          • I've reshuffled as you suggested. This also means that it's possible to return dirty on the first dirty editor rather than waiting until the end improving the performance further

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Andrew, Thanks for the feedback. I've made the changes you've suggested: comments corrected - the original workflow meant that the submit button cleared the dirty flag rather than setting it's own the try/catch was there to handle pages without any tinyMCE editors - if there aren't any editors, the tinyMCE variable does not exist. I've changed this to use an if (typeOf tinyMCE != 'undefined') so as to avoid over-catching any tinyMCE errors I've reshuffled as you suggested. This also means that it's possible to return dirty on the first dirty editor rather than waiting until the end improving the performance further Cheers, Andrew
          Hide
          Andrew Nicols added a comment -

          I also just noticed that the admin forms don't use the standard formslib like in the rest of Moodle (spent ages trying to work out how I'd broken my JS)
          I've added an instantiation for the adminsettings form in admin/settings.php which should cover all adminsettings forms.

          Show
          Andrew Nicols added a comment - I also just noticed that the admin forms don't use the standard formslib like in the rest of Moodle (spent ages trying to work out how I'd broken my JS) I've added an instantiation for the adminsettings form in admin/settings.php which should cover all adminsettings forms.
          Hide
          Andrew Nicols added a comment -

          Resubmitting for peer review with latest batch of changes

          Show
          Andrew Nicols added a comment - Resubmitting for peer review with latest batch of changes
          Hide
          Andrew Nicols added a comment -

          Ahem - helps to put the link to the diff and not the branch

          Show
          Andrew Nicols added a comment - Ahem - helps to put the link to the diff and not the branch
          Hide
          Andrew Davis added a comment -

          Looks good

          Show
          Andrew Davis added a comment - Looks good
          Hide
          Andrew Nicols added a comment -

          This should probably only go into 2.3

          Show
          Andrew Nicols added a comment - This should probably only go into 2.3
          Hide
          David Price added a comment - - edited

          We've been testing your patches on our production system. No problems, working perfectly, great job.
          You already posted a YouTube video to demonstrate the functionality but in case it helps we made this to explain to our students:- http://bishopshalt.co.uk/demonstrate_close_window.flv

          Show
          David Price added a comment - - edited We've been testing your patches on our production system. No problems, working perfectly, great job. You already posted a YouTube video to demonstrate the functionality but in case it helps we made this to explain to our students:- http://bishopshalt.co.uk/demonstrate_close_window.flv
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now.

          I've spent a bit of time exploring you solution here. To begin with I was in two minds about the approach however I've come around to how this has been done and as such in it went

          There are a couple of things I'd like to get your opinion on however, while looking at the code and how things functioned these are things that struck me:

          • The name formslib doesn't really suit the purpose of the introduced code. M.core.init_formslib doesn't really describe what this code is doing and could probably be better named. Something like formchangeprotection or something.
          • I wasn't entirely sold on the idea of attaching individual events to the input/textarea/select items within a form. It works I can't knock that, however I wonder whether a value comparison method would be better. e.g. read in the value of the fields at the start, and then compare it during the unload event to see whether it has changed. Event handling can be a little slow when there are many events on a page so perhaps a win for performance. This would also solve the situation that arises when a user enters something into a field, removes it, and then leaves the page. As part of my exploration of this issue I prototyped this without problems.
          • It would be great to remove the new JS from javascript-static if possible. I couldn't think of an simple solution to this however. One idea I did have would be to move the new functions to the new YUI module formslib (again renaming would be nice) and then use an event model to track changes. So the new functions would listen for a generic custom event we define and other areas of code such as filepicker, filemanager, tinyMCE etc would just need to fire said event. But I'm in two minds about that being more complicated than the advantage provided.

          Anyway food for thought there more than anything. Feel free to create issues if you think any of those ideas have merit.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now. I've spent a bit of time exploring you solution here. To begin with I was in two minds about the approach however I've come around to how this has been done and as such in it went There are a couple of things I'd like to get your opinion on however, while looking at the code and how things functioned these are things that struck me: The name formslib doesn't really suit the purpose of the introduced code. M.core.init_formslib doesn't really describe what this code is doing and could probably be better named. Something like formchangeprotection or something. I wasn't entirely sold on the idea of attaching individual events to the input/textarea/select items within a form. It works I can't knock that, however I wonder whether a value comparison method would be better. e.g. read in the value of the fields at the start, and then compare it during the unload event to see whether it has changed. Event handling can be a little slow when there are many events on a page so perhaps a win for performance. This would also solve the situation that arises when a user enters something into a field, removes it, and then leaves the page. As part of my exploration of this issue I prototyped this without problems. It would be great to remove the new JS from javascript-static if possible. I couldn't think of an simple solution to this however. One idea I did have would be to move the new functions to the new YUI module formslib (again renaming would be nice) and then use an event model to track changes. So the new functions would listen for a generic custom event we define and other areas of code such as filepicker, filemanager, tinyMCE etc would just need to fire said event. But I'm in two minds about that being more complicated than the advantage provided. Anyway food for thought there more than anything. Feel free to create issues if you think any of those ideas have merit. Cheers Sam
          Hide
          Andrew Nicols added a comment -

          Hi Sam,

          The name probably should be changed. My first thought was that this library could eventually replace some of the other formslib javascript (e.g. the onSubmit checks) but I've since changed my mind a little on that one - it would be pretty tricky to get it to play nicely with the various forms, though it may still be a longer-term aim. Would you like me to submit a new patch against this bug so you can revert + reapply, or submit as a new bug for next week's integration?

          I also considered storing the value of the fields at the start, and comparing them at the end, but I was concerned that this may have a greater impact on performance. If a user is editing a large form (e.g. a Multiple Choice Question) which has multiple editors (minimum of 15 in this example), and the form is already filled in with a large amount of data, then the amount of data stored in variables will be pretty large and bloat the memory consumption for the window. This can have quite a detrimental effect on a user's browsing in other ways.
          Conversely, adding the onChange handler isn't a massive burden, especially given that it's added after the page has loaded and there's no visible change to the user. The function onChange is very basic so shouldn't slow the browser down then, and the check at the end is also pretty basic.
          I agree that it would be nice to not display a warning if the user has made a change, and then returned the setting back to it's original value.
          One method would be to set the initial value in an onFocus event. Then in the onSubmit check compare the current value with the original value. This would still bloat the browser memory consumption, but only for the fields which were focussed. You could also have an onChange or onBlur handler which unsets fields which were focussed, but not changed (or subsequently unchanged). However, you still have to do a comparison at the end of the page, which has the potential to be quite slow. I'd be interested to see your prototype to see how it compares. It might be worth putting some benchmarks together on jsperf.com or something, though it would probably be pretty difficult to write anything meaningful.

          WRT the location in javascript-static, I originally wrote everything in the YUI module but found that the onbeforeunload event wasn't capable of accessing the YUI module context (e.g. for the get_form_dirty_state function). As a result, I moved the get_form_dirty_state function and the report_form_dirty_state function out, and it then made sense to move the others out too. YUI3 doesn't actually support onbeforeunload (see ahttp://yuilibrary.com/projects/yui3/ticket/2528059 for more information).
          Having some areas of code firing events would be a great way of doing it though, and would probably solve the issues with non-TinyMCE editors. The benefit of checking the TinyMCE editor in the get_form_dirty_state function has the benefit that any changes made but then unmade will not mark the editor as Dirty.

          Let me know about the rename and I'd be interested to have your thoughts on the other items.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Sam, The name probably should be changed. My first thought was that this library could eventually replace some of the other formslib javascript (e.g. the onSubmit checks) but I've since changed my mind a little on that one - it would be pretty tricky to get it to play nicely with the various forms, though it may still be a longer-term aim. Would you like me to submit a new patch against this bug so you can revert + reapply, or submit as a new bug for next week's integration? I also considered storing the value of the fields at the start, and comparing them at the end, but I was concerned that this may have a greater impact on performance. If a user is editing a large form (e.g. a Multiple Choice Question) which has multiple editors (minimum of 15 in this example), and the form is already filled in with a large amount of data, then the amount of data stored in variables will be pretty large and bloat the memory consumption for the window. This can have quite a detrimental effect on a user's browsing in other ways. Conversely, adding the onChange handler isn't a massive burden, especially given that it's added after the page has loaded and there's no visible change to the user. The function onChange is very basic so shouldn't slow the browser down then, and the check at the end is also pretty basic. I agree that it would be nice to not display a warning if the user has made a change, and then returned the setting back to it's original value. One method would be to set the initial value in an onFocus event. Then in the onSubmit check compare the current value with the original value. This would still bloat the browser memory consumption, but only for the fields which were focussed. You could also have an onChange or onBlur handler which unsets fields which were focussed, but not changed (or subsequently unchanged). However, you still have to do a comparison at the end of the page, which has the potential to be quite slow. I'd be interested to see your prototype to see how it compares. It might be worth putting some benchmarks together on jsperf.com or something, though it would probably be pretty difficult to write anything meaningful. WRT the location in javascript-static, I originally wrote everything in the YUI module but found that the onbeforeunload event wasn't capable of accessing the YUI module context (e.g. for the get_form_dirty_state function). As a result, I moved the get_form_dirty_state function and the report_form_dirty_state function out, and it then made sense to move the others out too. YUI3 doesn't actually support onbeforeunload (see ahttp://yuilibrary.com/projects/yui3/ticket/2528059 for more information). Having some areas of code firing events would be a great way of doing it though, and would probably solve the issues with non-TinyMCE editors. The benefit of checking the TinyMCE editor in the get_form_dirty_state function has the benefit that any changes made but then unmade will not mark the editor as Dirty. Let me know about the rename and I'd be interested to have your thoughts on the other items. Andrew
          Hide
          David Price added a comment -

          One thing I've noticed:-

          1. Add an advanced uploading of files activity
          2. Leave everything as default
          3. Log in as a student and upload a file
          4. Log back in as a teacher/administrator and "view 1 submitted assignment"
          5. Click on "Grade"
          6. Click on the file that the student uploaded. Opens/Downloads fine - no message.
          7. In the feedback area type something, but apply some formatting eg strikethrough, text colour, text background colour or text alignment. Save changes
          8. Status has changed from "Grade" to "Update". Click on Update
          9. Before clicking on anything else, or making any changes, click on the uploaded file (like a teacher would do if about to mark it).
          10. Changes have incorrectly been detected and the "Are you sure you want to leave this page?" message appears.
          11. Use the html button on TinyMCE editor to remove any trace of feedback (or leave some text but remove all formatting).
          12. Save Changes, Update again, click on the uploaded file and it downloads fine now, with no message.

          I was able to reproduce this problem reliably with a new assignment, using Chrome or Firefox and the steps above. IE9 seems to respond differently. I experienced the problem with IE9 yesterday with existing assignments with lots of uploaded files and extensive feedback, but I'm struggling to reproduce it from scratch.
          It may seem like an obscure scenario but it arose quite quickly in normal use after applying the changes. Our teachers use various html formatting in the assignment feedback areas and it seems to be something to do with that.

          If any of the teachers at our centre complain about this I'll just tell them to click on "Leave this page" (it doesn't actually navigate away of course, only downloads the file). This glitch is a small price to pay IMO for the crucial new functionality for students, but I just wanted to highlight it in case it hadn't been noticed...

          Hope this helps

          Show
          David Price added a comment - One thing I've noticed:- 1. Add an advanced uploading of files activity 2. Leave everything as default 3. Log in as a student and upload a file 4. Log back in as a teacher/administrator and "view 1 submitted assignment" 5. Click on "Grade" 6. Click on the file that the student uploaded. Opens/Downloads fine - no message. 7. In the feedback area type something, but apply some formatting eg strikethrough, text colour, text background colour or text alignment. Save changes 8. Status has changed from "Grade" to "Update". Click on Update 9. Before clicking on anything else, or making any changes, click on the uploaded file (like a teacher would do if about to mark it). 10. Changes have incorrectly been detected and the "Are you sure you want to leave this page?" message appears. 11. Use the html button on TinyMCE editor to remove any trace of feedback (or leave some text but remove all formatting). 12. Save Changes, Update again, click on the uploaded file and it downloads fine now, with no message. I was able to reproduce this problem reliably with a new assignment, using Chrome or Firefox and the steps above. IE9 seems to respond differently. I experienced the problem with IE9 yesterday with existing assignments with lots of uploaded files and extensive feedback, but I'm struggling to reproduce it from scratch. It may seem like an obscure scenario but it arose quite quickly in normal use after applying the changes. Our teachers use various html formatting in the assignment feedback areas and it seems to be something to do with that. If any of the teachers at our centre complain about this I'll just tell them to click on "Leave this page" (it doesn't actually navigate away of course, only downloads the file). This glitch is a small price to pay IMO for the crucial new functionality for students, but I just wanted to highlight it in case it hadn't been noticed... Hope this helps
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for the reply, in regards to the naming I think it would be fine to just create a new issue for it fix it during next weeks integration. As this is master only there is no harm in that.

          In regards to storing values for comparison I think you are spot on, the implications it would have on memory would be a real problem and given JS doesn't have native support for md5 hashes I think that we are certainly best to stick with what you are doing at the moment.
          The change event is likely the best event to use, I'd be cautious about using the focus event as JS widegets like the datetime_selector's calendar are unlikely to trigger it.
          As for my prototype unfortunately I didn't take a snapshot of it, it's quite common for me to hack around with solutions during a review to test out alternative paths and explore hard to reach functionality and I'm not in a habit of taking snapshots so things just get blown away when I move onto the next issue (perhaps a lesson to learn with the likes of this issue).
          The following is what I was doing:
          First up that I put all new instances of formslib objects into an array for later reference:

          M.core.formslib = M.core.formslib || [];
          M.core.formslib.push(new FORMSLIB(config));
          

          Next within the initialisation I commented out the onchange event and added the following

          Y.one(formid).get('elements').each(this.record_field, this);
          

          The new method record_field belongs to the formslib object, and requires a new property fields.

          record_field = function(field) {
              this.fields.push({'element':field,'value':field.get('value')});
          }
          

          The two bits of code go through each element in the form during initialisation and create an object containing the initial value of the field as well as a reference to the field.
          Then within the unload event I was doing something like the following:

          for (var i in M.core.formslib) {
              var form = M.core.formslib[i];
              for (var j in form.fields) {
                  var field = form.fields[j];
                  if (field.element.get('value') != field.value) {
                      return "There was a change";
                  }
              }
          }
          return false;
          

          And that is pretty much it, obviously the editor still needs to be checked and I left the set_form_changed function and checks in there so that it filepicker etc could still influence it.

          As for the location of the JS, moving it out of javascript-static it is a nice idea but there is no urgency on it. Since the release of 2.0 we've avoided adding code there unless there is no where better to put it, or time doesn't permit developing (or redeveloping) a solution that has it elsewhere.
          The use of events would be one such alternative solution, although it would need more thought.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for the reply, in regards to the naming I think it would be fine to just create a new issue for it fix it during next weeks integration. As this is master only there is no harm in that. In regards to storing values for comparison I think you are spot on, the implications it would have on memory would be a real problem and given JS doesn't have native support for md5 hashes I think that we are certainly best to stick with what you are doing at the moment. The change event is likely the best event to use, I'd be cautious about using the focus event as JS widegets like the datetime_selector's calendar are unlikely to trigger it. As for my prototype unfortunately I didn't take a snapshot of it, it's quite common for me to hack around with solutions during a review to test out alternative paths and explore hard to reach functionality and I'm not in a habit of taking snapshots so things just get blown away when I move onto the next issue (perhaps a lesson to learn with the likes of this issue). The following is what I was doing: First up that I put all new instances of formslib objects into an array for later reference: M.core.formslib = M.core.formslib || []; M.core.formslib.push( new FORMSLIB(config)); Next within the initialisation I commented out the onchange event and added the following Y.one(formid).get('elements').each( this .record_field, this ); The new method record_field belongs to the formslib object, and requires a new property fields. record_field = function(field) { this .fields.push({'element':field,'value':field.get('value')}); } The two bits of code go through each element in the form during initialisation and create an object containing the initial value of the field as well as a reference to the field. Then within the unload event I was doing something like the following: for ( var i in M.core.formslib) { var form = M.core.formslib[i]; for ( var j in form.fields) { var field = form.fields[j]; if (field.element.get('value') != field.value) { return "There was a change" ; } } } return false ; And that is pretty much it, obviously the editor still needs to be checked and I left the set_form_changed function and checks in there so that it filepicker etc could still influence it. As for the location of the JS, moving it out of javascript-static it is a nice idea but there is no urgency on it. Since the release of 2.0 we've avoided adding code there unless there is no where better to put it, or time doesn't permit developing (or redeveloping) a solution that has it elsewhere. The use of events would be one such alternative solution, although it would need more thought. Cheers Sam
          Hide
          Adrian Greeve added a comment -

          As I was working my way through the test instructions I found that if I just altered the course full name and then hit reset, or back, or tried to close the window, it would not display the dialogue box asking if I really wanted to navigate away from the page.
          I had the same problem when altering the course short name, but if I make changes to the Course summary then it will start working.
          Talking with Sam he suggested that I put an alert into the function M.util.set_form_changed (line1760) to see if it is firing. It isn't so he suspects that it's a problem with formlib JS.

          I tested this on a few different machines using firefox and chrome, but ended up with the same result.

          The rest of the test worked fine and I didn't find any other problems.

          Show
          Adrian Greeve added a comment - As I was working my way through the test instructions I found that if I just altered the course full name and then hit reset, or back, or tried to close the window, it would not display the dialogue box asking if I really wanted to navigate away from the page. I had the same problem when altering the course short name, but if I make changes to the Course summary then it will start working. Talking with Sam he suggested that I put an alert into the function M.util.set_form_changed (line1760) to see if it is firing. It isn't so he suspects that it's a problem with formlib JS. I tested this on a few different machines using firefox and chrome, but ended up with the same result. The rest of the test worked fine and I didn't find any other problems.
          Hide
          Aparup Banerjee added a comment - - edited

          hello,
          while testing MDL-31114 i'm getting confirmation popups (see attached screenshots) even when i haven't changed any character data on the form fields. I note i have changed the form but only the file.
          i understand that i wouldn't normally want to download stuff here. but something weird is going on. very un-intuitive. i i should be able to download, why would i want to navigate away from the page - especially whilst editing?

          perhaps this is a separate issue, but i've only noticed it here in integration with the current patch applied. (noting its failed atm)

          Show
          Aparup Banerjee added a comment - - edited hello, while testing MDL-31114 i'm getting confirmation popups (see attached screenshots) even when i haven't changed any character data on the form fields. I note i have changed the form but only the file. i understand that i wouldn't normally want to download stuff here. but something weird is going on. very un-intuitive. i i should be able to download, why would i want to navigate away from the page - especially whilst editing? perhaps this is a separate issue, but i've only noticed it here in integration with the current patch applied. (noting its failed atm)
          Hide
          Andrew Nicols added a comment -

          Aparup,

          I'm a little confused as to the problem you're describing - your screenshot shows a new forum discussion topic with no subject or message, but it does have a file attachment. I've considered the filepicker to be a form element so in your example I would expect the browser to throw up a warning if you've uploaded a file. This is actually one of the main points of adding this warning - to warn users when they have uploaded a file but not saved their changes. We've seen this a lot with users uploading files to assignments (where no other form element is present) and not saving changes but thinking that they have submitted their work for assessment.

          Show
          Andrew Nicols added a comment - Aparup, I'm a little confused as to the problem you're describing - your screenshot shows a new forum discussion topic with no subject or message, but it does have a file attachment. I've considered the filepicker to be a form element so in your example I would expect the browser to throw up a warning if you've uploaded a file. This is actually one of the main points of adding this warning - to warn users when they have uploaded a file but not saved their changes. We've seen this a lot with users uploading files to assignments (where no other form element is present) and not saving changes but thinking that they have submitted their work for assessment.
          Hide
          Andrew Nicols added a comment -

          Adrian,

          I'm guessing that you're not actually losing the focus from a form element before resetting, hitting back, or closing the window?
          From what I can tell, the onchange handler is only fired on the element blur for most browsers so the set_form_changed function is only being called when a user has finished changing some content. I'm not sure what we can do about this, short of the suggestions sam and I discussed above where we compare the original element value, and the value when trying to move away, but this has potential memory footprint issues.

          Show
          Andrew Nicols added a comment - Adrian, I'm guessing that you're not actually losing the focus from a form element before resetting, hitting back, or closing the window? From what I can tell, the onchange handler is only fired on the element blur for most browsers so the set_form_changed function is only being called when a user has finished changing some content. I'm not sure what we can do about this, short of the suggestions sam and I discussed above where we compare the original element value, and the value when trying to move away, but this has potential memory footprint issues.
          Hide
          Adrian Greeve added a comment -

          Hi Andrew.
          I had another look this morning and you're right. Once you move out of the text box the page will recognise that a change has been made and the function will work. I'm happy with how this patch is working. I'm creating a new issue for checking if the text box has been altered without losing focus, and that can be looked at some other stage.
          Thanks.

          Show
          Adrian Greeve added a comment - Hi Andrew. I had another look this morning and you're right. Once you move out of the text box the page will recognise that a change has been made and the function will work. I'm happy with how this patch is working. I'm creating a new issue for checking if the text box has been altered without losing focus, and that can be looked at some other stage. Thanks.
          Hide
          Sam Hemelryk added a comment -

          Hi all,

          I've reset this now and am marking it passed.
          I think it is widely agreed that this is a really good improvement and having talked to Adrian and Apu we've decided everyone is happy with it.
          The concerns people have will now be turned into bugs so that we can continue improving this before the release of 2.3.
          Thanks Andrew for the work and thanks everyone for participating.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi all, I've reset this now and am marking it passed. I think it is widely agreed that this is a really good improvement and having talked to Adrian and Apu we've decided everyone is happy with it. The concerns people have will now be turned into bugs so that we can continue improving this before the release of 2.3. Thanks Andrew for the work and thanks everyone for participating. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Hi Andrew,
          I've created MDL-31656 to explain this edge case (hopefully more clearly too).

          This patch here is a much better improvement than the usability issue i'm seeing. (ie: download + 'stay on page' = no download.)

          Show
          Aparup Banerjee added a comment - Hi Andrew, I've created MDL-31656 to explain this edge case (hopefully more clearly too). This patch here is a much better improvement than the usability issue i'm seeing. (ie: download + 'stay on page' = no download.)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

          Closing as fixed, heading to zzzZZZzzz, niao

          Show
          Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

            People

            • Votes:
              11 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: