-
Notifications
You must be signed in to change notification settings - Fork 328
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
Amend error summary markup to fix page load focus bug in JAWS 2022 #2677
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your hard work investigating this @owenatgov – glad we've got a solution that works for JAWS!
Given this change is likely to affect other assistive technologies as well, it'd be really useful if this PR included:
- how you have been testing the change
- the exact behaviour in JAWS before / after the change (mostly helps the reviewer, but also useful if a similar issue comes up in the future and we want to compare the behaviour)
- what happens in each of the main assistive tech / browser combinations that we support, ideally compared to the current behaviour (so we can be confident that we're not making things worse in e.g. VoiceOver on iOS)
It'd also be great if we can capture a bit more detail in the commit messages – in particular why the change is being made in 40d777e.
It might be worth taking a look at PRs for some of the other recent screenreader related changes to see what we've done in the past. (I know this isn't something we're particularly consistent at yet, and ideally this should be better documented!)
Very happy to talk things through if you want clarity on anything!
d0c3413
to
1777ad1
Compare
@36degrees I've updated the PR description with further testing details and updated the commit history with a more logical flow of work and more details per change. I'll be doing some final testing of my solution on other screen readers to ensure it's all working correctly soon. Let me know if there's more I can do to tidy this up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, the commits are logical and well explained. There's a small typo in the first commit ("remvoed").
Do we have a feel for which milestone this could go into? Quite excited to use this in our services :)
1777ad1
to
491645a
Compare
Results of final pass browser testingTesting done using this branch and the passport details page in the review app.
Personal analysis of this testing is that there are some imperfections, notably in VoiceOver, however we are making a marked improvements so recommend we proceed with solution. I'm keen to compare testing results from a reviewer. |
Trying the same browser/AT combinations as above, on the same page in this branch's review app. NVDA returned the same results as @owenatgov's testing, as did VoiceOver (with caveats), however my experience with JAWS was markedly different and incredibly inconsistent when making multiple runs. JAWS 2022 / Chrome 103I received some wildly different results each time I tried JAWS. When using a mouse, the result seemed to be contingent on how quickly I navigated. If I submitted the form before JAWS had completed announcing 'Continue button, to activate press enter' then the error summary on the next page wouldn't be announced. If I let the announcement finish first, then it would read 'heading level 2, There is a problem', but not the list of errors. When using a keyboard, 'heading level 2, There is a problem' was read out consistently, regardless of whether the previous announcement had finished or not. In one instance which I couldn't consistently reproduce, it read out 'There is a problem' and the text of the error list, without announcing the elements. On page refresh, the error summary was never read out. In all situations keyboard focus was moved to the error summary, so navigating downwards would announce the heading and list of errors. JAWS 2022 / Edge 103I couldn't get Edge to read out the error summary on page load by any method. Like Chrome, the keyboard focus was moved to the error summary so it was possible to read the error summary by manually navigating downwards. NVDA 2022.1 / Chrome 103, Firefox 102The 'There is a problem' heading and list of errors was read out consistently on page load or reload. The error summary consistently received keyboard focus. VoiceOver / Safari 15.5When navigating using VoiceOver controls (VO key + arrows), the error summary was not announced on page load or reload, and was only read in the context of the entire page. The heading of the error summary received focus, and that the user was currently on a 'heading level 2' was announced, but the text of the heading was not.1 I was able to announce the heading and navigate to the list of errors using VO controls. When using the tab key to navigate, the 'There is a problem' heading's text was announced, and the error summary container received focus. However, it was not possible to navigate into the list of errors using the tab key. Footnotes
|
I was hoping this would solve this problem with NVDA: #2055 but it looks like it's still an issue. Just popping it here for completeness - we've filed it as a bug with NVDA. |
491645a
to
957e7d7
Compare
I've been playing around with this again in response to @querkmachine and @domoscargin's testing and have some odd findings: Firstly to verify the above review testing, JAWs is indeed very inconsistent. There doesn't seem to be any rhyme or reason to when it reads out the full error summary and when it just reads out the heading or just nothing at all ("blank"). I have a hypothesis that either there is a definite bug in JAWs that we've been boxing with this whole time or there's something else in the component that's causing problems. Secondly, I've been playing around with the component js a bit more and I tried setting the programatic |
4b84871
to
7f69fb6
Compare
Have updated the solution to use 0 on the tabindex instead of -1. Testing again using the passport details page. ResultsJAWS 2022 / Chrome 103JAWS continues to be flakey. It read out the entire contents of the error summary more often than in didn't and when it didn't, screen reader focus consistently remained in the correct place, allowing users to reach the links next. JAWS 2022 / Edge 103Seems to be more flakey than Chrome. Read out the entire error summary about 25% of the time. Would never have focus in the correct place, often it was either on the continue button or one of the questions. NVDA 2022.1 / Firefox 102Reads out the entire error summary on load consistently. It also maintained screen reader focus on the error summary on refresh every time. @domoscargin It may be worth you retesting the issue you linked to make sure that I haven't had an exceptionally lucky session. VoiceOver / Safari 15.5Unexpectedly chaotic. Wouldn't move focus to the error summary on page reload when using VO navigation. The user would eventually receive the error summary content via the entire page getting announced, which seemed to be triggered 75% of the time in testing. ThoughtsThere are obviously still problems however from extensive testing I can't find anything that could be causing this on our end. I'll leave this to those reviewing and testing this to decide if this is worth merging regardless. I feel like it's still a positive addition as it cleans up the markup and provides a broadly better experience than we had previously. I'm planning on updating the original issue and finally raising issues to screen reader vendors about this shortly. |
Thanks for the continued picking apart of this stuff! I can still get flaky NVDA behaviour on the deployment, but again, I wouldn't let that stop this PR in any way. If, as part of raising issues to screen reader vendors you come up with a good minimal test case for these related issues, please do let me know and I can update our issue with NVDA so they can take a look. |
7f69fb6
to
a202e6c
Compare
Following a bit of a return to centre after updating the version of JAWS in the GDS AssistivLabs account, I've updated this PR along with the description with an amended solution and some more methodical testing results as per our accessibility testing guidance. |
a202e6c
to
6bd8421
Compare
@vanitabarrett Changelog entry added. Let me know what you think, although it in theory is gonna get reviewed by content folks before the next release. |
6bd8421
to
60257b5
Compare
60257b5
to
4ace1d8
Compare
CHANGELOG.md
Outdated
- Add a `div` wrapper around the contents of `govuk-error-summary` with the attribute `role="alert"` | ||
- Remove `id="error-summary-title"` from the error summary `h2` (`govuk-error-summary__title`) | ||
|
||
This will enable screen reader users to have a better, more coherent experience with the error summary. Most notably it will ensure that users of JAWS 2022 and up will hear the entire contents of the error summary on page load and therefore have further context on why there is an error on the page they're on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good here, thanks! I don't feel too concerned about length, seems pretty typical for the changelog?
Did a quick ctrl-F and looks like we say "or later" when referring to software versions.
This will enable screen reader users to have a better, more coherent experience with the error summary. Most notably it will ensure that users of JAWS 2022 and up will hear the entire contents of the error summary on page load and therefore have further context on why there is an error on the page they're on. | |
This will enable screen reader users to have a better, more coherent experience with the error summary. Most notably it will ensure that users of JAWS 2022 and later will hear the entire contents of the error summary on page load and therefore have further context on why there is an error on the page they're on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Calv! I've made this change locally and I'm just waiting for the 4.3.1 release before I push and merge
In JAWS 2022, this attribute was creating a race condition between the role="alert" and the existing javascript which moves focus on page load. Moving the role="alert" into its own child div decouples the race condition and lets screen readers announce the alert whilst being located at the error summary in relation to sequential keyboard navigation.
In JAWS 2022, the screen reader is focusing on the error summary on load and only reading out the title because of this aria-labelledby. By removing it, we make it possible to access the content inside the error summary, communicating to screen readers why there is a problem.
4ace1d8
to
368a404
Compare
🙌 Amazing work all!! |
We were a bit behind latest versions, and also in the last version of the govuk frontend, the markup for the error summary has been changed to improve accessibility. Reference: alphagov/govuk-frontend#2677 In addition to this I noticed a JS error in the initialisation of the accessible autocomplete as it didn't check if we had elements to apply the code or not. Fixed this and also now it uses a `data` attribute for more flexibility if we add more of these autocompletes in the future.
Bump form builder gem and frontend We were a bit behind latest versions, and also in the last version of the govuk frontend, the markup for the error summary has been changed to improve accessibility. Reference: alphagov/govuk-frontend#2677 In addition to this I noticed a JS error in the initialisation of the accessible autocomplete as it didn't check if we had elements to apply the code or not. Fixed this and also now it uses a `data` attribute for more flexibility if we add more of these autocompletes in the future.
Fixes #2657
What/Why
Makes the following changes to the error summary component:
role="alert"
from the parent error summary element to a child container as it was causing a race conditionaria-labelledby
and the associatedid
from the error summary title to reduce unnecessary verbosityTesting
As well as the latest version of JAWS 2022, this was tested against screen reader/combinations as outlined in the service manual's assistive technology testing guidance. I applied the following criteria when testing:
Testing results
Credits
Thank you to @tvararu, @fofr and their team at DfE for their continued support on solving this issue.