Moodle
  1. Moodle
  2. MDL-28987

Javascript validation doesn't check correct fields

    Details

    • Testing Instructions:
      Hide

      Test 1:
      1. create a lesson activity
      2. add question page for multiple choice answer
      3. Fill all the fields except one "required" field
      4. Submit form
      5. Make sure required message is display and form should not be submitted. (Try keep an eye on url, it should not change)
      6. Try submit again (2-3 times) and form should not be submitted and "required" error should be visible.
      7. Now fill all fields and try submit, form should be submitted.
      8. Create a new multichoice question and fill all fields except one "non-required" field and make sure it gets submitted.

      Test 2:
      1. Login as admin
      2. Create a new course
      3. Fill all fields, except "course summary"
      4. Make sure form should be submitted.

      Note:
      1. Check this on IE, FF, Opera and Chrome to make sure it's working fine.
      2. Make sure to test it on 20 and 21 branch as well.

      Show
      Test 1: 1. create a lesson activity 2. add question page for multiple choice answer 3. Fill all the fields except one "required" field 4. Submit form 5. Make sure required message is display and form should not be submitted. (Try keep an eye on url, it should not change) 6. Try submit again (2-3 times) and form should not be submitted and "required" error should be visible. 7. Now fill all fields and try submit, form should be submitted. 8. Create a new multichoice question and fill all fields except one "non-required" field and make sure it gets submitted. Test 2: 1. Login as admin 2. Create a new course 3. Fill all fields, except "course summary" 4. Make sure form should be submitted. Note: 1. Check this on IE, FF, Opera and Chrome to make sure it's working fine. 2. Make sure to test it on 20 and 21 branch as well.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-28987
    • Rank:
      18540

      Description

      MDL-27728 provided a way to skip over elements the formslib failed to find. While this stops the javascript dying and submitting the form erroneously, the form never performs validation on editor type elements.

      This is because editor elements we want to check are named name="<elementname>[text]". The javascript validation is only looking for <elementname>.

      My proposed fix for this is when generated the validation js, to check if the element type is 'editor' and if so, to append [text] onto the name used in the JS.

      Replication instructions:
      1. Try editing a form with a editor (one that's required).
      2. Submit the form after having filled out all required fields except a required editor field.
      3. Form should not be submitted and display error message ("Required") on the editor field.
      4. If the page POST'ed at all when submitted, client side validation failed.

      When this fails, the form does submit and the server side validation (if any) catches the missing required field(s) and prevents the form submitting.

        Issue Links

          Activity

          Hide
          Adam Olley added a comment -

          Proposed fix added to github.

          Show
          Adam Olley added a comment - Proposed fix added to github.
          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that, Adam.

          I'll pass this back to Raj.

          Show
          Michael de Raadt added a comment - Thanks for spotting that, Adam. I'll pass this back to Raj.
          Hide
          Adam Olley added a comment -

          Hi Raj,

          Further to your testing instructions, theres one more thing you can check for. As per the replication instructions in the description, hitting save/submit on a form with tinymce should not allow the page to post. The client side validation should stop it and put the 'required' text above tinymce.

          Show
          Adam Olley added a comment - Hi Raj, Further to your testing instructions, theres one more thing you can check for. As per the replication instructions in the description, hitting save/submit on a form with tinymce should not allow the page to post. The client side validation should stop it and put the 'required' text above tinymce.
          Hide
          Rajesh Taneja added a comment -

          Thanks for providing fix for this issue
          Unfortunately this fix is not complete. Editor validation should be done on changing the focus as well.
          If you can please update your branches with following changes, it will be a complete solution
          Replace

          lib/form/editor.php
          $str .= '<div><textarea id="'.$id.'" name="'.$elname.'[text]" rows="'.$rows.'" cols="'.$cols.'">';
          

          with

          lib/form/editor.php - line 207
          //Apply editor rules is required
          $editorrules = '';
          if (!is_null($this->getAttribute('onblur')) && !is_null($this->getAttribute('onchange'))) {
              $editorrules = 'onblur="'.$this->getAttribute('onblur').'" onchange="'.$this->getAttribute('onchange').'"';
          }
          $str .= '<div><textarea id="'.$id.'" name="'.$elname.'[text]" rows="'.$rows.'" cols="'.$cols.'"'.$editorrules.'>';
          

          Let me know if you want me to do this

          Show
          Rajesh Taneja added a comment - Thanks for providing fix for this issue Unfortunately this fix is not complete. Editor validation should be done on changing the focus as well. If you can please update your branches with following changes, it will be a complete solution Replace lib/form/editor.php $str .= '<div><textarea id= "'.$id.'" name= "'.$elname.'[text]" rows= "'.$rows.'" cols= "'.$cols.'" >'; with lib/form/editor.php - line 207 //Apply editor rules is required $editorrules = ''; if (!is_null($ this ->getAttribute('onblur')) && !is_null($ this ->getAttribute('onchange'))) { $editorrules = 'onblur= "'.$ this ->getAttribute('onblur').'" onchange= "'.$ this ->getAttribute('onchange').'" '; } $str .= '<div><textarea id= "'.$id.'" name= "'.$elname.'[text]" rows= "'.$rows.'" cols= "'.$cols.'" '.$editorrules.'>'; Let me know if you want me to do this
          Hide
          Adam Olley added a comment -

          I've updated my branches to reflect the change you've suggested. I had just assumed tinymce was to blame for the blur not working on it

          Show
          Adam Olley added a comment - I've updated my branches to reflect the change you've suggested. I had just assumed tinymce was to blame for the blur not working on it
          Hide
          Rajesh Taneja added a comment -

          Thanks for such a quick fix Adam

          Actually tinymce has it's own event mechanism and provide callback.
          Will open a separate bug to fix that.

          Thanks for improving Moodle.

          Show
          Rajesh Taneja added a comment - Thanks for such a quick fix Adam Actually tinymce has it's own event mechanism and provide callback. Will open a separate bug to fix that. Thanks for improving Moodle.
          Hide
          Adam Olley added a comment -

          Hmmm, looks like there's still issues. Try adding a label for example. Type some text in tinymce and hit submit. It'll warn its required, hitting submit again it then posts fine.

          Show
          Adam Olley added a comment - Hmmm, looks like there's still issues. Try adding a label for example. Type some text in tinymce and hit submit. It'll warn its required, hitting submit again it then posts fine.
          Hide
          Adam Olley added a comment -

          Adding a call to save the editor forces it to update the textarea at validation time so validation can actually occur. It feels like this would fit better elsewhere rather than a 'if element is editor' statement.

           
          diff --git a/lib/formslib.php b/lib/formslib.php
          --- a/lib/formslib.php
          +++ b/lib/formslib.php
          @@ -1765,6 +1765,9 @@ function qf_errorHandler(element, _qfMsg) {
                           '/[_\[\]]/',
                           create_function('$matches', 'return sprintf("_%2x",ord($matches[0]));'),
                           $elementName);
          +            if ($element->_type == 'editor') {
          +                $tinyjs = "
          +  if (tinyMCE.get('id_{$element->getName()}')) {
          +    tinyMCE.get('id_{$element->getName()}').save();
          +  }";
          +                array_unshift($jsArr, $tinyjs);
          +            }
          
          
          
          Show
          Adam Olley added a comment - Adding a call to save the editor forces it to update the textarea at validation time so validation can actually occur. It feels like this would fit better elsewhere rather than a 'if element is editor' statement. diff --git a/lib/formslib.php b/lib/formslib.php --- a/lib/formslib.php +++ b/lib/formslib.php @@ -1765,6 +1765,9 @@ function qf_errorHandler(element, _qfMsg) { '/[_\[\]]/', create_function('$matches', 'return sprintf("_%2x",ord($matches[0]));'), $elementName); + if ($element->_type == 'editor') { + $tinyjs = " + if (tinyMCE.get('id_{$element->getName()}')) { + tinyMCE.get('id_{$element->getName()}').save(); + }"; + array_unshift($jsArr, $tinyjs); + }
          Hide
          Adam Olley added a comment -

          Hi Rossiani,

          Note I only just now added a commit to the git repos I've supplied that handles the extra issue with tinymce updating at validation time, so if you've already grabbed a copy of these you'll need to pull down the latest change.

          Show
          Adam Olley added a comment - Hi Rossiani, Note I only just now added a commit to the git repos I've supplied that handles the extra issue with tinymce updating at validation time, so if you've already grabbed a copy of these you'll need to pull down the latest change.
          Hide
          Rossiani Wijaya added a comment -

          Hi Adam,

          Thanks for your help with this bug.

          I tested your patch plus the additional patch from your comment on 22/Aug/11 11:40 AM. The additional patch does solve the issue, however, the missing field element is not set to focus.

          Raj, could you take a look Adam's latest patch? You might have better opinion to address this.

          Show
          Rossiani Wijaya added a comment - Hi Adam, Thanks for your help with this bug. I tested your patch plus the additional patch from your comment on 22/Aug/11 11:40 AM. The additional patch does solve the issue, however, the missing field element is not set to focus. Raj, could you take a look Adam's latest patch? You might have better opinion to address this.
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that Adam,

          This will only happen, if text in tinymce is altered. As per my understanding this should be handled by tinyMCE callback validation function.
          For now, if you are keen on this, then the right place is:

          lib/formslib.php line 1709
          $tinymce = '';
          if ($element->_type == 'editor') {
              $elementName .= '[text]';
              //TODO: Should be handled by tinymce callback validation
              $tinymce = "if (tinyMCE && tinyMCE.get('id_".$element->getName()."')) {
                              tinyMCE.get('id_".$element->getName()."').save();
                          }";
          }
          
          $test[$elementName][0][] = $tinymce.$registry->getValidationScript($element, $elementName, $rule);
          

          Probably, it's a nice idea to add a tinymce callback now and make it a perfect solution. It will be Great if you can provide a tinymce callback, else I will try to work on this in next sprint (Probably after Wednesday)

          Thanks again for being pro-active Adam. Hopefully we can come-up with a full solution by next week.

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Adam, This will only happen, if text in tinymce is altered. As per my understanding this should be handled by tinyMCE callback validation function. For now, if you are keen on this, then the right place is: lib/formslib.php line 1709 $tinymce = ''; if ($element->_type == 'editor') { $elementName .= '[text]'; //TODO: Should be handled by tinymce callback validation $tinymce = " if (tinyMCE && tinyMCE.get('id_" .$element->getName()."')) { tinyMCE.get('id_ ".$element->getName()." ').save(); }"; } $test[$elementName][0][] = $tinymce.$registry->getValidationScript($element, $elementName, $rule); Probably, it's a nice idea to add a tinymce callback now and make it a perfect solution. It will be Great if you can provide a tinymce callback, else I will try to work on this in next sprint (Probably after Wednesday) Thanks again for being pro-active Adam. Hopefully we can come-up with a full solution by next week.
          Hide
          Rajesh Taneja added a comment -

          Added two check-ins to separate tinymce validation on form submission and tinymce callback for on_blur event.

          Leaving it for integrator to decide if on_blur event is expensive or not.

          Show
          Rajesh Taneja added a comment - Added two check-ins to separate tinymce validation on form submission and tinymce callback for on_blur event. Leaving it for integrator to decide if on_blur event is expensive or not.
          Hide
          Marina Glancy added a comment -

          Raj, great job!
          One small coding comment:
          To include onchange and onblur in MoodleQuickForm_editor.toHTML() the element I would use function similar to what is used in HTML_QuickForm_input.toHTML():
          '<input' . $this->_getAttrString($this->_attributes) . ' />';
          It includes all attributes and calls htmlspecialchars to escape them.

          Show
          Marina Glancy added a comment - Raj, great job! One small coding comment: To include onchange and onblur in MoodleQuickForm_editor.toHTML() the element I would use function similar to what is used in HTML_QuickForm_input.toHTML(): '<input' . $this->_getAttrString($this->_attributes) . ' />'; It includes all attributes and calls htmlspecialchars to escape them.
          Hide
          Marina Glancy added a comment -

          looks good to me Raj

          Show
          Marina Glancy added a comment - looks good to me Raj
          Hide
          Rajesh Taneja added a comment -

          Thanks Marina

          Show
          Rajesh Taneja added a comment - Thanks Marina
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Just been looking over this now and I'm going to send it back this week so that we can clean up the following minor things.

          • The blur event gets added in the wrong place. Currently every time a focus event occurs a new blur event is added to the stack. If you add a little bit of JS to alert when a blur event is fired and toggle the focus event several times you will see what I mean. This needs to be cleaned up so that only one blur event is ever added.
          • When checking the type of an element you should be using the elements getType method rather than directly accessing the _type property.
          • I don't think the editor should be checking the rule on focus, this makes it inconsistent with the existing required rule implementation which is only checked on blur and submission.

          In general I don't like the approach so far, certainly there is a better solution than hacking hardcoded tinymce checks and adjustments into formslib and the editor element. Things should be [at that level] generic and should not be editor specific. Of course that is all part of the pipe dream of multi-editor support and switching and is just how we have done things up till now.

          • The hardcoding of tinymce into the editor element (isRequiredTinymce, options['required_tinymce']) is really nasty.
          • Setting the editor name in forms lib.php ln:1715 should not be happening here, it should be occurring within JS produced by RuleRegistry (see RuleRegistry::_getJsValue). It of course would allow you to get rid of the hardcoded editor check and isRequiredTinymce check in formslib.php:getValidationScript. This would involve overriding the default rule registry and using our own, or better yet improving the default rule registry and getting it to utilise a callback within the element rather than a hardcoded switch statement. A lot of work likely and perhaps more than we want to commit to.

          Presently this gets my -1, at least until the top three points have been corrected and the possibility of moving hardcoded editor changes back to the specific editor has been discussed.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Just been looking over this now and I'm going to send it back this week so that we can clean up the following minor things. The blur event gets added in the wrong place. Currently every time a focus event occurs a new blur event is added to the stack. If you add a little bit of JS to alert when a blur event is fired and toggle the focus event several times you will see what I mean. This needs to be cleaned up so that only one blur event is ever added. When checking the type of an element you should be using the elements getType method rather than directly accessing the _type property. I don't think the editor should be checking the rule on focus, this makes it inconsistent with the existing required rule implementation which is only checked on blur and submission. In general I don't like the approach so far, certainly there is a better solution than hacking hardcoded tinymce checks and adjustments into formslib and the editor element. Things should be [at that level] generic and should not be editor specific. Of course that is all part of the pipe dream of multi-editor support and switching and is just how we have done things up till now. The hardcoding of tinymce into the editor element (isRequiredTinymce, options ['required_tinymce'] ) is really nasty. Setting the editor name in forms lib.php ln:1715 should not be happening here, it should be occurring within JS produced by RuleRegistry (see RuleRegistry::_getJsValue). It of course would allow you to get rid of the hardcoded editor check and isRequiredTinymce check in formslib.php:getValidationScript. This would involve overriding the default rule registry and using our own, or better yet improving the default rule registry and getting it to utilise a callback within the element rather than a hardcoded switch statement. A lot of work likely and perhaps more than we want to commit to. Presently this gets my -1, at least until the top three points have been corrected and the possibility of moving hardcoded editor changes back to the specific editor has been discussed. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Sam

          I have one question on point 3:

          Checking the rule on focus:
          This was intentionally done. Currently, field error will reset "required" error (if visible). This was done to avoid cases where, user clicks on any image/video insertion button (without entering the text). At this point, blur event is encountered and show error message, which is not reset after user has inserted image or link.

          Please suggest if there is any other good way to get around this issue, or is it acceptable to show error in case user has not entered text and clicked on image button?

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Sam I have one question on point 3: Checking the rule on focus: This was intentionally done. Currently, field error will reset "required" error (if visible). This was done to avoid cases where, user clicks on any image/video insertion button (without entering the text). At this point, blur event is encountered and show error message, which is not reset after user has inserted image or link. Please suggest if there is any other good way to get around this issue, or is it acceptable to show error in case user has not entered text and clicked on image button?
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the help Sam

          I have updated all branches with your feedback.
          Changes done:

          1. Removed tinymce specific code from lib/formslib.php
          2. Focus evet is not attached once.
          3. Renamed required_tinymce to required

          As per behavior on focus event. I have kept it for better user experience. I personally feel it's better to hide error then to show invalid error Let me know if you have other thoughts on it.

          Show
          Rajesh Taneja added a comment - Thanks for all the help Sam I have updated all branches with your feedback. Changes done: Removed tinymce specific code from lib/formslib.php Focus evet is not attached once. Renamed required_tinymce to required As per behavior on focus event. I have kept it for better user experience. I personally feel it's better to hide error then to show invalid error Let me know if you have other thoughts on it.
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Making you integrator of this one, will talk to you about it when you come online today.
          Thanks for making the changes Raj, it gets a tentative +1 from me now, obviously I'd still like to at somepoint tidy up the JS handling for Quickforms/Moodleforms, however it can wait till another day.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Making you integrator of this one, will talk to you about it when you come online today. Thanks for making the changes Raj, it gets a tentative +1 from me now, obviously I'd still like to at somepoint tidy up the JS handling for Quickforms/Moodleforms, however it can wait till another day. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Hello Sam and Eloy,

          I have added one more commit, to remove validation on blur event.
          Last commit will not show any validation required error till the form is submitted, saving us from the hacky validation issue.
          This was done keeping in mind MDL-13210

          Leaving it up to you to decide.

          Show
          Rajesh Taneja added a comment - Hello Sam and Eloy, I have added one more commit, to remove validation on blur event. Last commit will not show any validation required error till the form is submitted, saving us from the hacky validation issue. This was done keeping in mind MDL-13210 Leaving it up to you to decide.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well:

          1) I understand nothing (and have read it like 5 times).
          2) I hate on blur validations, so perfect if it only happens on submit.
          3) Some day we need out own formslib, there are so many cryptic things into quickforms.
          4) My +0.5 based on Sam's one mainly.

          Note: Perhaps the testing instructions need some love:

          a) They talk about "On blur" validations
          b) I'd recommend to test also one non-required editor under all browsers, just to check code does not break anything there.

          Show
          Eloy Lafuente (stronk7) added a comment - Well: 1) I understand nothing (and have read it like 5 times). 2) I hate on blur validations, so perfect if it only happens on submit. 3) Some day we need out own formslib, there are so many cryptic things into quickforms. 4) My +0.5 based on Sam's one mainly. Note: Perhaps the testing instructions need some love: a) They talk about "On blur" validations b) I'd recommend to test also one non-required editor under all browsers, just to check code does not break anything there.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Please, don't forget to tidy testing instructions:

          1) Test both 20_STABLE and 21_STABLE
          2) Test one editor with required rule and one without it
          3) As many browsers as possible.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Please, don't forget to tidy testing instructions: 1) Test both 20_STABLE and 21_STABLE 2) Test one editor with required rule and one without it 3) As many browsers as possible.
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam and Eloy,

          Testing instructions are modified to check new behavior

          Show
          Rajesh Taneja added a comment - Thanks Sam and Eloy, Testing instructions are modified to check new behavior
          Hide
          Sam Hemelryk added a comment -

          Passed, thanks

          Show
          Sam Hemelryk added a comment - Passed, thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git & cvs repositories updated with your gorgeous code. Many thanks!

          Closing and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: