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.

Sign up for GitHub

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

Jump to bottom

Accessible Routing #19290

Merged
merged 16 commits into from Jan 17, 2020
Merged

Accessible Routing #19290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
164eeba
add announcer live region for routing
Nov 5, 2019
1b8221f
Update packages/gatsby/cache-dir/production-app.js
madalynrose Nov 6, 2019
c525e4a
Merge branch 'master' of github.com:gatsbyjs/gatsby into a11y-routing…
Dec 11, 2019
10024e1
merged in master
Dec 17, 2019
752682e
refactor announcement updates to happen in lifecycle method instead o…
Dec 18, 2019
6f52df1
remove unused imports
Dec 18, 2019
81cfa5a
Merge branch 'a11y-routing-mvp' of github.com:gatsbyjs/gatsby into a1…
Dec 18, 2019
842830f
move RouteAnnouncer inside of RouteUpdates in navigation.js
Jan 7, 2020
810b9e1
remove console.logs
Jan 7, 2020
828f05f
Trying with ref
freiksenet Jan 9, 2020
505d0c1
Merge branch 'master' of github.com:gatsbyjs/gatsby into a11y-routing…
Jan 10, 2020
2b0b6b6
remove stray console.log
Jan 10, 2020
4fb2972
Merge branch 'master' of github.com:gatsbyjs/gatsby into a11y-routing…
Jan 16, 2020
73caa3e
change div for adding announcer to a react fragment
Jan 16, 2020
ba53b68
remove commented out code
Jan 17, 2020
5b15471
add accidentally deleted code back in, remove changes in root.js
Jan 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 55 additions & 2 deletions packages/gatsby/cache-dir/navigation.js
View file
Open in desktop
Expand Up @@ -43,7 +43,6 @@ const onPreRouteUpdate = (location, prevLocation) => {
const onRouteUpdate = (location, prevLocation) => {
if (!maybeRedirect(location.pathname)) {
apiRunner(`onRouteUpdate`, { location, prevLocation })

// Temp hack while awaiting https://github.com/reach/router/issues/119
window.__navigatingToLink = false
}
Expand Down Expand Up @@ -159,6 +158,55 @@ function init() {
maybeRedirect(window.location.pathname)
}

class RouteAnnouncer extends React.Component {
constructor(props) {
super(props)
this.announcementRef = React.createRef()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 tip (feel free to do this if you want). But you can ditch the constructor here so we're just instantiating a class variable. Do it like this instead:

class Router {
  announcementRef = React.createRef()
}

It'll achieve the same result! (if this doesn't work ignore, it does require a specific babel setup to make it work and maybe we don't have that)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to leave this for a later iteration. I'm ready to get this thing OUT!


componentDidUpdate(prevProps, nextProps) {
requestAnimationFrame(() => {
let pageName = `new page at ${this.props.location.pathname}`
if (document.title) {
pageName = document.title
}
const pageHeadings = document
.getElementById(`gatsby-focus-wrapper`)
.getElementsByTagName(`h1`)
if (pageHeadings && pageHeadings.length) {
pageName = pageHeadings[0].textContent
}
const newAnnouncement = `Navigated to ${pageName}`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about internationalization support?

const oldAnnouncement = this.announcementRef.current.innerText
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, one thing I just thought of. We should add a bailout check if the ref isn't attached yet. It feels unlikely to occur, but technically ref's type is {current: HTMLElement | null}. It's possible this could come up later on with suspense. All you need to do is at the top of this rAF, put a if (!this.announcementRef.current) return

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blainekasten Suspense won’t change anything here.

It’s impossible for the ref to be null during componentDidUpdate, though note that because of the rAF call it is possible (though probably unlikely) for the component to unmount before this code runs, so a check is appropriate.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blainekasten I think this check is still missing in the code. Just talked to someone on Discord getting Cannot read property 'innerText' of null on this line. Don't know if that's the cause of his problem, or a side-effect.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting this error while trying to debug problems with Ms Edge (14). Happens when I try to navigate to a new page.

if (oldAnnouncement !== newAnnouncement) {
this.announcementRef.current.innerText = newAnnouncement
}
})
}

render() {
return (
<div
id="gatsby-announcer"
style={{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please extract object to const outside of class scope, or React.useMemo hook

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add eslint-plugin-react-perf, to escape this kind of performance implications.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a very minor perf issue. Creating one object on re-renders (which only happen on route change). It's probably easier to just implement a shouldComponentUpdate: () => false

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easier - does not mean best way. Counting that react moving to functions, instead of components, and original advise do not use shouldComponentUpdate, this style object already a symptom that there could be much more performance issues in gatsby.js project itself. A lot of very minor issues adds up pretty darn fast

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Former React team manager here – I can guarantee this will never be a performance problem and there is nothing wrong with writing the style object like this. I’d recommend you stop worrying about microoptimizing code.

position: `absolute`,
width: 1,
height: 1,
padding: 0,
overflow: `hidden`,
clip: `rect(0, 0, 0, 0)`,
whiteSpace: `nowrap`,
border: 0,
}}
role="alert"
aria-live="assertive"
aria-atomic="true"
ref={this.announcementRef}
></div>
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this component doesnt render any dynamic data, I wonder if we could add a

shouldComponentUpdate() { return false; }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is still getting used to using refs like this

does this.announcementRef.current.innerText not count as dynamic data being rendered?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope. On initial render, the ref will be passed to React, who will attach it to the node. It happens asynchronous and does not affect component lifecycles.

Changing the ref will not cause any re-renders ever!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I'm trying to leverage my new-ness to React as something useful and ask my potentially trivial questions out in the open 🙈

Our changes happen to the announcement ref happen within the componentDidUpdate lifecycle method. Wouldn't returning false inside shouldComponentUpdate prevent it from being invoked?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great questions! And you know what, I'm wrong. 😆 Since you are doing it in DidUpdate, that occurs after a render. So stopping update stops that cycle. Let's just ship this, I think there are some changes that would better align with React, but this works. So let's ship and we can see if there are better iterations later :)


// Fire on(Pre)RouteUpdate APIs
class RouteUpdates extends React.Component {
constructor(props) {
Expand Down Expand Up @@ -186,7 +234,12 @@ class RouteUpdates extends React.Component {
}

render() {
return this.props.children
return (
<React.Fragment>
{this.props.children}
<RouteAnnouncer location={location} />
</React.Fragment>
)
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/gatsby/cache-dir/production-app.js
View file
Open in desktop
Expand Up @@ -142,7 +142,7 @@ apiRunnerAsync(`onClientEntry`).then(() => {
}
).pop()

let NewRoot = () => WrappedRoot
const NewRoot = () => WrappedRoot

const renderer = apiRunner(
`replaceHydrateFunction`,
Expand Down
2 changes: 1 addition & 1 deletion packages/gatsby/cache-dir/root.js
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@ const RouteHandler = props => (

class LocationHandler extends React.Component {
render() {
let { location } = this.props
const { location } = this.props

if (!loader.isPageNotFound(location.pathname)) {
return (
Expand Down