-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fix focus issues #381
Merged
Merged
Fix focus issues #381
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
565ab84
to
d7f16a6
Compare
kr8n3r
approved these changes
Jan 14, 2025
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.
changes look good
seems like an issue with latest ubuntu image, ssl version and phantomjs |
When using the dialog with a keyboard, focus is not set when you close it so the browser has to guess what to focus. This is usually on the document itself or the main content area, both of which are confusing to users. This focuses the button that opens the dialog when it closes, as per WAI ARIA advice from the W3C: https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/
The main content area has the following markup: ``` <div class="app-pane__content" tabindex="0"> <main> ... </main> </div> ``` `div.app-pane__content` has `tabindex=0` to make it focusable (`div`s are not by default). It is focusable so keyboard users can scroll it, using their arrow keys when it is tabbed to. This is important because the docs' page is split into 2 panes that scroll independently. `main` is focused when you click the 'skip to main content' link, at the start of the page, using the design system skip link component: https://design-system.service.gov.uk/components/skip-link/ These changes don't try to merge these tags but rather make the visuals show a single focus style for both, because users shouldn't care which one is focused. Both alow you to scroll the pane and represent the main content area. This commit also includes a change that removes the outline from links in the table of contents. Testing the other changes in this commit, I saw the outline style from the browser styles applied to table of contents links. focus-visible styles are displayed based on browser heuristics, which seem to kick in when the other changes are applied. This cancels them on the link without removing them from the child `<span>`.
d7f16a6
to
bcc23c5
Compare
tombye
added a commit
that referenced
this pull request
Jan 15, 2025
- [Fix focus issues with table of contents and main content pane](#381)
Merged
tombye
added a commit
that referenced
this pull request
Jan 21, 2025
- [Fix focus issues with table of contents and main content pane](#381)
tombye
added a commit
to alphagov/notifications-tech-docs
that referenced
this pull request
Feb 26, 2025
Brings in the following accessibility fixes: - making focus return to the table of contents button when its dialog closes - stop the main content pane being focusable on mobile or at high zoom levels - give the main content pane a focus style on desktop or at the default zoom level See alphagov/tech-docs-gem#381
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What’s changed
Fixes the following accessibility issues discovered when using Notify's tech docs with a keyboard:
Identifying a user need
Something always needs to be in focus when using the keyboard, so you know what you're interacting with. These issues directly affect keyboard users by breaking that in a few places.
What happens to focus when you close the toc dialog
Before
toc_dialog_close_focus_issue_before.mov
After
toc_dialog_close_focus_issue_after.mov
Main content area being focusable on mobile
Before
main_content_area_focusable_on_mobile_before.mov
After
main_content_area_focusable_on_mobile_after.mov
Main area focus style
Before
main_content_area_focus_fix_before.mov
After
main_content_focus_fix_after.mov
Notes for reviewers
Better to read this commit-by-commit.