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

[a11y] Route Announcements #20428

Merged
merged 31 commits into from
Mar 15, 2021
Merged

[a11y] Route Announcements #20428

merged 31 commits into from
Mar 15, 2021

Conversation

kyleboss
Copy link
Contributor

@kyleboss kyleboss commented Dec 23, 2020

Route Announcements

Summary

This PR improves the accessibility of NextJS's client-side navigation by announcing route changes to screen readers.

Context

When a user who is sighted clicks on a link, they can see the content change. It's an affirmation that what the user intended to do by clicking a link actually worked! Users navigating the page via a screen-reader will not get this feedback on NextJS sites (This is an issue on many SPA-like architectures).

nextjs-before.mov

Solution

Whenever there is a route change, the new <RouteAnnouncer /> will look for a name to give the new page and then announce it! The name is found by first looking for an h1, falling back to document.title, and lastly to pathname. <RouteAnnouncer /> is a visually hidden component placed within the <AppContainer />.

Demo

nextjs-after.mov

Inspiration

First and foremost, this PR was inspired by @marcysutton's studies and writing, What we learned from user testing of accessible client-side routing techniques with Fable Tech Labs
as well as @madalynrose's Accessible Routing PR for Gatsby.

There were also learnings gleaned from the conversations within #7681.

Related Issues & PRs

Announce client-navigation changes.
Place focus on relevant content after a navigation change.

Tests included.
@vercel vercel bot temporarily deployed to Preview December 23, 2020 16:34 Inactive
@ijjk

This comment has been minimized.

@vercel vercel bot temporarily deployed to Preview December 23, 2020 16:41 Inactive
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@vercel vercel bot temporarily deployed to Preview December 28, 2020 18:20 Inactive
@vercel vercel bot temporarily deployed to Preview December 28, 2020 18:22 Inactive
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@vercel vercel bot temporarily deployed to Preview December 28, 2020 21:15 Inactive
@vercel vercel bot temporarily deployed to Preview December 28, 2020 21:17 Inactive
@ijjk

This comment has been minimized.

@kyleboss kyleboss dismissed a stale review via ac60dcc December 30, 2020 18:15
@vercel vercel bot temporarily deployed to Preview December 30, 2020 18:15 Inactive
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@marcysutton
Copy link

marcysutton commented Jan 12, 2021

Cool PR! I's really great that you took the time and effort to put this together, as the impact of inaccessible client-side routing is quite broad with the number of NextJS sites out there.

A couple of high-level notes:

Without routing-change announcements, keyboard users currently do not get that same feedback on NextJS sites (FWIW: This is an issue on many SPA-like architectures).

The announcements are for screen readers and other assistive technologies, so keyboard users wouldn't really benefit from that detail. Moving keyboard focus is more relevant in that case.

The page will then look for an h1, falling back to a main, and lastly to document.body to bring the user's focus to. Do note that in order to give focus to these elements, a tabIndex is levied temporarily until the element is unfocused.

The research of mine that you mentioned recommended that focus should be sent to a small, keyboard-interactive element like a skip link. Sending focus to an h1 or wrapper element works for some aspects, but wasn't really the recommended solution. I also want to make sure visible focus outlines are considered, even if it takes using :focus-visible or something like what-input to keep mouse users happy. There is a lot of value in showing the user's focus point after it moves, and focus outlines are often turned off for wrapper elements (and in general).

Lastly, to the Vercel team: I wish this issue hadn't been left to the community to take on as accessibility is an important concern for all framework core teams, and you need to address it. I hope this PR will get the reviews and attention it requires to move forward.

@jhackett1
Copy link

the example videos seem to be broken - can we fix it?

@kyleboss
Copy link
Contributor Author

Hey @jhackett1 -- Which example videos appear to be broken? The ones in the PR description seem to be working for me.

@jonasatmonday
Copy link

jonasatmonday commented Jul 13, 2021

Hello, we are using Next 10.2.0 and as I read it, this feature should be available since Next 10.0.9. For some reason, it does not work and still focuses something in the middle of the page upon rendering the new page. We have however a <h1> element (two in fact due to a chat overlay - is this a problem or will it just use the first h1 it finds?). As a fallback, the mechanism should also be able to use our title in the HTML-head:

<title>Rückrufservice</title>

Do we need to configure anything specific to use the new <RouteAnnouncer/>?

Edit: I tested with the MacOS VoiceOver for the announcement of the new page.
Edit: I now tested it on a page that only has one h1 and there the announcement works. Google states that multiple <h1> element on a page are not a problem as long as they semantically make sense. Which is the case with a chat overlay button bringing in a <h1>. I will open a separate issue with a request to support multiple <h1>.
Last edit (I guess): I debugged the route-announcer and figured out, that multiple <h1> are not the issue as the document.querySelector('h1') will find the first element. The problem is with the path not changing and therefore the useEffect is not triggered. That's an issue in our project we need to figure out.

@gaearon
Copy link
Contributor

gaearon commented Nov 7, 2021

Is reliance on innerText over textContent (where available) intentional? I'm seeing it in the style recalc traces.

Screenshot 2021-11-07 at 01 36 02

Screenshot 2021-11-07 at 01 36 12

Screenshot 2021-11-07 at 01 36 17

Per https://gist.github.com/paulirish/5d52fb081b3570c81e3a, reading innerText invalidates layout.


Based on their difference seems like it's intentional but it's still nice if there was some way to opt out of the innerText use and somehow let the developer specify the text to announce directly.

@KittyGiraudel
Copy link
Contributor

I didn’t do extensive tests but my understanding is that textContent could yield incorrect results if the h1 element we read the text from contains visually hidden content.

This StackOverflow answer explains it nicely with a concise example:

innerText returns the visible text contained in a node, while textContent returns the full text. For example, on the following HTML <span>Hello <span style="display: none;">World</span></span>, innerText will return 'Hello', while textContent will return 'Hello World'.

Invisible content within a node could happen for a variety of reasons: it could be added to provide additional context for assistive technologies, it could be an editing artefact from a CMS, it could be an SVG icon or a temporarily hidden anchor link to jump to the h1.

It feels like innerText is overall safer and more aligned with what is expected (visual text). I’m curious though, is the performance difference that significant?

@shuding
Copy link
Member

shuding commented Nov 8, 2021

Another choice is to make document.title take priority over h1.innerText/h1.textContent, which also makes sense. I don’t remember why we went with the other way though.

@KittyGiraudel
Copy link
Contributor

KittyGiraudel commented Nov 8, 2021

Another choice is to make document.title take priority over h1.innerText/h1.textContent, which also makes sense. I don’t remember why we went with the other way though.

I’ve been meaning to issue a pull-request for this since April and I originally didn’t have time and then I simply forgot. But yes, we definitely should, as mentioned in that issue. I’ll put this back onto my radar.

This won’t eliminate the issue outlined by Dan though, it will just make it less likely.

Edit: done. :)

kodiakhq bot pushed a commit that referenced this pull request Nov 8, 2021
…31147)

## Improvement

This pull-request should address #24021, improving the page change announcement for assistive technologies by giving priority to `document.title` over `h1`. Interestingly, it would also improve a potential performance bottleneck by skipping calls to `innerText` on the main `h1` raised in [this comment](#20428 (comment)).
@gaearon
Copy link
Contributor

gaearon commented Nov 8, 2021

Amazing, thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Routing isn't screen-reader accessible?