-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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 on the root div after navigation #5593
Conversation
This fix applies focus to the main app div after page navigation, which triggers screen readers and other assistive tech to announce the new page. Thanks to @NickColley for initially pointing this out, and to @LJWatson for jumping in with a fix! fix gatsbyjs#5581
@calcsam Can you restart this build? It said "gatsby is not found", which makes me thing something hiccupped. https://app.netlify.com/sites/eloquent-golick-4ddc79/deploys/5b0db4f0b13fb1339e945589 |
Deploy preview for using-drupal ready! Built with commit 648ce92 |
Deploy preview for gatsbygram ready! Built with commit 648ce92 |
Nice work, I think we need to test in the following assistive technologies to know this has got us good coverage: https://www.gov.uk/service-manual/technology/testing-with-assistive-technologies#what-to-test Happy to do this for you :) |
@@ -149,6 +149,7 @@ class GatsbyLink extends React.Component { | |||
// This is just a normal link to the current page so let's emulate default | |||
// browser behavior by scrolling now to the top of the page. | |||
window.scrollTo(0, 0) | |||
document.querySelector(`#___gatsby`).focus() |
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 believe you'll need to add a tabindex="-1"
to the element, otherwise you cannot trigger programmatic focus.
This'll then likely show a focus outline that is not useful for most users, so you'd want to add some styles to remove this outline.
Just tested with NVDA, Firefox, Windows 10 and there's no response, I believe this is because of what I've noted inline. Can test again, or make this update for you if you fancy. Also, when pressing 'Down' the read the next item, it starts at 'Category: Salads' rather than the top of the page. |
I don't think adding this to gatsby-link cover all grounds here - for example using I think we should create internal plugin that will use |
Good points, @NickColley and @pieh. @NickColley, if you want to take this PR and run with it, I'd be happy to hand over the reins. @pieh I don't have much experience with internal plugins. Any good places to start learning about that? |
@jlengstorf There is not much to learn really about internal plugins:
|
Would love to run with this, to avoid bugging you with back and forth. Appreciate all the time you've put in so far! |
I tested the Drupal demo on JAWS 2018 on Windows 10 in Chrome and IE11. Chrome does not read the page title aloud on page change. IE11 does read the title (twice), although I'm thinking this is because IE11 doesn't support service workers and so is not getting the SPA experience and is actually just loading the page. I'm happy to test again with JAWS once we have an updated PR. |
Also just wanted to note that since Gatsbygram doesn't have unique page titles I don't think it will be that helpful to test there. |
Of note: https://reach.tech/router |
See also: #5656 |
quick update: and it does focus app container on navigation (app container is visibly outlined), but this doesn't seem to trigger screen reader I used for test (NVDA) to read out new page title/headings. Disabling js (so doing fallback to default browser navigation) does make NVDA to read out new page title/headings so not sure if focus trick will do good job with this :/ |
@pieh as noted in my comment in 5581, the screen reader behaviour can't be entirely faked, but it's ok because focus is in the expected part of the page to enable the user to explore/discover the information they need. |
@LJWatson https://reach.tech/router has managed to solve this issue so I'm not sure we have to compromise on that. |
@NickColley because changing router component that is deeply embeded into |
I meant more, we can copy the approach, not that we should integrate that project. |
I'm going to attempt moving us to reach-router for v2. It's both smaller (33kb -> 18kb) and ready for async React and it fixes this issue. My understanding are that the API changes are minimal and most sites shouldn't have to do anything to upgrade. |
FYI, my PR adding @reach/router is about ready to go in #6918 |
This fix applies focus to the main app div after page navigation, which triggers screen readers and other assistive tech to announce the new page. Thanks to @NickColley for initially pointing this out, and to @LJWatson for jumping in with a fix!
fix #5581