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.
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
[a11y] Route Announcements #20428
[a11y] Route Announcements #20428
Changes from 14 commits
eafdb89
323a952
e22817b
9a48bca
333ab3d
7156183
991cd67
ac60dcc
1c4bf60
8cc9a83
c15507b
5c2dbe1
0080959
8adf039
845063c
9db8424
39ccbde
54dcfa3
85ef996
b437b2e
8884236
902426a
5293465
2b15725
8c28dd9
0ce69dc
596ae18
9b21d10
54b5721
b70aa27
8fc903e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A paragraph would be a little better semantically. :)
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.
So I did not test it yet, but I am pretty sure this is going to announce the document’s title twice. Once from
RouteAnnouncer
, since it’s an assertive live region, and then a second time when focusing the document’s main title.I’m not entirely certain, but it feels like you are mixing two approaches here:
Focusing the main heading. Upon route change, and provided you can guarantee there will be a
<h1>
element regardless of state such as loading, move the focus to the main heading. As a result, its content will be announced.Announcing the page title. If the presence of a
<h1>
cannot be guaranteed for any reason (because left to the implementor who might not know, or because depending on loading states for instance), move the focus to a visually hidden node as<body>
’s first child containing the new document title (which doesn‘t have to be a region whatsoever).Both are equally good as far as I know. The main difference is where the focus ends up being. In the first approach, the focus is moved to the main page title, which could be anywhere in the page, really. Some sites have it in the title, some have it in the main container, some have it after the navigation, some before… In the second case, the focus is consistently moved at the top of the body (where the
RouteAnnouncer
DOM node lives), just like a regularhref
link would after a page load.Because you appear to be essentially doing both, I am pretty certain this will announce the document’s main title twice, which is probably not what you want. I might be entirely mistaken though, so feel free to correct me. :)
Edit: a little more information in that article.
Edit 2: Finding the first
<h1>
element on the page could lead to issues if a hidden dialog with a<h1>
element is being rendered high up the body element. That heading will be queried because it’s the first, but attempting to focus it would not work because it is within a hidden dialog.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.
I agree with the last point about the h1 selector which is a bit dangerous because it contains an assumption about the website structure.
Also the explanation about how the title is now announced twice sounds reasonable to me, but haven't tried what happens.
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.
Hey! Thank you so much for taking the time to look at this, and more importantly, for the feedback! ❤️
I think your judgement is 100% valid. The intent here was to bring focus to the most important part of the page; however, this makes quite a few assumptions about the user's site. For example, the heading might be visually hidden. This alone makes me think that, while focusing on the top-most heading might be helpful for many sites, just focusing on the body will be best. So I will remove the logic for querying the header for the purpose of focusing.
The following is probably a moot point, but because the
<RouteAnnouncer />
usesaria-live="assertive"
, the screen reader is expected to stop what it's doing and announce the new page name. Since the focus & route announcement happen at the same time, most screen readers will give priority to the<RouteAnnouncer />
and not announce the focused element. I've tested this on a few screen readers.Look out for a commit soon with this change 😉
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.
Hey Kyle! Thank you for taking the time to reply. 😊
My personal suggestion—which was recommended to me by @goetsu when he conducted an audit of the N26 platform a few years back—is to have this small visually hidden text node containing the up-to-date page title at the top of the body element, and moving the focus to it on page change.
By focusing this element—which is ultimately similar to focusing the body—you can skip the whole aria region thing. When this element receives the focus, its content is read out loud. Done and simple.
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.
Oh, I absolutely love this idea! It's really elegant. I can already tell that this will decrease the complexity significantly. Thanks @KittyGiraudel!
I am putting this into a commit now!
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.
I don't know if it's solvable here, but I don't see that being discussed—this refocus behavior breaks
autoFocus
elements as they won't be focused on page transition anymore. However theautoFocus
attribute itself might be a bad thing for a11y, so I'm not sure what the best solution is.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.
@shuding, that's a very good point.
autoFocus
is broken with this PR. As to not break any functionality, I'll hold off on the focusing-side of this PR and dedicate a new PR for it in the near-future.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.
Thank you @kyleboss, I'll land the Route Announcer part then 👍