Moodle
  1. Moodle
  2. MDL-30839

form validation and error recovery

    Details

    • Story Points:
      20
    • Rank:
      51651
    • Sprint:
      FRONTEND Sprint 3

      Description

      A better mechanism needs to be used to help users identify errors in form submissions. The text that appears on the page after an invalid submission is not easily found by screen reader users. There are several techniques for improving error recovery in forms. The basic premise is to
      1. either automatically set the focus to the error message (usually used server-side validation) or use ARIA attributes to notify the user of an error message (usually used client-side validation)
      2. provide enough detail in the error message so the user knows what the error was
      3. provide links within the context of the error message to allow users to easily jump to the parts of the form where errors occurred

      WebAIM's tutorial on Error Recovery in Forms
      http://webaim.org/techniques/formvalidation/#error

      W3C Technique for "Creating a mechanism that allows users to jump to errors"
      http://www.w3.org/TR/WCAG20-TECHS/G139.html

        Issue Links

          Activity

          Hide
          Nadav Kavalerchik added a comment -

          The non validated field could be Highlighted, for example:
          theme/base/style/core.css

           
          .error input[type=text] {background-color: #fafad2;}
          
          Show
          Nadav Kavalerchik added a comment - The non validated field could be Highlighted, for example: theme/base/style/core.css .error input[type=text] {background-color: #fafad2;}
          Hide
          Nadav Kavalerchik added a comment -

          And we can scroll the page directly to the problematic element too:

          --- moodle/lib/formslib.php	(revision )
          +++ moodle/lib/formslib.php	(revision )
          @@ -1804,6 +1804,7 @@
                 errorSpan.id = \'id_error_\'+element.name;
                 errorSpan.className = "error";
                 element.parentNode.insertBefore(errorSpan, element.parentNode.firstChild);
          +      document.getElementById(errorSpan.id).scrollIntoView();
               }
           
               while (errorSpan.firstChild) {
          
          Show
          Nadav Kavalerchik added a comment - And we can scroll the page directly to the problematic element too: --- moodle/lib/formslib.php (revision ) +++ moodle/lib/formslib.php (revision ) @@ -1804,6 +1804,7 @@ errorSpan.id = \'id_error_\'+element.name; errorSpan.className = "error" ; element.parentNode.insertBefore(errorSpan, element.parentNode.firstChild); + document.getElementById(errorSpan.id).scrollIntoView(); } while (errorSpan.firstChild) {
          Hide
          Nadav Kavalerchik added a comment -

          attaching "scroll non valid element into view" patch

          Show
          Nadav Kavalerchik added a comment - attaching "scroll non valid element into view" patch
          Hide
          Nadav Kavalerchik added a comment -

          Dan,
          What do you think about the suggested CSS rule and the JS code? (Can we push it into 2.4dev?)

          Show
          Nadav Kavalerchik added a comment - Dan, What do you think about the suggested CSS rule and the JS code? (Can we push it into 2.4dev?)
          Hide
          Nadav Kavalerchik added a comment - - edited

          How about a nicer styling for the "There are required fields in this form marked" notice at the end of each Form page:
          (Should go into: theme/base/style/core.css)

          .mform .fdescription.required {
              padding: 7px;
              background-color: #FFF0F0;
              text-align: center;
              border-radius: 5px;
              width: 40%;
              margin: 15px auto;
          }
          

          See attached screen capture for applying the above style outcome.

          Show
          Nadav Kavalerchik added a comment - - edited How about a nicer styling for the "There are required fields in this form marked" notice at the end of each Form page: (Should go into: theme/base/style/core.css) .mform .fdescription.required { padding: 7px; background-color: #FFF0F0; text-align: center; border-radius: 5px; width: 40%; margin: 15px auto; } See attached screen capture for applying the above style outcome.
          Hide
          Dan Poltawski added a comment -

          Adding Andrew N here, because he was working on this, so knows this better than I do right now.

          It makes sense to me Nadav, though your commit is missing a MDL number and like I say, i'm not that familiar with this problem area.

          Show
          Dan Poltawski added a comment - Adding Andrew N here, because he was working on this, so knows this better than I do right now. It makes sense to me Nadav, though your commit is missing a MDL number and like I say, i'm not that familiar with this problem area.
          Hide
          Andrew Nicols added a comment -

          I worked on this a while back but got stuck with IE compatibility. It seems that our base theme causes issues with scrollIntoView() because of the negative left margin whereby the viewport is shifted off to the right so that the left-most element on the screen is the element you're focusing on. As a result, you lose any of the side-pre blocks etc.

          I never got any further with a workaround I'm afraid.

          Show
          Andrew Nicols added a comment - I worked on this a while back but got stuck with IE compatibility. It seems that our base theme causes issues with scrollIntoView() because of the negative left margin whereby the viewport is shifted off to the right so that the left-most element on the screen is the element you're focusing on. As a result, you lose any of the side-pre blocks etc. I never got any further with a workaround I'm afraid.
          Hide
          Andrew Nicols added a comment -

          The other thing that I think it would be worthwhile looking into would be to validate forms as they're changed to give feedback as you go. The basic testing should be reasonably simple as we're already doing some of this anyway but I think we need to highlight it better. Perhaps a green tick when a field is correct, red cross if not, and greyed-out tick if it's required but not done yet, and a grey square placeholder if it's not a required element.
          The difficulties with such a solution that spring to mind are:

          • we have far too many fields on most of our forms. This could work well with MDL-30637 to hide any non-required fields
          • for fields which are required but have server-side validation, we should ideally have an background ajax call to check them
          • consistent styling for fields with different widths to prevent a waterfall-looking effect
          Show
          Andrew Nicols added a comment - The other thing that I think it would be worthwhile looking into would be to validate forms as they're changed to give feedback as you go. The basic testing should be reasonably simple as we're already doing some of this anyway but I think we need to highlight it better. Perhaps a green tick when a field is correct, red cross if not, and greyed-out tick if it's required but not done yet, and a grey square placeholder if it's not a required element. The difficulties with such a solution that spring to mind are: we have far too many fields on most of our forms. This could work well with MDL-30637 to hide any non-required fields for fields which are required but have server-side validation, we should ideally have an background ajax call to check them consistent styling for fields with different widths to prevent a waterfall-looking effect
          Hide
          Nadav Kavalerchik added a comment -

          Androw, I agree with all your (the above comment) points.
          How is developing this?

          Some interesting info:
          http://moodle.org/mod/forum/discuss.php?d=200124#p875463
          http://moodle.org/mod/forum/discuss.php?d=211454

          Regarding the ScrollIntoView, How about testing for IE and disabling this feature for it?
          (So other can enjoy the feature)

          Show
          Nadav Kavalerchik added a comment - Androw, I agree with all your (the above comment) points. How is developing this? Some interesting info: http://moodle.org/mod/forum/discuss.php?d=200124#p875463 http://moodle.org/mod/forum/discuss.php?d=211454 Regarding the ScrollIntoView, How about testing for IE and disabling this feature for it? (So other can enjoy the feature)
          Show
          Nadav Kavalerchik added a comment - Andrew, I think there is a solution for IE too: http://www.webdeveloper.com/forum/showthread.php?96394-RESOLVED-IE-s-scrollIntoView()&s=a0632c02488c973a6fd8c9fbdaf3062d&p=518772#post518772
          Hide
          Michael de Raadt added a comment -

          I've just reset this issue to Open.

          Nadav or Andrew: feel free to assign this to yourself.

          Show
          Michael de Raadt added a comment - I've just reset this issue to Open. Nadav or Andrew: feel free to assign this to yourself.
          Hide
          Michael de Raadt added a comment -

          I'm shifting this issue into a new Epic issue, where we are collecting together accessibility issues.

          Show
          Michael de Raadt added a comment - I'm shifting this issue into a new Epic issue, where we are collecting together accessibility issues.
          Hide
          Jason Fowler added a comment -

          Looking over this and the comments.

          It seems to be a great solution for everything except IE. An alternative to this is to give the error a tabindex="0" attribute, then focus() on it.

          I don't see this conflicting with anything, but I haven't examined the code surrounding it too deeply.

          In the mean time, I am going to recommend this be developed further so we can solve the problem for all browsers.

          Nadav, if you're still interested in working on a solution for this, please do, otherwise I will take over the issue on Tuesday next week, and develop a solution.

          Show
          Jason Fowler added a comment - Looking over this and the comments. It seems to be a great solution for everything except IE. An alternative to this is to give the error a tabindex="0" attribute, then focus() on it. I don't see this conflicting with anything, but I haven't examined the code surrounding it too deeply. In the mean time, I am going to recommend this be developed further so we can solve the problem for all browsers. Nadav, if you're still interested in working on a solution for this, please do, otherwise I will take over the issue on Tuesday next week, and develop a solution.
          Hide
          Nadav Kavalerchik added a comment -

          Jason Fowler, thank you for your feedback.

          I do not have an IE to test it (I am a linux user) so it's better you take over this issue and add what is necessary for it to work on IE too.

          Show
          Nadav Kavalerchik added a comment - Jason Fowler , thank you for your feedback. I do not have an IE to test it (I am a linux user) so it's better you take over this issue and add what is necessary for it to work on IE too.
          Hide
          Jason Fowler added a comment -

          Thanks Nadav, will start work on this now.

          Show
          Jason Fowler added a comment - Thanks Nadav, will start work on this now.
          Hide
          Jason Fowler added a comment -

          Andrew, all those points you've made are nice, but should probably go in to a different issue, as they are well outside the scope of this issue - some/most of them are improvements, and would prevent this issue from making it in to stable branches.

          Show
          Jason Fowler added a comment - Andrew, all those points you've made are nice, but should probably go in to a different issue, as they are well outside the scope of this issue - some/most of them are improvements, and would prevent this issue from making it in to stable branches.
          Hide
          Andrew Davis added a comment -

          This issue is missing testing instructions. I notice that there is a change to the theme "base". Include testing both Standard and Clean in the testing instructions (and check them both yourself if you haven't already).

          Will shifting focus be ok with a screen reader? I don't know that it won't but hopefully its not too disorienting.

          Once these are dealt with put this up for integration.

          Show
          Andrew Davis added a comment - This issue is missing testing instructions. I notice that there is a change to the theme "base". Include testing both Standard and Clean in the testing instructions (and check them both yourself if you haven't already). Will shifting focus be ok with a screen reader? I don't know that it won't but hopefully its not too disorienting. Once these are dealt with put this up for integration.
          Hide
          Jason Fowler added a comment -

          Thanks Andrew, yeah, shifting the focus is intentional, and screen reader users are used to this sort of thing. It will only be triggered by them leaving the field anyway, and then only if there is an error in what they have filled in, so it's to be expected. Will make sure Clean is right too before submitting for integration. And will provide precise testing instructions.

          Show
          Jason Fowler added a comment - Thanks Andrew, yeah, shifting the focus is intentional, and screen reader users are used to this sort of thing. It will only be triggered by them leaving the field anyway, and then only if there is an error in what they have filled in, so it's to be expected. Will make sure Clean is right too before submitting for integration. And will provide precise testing instructions.
          Hide
          Jason Fowler added a comment -

          I've finalised the master branch patch, and will start working on the stable branches.

          Show
          Jason Fowler added a comment - I've finalised the master branch patch, and will start working on the stable branches.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Marina Glancy added a comment -

          I did a review and quick testing, it looks fine. But I have some problems with recess on my computer so would prefer if somebody else integrates it.

          Show
          Marina Glancy added a comment - I did a review and quick testing, it looks fine. But I have some problems with recess on my computer so would prefer if somebody else integrates it.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 25 and 24 - thanks. Please could you expand the testing instructions to cover other cases than tabbing too? Since it also applies when trying to submit the form.

          Also, nice, this seems to fix a long standing problem where required fields in tinymce did not get focus. Is there a duplicate issue for that? We should have a look.

          Show
          Dan Poltawski added a comment - Integrated to master, 25 and 24 - thanks. Please could you expand the testing instructions to cover other cases than tabbing too? Since it also applies when trying to submit the form. Also, nice, this seems to fix a long standing problem where required fields in tinymce did not get focus. Is there a duplicate issue for that? We should have a look.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jason,

          While testing this I noticed the following:

          • when required field is empty, hitting tab will displayed the error message above the field. Fill the field with some input will removed the error message. However it doesn't removed the <br>. If you repeated this step multiple times, it will display blank lines on the page.
          • Other things I noticed is the focus element for save changes button. If there's an error in the form and hit the save changes button, it sets the focus on the input field instead of the error field. I think it should focus on the error field just like it occurs for tabbing.

          Could you provide some feedback for the above comments?

          Thanks.

          Show
          Rossiani Wijaya added a comment - Hi Jason, While testing this I noticed the following: when required field is empty, hitting tab will displayed the error message above the field. Fill the field with some input will removed the error message. However it doesn't removed the <br>. If you repeated this step multiple times, it will display blank lines on the page. Other things I noticed is the focus element for save changes button. If there's an error in the form and hit the save changes button, it sets the focus on the input field instead of the error field. I think it should focus on the error field just like it occurs for tabbing. Could you provide some feedback for the above comments? Thanks.
          Hide
          Jason Fowler added a comment -

          Ah, didn't notice that with the multiple errors. Thanks for pointing these out. Fail the issue, and I will see what I can do to fix this it before the end of the day.

          Show
          Jason Fowler added a comment - Ah, didn't notice that with the multiple errors. Thanks for pointing these out. Fail the issue, and I will see what I can do to fix this it before the end of the day.
          Hide
          Rossiani Wijaya added a comment -

          Thanks Jason for commenting.

          Failing issue for more improvement.

          Show
          Rossiani Wijaya added a comment - Thanks Jason for commenting. Failing issue for more improvement.
          Hide
          Jason Fowler added a comment -

          New commits added to all the branches to solve the first issue

          The second point you raised is not really a problem here. If the user leaves a field blank and clicks on the save button first (before anything else) it will focus on the error message. If the user leaves a field blank and clicks/tabs to something else then clicks the save button they will have had focus on the error message anyway, so the requirements are met.

          Show
          Jason Fowler added a comment - New commits added to all the branches to solve the first issue The second point you raised is not really a problem here. If the user leaves a field blank and clicks on the save button first (before anything else) it will focus on the error message. If the user leaves a field blank and clicks/tabs to something else then clicks the save button they will have had focus on the error message anyway, so the requirements are met.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jason,

          To clear up on the second comment, I leave the field empty and hit tab, it displays the error message and set the focus to the error message (that's correct). Then I keep hitting tab and fill other fields and hit the save button. The focus then set to field instead of the error message. I think it should focus on the error message.

          Show
          Rossiani Wijaya added a comment - Hi Jason, To clear up on the second comment, I leave the field empty and hit tab, it displays the error message and set the focus to the error message (that's correct). Then I keep hitting tab and fill other fields and hit the save button. The focus then set to field instead of the error message. I think it should focus on the error message.
          Hide
          Dan Poltawski added a comment -

          thanks, back to testing.

          Show
          Dan Poltawski added a comment - thanks, back to testing.
          Hide
          Rossiani Wijaya added a comment -

          Hi Jason,

          The patch works great on master.

          However in 2.4 and 2.5, the issue still occurs. Looking at the patch for 2.4 and 2.5, you didn't have the fix to remove the child element for linebreak.

          Also, I think it would be great to have a second opinion on setting up the focus for save button (see above comment).

          Sorry, I'm failing this issue again.

          Show
          Rossiani Wijaya added a comment - Hi Jason, The patch works great on master. However in 2.4 and 2.5, the issue still occurs. Looking at the patch for 2.4 and 2.5, you didn't have the fix to remove the child element for linebreak. Also, I think it would be great to have a second opinion on setting up the focus for save button (see above comment). Sorry, I'm failing this issue again.
          Hide
          Dan Poltawski added a comment -

          I agree with Rosie here, we should be consistent for both (sorry I should've read the message properly before)

          Show
          Dan Poltawski added a comment - I agree with Rosie here, we should be consistent for both (sorry I should've read the message properly before)
          Hide
          Jason Fowler added a comment -

          In my haste to get this fixed before leaving today I forgot to copy the second part of the fix. Will do that first thing tomorrow.

          As for the whole saving while there is already an error on the screen... that behaviour is unlikely to occur for someone using a screen reader. Not impossible. But unlikely. They would have to ignore the error message the first time and continue through to the save button. They have heard that there is an error already. You're sacrificing ease of use for non-screen reader users, in order to help a user who was stubborn enough to try and submit a form while ignoring error messages that have already been announced. That's poor UX in my opinion.

          Show
          Jason Fowler added a comment - In my haste to get this fixed before leaving today I forgot to copy the second part of the fix. Will do that first thing tomorrow. As for the whole saving while there is already an error on the screen... that behaviour is unlikely to occur for someone using a screen reader. Not impossible. But unlikely. They would have to ignore the error message the first time and continue through to the save button. They have heard that there is an error already. You're sacrificing ease of use for non-screen reader users, in order to help a user who was stubborn enough to try and submit a form while ignoring error messages that have already been announced. That's poor UX in my opinion.
          Hide
          Jason Fowler added a comment -

          The missing parts from the stable branches are now there. Sorry about that over sight.

          Show
          Jason Fowler added a comment - The missing parts from the stable branches are now there. Sorry about that over sight.
          Hide
          Jason Fowler added a comment -

          After talking to Damyon - who pointed out I would need an ARIA guideline to support my position - I have decided to just implement the change that Rossi has mentioned. Focusing on the error message when the form is submitted now.

          Show
          Jason Fowler added a comment - After talking to Damyon - who pointed out I would need an ARIA guideline to support my position - I have decided to just implement the change that Rossi has mentioned. Focusing on the error message when the form is submitted now.
          Hide
          Dan Poltawski added a comment -

          Back to testing again

          Show
          Dan Poltawski added a comment - Back to testing again
          Hide
          Jason Fowler added a comment -

          Sorry about this mess. It's been fixed, Rossi has seen the patch in action, and this should be the last problem on this issue.

          Show
          Jason Fowler added a comment - Sorry about this mess. It's been fixed, Rossi has seen the patch in action, and this should be the last problem on this issue.
          Hide
          Rossiani Wijaya added a comment -

          Thanks for the quick fix. Will re-test this once the latest patch gets integrated.

          Show
          Rossiani Wijaya added a comment - Thanks for the quick fix. Will re-test this once the latest patch gets integrated.
          Hide
          Dan Poltawski added a comment -

          Ok, integrated that for the final chance..

          Show
          Dan Poltawski added a comment - Ok, integrated that for the final chance..
          Hide
          Rossiani Wijaya added a comment -

          Thanks Dan and Jason.

          This is working as expected.

          Tested for 2.4, 2.5 and master.

          Test passed.

          Show
          Rossiani Wijaya added a comment - Thanks Dan and Jason. This is working as expected. Tested for 2.4, 2.5 and master. Test passed.
          Hide
          Damyon Wiese added a comment -

          Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

          Hurray!

          Show
          Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile