Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      • 1 standard forum
      • Use your HTML inspector and screen reader to assert that the tests are correct

      Test steps

      1. Go to the standard forum and create a new topic
      2. Save the new topic and follow the 'Reply' link to add another post
      3. Make sure the page scrolls to the form because of the anchor in the URL
      4. Make sure there is a heading H2 (hidden) above the form
      5. Make sure it makes sense and is helpful using a screen header
      Show
      Test pre-requisites 1 standard forum Use your HTML inspector and screen reader to assert that the tests are correct Test steps Go to the standard forum and create a new topic Save the new topic and follow the 'Reply' link to add another post Make sure the page scrolls to the form because of the anchor in the URL Make sure there is a heading H2 (hidden) above the form Make sure it makes sense and is helpful using a screen header
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30909-master
    • Rank:
      33917

      Description

      Screen reader users may have a hard time finding the actual box to type their reply in after clicking the "Reply" link because it is so far down on the page.

      This problem should be considered in the context of redesigning the entire forum. One easy solution is to put a heading at the reply entry location.

        Issue Links

          Activity

          Greg Kraus created issue -
          Greg Kraus made changes -
          Field Original Value New Value
          Fix Version/s 2.3 [ 10657 ]
          Michael de Raadt made changes -
          Fix Version/s STABLE backlog [ 10463 ]
          Fix Version/s 2.3 [ 10657 ]
          Labels triaged
          Frédéric Massart made changes -
          Fix Version/s STABLE Sprint 24 Omega [ 12364 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Assignee moodle.com [ moodle.com ] Frédéric Massart [ fred ]
          Frédéric Massart made changes -
          Fix Version/s STABLE Sprint 24 Alpha [ 12363 ]
          Fix Version/s STABLE Sprint 24 Omega [ 12364 ]
          Frédéric Massart made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Hide
          Frédéric Massart added a comment -

          I am providing here an alternative solution. I'm afraid that setting a header looks too much like a hack where as using an anchor would do the trick. Please let me know what you think about that solution.

          Show
          Frédéric Massart added a comment - I am providing here an alternative solution. I'm afraid that setting a header looks too much like a hack where as using an anchor would do the trick. Please let me know what you think about that solution.
          Frédéric Massart made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Pull Master Diff URL https://github.com/FMCorz/moodle/compare/MDL-30909-master
          Pull 2.3 Diff URL https://github.com/FMCorz/moodle/compare/moodle:MOODLE_23_STABLE...MDL-30909-23
          Pull Master Branch MDL-30909-master
          Pull from Repository git://github.com/FMCorz/moodle.git
          Pull 2.2 Diff URL https://github.com/FMCorz/moodle/compare/moodle:MOODLE_22_STABLE...MDL-30909-22
          Pull 2.2 Branch MDL-30909-22
          Pull 2.3 Branch MDL-30909-23
          Ankit Agarwal made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer ankit_frenz
          Hide
          Ankit Agarwal added a comment -

          Hi Fred,
          I am not sure how helpful an hidden h2 is for screen readers, but under the given case this seems most feasible solution.
          Feel free to push for integration.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Fred, I am not sure how helpful an hidden h2 is for screen readers, but under the given case this seems most feasible solution. Feel free to push for integration. Thanks
          Ankit Agarwal made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Frédéric Massart made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Frédéric Massart made changes -
          Testing Instructions *Test pre-requisites*

          - 1 standard forum
          - 1 Q & A forum
          - Use your HTML inspector to assert the test

          *Test steps*

          # Go to the standard forum and create a new topic
          # *Make sure* there is a heading H2 above the form
          # Save the new topic and follow the 'Reply' link to add another post
          # *Make sure* there is a heading H2 above the form
          # Go to the Q & A forum and create a new question
          # *Make sure* there is a heading H2 above the form
          Frédéric Massart made changes -
          Testing Instructions *Test pre-requisites*

          - 1 standard forum
          - 1 Q & A forum
          - Use your HTML inspector to assert the test

          *Test steps*

          # Go to the standard forum and create a new topic
          # *Make sure* there is a heading H2 above the form
          # Save the new topic and follow the 'Reply' link to add another post
          # *Make sure* there is a heading H2 above the form
          # Go to the Q & A forum and create a new question
          # *Make sure* there is a heading H2 above the form
          *Test pre-requisites*

          - 1 standard forum
          - 1 Q & A forum
          - Use your HTML inspector to assert that the tests are correct

          *Test steps*

          # Go to the standard forum and create a new topic
          # *Make sure* there is a heading H2 above the form
          # Save the new topic and follow the 'Reply' link to add another post
          # *Make sure* there is a heading H2 above the form
          # Go to the Q & A forum and create a new question
          # *Make sure* there is a heading H2 above the form
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          After much deliberation over this seemingly simple issue I've decided for the time being that we should hold off on this change.

          I think the use of the anchor to automatically jump to the reply form is a good idea, although I think perhaps in master only we should give it a proper id to make the link more meaningful.

          Like Ankit the hidden heading strikes me as a bit of an odd ball. Having had a play with the post form and seeing why it is needed I really think that this approach is less than ideal.
          While accessibility is a important point I think it shouldn't be acquired at the cost of sensibility and functionality.

          The issue with the headings appears to be mostly because mforms heading element gets translated into a fieldset and legend rather than an actual heading and I gather that because this is being addressed screen readers don't cope with it too well.
          Assuming so then I think perhaps the best thing to do would be to address the design of that form, and either using a static/html element, or using some clever display style the form similar to what it is now but using a header.
          Of course a couple of things factor into whether this should be done or not, mainly that forum is due to be looked at and likely will be replaced.
          Anyway it's not really my decision to make (heh I'm certainly no accessibility expert) I'm sure that this can be talked about at the stable scrum and I'll let you guys decide.

          However from me this gets a -0.5. I think the use of the anchor is good, but could be improved for master. But I don't like the new hidden heading.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, After much deliberation over this seemingly simple issue I've decided for the time being that we should hold off on this change. I think the use of the anchor to automatically jump to the reply form is a good idea, although I think perhaps in master only we should give it a proper id to make the link more meaningful. Like Ankit the hidden heading strikes me as a bit of an odd ball. Having had a play with the post form and seeing why it is needed I really think that this approach is less than ideal. While accessibility is a important point I think it shouldn't be acquired at the cost of sensibility and functionality. The issue with the headings appears to be mostly because mforms heading element gets translated into a fieldset and legend rather than an actual heading and I gather that because this is being addressed screen readers don't cope with it too well. Assuming so then I think perhaps the best thing to do would be to address the design of that form, and either using a static/html element, or using some clever display style the form similar to what it is now but using a header. Of course a couple of things factor into whether this should be done or not, mainly that forum is due to be looked at and likely will be replaced. Anyway it's not really my decision to make (heh I'm certainly no accessibility expert) I'm sure that this can be talked about at the stable scrum and I'll let you guys decide. However from me this gets a -0.5. I think the use of the anchor is good, but could be improved for master. But I don't like the new hidden heading. Cheers Sam
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Reopened [ 4 ]
          CiBoT made changes -
          Status Reopened [ 4 ] Reopened [ 4 ]
          Currently in integration Yes [ 10041 ]
          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 -

          Hi Sam,
          I agree that we should use a nice id for the form.
          About the header thing, I am not sure we can do a lot about it. The possible solution that I can think of are :-
          1- Redo form design to use headers instead of legends/fieldsets
          2- Use hidden header.
          3- Display both header and legend
          4- Do nothing about the header.

          my +1 for any 2 and 4

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Sam, I agree that we should use a nice id for the form. About the header thing, I am not sure we can do a lot about it. The possible solution that I can think of are :- 1- Redo form design to use headers instead of legends/fieldsets 2- Use hidden header. 3- Display both header and legend 4- Do nothing about the header. my +1 for any 2 and 4 Thanks
          Hide
          Frédéric Massart added a comment -

          Hi Sam,

          I have created a functionality to use custom IDs in forms and attached it to master. For 2.2 and 2.3 I am still using the ugly #mform1.

          Regarding the header, I agree that it is not really beautiful but we are already using H2 with accesshide at some other places. I have also investigated a way of using a heading in the forms but that looks really tricky as it would need a big refactoring. For now, it would not be possible to add (let's say) a addTitle method, or even a title element as they would be wrapped in a fieldset. Adding them outside a fieldset would either be a hack or changing formslib to be able to add something outside of a fieldset.

          Also, this specific form requires a heading but most of them don't, and when they have one it was added manually not using the mform.

          I am pushing this for peer review again.

          Cheers!

          Show
          Frédéric Massart added a comment - Hi Sam, I have created a functionality to use custom IDs in forms and attached it to master. For 2.2 and 2.3 I am still using the ugly #mform1. Regarding the header, I agree that it is not really beautiful but we are already using H2 with accesshide at some other places. I have also investigated a way of using a heading in the forms but that looks really tricky as it would need a big refactoring. For now, it would not be possible to add (let's say) a addTitle method, or even a title element as they would be wrapped in a fieldset. Adding them outside a fieldset would either be a hack or changing formslib to be able to add something outside of a fieldset. Also, this specific form requires a heading but most of them don't, and when they have one it was added manually not using the mform. I am pushing this for peer review again. Cheers!
          Frédéric Massart made changes -
          Testing Instructions *Test pre-requisites*

          - 1 standard forum
          - 1 Q & A forum
          - Use your HTML inspector to assert that the tests are correct

          *Test steps*

          # Go to the standard forum and create a new topic
          # *Make sure* there is a heading H2 above the form
          # Save the new topic and follow the 'Reply' link to add another post
          # *Make sure* there is a heading H2 above the form
          # Go to the Q & A forum and create a new question
          # *Make sure* there is a heading H2 above the form
          *Test pre-requisites*

          - 1 standard forum
          - Use your HTML inspector and screen reader to assert that the tests are correct

          *Test steps*

          # Go to the standard forum and create a new topic
          # Save the new topic and follow the 'Reply' link to add another post
          # *Make sure* the page scrolls to the form because of the anchor in the URL
          # *Make sure* there is a heading H2 (hidden) above the form
          # *Make sure* it makes sense and is helpful using a screen header
          Frédéric Massart made changes -
          Status Reopened [ 4 ] Waiting for peer review [ 10012 ]
          Ankit Agarwal made changes -
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Hide
          Ankit Agarwal added a comment -

          Looks good Fred,
          This might need mentioning in dev docs, please make sure they are updated once this is integrated.

          Thanks

          Show
          Ankit Agarwal added a comment - Looks good Fred, This might need mentioning in dev docs, please make sure they are updated once this is integrated. Thanks
          Frédéric Massart made changes -
          Labels triaged dev_docs_required triaged
          Frédéric Massart made changes -
          Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Integrator samhemelryk
          Currently in integration Yes [ 10041 ]
          Dan Poltawski made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator poltawski
          Hide
          Dan Poltawski added a comment -

          Hmmm.

          1. Could we just use an id tag on the h2 to jump to the right place? Would be less horrible
          2. Given the legacy heap which mforms is, it seems like putting a form within an element where we control the id would work for most cases, no? Rather than shoehorn the custom id, which worries me because everything in formslib worries me and we also have v.old JS depending on this kind of stuff.
          3. I'm not a fan of like the name $accessibleheading, does describe really what it does? Or is it only 'accessibleheading' at the point its put into a h2 with accesshide
          Show
          Dan Poltawski added a comment - Hmmm. Could we just use an id tag on the h2 to jump to the right place? Would be less horrible Given the legacy heap which mforms is, it seems like putting a form within an element where we control the id would work for most cases, no? Rather than shoehorn the custom id, which worries me because everything in formslib worries me and we also have v.old JS depending on this kind of stuff. I'm not a fan of like the name $accessibleheading, does describe really what it does? Or is it only 'accessibleheading' at the point its put into a h2 with accesshide
          Hide
          Dan Poltawski added a comment -

          Discussed with Fred.

          The problem with the h2 is that it won't work if its accesshiden and discussed my caution about the mforms thing.

          Fred is looking into the JS which me using this ID

          Show
          Dan Poltawski added a comment - Discussed with Fred. The problem with the h2 is that it won't work if its accesshiden and discussed my caution about the mforms thing. Fred is looking into the JS which me using this ID
          Hide
          Frédéric Massart added a comment -

          Hi Dan,

          as we discussed on the chat:

          • The ID on the H2 cannot be used to jump as it has the class accesshide and so position: absolute.
          • I understand that this could eventually create issues, but this is a master only change. There is a fallback on the current way of naming the forms. There does not seem to be any Javascript error on the forum page while using this ID. I haven't found hardcoded #mform1 in Javascript in the forum. So I think this can be integrated.
          • The variable has been renamed.

          Cheers!

          Show
          Frédéric Massart added a comment - Hi Dan, as we discussed on the chat: The ID on the H2 cannot be used to jump as it has the class accesshide and so position: absolute. I understand that this could eventually create issues, but this is a master only change. There is a fallback on the current way of naming the forms. There does not seem to be any Javascript error on the forum page while using this ID. I haven't found hardcoded #mform1 in Javascript in the forum. So I think this can be integrated. The variable has been renamed. Cheers!
          Hide
          Dan Poltawski added a comment -

          Thanks Fred, i've integrated this now (22, 23 and master)

          Show
          Dan Poltawski added a comment - Thanks Fred, i've integrated this now (22, 23 and master)
          Dan Poltawski made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Fix Version/s 2.2.6 [ 12372 ]
          Fix Version/s 2.3.3 [ 12373 ]
          Tim Barker made changes -
          Tester timb
          Hide
          Greg Kraus added a comment -

          The h2 works there for screen reader users.

          Show
          Greg Kraus added a comment - The h2 works there for screen reader users.
          Tim Barker made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Tim Barker added a comment -

          All good in 2.4

          Show
          Tim Barker added a comment - All good in 2.4
          Hide
          Tim Barker added a comment -

          all good in 2.3

          Show
          Tim Barker added a comment - all good in 2.3
          Hide
          Tim Barker added a comment -

          And finally it all works on 2.2 as well. Great work it's in!

          Show
          Tim Barker added a comment - And finally it all works on 2.2 as well. Great work it's in!
          Tim Barker made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!
          Dan Poltawski made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 20/Sep/12
          Hide
          Juan Segarra Montesinos added a comment - - edited

          Hi guys. In 2.3, Reply buttons in forums are removed in MyMobile theme because of the anchor introduced at reply command (commit 6d3249780b401d7864ba61b57f55b4290f811e69).

          I've reported this in MDL-36305 (i've not found no suitable Component for reporting this :-/).

          Show
          Juan Segarra Montesinos added a comment - - edited Hi guys. In 2.3, Reply buttons in forums are removed in MyMobile theme because of the anchor introduced at reply command (commit 6d3249780b401d7864ba61b57f55b4290f811e69). I've reported this in MDL-36305 (i've not found no suitable Component for reporting this :-/).
          Michael de Raadt made changes -
          Link This issue caused a regression MDL-36305 [ MDL-36305 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s STABLE Sprint 24 Alpha [ 12363 ]
          Frédéric Massart made changes -
          Labels dev_docs_required triaged triaged

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: