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

            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.
            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
            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
            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!
            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
            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
            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)
            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.
            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!
            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!
            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 :-/).

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Nov/12