Skip to content

Commit

Permalink
Fix <RouteAnnouncer/> shouldn't announce initial path under strict …
Browse files Browse the repository at this point in the history
…mode and React 18 (#34338)

As the comment mentioned, React 18 with Strict Mode enabled might cause double invocation of lifecycle methods. This makes the `<RouteAnnouncer/>` being non-empty for the initial page, which is a bug (it should only announce page change whenever a navigation happens).

## Bug

- [ ] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `yarn lint`
  • Loading branch information
shuding committed Feb 14, 2022
1 parent 41ecd5e commit 468833e
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 6 deletions.
9 changes: 4 additions & 5 deletions packages/next/client/route-announcer.tsx
Expand Up @@ -7,7 +7,7 @@ export function RouteAnnouncer() {

// Only announce the path change, but not for the first load because screen
// reader will do that automatically.
const initialPathLoaded = React.useRef(false)
const previouslyLoadedPath = React.useRef(asPath)

// Every time the path changes, announce the new page’s title following this
// priority: first the document title (from head), otherwise the first h1, or
Expand All @@ -17,10 +17,9 @@ export function RouteAnnouncer() {
// https://www.gatsbyjs.com/blog/2019-07-11-user-testing-accessible-client-routing/
React.useEffect(
() => {
if (!initialPathLoaded.current) {
initialPathLoaded.current = true
return
}
// If the path hasn't change, we do nothing.
if (previouslyLoadedPath.current === asPath) return
previouslyLoadedPath.current = asPath

if (document.title) {
setRouteAnnouncement(document.title)
Expand Down
9 changes: 8 additions & 1 deletion test/integration/client-navigation-a11y/test/index.test.js
Expand Up @@ -10,6 +10,7 @@ import webdriver from 'next-webdriver'
import { join } from 'path'

const context = {}
const appDir = join(__dirname, '../')

const navigateTo = async (browser, selector) =>
await browser
Expand All @@ -28,7 +29,7 @@ const getMainHeadingTitle = async (browser) =>
describe('Client Navigation accessibility', () => {
beforeAll(async () => {
context.appPort = await findPort()
context.server = await launchApp(join(__dirname, '../'), context.appPort, {
context.server = await launchApp(appDir, context.appPort, {
env: { __NEXT_TEST_WITH_DEVTOOL: 1 },
})

Expand All @@ -44,6 +45,12 @@ describe('Client Navigation accessibility', () => {
afterAll(() => killApp(context.server))

describe('<RouteAnnouncer />', () => {
it('should not have the initial route announced', async () => {
const browser = await webdriver(context.appPort, '/')
const title = await getAnnouncedTitle(browser)
expect(title).toBe('')
})

it('has aria-live="assertive" and role="alert"', async () => {
const browser = await webdriver(context.appPort, '/')
const routeAnnouncer = await browser.waitForElementByCss(
Expand Down
1 change: 1 addition & 0 deletions test/integration/react-18/app/next.config.js
@@ -1,6 +1,7 @@
const withReact18 = require('../test/with-react-18')

module.exports = withReact18({
// reactStrictMode: true,
experimental: {
reactRoot: true,
// runtime: 'edge',
Expand Down
21 changes: 21 additions & 0 deletions test/integration/react-18/test/index.test.js
Expand Up @@ -15,6 +15,7 @@ import {
import blocking from './blocking'
import concurrent from './concurrent'
import basics from './basics'
import strictMode from './strict-mode'

// overrides react and react-dom to v18
const nodeArgs = ['-r', join(__dirname, 'require-hook.js')]
Expand Down Expand Up @@ -106,6 +107,26 @@ describe('Basics', () => {
})
})

// React 18 with Strict Mode enabled might cause double invocation of lifecycle methods.
describe('Strict mode - dev', () => {
const context = { appDir }

beforeAll(async () => {
nextConfig.replace('// reactStrictMode: true,', 'reactStrictMode: true,')
context.appPort = await findPort()
context.server = await launchApp(context.appDir, context.appPort, {
nodeArgs,
})
})

afterAll(() => {
nextConfig.restore()
killApp(context.server)
})

strictMode(context)
})

describe('Blocking mode', () => {
beforeAll(() => {
dynamicHello.replace('suspense = false', `suspense = true`)
Expand Down
16 changes: 16 additions & 0 deletions test/integration/react-18/test/strict-mode.js
@@ -0,0 +1,16 @@
/* eslint-env jest */

import webdriver from 'next-webdriver'

export default (context) => {
describe('<RouteAnnouncer />', () => {
it('should not have the initial route announced', async () => {
const browser = await webdriver(context.appPort, '/')
const title = await browser
.waitForElementByCss('#__next-route-announcer__')
.text()

expect(title).toBe('')
})
})
}

0 comments on commit 468833e

Please sign in to comment.