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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            gdkraus Greg Kraus created issue -
            gdkraus Greg Kraus made changes -
            Field Original Value New Value
            Fix Version/s 2.3 [ 10657 ]
            salvetore Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Fix Version/s 2.3 [ 10657 ]
            Labels triaged
            fred 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 ]
            fred Frédéric Massart made changes -
            Fix Version/s STABLE Sprint 24 Alpha [ 12363 ]
            Fix Version/s STABLE Sprint 24 Omega [ 12364 ]
            fred Frédéric Massart made changes -
            Status Open [ 1 ] Development in progress [ 3 ]
            Hide
            fred 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
            fred 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.
            fred 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_frenz 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_frenz 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_frenz 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_frenz Ankit Agarwal made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            fred Frédéric Massart made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            fred 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
            fred 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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator samhemelryk
            Hide
            samhemelryk 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
            samhemelryk 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
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            cibot CiBoT made changes -
            Status Reopened [ 4 ] Reopened [ 4 ]
            Currently in integration Yes [ 10041 ]
            Hide
            cibot CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            ankit_frenz 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_frenz 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
            fred 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
            fred 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!
            fred 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
            fred Frédéric Massart made changes -
            Status Reopened [ 4 ] Waiting for peer review [ 10012 ]
            ankit_frenz Ankit Agarwal made changes -
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Hide
            ankit_frenz 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_frenz 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
            fred Frédéric Massart made changes -
            Labels triaged dev_docs_required triaged
            fred Frédéric Massart made changes -
            Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Integrator samhemelryk
            Currently in integration Yes [ 10041 ]
            poltawski Dan Poltawski made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator poltawski
            Hide
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            fred 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
            fred 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Thanks Fred, i've integrated this now (22, 23 and master)
            poltawski 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 ]
            timb Tim Barker made changes -
            Tester timb
            Hide
            gdkraus Greg Kraus added a comment -

            The h2 works there for screen reader users.

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

            All good in 2.4

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

            all good in 2.3

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

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

            Show
            timb Tim Barker added a comment - And finally it all works on 2.2 as well. Great work it's in!
            timb Tim Barker made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            poltawski 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
            poltawski 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!
            poltawski Dan Poltawski made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 20/Sep/12
            Hide
            jsegarra 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
            jsegarra 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 :-/).
            salvetore Michael de Raadt made changes -
            Link This issue caused a regression MDL-36305 [ MDL-36305 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Fix Version/s STABLE Sprint 24 Alpha [ 12363 ]
            fred 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:
                  Fix Release Date:
                  12/Nov/12