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

Fix <RouteAnnouncer/> shouldn't announce initial path under strict mode and React 18 #34338

Merged
merged 3 commits into from Feb 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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('')
})
})
}