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

Accessible Routing #19290

Merged
merged 16 commits into from
Jan 17, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions packages/gatsby/cache-dir/navigation.js
Original file line number Diff line number Diff line change
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}`

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

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.

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.

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

@JustFly1984 JustFly1984 Jan 18, 2020

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

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

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

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

@madalynrose madalynrose Jan 17, 2020

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
8 changes: 1 addition & 7 deletions packages/gatsby/cache-dir/production-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,6 @@ apiRunnerAsync(`onClientEntry`).then(() => {
}
}

const { pagePath, location: browserLoc } = window

// Explicitly call navigate if the canonical path (window.pagePath)
// is different to the browser path (window.location.pathname). But
// only if NONE of the following conditions hold:
//
// - The url matches a client side route (page.matchPath)
// - it's a 404 page
// - it's the offline plugin shell (/offline-plugin-app-shell-fallback/)
Expand Down Expand Up @@ -142,7 +136,7 @@ apiRunnerAsync(`onClientEntry`).then(() => {
}
).pop()

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

const renderer = apiRunner(
`replaceHydrateFunction`,
Expand Down
10 changes: 7 additions & 3 deletions packages/gatsby/cache-dir/root.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,19 @@ const RouteHandler = props => (
</BaseContext.Provider>
)

let locationListener
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this? I can't see how it's used at the moment

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

if (!loader.isPageNotFound(location.pathname)) {
return (
<EnsureResources location={location}>
{locationAndPageResources => (
<RouteUpdates location={location}>
<RouteUpdates
location={location}
locationListener={locationListener}
>
<ScrollContext
location={location}
shouldUpdateScroll={shouldUpdateScroll}
Expand Down Expand Up @@ -96,7 +100,7 @@ class LocationHandler extends React.Component {
}

return (
<RouteUpdates location={location}>
<RouteUpdates location={location} locationListener={locationListener}>
<Router
basepath={__BASE_PATH__}
location={location}
Expand Down