From 468833e0b4cfd395dd163ab5c755ebaa70e83a34 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Mon, 14 Feb 2022 20:18:39 +0100 Subject: [PATCH] Fix `` shouldn't announce initial path under strict 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 `` 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` --- packages/next/client/route-announcer.tsx | 9 ++++---- .../client-navigation-a11y/test/index.test.js | 9 +++++++- test/integration/react-18/app/next.config.js | 1 + test/integration/react-18/test/index.test.js | 21 +++++++++++++++++++ test/integration/react-18/test/strict-mode.js | 16 ++++++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 test/integration/react-18/test/strict-mode.js diff --git a/packages/next/client/route-announcer.tsx b/packages/next/client/route-announcer.tsx index 2c5a772b5fcb8..022342e0b3003 100644 --- a/packages/next/client/route-announcer.tsx +++ b/packages/next/client/route-announcer.tsx @@ -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 @@ -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) diff --git a/test/integration/client-navigation-a11y/test/index.test.js b/test/integration/client-navigation-a11y/test/index.test.js index 99c347726aa4d..97ccb29b65232 100644 --- a/test/integration/client-navigation-a11y/test/index.test.js +++ b/test/integration/client-navigation-a11y/test/index.test.js @@ -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 @@ -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 }, }) @@ -44,6 +45,12 @@ describe('Client Navigation accessibility', () => { afterAll(() => killApp(context.server)) describe('', () => { + 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( diff --git a/test/integration/react-18/app/next.config.js b/test/integration/react-18/app/next.config.js index 54f5fa1a49795..270a344a62435 100644 --- a/test/integration/react-18/app/next.config.js +++ b/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', diff --git a/test/integration/react-18/test/index.test.js b/test/integration/react-18/test/index.test.js index 3d85d9b65bac0..f0e634e924778 100644 --- a/test/integration/react-18/test/index.test.js +++ b/test/integration/react-18/test/index.test.js @@ -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')] @@ -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`) diff --git a/test/integration/react-18/test/strict-mode.js b/test/integration/react-18/test/strict-mode.js new file mode 100644 index 0000000000000..fc305ec469fdc --- /dev/null +++ b/test/integration/react-18/test/strict-mode.js @@ -0,0 +1,16 @@ +/* eslint-env jest */ + +import webdriver from 'next-webdriver' + +export default (context) => { + describe('', () => { + 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('') + }) + }) +}