Moodle
  1. Moodle
  2. MDL-27843 META: Accessibility compliance for 2.x
  3. MDL-30845

read-only text input fields rendered as plain text and not as a read-only input field

    Details

    • Testing Instructions:
      Hide

      Config changes:-

      1. Grant a user following permissions
        ->moodle/course:changefullname
        ->moodle/course:update
      2. Make sure the user doesnt have the following permissions
        Change course ID numbermoodle/course:changeidnumber
        Change course short namemoodle/course:changeshortname
        Change course summarymoodle/course:changesummary

      Test:-

      1. you can set above in a number of ways I just changed the student role from site admin>users>perm>define roles
      2. Now loging as this user and go to a course>edit
      3. Make sure you dont see a static content for fields course id and course short name. Instead its a input field with readonly attribute
      4. Hack the form using a tool of your choice and change the value to something other than the existing one.
      5. submit the form, make sure the hacked value is not submited. (you can print the $data = $editform->get_data() in edit.php to view submitted values)

      Exploratory Testing
      Test as many forms as possible in Moodle. All areas which use mforms widely should be tested. It needs to be tested how JS dependencies are working with frozen and Hardfrozen elements(text ones) as well as how frozen elements behave in general and no regressions are caused.

      Things to test include trying to update the field as role that do not have permissions to do so, selecting/deselecting any checkboxes, testing with javascript turned on and off and removing the read only flag using Firebug always submitting the form to ensuring there are no errors generated.

      Show
      Config changes:- Grant a user following permissions ->moodle/course:changefullname ->moodle/course:update Make sure the user doesnt have the following permissions Change course ID numbermoodle/course:changeidnumber Change course short namemoodle/course:changeshortname Change course summarymoodle/course:changesummary Test:- you can set above in a number of ways I just changed the student role from site admin>users>perm>define roles Now loging as this user and go to a course>edit Make sure you dont see a static content for fields course id and course short name. Instead its a input field with readonly attribute Hack the form using a tool of your choice and change the value to something other than the existing one. submit the form, make sure the hacked value is not submited. (you can print the $data = $editform->get_data() in edit.php to view submitted values) Exploratory Testing Test as many forms as possible in Moodle. All areas which use mforms widely should be tested. It needs to be tested how JS dependencies are working with frozen and Hardfrozen elements(text ones) as well as how frozen elements behave in general and no regressions are caused. Things to test include trying to update the field as role that do not have permissions to do so, selecting/deselecting any checkboxes, testing with javascript turned on and off and removing the read only flag using Firebug always submitting the form to ensuring there are no errors generated.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-30845-master
    • Rank:
      33845

      Description

      When a screen reader encounters a form it enters "forms mode" and the information read by the screen reader might not be what is apparent visually. Moodle regularly renders read-only text within forms as plain text and not as input elements with the readonly attribute set. In forms mode in screen readers, this information will normally be skipped over when the user is browsing through the form because the screen reader does not recognize it as part of the form. Changing the plain text back to an input element with the readonly attribute set will fix this problem.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          While this could be tackled within the forms library, but the big task would be to locate and fix the various form implementations that are using plain text in place of disabled inputs.

          Show
          Michael de Raadt added a comment - While this could be tackled within the forms library, but the big task would be to locate and fix the various form implementations that are using plain text in place of disabled inputs.
          Hide
          Greg Kraus added a comment -

          Here is a blog post about this technique.

          http://accessibility.oit.ncsu.edu/blog/2012/01/12/how-to-display-read-only-or-non-editable-parts-of-a-form/

          In short, it should be coded like this.

          <label for="login">Login ID</label>
          <input name="login" type="text" id="login" readonly value="jdoe">

          Show
          Greg Kraus added a comment - Here is a blog post about this technique. http://accessibility.oit.ncsu.edu/blog/2012/01/12/how-to-display-read-only-or-non-editable-parts-of-a-form/ In short, it should be coded like this. <label for="login">Login ID</label> <input name="login" type="text" id="login" readonly value="jdoe">
          Hide
          Rajesh Taneja added a comment -

          Hello Greg,

          Can you please point to place/'s where text input is rendered without label.

          Show
          Rajesh Taneja added a comment - Hello Greg, Can you please point to place/'s where text input is rendered without label.
          Hide
          Rajesh Taneja added a comment -

          Hello Greg,

          can you please confirm where in moodle did you see read-only text field is not rendered with label.

          Show
          Rajesh Taneja added a comment - Hello Greg, can you please confirm where in moodle did you see read-only text field is not rendered with label.
          Hide
          Greg Kraus added a comment -

          I usually found them when trying to edit a personal profile page. Usually some of the fields would be locked down after an account was created. That's not to say there aren't other places where it occurs, but that's the place I remember them the most.

          Show
          Greg Kraus added a comment - I usually found them when trying to edit a personal profile page. Usually some of the fields would be locked down after an account was created. That's not to say there aren't other places where it occurs, but that's the place I remember them the most.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the reply Greg,

          Unfortunately, I can't locate any place where text fields are without labels. Are you mentioning about view profile page or Edit profile page?
          FYI:
          Any form element created by moodleform has label. So the issue seems to be locating non-standard elements and fixing them.

          Show
          Rajesh Taneja added a comment - Thanks for the reply Greg, Unfortunately, I can't locate any place where text fields are without labels. Are you mentioning about view profile page or Edit profile page? FYI: Any form element created by moodleform has label. So the issue seems to be locating non-standard elements and fixing them.
          Hide
          Greg Kraus added a comment -

          When editing a profile(user/editadvanced.php?...), for instance, the "Choose an authentication method" field is read only, and thus is rendered as a label followed by plain text. My guess is other entries on that page will do the same thing if the user doesn't have permission to edit them.

          Show
          Greg Kraus added a comment - When editing a profile(user/editadvanced.php?...), for instance, the "Choose an authentication method" field is read only, and thus is rendered as a label followed by plain text. My guess is other entries on that page will do the same thing if the user doesn't have permission to edit them.
          Hide
          Greg Kraus added a comment -

          <label>Choose an authentication method </label> <div class="felement fstatic">LDAP server </div>

          Show
          Greg Kraus added a comment - <label>Choose an authentication method </label> <div class="felement fstatic">LDAP server </div>
          Hide
          Michael de Raadt added a comment -

          Carrying over to new sprint.

          Show
          Michael de Raadt added a comment - Carrying over to new sprint.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Greg,
          Can you suggest which makes better sense considering accessibility 'disabled' or 'readonly' ?
          A have just putin a patch with disabled, let me know if "readonly" makes more sense, I will update the patch
          PS:- Here only the behaviour of input text fields are altered, I will create another issue to handle the behaviour of select fields
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Greg, Can you suggest which makes better sense considering accessibility 'disabled' or 'readonly' ? A have just putin a patch with disabled, let me know if "readonly" makes more sense, I will update the patch PS:- Here only the behaviour of input text fields are altered, I will create another issue to handle the behaviour of select fields Thanks
          Hide
          Greg Kraus added a comment -

          Hi Ankit,

          It definitely needs to be readonly. When disabled is set screen readers skip over it, which ends up creating the same problem as before. You can optionally use some CSS to style it so it is more apparent that it is readonly.

          Show
          Greg Kraus added a comment - Hi Ankit, It definitely needs to be readonly. When disabled is set screen readers skip over it, which ends up creating the same problem as before. You can optionally use some CSS to style it so it is more apparent that it is readonly.
          Hide
          Ankit Agarwal added a comment -

          Thanks Greg for the fast response, the reason I preferred disabled intially was, it wont allow you to "tab" in those fields.
          Anyways if screen reader cant catch it than its of no use.
          Patch updated to use readonly and now submitting for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Greg for the fast response, the reason I preferred disabled intially was, it wont allow you to "tab" in those fields. Anyways if screen reader cant catch it than its of no use. Patch updated to use readonly and now submitting for integration. Thanks
          Hide
          Rajesh Taneja added a comment -

          I have added more people to comment on this issue. I think we need to look at more alternatives then just using readonly.

          IMO we should not go with readonly because of following reasons:

          1. mform doesn't use readonly and if we plan to use it then, we have to change disableif functionality and probably look at other places in moodle. Prone to cause regression
          2. Introduce tab focus on non-editable field, which might be inconvenience for others (introducing extra tab hit)
          3. Submit value for non-editable field, which is prone to security vulnerability
          4. IE represent read-only text field with cursor focus and gives impression of editable field (We have to introduce css to make it look disabled)
          5. Not sure if all screen-readers on different browsers, support read-only nicely

          I propose:

          1. Either we should use ARIA properties (support for HTML5), which can get value of disabled elements (ariaDisabled) or
          2. Introduce these changes with a configuration variable, so it doesn't affect everyone.

          FYI:

          Disabled Form Fields
          The disabled attribute can be applied to button, input, optgroup, option, select and submit form fields. For example:
          <input type=”text” disabled=”disabled” />
          When applied to a form field, the disabled attribute means that the field:

          1. Should not receive focus.
          2. Should be skipped in the tab order.
          3. Should not successfully submit data.

          Read Only Form Fields
          The readonly attribute can be applied to input and textarea form fields. For example:
          <input type=”text” readonly=”readonly” />
          When applied to a form field, the readonly attribute means that a field:

          1. Should receive focus, but cannot be modified by the user.
          2. Should be included in the tab order.
          3. Should successfully submit data.

          Reference: http://tink.co.uk/2010/02/screen-reader-support-for-disabled-read-only-form-fields/

          Show
          Rajesh Taneja added a comment - I have added more people to comment on this issue. I think we need to look at more alternatives then just using readonly. IMO we should not go with readonly because of following reasons: mform doesn't use readonly and if we plan to use it then, we have to change disableif functionality and probably look at other places in moodle. Prone to cause regression Introduce tab focus on non-editable field, which might be inconvenience for others (introducing extra tab hit) Submit value for non-editable field, which is prone to security vulnerability IE represent read-only text field with cursor focus and gives impression of editable field (We have to introduce css to make it look disabled) Not sure if all screen-readers on different browsers, support read-only nicely I propose: Either we should use ARIA properties (support for HTML5), which can get value of disabled elements (ariaDisabled) or Introduce these changes with a configuration variable, so it doesn't affect everyone. FYI: Disabled Form Fields The disabled attribute can be applied to button, input, optgroup, option, select and submit form fields. For example: <input type=”text” disabled=”disabled” /> When applied to a form field, the disabled attribute means that the field: Should not receive focus. Should be skipped in the tab order. Should not successfully submit data. Read Only Form Fields The readonly attribute can be applied to input and textarea form fields. For example: <input type=”text” readonly=”readonly” /> When applied to a form field, the readonly attribute means that a field: Should receive focus, but cannot be modified by the user. Should be included in the tab order. Should successfully submit data. Reference: http://tink.co.uk/2010/02/screen-reader-support-for-disabled-read-only-form-fields/
          Hide
          Tim Hunt added a comment -

          Rajesh, it has to be readonly, not disabled. You will find that the quiz shortanswer question type works that way (as an example). That is because, in the past, I worked with the accessibility people at the OU, and they recommended that approach, just like Greg is recommended it above. I cannot, however, remember all the reasons why that is the best way, so hopefully someone can provide a clear explanation, but it is to do with how screenreaders work.

          And yes, for select menus, etc. you do have to used disabled because that is all there is.

          Another important point is: have you found out why formslib was implemented this way in the first place? What I am taking about here is the fact that it displays disabled form fields in static HTML, not as disabled form fields.

          That is certainly a weird design, and there must have been a good reason for it (I assume). It seems that is a design feature of formslib, not something Moodle added, so we may never know.

          I do know that there is the hardFreeze this which is something we added (I think it was mostly Petr who added this). That was something to do with security - with making sure that someone cannot change the value of a 'frozen' form field using a tool like Firebug. I think it may have been the user edit form that lead to that being introduced.

          Anyway, I suggest you try to find some of the bugs that lead to hardFreeze being added, and then add some steps to the testing instructions to verify that the security problems it fixed have not come back with whatever changes you made.

          Show
          Tim Hunt added a comment - Rajesh, it has to be readonly, not disabled. You will find that the quiz shortanswer question type works that way (as an example). That is because, in the past, I worked with the accessibility people at the OU, and they recommended that approach, just like Greg is recommended it above. I cannot, however, remember all the reasons why that is the best way, so hopefully someone can provide a clear explanation, but it is to do with how screenreaders work. And yes, for select menus, etc. you do have to used disabled because that is all there is. Another important point is: have you found out why formslib was implemented this way in the first place? What I am taking about here is the fact that it displays disabled form fields in static HTML, not as disabled form fields. That is certainly a weird design, and there must have been a good reason for it (I assume). It seems that is a design feature of formslib, not something Moodle added, so we may never know. I do know that there is the hardFreeze this which is something we added (I think it was mostly Petr who added this). That was something to do with security - with making sure that someone cannot change the value of a 'frozen' form field using a tool like Firebug. I think it may have been the user edit form that lead to that being introduced. Anyway, I suggest you try to find some of the bugs that lead to hardFreeze being added, and then add some steps to the testing instructions to verify that the security problems it fixed have not come back with whatever changes you made.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the reply Tim,
          I am not sure why Quickform is designed that way. Will try to find answers for it
          @Ankit: If we want to use readonly, then you might have to fix following:

          1. disableif for text field should be readonly and not disable
          2. CSS can be improved for ie to make it look like disabled (non-editable)
          3. Good time to introduce ARIA (probably a sub task)

          Will check again, if we need to modify more code.

          Show
          Rajesh Taneja added a comment - Thanks for the reply Tim, I am not sure why Quickform is designed that way. Will try to find answers for it @Ankit: If we want to use readonly, then you might have to fix following: disableif for text field should be readonly and not disable CSS can be improved for ie to make it look like disabled (non-editable) Good time to introduce ARIA (probably a sub task) Will check again, if we need to modify more code.
          Hide
          Tim Hunt added a comment -

          I think disabledIf should stay as it is. that is about disabling irrelevant form fields based on other form field. I think the disabled attribute is good there, and that read-only form fields are something different.

          Show
          Tim Hunt added a comment - I think disabledIf should stay as it is. that is about disabling irrelevant form fields based on other form field. I think the disabled attribute is good there, and that read-only form fields are something different.
          Hide
          Rajesh Taneja added a comment -

          Hello Tim,

          I am not sure about this. But if you think disabling code should not be touched then, it's fine with me. what do you think about ie CSS ?

          Show
          Rajesh Taneja added a comment - Hello Tim, I am not sure about this. But if you think disabling code should not be touched then, it's fine with me. what do you think about ie CSS ?
          Hide
          Tim Hunt added a comment -

          I think I would not mess with CSS. However, I really don't know. I think you need to find an accessibility expert.

          Show
          Tim Hunt added a comment - I think I would not mess with CSS. However, I really don't know. I think you need to find an accessibility expert.
          Hide
          Greg Kraus added a comment -

          I'm not completely sure what you are asking about the CSS, but if the question is can the CSS be changed for a readonly field, that is fine. You can do things like remove the border around the input, and even change the background color of the input to match the surrounding page background. If you want to leave the CSS the same that is fine too, but changing the look of the readonly field will help sighted users know there is something different about this field. If that's not what you are asking please let me know.

          Show
          Greg Kraus added a comment - I'm not completely sure what you are asking about the CSS, but if the question is can the CSS be changed for a readonly field, that is fine. You can do things like remove the border around the input, and even change the background color of the input to match the surrounding page background. If you want to leave the CSS the same that is fine too, but changing the look of the readonly field will help sighted users know there is something different about this field. If that's not what you are asking please let me know.
          Hide
          Ankit Agarwal added a comment - - edited

          Thanks guys for the feedback. I will use readonly and create a separate issue for the CSS improvment (if needed)
          @Tim
          Yeah I tried to find the original issue that led to the development of hardfreeze, but unfortunately the commit was done in 2007 and there was no MDL associated with it, I tried a few other things but was not able to find any original issue associated to this.
          Anyways IMO it should not cause any security issues, looking at the implementations of hardfreeze.

          Summary:-

          input fields will be rendered as readonly
          disabledif funtions are not altered
          Separate issue will consider the possibility changing/improving the css
          Separate issue will be created to convert select dropdowns from static to their normal type.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Thanks guys for the feedback. I will use readonly and create a separate issue for the CSS improvment (if needed) @Tim Yeah I tried to find the original issue that led to the development of hardfreeze, but unfortunately the commit was done in 2007 and there was no MDL associated with it, I tried a few other things but was not able to find any original issue associated to this. Anyways IMO it should not cause any security issues, looking at the implementations of hardfreeze. Summary:- input fields will be rendered as readonly disabledif funtions are not altered Separate issue will consider the possibility changing/improving the css Separate issue will be created to convert select dropdowns from static to their normal type. Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks Greg and Tim,

          yes, I meant css to be changed to show readonly field as disabled field in ie.

          @Ankit: You might want to update you git branch, as it's still updating disable attribute and not readonly.
          Also, please check if $this->_getPersistantData(); is required with readonly? As, read-only field will submit data.

          Show
          Rajesh Taneja added a comment - Thanks Greg and Tim, yes, I meant css to be changed to show readonly field as disabled field in ie. @Ankit: You might want to update you git branch, as it's still updating disable attribute and not readonly. Also, please check if $this->_getPersistantData(); is required with readonly? As, read-only field will submit data.
          Hide
          Ankit Agarwal added a comment -

          we found a possible security issue with the way mform handles submitted data.
          This issue cannot go further untill MDL-32194 is resolved
          Thanks

          Show
          Ankit Agarwal added a comment - we found a possible security issue with the way mform handles submitted data. This issue cannot go further untill MDL-32194 is resolved Thanks
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          By the looks of it this should not be up for integration yet as the linked blocker MDL-32194 isn't up.
          At a glance it appears that the hack on MDL-32194 would be possible regardless of this change, is that correct?
          If so then I would suggest integrating this regardless of the state of MDL-32194 as this is still an improvement over existing code and doesn't actually open the exploit.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, By the looks of it this should not be up for integration yet as the linked blocker MDL-32194 isn't up. At a glance it appears that the hack on MDL-32194 would be possible regardless of this change, is that correct? If so then I would suggest integrating this regardless of the state of MDL-32194 as this is still an improvement over existing code and doesn't actually open the exploit. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Sorry wrong button

          Show
          Sam Hemelryk added a comment - Sorry wrong button
          Hide
          Rajesh Taneja added a comment -

          MDL-32194 is still possible without this fix. But this improvement makes it easier for user to hack mform.
          As this is only for 2.3, it may land safely, provided we make sure 2.3 contains fix for MDL-32194.

          Show
          Rajesh Taneja added a comment - MDL-32194 is still possible without this fix. But this improvement makes it easier for user to hack mform. As this is only for 2.3, it may land safely, provided we make sure 2.3 contains fix for MDL-32194 .
          Hide
          Sam Hemelryk added a comment -

          Reopening this now, it will have to wait until MDL-32194 has been integrated.
          The noted exploit is there regardless, however the linked issue landing first will greatly reduce the risk of regressions caused by this issue.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Reopening this now, it will have to wait until MDL-32194 has been integrated. The noted exploit is there regardless, however the linked issue landing first will greatly reduce the risk of regressions caused by this issue. Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          Pushing to next sprint as the issue that blocks this hasn't been fixed yet.
          Thanks

          Show
          Ankit Agarwal added a comment - Pushing to next sprint as the issue that blocks this hasn't been fixed yet. Thanks
          Hide
          Ankit Agarwal added a comment -

          Discussed this with Raj and we concluded following things:-

          • As mentioned earlier a new issue will be created to change the behavior of select fields and other input elements when they are frozen
          • As of now we are leaving persistantFreeze feature as:-
            • It keeps form design consistent
            • Backward compatibility
            • In mform readonly fields are not submitted when there value is empty (where as hidden input fields are submitted)
          • The complete persistantFreeze system can be removed once rendering of all input field types are changed for frozen elements

          Pushing for integration review.
          Thanks

          Show
          Ankit Agarwal added a comment - Discussed this with Raj and we concluded following things:- As mentioned earlier a new issue will be created to change the behavior of select fields and other input elements when they are frozen As of now we are leaving persistantFreeze feature as:- It keeps form design consistent Backward compatibility In mform readonly fields are not submitted when there value is empty (where as hidden input fields are submitted) The complete persistantFreeze system can be removed once rendering of all input field types are changed for frozen elements Pushing for integration review. Thanks
          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
          Sam Hemelryk added a comment - - edited

          Hi Ankit,

          Reopening sorry.
          A quick test shows that this is leading to two elements with the same name appearing in forms.
          The first is the text box with readonly set, and the second is a hidden element.

          To test I used the following script saved as a test.php in the Moodle root dir.

          <?php
          require_once('config.php');
          require_once($CFG->libdir.'/formslib.php');
          
          class myform extends moodleform {
              public function definition() {
                  $this->_form->addElement('text', 'test', 'Test');
                  $this->_form->addElement('text', 'test2', 'Test 2');
                  $this->_form->freeze('test2');
                  $this->_form->addElement('submit', 'submit', 'Sumbit');
              }
          }
          
          $form = new myform();
          $form->set_data(array('test' => 'This is a test', 'test2' => 'This is a frozen test'));
          if ($data = $form->get_data()) {
              echo "<pre>".print_r($data, true)."</pre>";
          }
          
          $form->display();
          
          Show
          Sam Hemelryk added a comment - - edited Hi Ankit, Reopening sorry. A quick test shows that this is leading to two elements with the same name appearing in forms. The first is the text box with readonly set, and the second is a hidden element. To test I used the following script saved as a test.php in the Moodle root dir. <?php require_once('config.php'); require_once($CFG->libdir.'/formslib.php'); class myform extends moodleform { public function definition() { $ this ->_form->addElement('text', 'test', 'Test'); $ this ->_form->addElement('text', 'test2', 'Test 2'); $ this ->_form->freeze('test2'); $ this ->_form->addElement('submit', 'submit', 'Sumbit'); } } $form = new myform(); $form->set_data(array('test' => 'This is a test', 'test2' => 'This is a frozen test')); if ($data = $form->get_data()) { echo "<pre>" .print_r($data, true ). "</pre>" ; } $form->display();
          Hide
          CiBoT added a comment -

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

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

          Discussed this with Sam, And we decided to remove hidden inupt fields as it can cause issues with JS validations, etc.

          Hi Sam,
          I have reviewed all uses of persistentfreeze in core and made changes whenever needed (only one instance needed change). I have made change to the api to set it to false always.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Discussed this with Sam, And we decided to remove hidden inupt fields as it can cause issues with JS validations, etc. Hi Sam, I have reviewed all uses of persistentfreeze in core and made changes whenever needed (only one instance needed change). I have made change to the api to set it to false always. Thanks
          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
          Ankit Agarwal added a comment -

          Rebased submitting for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Rebased submitting for integration. Thanks
          Hide
          Tim Hunt added a comment -

          I would be interested in the details of your discussion with Sam. Presumably those hidden fields were originally added for a reason. Do you know what that reason is? (That is, what are the MDL- issues numbers or git commit ids that lead to them). Without knowing that, how can we be sure that this will not cause regressions?

          Show
          Tim Hunt added a comment - I would be interested in the details of your discussion with Sam. Presumably those hidden fields were originally added for a reason. Do you know what that reason is? (That is, what are the MDL- issues numbers or git commit ids that lead to them). Without knowing that, how can we be sure that this will not cause regressions?
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Tim,
          Previously the value was displayed as static field so it was never submitted. So to get these values in the script, hidden fields were used. Now since the fields get submitted anyway, we donot need those hidden fields anymore. PersistantFreeze is a feature of Quick form and is been there since the start afaik, still will try to see if there are any other associated issues.
          The main reason removing this being we cannot have two elements with same id (http://www.w3.org/TR/html401/struct/global.html#h-7.5.2). And also as Sam mentioned it might create issues with JS.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Tim, Previously the value was displayed as static field so it was never submitted. So to get these values in the script, hidden fields were used. Now since the fields get submitted anyway, we donot need those hidden fields anymore. PersistantFreeze is a feature of Quick form and is been there since the start afaik, still will try to see if there are any other associated issues. The main reason removing this being we cannot have two elements with same id ( http://www.w3.org/TR/html401/struct/global.html#h-7.5.2 ). And also as Sam mentioned it might create issues with JS. Thanks
          Hide
          Tim Hunt added a comment -

          Thank you for explaining. I get it now.

          Effectively you are replacing the previous hidden fields with new read-only text fields; and read-only Unable to render embedded object: File (= disabled. Therefore, what you have done is least likely to cause regressions. Neat) not found.

          Show
          Tim Hunt added a comment - Thank you for explaining. I get it now. Effectively you are replacing the previous hidden fields with new read-only text fields; and read-only Unable to render embedded object: File (= disabled. Therefore, what you have done is least likely to cause regressions. Neat) not found.
          Hide
          Rajesh Taneja added a comment -

          Apu asked me to look at this again,

          Few things which might be of interest:

          1. As we have set persistent to false, then why do we need to call _getPersistantData (https://github.com/ankitagarwal/moodle/compare/MDL-30845-master#L0R91) ?
          2. Not sure why we need https://github.com/ankitagarwal/moodle/compare/MDL-30845-master#L0R78, Why do we need to override freeze? IMO, we just need to remove getpersistanceData use (at point 1)
          3. It is getting introduced in 2.4, so https://github.com/ankitagarwal/moodle/compare/MDL-30845-master#L0R84 needs correction
          4. ignoring code style, as this goes with current forms coding style
          Show
          Rajesh Taneja added a comment - Apu asked me to look at this again, Few things which might be of interest: As we have set persistent to false, then why do we need to call _getPersistantData ( https://github.com/ankitagarwal/moodle/compare/MDL-30845-master#L0R91 ) ? Not sure why we need https://github.com/ankitagarwal/moodle/compare/MDL-30845-master#L0R78 , Why do we need to override freeze? IMO, we just need to remove getpersistanceData use (at point 1) It is getting introduced in 2.4, so https://github.com/ankitagarwal/moodle/compare/MDL-30845-master#L0R84 needs correction ignoring code style, as this goes with current forms coding style
          Hide
          Ankit Agarwal added a comment -

          Hi Raj,

          1. I thought of removing the function completely, but left it intact just for compatibility reasons. So that the contrib codes are not effected. If everyone thinks it should be removed, I can go ahead and remove it.
          2. To set the persistantfreeze flag to false which is inherited from quickform. We need to do this to keep things consistent.
          3. Right will change that to 2.4
          4. Also please suggest if this needs to mentioned anywhere in docs.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Raj, I thought of removing the function completely, but left it intact just for compatibility reasons. So that the contrib codes are not effected. If everyone thinks it should be removed, I can go ahead and remove it. To set the persistantfreeze flag to false which is inherited from quickform. We need to do this to keep things consistent. Right will change that to 2.4 Also please suggest if this needs to mentioned anywhere in docs. Thanks
          Hide
          Rajesh Taneja added a comment -

          Sorry Ankit,

          I don't see any reason of freeze calling setPersistantFreeze.
          You are overriding default behaviour of freeze api and IMO missing freeze with setPersistantFreeze

          Show
          Rajesh Taneja added a comment - Sorry Ankit, I don't see any reason of freeze calling setPersistantFreeze. You are overriding default behaviour of freeze api and IMO missing freeze with setPersistantFreeze
          Hide
          Ankit Agarwal added a comment - - edited

          So should we just leave the MoodleQuickForm_text::_persistantFreeze to true? which is the default behavior?

          Show
          Ankit Agarwal added a comment - - edited So should we just leave the MoodleQuickForm_text::_persistantFreeze to true? which is the default behavior?
          Hide
          Aparup Banerjee added a comment -

          Hm i was reading http://www.w3.org/TR/html4/interact/forms.html#h-17.12 and http://www.w3.org/TR/html4/interact/forms.html#successful-controls so glad we're going with the 'successful' read-only.

          i found some documentation in master about the difference between persistent and simply frozen here:

          grade/grading/form/lib.php-833-     * Plugins may use $gradingformelement->getValue() to get the value passed on previous
          grade/grading/form/lib.php-834-     * form submit
          grade/grading/form/lib.php-835-     *
          grade/grading/form/lib.php-836-     * When forming html it is a plugin's responsibility to analyze flags
          grade/grading/form/lib.php:837:     * $gradingformelement->_flagFrozen and $gradingformelement->_persistantFreeze:
          grade/grading/form/lib.php-838-     *
          grade/grading/form/lib.php-839-     * (_flagFrozen == false) => form element is editable
          grade/grading/form/lib.php-840-     *
          grade/grading/form/lib.php:841:     * (_flagFrozen == false && _persistantFreeze == true) => form element is not editable
          grade/grading/form/lib.php-842-     * but all values are passed as hidden elements
          grade/grading/form/lib.php-843-     *
          grade/grading/form/lib.php:844:     * (_flagFrozen == false && _persistantFreeze == false) => form element is not editable
          grade/grading/form/lib.php-845-     * and no values are passed as hidden elements
          

          perhaps that can help with decision about what the persistantFreeze setting should be but really we need some proper big clarifying comment or doc somewhere about this. perhaps a dev doc?
          it seems _persistantFreeze == true is for passing values through which maps to the effect of 'readonly' - so it seems true to be true. :-p

          reopening to decide the above persistant one and for some adding of comment to code for background here. (also its a little late in the week late now to introduce exploratory test of this scale sorry.)

          re:coding style - i think its understood among integrators about the code area style being duplicated in the area but feel free to bring it up.

          ps: (after too much searching my likely wrong guess is hardFeeze() came about from MDL-8143 since it was resolved around that date)

          Show
          Aparup Banerjee added a comment - Hm i was reading http://www.w3.org/TR/html4/interact/forms.html#h-17.12 and http://www.w3.org/TR/html4/interact/forms.html#successful-controls so glad we're going with the 'successful' read-only. i found some documentation in master about the difference between persistent and simply frozen here: grade/grading/form/lib.php-833- * Plugins may use $gradingformelement->getValue() to get the value passed on previous grade/grading/form/lib.php-834- * form submit grade/grading/form/lib.php-835- * grade/grading/form/lib.php-836- * When forming html it is a plugin's responsibility to analyze flags grade/grading/form/lib.php:837: * $gradingformelement->_flagFrozen and $gradingformelement->_persistantFreeze: grade/grading/form/lib.php-838- * grade/grading/form/lib.php-839- * (_flagFrozen == false ) => form element is editable grade/grading/form/lib.php-840- * grade/grading/form/lib.php:841: * (_flagFrozen == false && _persistantFreeze == true ) => form element is not editable grade/grading/form/lib.php-842- * but all values are passed as hidden elements grade/grading/form/lib.php-843- * grade/grading/form/lib.php:844: * (_flagFrozen == false && _persistantFreeze == false ) => form element is not editable grade/grading/form/lib.php-845- * and no values are passed as hidden elements perhaps that can help with decision about what the persistantFreeze setting should be but really we need some proper big clarifying comment or doc somewhere about this. perhaps a dev doc? it seems _persistantFreeze == true is for passing values through which maps to the effect of 'readonly' - so it seems true to be true. :-p reopening to decide the above persistant one and for some adding of comment to code for background here. (also its a little late in the week late now to introduce exploratory test of this scale sorry.) — re:coding style - i think its understood among integrators about the code area style being duplicated in the area but feel free to bring it up. — ps: (after too much searching my likely wrong guess is hardFeeze() came about from MDL-8143 since it was resolved around that date)
          Hide
          Ankit Agarwal added a comment - - edited

          I will check this in the grade code and confirm, but this doesnt seem right to me:- flagfrozen should be true.
          Edit:- confirmed that is incorrect documentation, code confirms it. flagfrozen needs to be true to the element to be not editable.

          grade/grading/form/lib.php:841:     * (_flagFrozen == false && _persistantFreeze == true) => form element is not editable
          grade/grading/form/lib.php-842-     * but all values are passed as hidden elements
          grade/grading/form/lib.php-843-     *
          grade/grading/form/lib.php:844:     * (_flagFrozen == false && _persistantFreeze == false) => form element is not editable
          grade/grading/form/lib.php-845-     * and no values are passed as hidden elements
          

          Also _persistantFreeze is a flag used to indicate if hidden values should be passed or not. Sorry guys, but am still not convinced why we should leave this to true.

              /**
               * Sets wether an element value should be kept in an hidden field
               * when the element is frozen or not
               * 
               * @param     bool    $persistant   True if persistant value
               * @since     2.0
               * @access    public
               * @return    void
               */
              function setPersistantFreeze($persistant=false)
              {
                  $this->_persistantFreeze = $persistant;
              } //end func setPersistantFreeze
          
          Show
          Ankit Agarwal added a comment - - edited I will check this in the grade code and confirm, but this doesnt seem right to me:- flagfrozen should be true. Edit:- confirmed that is incorrect documentation, code confirms it. flagfrozen needs to be true to the element to be not editable. grade/grading/form/lib.php:841: * (_flagFrozen == false && _persistantFreeze == true ) => form element is not editable grade/grading/form/lib.php-842- * but all values are passed as hidden elements grade/grading/form/lib.php-843- * grade/grading/form/lib.php:844: * (_flagFrozen == false && _persistantFreeze == false ) => form element is not editable grade/grading/form/lib.php-845- * and no values are passed as hidden elements Also _persistantFreeze is a flag used to indicate if hidden values should be passed or not. Sorry guys, but am still not convinced why we should leave this to true. /** * Sets wether an element value should be kept in an hidden field * when the element is frozen or not * * @param bool $persistant True if persistant value * @since 2.0 * @access public * @ return void */ function setPersistantFreeze($persistant= false ) { $ this ->_persistantFreeze = $persistant; } //end func setPersistantFreeze
          Hide
          Aparup Banerjee added a comment -

          cool thanks Ankit, well rest assured this will make it to some exploratory testing next week (had a chat with SamH)

          Show
          Aparup Banerjee added a comment - cool thanks Ankit, well rest assured this will make it to some exploratory testing next week (had a chat with SamH)
          Hide
          CiBoT added a comment -

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

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

          i'm bringing this in for integration, never a fan (nor understood properly ) of _persistantFreeze usage in the lib but it improves accessiblity so pushing it thru for wider testing.

          Show
          Aparup Banerjee added a comment - i'm bringing this in for integration, never a fan (nor understood properly ) of _persistantFreeze usage in the lib but it improves accessiblity so pushing it thru for wider testing.
          Hide
          Aparup Banerjee added a comment -

          Thanks all, this has been integrated into master now.

          Testers:
          Please perform some form of exhaustive and exploratory testing here to weed any any possible and unforeseen regressions here.

          Show
          Aparup Banerjee added a comment - Thanks all, this has been integrated into master now. Testers: Please perform some form of exhaustive and exploratory testing here to weed any any possible and unforeseen regressions here.
          Hide
          Tim Barker added a comment -

          If the landscape is agreeable I think I'll take a look at the exploratory side of this today. To determine that landscape I need to know if there are there any other fixes that haven't landed in integration yet affecting forms?

          Show
          Tim Barker added a comment - If the landscape is agreeable I think I'll take a look at the exploratory side of this today. To determine that landscape I need to know if there are there any other fixes that haven't landed in integration yet affecting forms?
          Hide
          Ankit Agarwal added a comment -

          Hi Tim,
          Apu can confirm from integrators side. From my side there are no other issues affecting this patch in current integration.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Tim, Apu can confirm from integrators side. From my side there are no other issues affecting this patch in current integration. Thanks
          Hide
          Aparup Banerjee added a comment -

          Hello Tim,
          Integration is stil on going at the moment however from our backlog for this week's integration there is no other bug related to forms library. See integration dashboard or http://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350#Two-Dimensional-Filter-Statistics/14541 for summary by component for this week.

          Show
          Aparup Banerjee added a comment - Hello Tim, Integration is stil on going at the moment however from our backlog for this week's integration there is no other bug related to forms library. See integration dashboard or http://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350#Two-Dimensional-Filter-Statistics/14541 for summary by component for this week.
          Hide
          Tim Barker added a comment -

          Thanks

          Show
          Tim Barker added a comment - Thanks
          Hide
          Tim Barker added a comment -

          The enclosed spreadsheet shows the forms that I have covered with the exploratory test session. Forms highlighted in yellow are covered.

          Show
          Tim Barker added a comment - The enclosed spreadsheet shows the forms that I have covered with the exploratory test session. Forms highlighted in yellow are covered.
          Hide
          Tim Barker added a comment -

          I think I've seen enough forms now to satisfy me that this is OK. I have found a regression - MDL-36086 - but it is completely unrelated to Ankit's work here.

          I tested Ankit's instructions, did some role based testing on the mods and the regression exploratory sessions below. Good work Ankit

          Show
          Tim Barker added a comment - I think I've seen enough forms now to satisfy me that this is OK. I have found a regression - MDL-36086 - but it is completely unrelated to Ankit's work here. I tested Ankit's instructions, did some role based testing on the mods and the regression exploratory sessions below. Good work Ankit
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: