Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change error summary ARIA role to "alert" #511

Merged
merged 1 commit into from Jul 25, 2017

Conversation

@selfthinker
Copy link
Member

commented Jun 29, 2017

What problem does the pull request solve?

Currently neither VoiceOver on iOS or macOS nor TalkBack on Android announce that there is an error when submitting a form with errors. Although we focus on the error summary box, it doesn't have the desired effect. In the iOS case it's because of one browser bug and in the macOS case because of another browser bug. At least the iOS and Android issue can be fixed by adding the ARIA role="alert" to the error summary box.

I am not sure what the effect will be of removing the group role. It didn't seem to have an effect when I tested this but I would like to have someone else's opinion on this.

How has this been tested?

Changing this doesn't change the behaviour (postively or negatively) in JAWS, NVDA or VO on macOS, but it does mean that ZoomText is not announcing anything.
As VO on iOS is used much more often by blind people than ZoomText (ZoomText is used more by visually impaired people who would be able to see the big red alert box), this seems to be a sensible sacrifice.

What type of change is it?

  • Bug fix (non-breaking change which fixes an issue)

Has the documentation been updated?

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-511 Jun 29, 2017 Inactive

@@ -30,9 +30,9 @@ <h2 class="heading-medium implementation-advice">Implementation advice</h2>
</span>
</li>
<li>
indicate to screenreaders that the summary represents a collection of information
make sure that the summary gets announced to as many screen readers as possible

This comment has been minimized.

Copy link
@36degrees

36degrees Jul 3, 2017

Contributor

Minor copy suggestion to improve flow and be consistent with the other points:

Ensure that the summary is announced to as many screen readers as possible
use role="alert" on the containing div

@36degrees

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2017

Makes sense to me. I'm not an expert on roles by any means, but from a quick Google I don't think we're losing anything important by removing the group role.

@hannalaakso hannalaakso self-requested a review Jul 3, 2017

@hannalaakso

This comment has been minimized.

Copy link
Member

commented Jul 3, 2017

I'm guessing the role role "group" was meant to group related elements (h1, p, ul) together, just looking at form_error_radio_show_errors.html. But W3C say role "group" should be used for form controls so I think the group role here is not doing anything, unless there are form controls inside any of the divs which I couldn't see.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2017

I wonder if the aria-labelledby has any symbiotic relationship with the role=group and if it can be accessed the same way without it?

@36degrees, the copy change makes sense, I will change it when I am back.

@gemmaleigh

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

I had a look for the reason why role="group" was added, I'm afraid can't find the history of it. I can only assume it was added so that assistive technologies know that it's a container for a group of related content. https://www.w3.org/TR/wai-aria/roles#group

The aria-labelledby attribute is there to ensure the heading in the summary is associated with the summary box when using role="group". So when focus is set to the error summary, the heading is announced.

I'd be keen to know if the JavaScript which is setting focus to the error summary box can also be removed?

The documentation for role="alert" implies there isn't a need to also set focus to the summary box: https://www.w3.org/TR/wai-aria/roles#alert

Neither authors nor user agents are required to set or manage focus to them in order for them to be processed.

@hannalaakso

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

I've finally got a working machine so can do some testing on this. I'll pick this up tomorrow.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2017

@gemmaleigh, I have tested lots of variations, but I have not tested what happens if we don't set the focus. Good question. I would guess it's still needed due to inconsistent screen reader support but I can make sure and test it.

@hannalaakso, I can introduce you to our AT laptops and show you how to test in our various screen readers etc.

@hannalaakso

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

This claims that NVDA + Edge doesn't support role="alert". I guess we need to do some testing.

I did test removing the JS that sets focus and using role="alert" and VoiceOver on MacOS 10.11.6 announces it correctly. I was testing in form_error_multiple.html.

Thanks @selfthinker have slacked you separately re:intro 👍

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-511 Jul 13, 2017 Inactive

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2017

I don't think NVDA officially supports Edge yet. (Struggling to find evidence of that, though.) So, I think we can ignore that.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2017

I can currently not fully test this without the focus as our laptops with JAWS etc on them have connection issues.
I have just tested this in NVDA and Firefox, though, and that does definitely not work properly without the focus. It jumps straight to the first input field (after reading the page title) and skips the error summary box completely.
Another reason why the focus is still useful is mobile devices. It helps move the box into the viewport!

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

I have asked Leonie for feedback on this and she responded:

We used the group role to set an explicit role on the div element (without which the aria-labelledby attribute won't work). More on this here:
https://www.paciellogroup.com/blog/2017/07/short-note-on-aria-label-aria-labelledby-and-aria-describedby/

If the alert role is used instead it will achieve the same thing, so no problem with switching it over.

I haven't made the copy change @36degrees requested yet because I'm waiting for #509 to be merged as I know there will be conflicts.

Change error summary ARIA role to "alert"
An ARIA `role="alert"` on the error summary should not be necessary
due to the focus on that area.
But without this VoiceOver on iOS doesn't announce the error message
at all due to this bug: https://bugs.webkit.org/show_bug.cgi?id=163719
Changing this doesn't change the behaviour in JAWS, NVDA or VO on macOS,
but it does mean that ZoomText is not announcing anything.
As VO on iOS is used much more often by blind people than ZoomText
(ZoomText is used more by visually impaired people who would see
the big red alert box), this seems to be a sensible sacrifice.

@selfthinker selfthinker force-pushed the error-summary-with-role-alert branch from 126b3c2 to 7ef90d6 Jul 25, 2017

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2017

I have just made the copy changes. Although #509 isn't merged yet, this one could be merged before it. I guess it doesn't really matter which one is the one that is causing the other to conflict.

@36degrees
Copy link
Contributor

left a comment

LGTM

@selfthinker selfthinker merged commit 567f5c2 into master Jul 25, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@36degrees 36degrees deleted the error-summary-with-role-alert branch Jul 25, 2017

@36degrees 36degrees referenced this pull request Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.