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

Accessible Routing #19290

Merged
merged 16 commits into from
Jan 17, 2020
Merged

Accessible Routing #19290

Show file tree
Hide file tree
Changes from 9 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 58 additions & 2 deletions packages/gatsby/cache-dir/navigation.js
View file
Open in desktop
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,58 @@ function init() {
maybeRedirect(window.location.pathname)
}

class RouteAnnouncer extends React.Component {
constructor(props) {
super(props)
this.state = { announcement: `` }
}
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) {
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) {
pageName = pageHeadings[0].textContent
}
let newAnnouncement = `Navigated to ${pageName}`
if (this.state.announcement !== newAnnouncement) {
this.setState({
announcement: newAnnouncement,
})
}
})
}

render() {
const { announcement } = this.state
return (
<div
id="gatsby-announcer"
style={{
Copy link

@JustFly1984 JustFly1984 Jan 18, 2020 •
edited
Loading

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"
>
{announcement}
</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 •
edited
Loading

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 +237,12 @@ class RouteUpdates extends React.Component {
}

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

Expand Down
66 changes: 65 additions & 1 deletion packages/gatsby/cache-dir/production-app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ window.___loader = publicLoader
navigationInit()

apiRunnerAsync(`onClientEntry`).then(() => {
console.log(`HELLO`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray console log.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why HELLO to you too

// Let plugins register a service worker. The plugin just needs
// to return true.
if (apiRunner(`registerServiceWorker`).length > 0) {
Expand Down Expand Up @@ -92,6 +93,69 @@ apiRunnerAsync(`onClientEntry`).then(() => {
}
}

// class RouteAnnouncer extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

// constructor(props) {
// super(props)
// this.state = { announcement: `` }
// console.log(`constructor`)
// }

// componentDidUpdate(prevProps) {
// console.log(`did update`, this.props.location.pathname)
// if (this.props.location.pathname !== prevProps.location.pathname) {
// requestAnimationFrame(() => {
// console.log(`updating`, this.props.location.pathname)
// 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) {
// pageName = pageHeadings[0].textContent
// }
// let newAnnouncement = `Navigated to ${pageName}`
// if (this.state.announcement !== newAnnouncement) {
// console.log(
// `setting state`,
// this.state.announcement,
// newAnnouncement
// )
// this.setState({
// announcement: newAnnouncement,
// })
// }
// })
// }
// }

// render() {
// console.log(`rendering`, this.props.location.pathname)
// const { announcement } = this.state
// return (
// <div
// id="gatsby-announcer"
// style={{
// 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"
// >
// {announcement}
// </div>
// )
// }
// }

const { pagePath, location: browserLoc } = window

// Explicitly call navigate if the canonical path (window.pagePath)
Expand Down Expand Up @@ -140,7 +204,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
View file
Open in desktop
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