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

Sub-optimal page title vocal announcement #24021

Closed
KittyGiraudel opened this issue Apr 13, 2021 · 5 comments
Closed

Sub-optimal page title vocal announcement #24021

KittyGiraudel opened this issue Apr 13, 2021 · 5 comments
Labels
bug Issue was opened via the bug report template.

Comments

@KittyGiraudel
Copy link
Contributor

What version of Next.js are you using?

10.1.x

What version of Node.js are you using?

14.15.x

What browser are you using?

Brave

What operating system are you using?

macOS

How are you deploying your application?

next export

Describe the Bug

Originally issued as a comment: #23086 (comment).


PR #23086 introduced an accessibility improvement aiming at making same-page routing more accessible by voicing the page title upon route change. That’s great, and it works alright as far as I can tell. That being said, I think the logic might be a little backwards.

In my opinion, we should use the document.title first if available, since this is what would happen in a non-SPA scenario upon page change. And if we do not have a page title, then we should use the h1 (if any, of course). Right now it will announce the content of the h1, despite having a perfectly clean document.title, which I think is not the best experience.

useEffect(
() => {
if (!initialPathLoaded.current) {
initialPathLoaded.current = true
return
}
let newRouteAnnouncement
const pageHeader = document.querySelector('h1')
if (pageHeader) {
newRouteAnnouncement = pageHeader.innerText || pageHeader.textContent
}
if (!newRouteAnnouncement) {
if (document.title) {
newRouteAnnouncement = document.title
} else {
newRouteAnnouncement = asPath
}
}
setRouteAnnouncement(newRouteAnnouncement)
},
// TODO: switch to pathname + query object of dynamic route requirements
[asPath]
)

I’m happy opening an issue/PR if you think this is a valid concern.

Pinging @kyleboss, @shuding and @olpeh since they were all involved in the PR.

Expected Behavior

Taking the home page of the Gorillas website as an example: the page title is “Home | Gorillas” while the h1 is “Groceries delivered in 10 minutes”. Navigating to the home page should announce “Home | Gorillas”, not the baseline (or catch phrase, or whatever we want to name it). And while in many cases, the h1 and the document.title would be in sync, I believe this is not always the case. In a case like this, we also don’t want the h1 to just contain “Home.”

To Reproduce

  1. Open https://gorillas.io/en/accessibility-statement with VoiceOver.
  2. Open the menu and click “Home”.
  3. Listen to the page title announcement.
@KittyGiraudel KittyGiraudel added the bug Issue was opened via the bug report template. label Apr 13, 2021
@shuding
Copy link
Member

shuding commented Apr 16, 2021

Thanks @KittyGiraudel, yeah I like the idea of taking document.title first, feel free to open a PR! 👍

kodiakhq bot pushed a commit that referenced this issue 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)).
@davidluhr
Copy link

Just started work on a new client's NextJS app today and was happy to find this issue was already reported and that @KittyGiraudel happened to fix it today. Thank you for this!

I'm excited to inherit this improvement, since it's something I noted in my initial accessibility assessment.

@KittyGiraudel
Copy link
Contributor Author

@shuding Shall we close this?

@shuding
Copy link
Member

shuding commented Nov 13, 2021

Oops, sure this can be closed now!

@shuding shuding closed this as completed Nov 13, 2021
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. 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.
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

No branches or pull requests

4 participants