From 3ceb9c567339e50fc6326a228841615a9a9edeac Mon Sep 17 00:00:00 2001 From: Kitty Giraudel <1889710+KittyGiraudel@users.noreply.github.com> Date: Mon, 8 Nov 2021 15:58:21 +0100 Subject: [PATCH] Give priority to document.title over h1 when announcing page change (#31147) ## Improvement This pull-request should address https://github.com/vercel/next.js/issues/24021, improving the page change announcement for assistive technologies by giving priority to `document.title` over `h1`. Interestingly, it would also improve a potential performance bottleneck by skipping calls to `innerText` on the main `h1` raised in [this comment](https://github.com/vercel/next.js/pull/20428#issuecomment-962537038). --- packages/next/client/route-announcer.tsx | 30 +++---- .../client-navigation-a11y/pages/index.js | 10 ++- .../pages/page-with-h1-and-title.js | 11 +++ .../pages/page-with-h1.js | 5 -- .../client-navigation-a11y/test/index.test.js | 84 +++++++++++-------- 5 files changed, 81 insertions(+), 59 deletions(-) create mode 100644 test/integration/client-navigation-a11y/pages/page-with-h1-and-title.js diff --git a/packages/next/client/route-announcer.tsx b/packages/next/client/route-announcer.tsx index a778fe5dac44..2c5a772b5fcb 100644 --- a/packages/next/client/route-announcer.tsx +++ b/packages/next/client/route-announcer.tsx @@ -5,12 +5,15 @@ export function RouteAnnouncer() { const { asPath } = useRouter() const [routeAnnouncement, setRouteAnnouncement] = React.useState('') - // Only announce the path change, but not for the first load because screen reader will do that automatically. + // Only announce the path change, but not for the first load because screen + // reader will do that automatically. const initialPathLoaded = React.useRef(false) - // Every time the path changes, announce the route change. The announcement will be prioritized by h1, then title - // (from metadata), and finally if those don't exist, then the pathName that is in the URL. This methodology is - // inspired by Marcy Sutton's accessible client routing user testing. More information can be found here: + // 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 + // if none of these exist, then the pathname from the URL. This methodology is + // inspired by Marcy Sutton’s accessible client routing user testing. More + // information can be found here: // https://www.gatsbyjs.com/blog/2019-07-11-user-testing-accessible-client-routing/ React.useEffect( () => { @@ -19,21 +22,14 @@ export function RouteAnnouncer() { return } - let newRouteAnnouncement - const pageHeader = document.querySelector('h1') + if (document.title) { + setRouteAnnouncement(document.title) + } else { + const pageHeader = document.querySelector('h1') + const content = pageHeader?.innerText ?? pageHeader?.textContent - if (pageHeader) { - newRouteAnnouncement = pageHeader.innerText || pageHeader.textContent + setRouteAnnouncement(content || asPath) } - if (!newRouteAnnouncement) { - if (document.title) { - newRouteAnnouncement = document.title - } else { - newRouteAnnouncement = asPath - } - } - - setRouteAnnouncement(newRouteAnnouncement) }, // TODO: switch to pathname + query object of dynamic route requirements [asPath] diff --git a/test/integration/client-navigation-a11y/pages/index.js b/test/integration/client-navigation-a11y/pages/index.js index b616bbe516a5..04ed7407e7d2 100644 --- a/test/integration/client-navigation-a11y/pages/index.js +++ b/test/integration/client-navigation-a11y/pages/index.js @@ -2,14 +2,22 @@ import Link from 'next/link' export default () => (
+ + + Go to a page with a title and an h1 + + + - Go to a page with an h1 + Go to a page without a title but with an h1 + Go to a page without an h1, but with a title + Go to a page without an h1 or a title diff --git a/test/integration/client-navigation-a11y/pages/page-with-h1-and-title.js b/test/integration/client-navigation-a11y/pages/page-with-h1-and-title.js new file mode 100644 index 000000000000..f746c7b0fe56 --- /dev/null +++ b/test/integration/client-navigation-a11y/pages/page-with-h1-and-title.js @@ -0,0 +1,11 @@ +import Head from 'next/head' + +export default () => ( +
+ + Another Page's Title + +

My heading

+
Extraneous stuff
+
+) diff --git a/test/integration/client-navigation-a11y/pages/page-with-h1.js b/test/integration/client-navigation-a11y/pages/page-with-h1.js index b3624846bc32..d531c0df5b1e 100644 --- a/test/integration/client-navigation-a11y/pages/page-with-h1.js +++ b/test/integration/client-navigation-a11y/pages/page-with-h1.js @@ -1,10 +1,5 @@ -import Head from 'next/head' - export default () => (
- - Another Page's Title -

My heading

Extraneous stuff
diff --git a/test/integration/client-navigation-a11y/test/index.test.js b/test/integration/client-navigation-a11y/test/index.test.js index 302b1d13abc9..99c347726aa4 100644 --- a/test/integration/client-navigation-a11y/test/index.test.js +++ b/test/integration/client-navigation-a11y/test/index.test.js @@ -11,6 +11,20 @@ import { join } from 'path' const context = {} +const navigateTo = async (browser, selector) => + await browser + .waitForElementByCss('#' + selector + '-link') + .click() + .waitForElementByCss('#' + selector) + +const getAnnouncedTitle = async (browser) => + await browser.waitForElementByCss('#__next-route-announcer__').text() + +const getDocumentTitle = async (browser) => await browser.eval('document.title') + +const getMainHeadingTitle = async (browser) => + await browser.elementByCss('h1').text() + describe('Client Navigation accessibility', () => { beforeAll(async () => { context.appPort = await findPort() @@ -19,7 +33,7 @@ describe('Client Navigation accessibility', () => { }) const prerender = [ - '/page-with-h1, /page-with-title, /page-without-h1-or-title', + '/page-with-h1-and-title, /page-with-h1, /page-with-title, /page-without-h1-or-title', ] await Promise.all( @@ -42,57 +56,55 @@ describe('Client Navigation accessibility', () => { expect(roleValue).toBe('alert') await browser.close() }) - describe('There is an h1 tag', () => { - it('has the same innerText value as the h1 tag', async () => { + + describe('There is a title but no h1 tag', () => { + it('has the innerText equal to the value of document.title', async () => { const browser = await webdriver(context.appPort, '/') - const h1Value = await browser - .waitForElementByCss('#page-with-h1-link') - .click() - .waitForElementByCss('#page-with-h1') - .elementByCss('h1') - .text() - - const routeAnnouncerValue = await browser - .waitForElementByCss('#__next-route-announcer__') - .text() - - expect(h1Value).toBe(routeAnnouncerValue) + await navigateTo(browser, 'page-with-title') + + const routeAnnouncerValue = await getAnnouncedTitle(browser) + const title = await getDocumentTitle(browser) + + expect(routeAnnouncerValue).toBe(title) await browser.close() }) }) - describe('There is a document.title, but no h1 tag', () => { - it('has the innerText equal to the value of document.title', async () => { + + describe('There is no title but a h1 tag', () => { + it('has the innerText equal to the value of h1', async () => { const browser = await webdriver(context.appPort, '/') - await browser - .waitForElementByCss('#page-with-title-link') - .click() - .waitForElementByCss('#page-with-title') + await navigateTo(browser, 'page-with-h1') - const title = await browser.eval('document.title') + const routeAnnouncerValue = await getAnnouncedTitle(browser) + const h1Value = await getMainHeadingTitle(browser) - const routeAnnouncerValue = await browser - .waitForElementByCss('#__next-route-announcer__') - .text() + expect(routeAnnouncerValue).toBe(h1Value) + await browser.close() + }) + }) - expect(title).toBe(routeAnnouncerValue) + describe('There is a title and a h1 tag', () => { + it('has the innerText equal to the value of h1', async () => { + const browser = await webdriver(context.appPort, '/') + await navigateTo(browser, 'page-with-h1-and-title') + + const routeAnnouncerValue = await getAnnouncedTitle(browser) + const title = await getDocumentTitle(browser) + + expect(routeAnnouncerValue).toBe(title) await browser.close() }) }) - describe('There is neither an h1 or a title tag', () => { + + describe('There is no title and no h1 tag', () => { it('has the innerText equal to the value of the pathname', async () => { const browser = await webdriver(context.appPort, '/') - await browser - .waitForElementByCss('#page-without-h1-or-title-link') - .click() - .waitForElementByCss('#page-without-h1-or-title') + await navigateTo(browser, 'page-without-h1-or-title') + const routeAnnouncerValue = await getAnnouncedTitle(browser) const pathname = '/page-without-h1-or-title' - const routeAnnouncerValue = await browser - .waitForElementByCss('#__next-route-announcer__') - .text() - - expect(pathname).toBe(routeAnnouncerValue) + expect(routeAnnouncerValue).toBe(pathname) await browser.close() }) })