From bf3426dad7be8bafa9e99121bbca9cfbab2d8522 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Fri, 15 Dec 2023 12:35:36 -0500 Subject: [PATCH 1/5] fix(title): title is level one heading if one is not present --- core/src/components/title/test/title.spec.ts | 86 ++++++++++++++++++++ core/src/components/title/title.tsx | 20 +++++ 2 files changed, 106 insertions(+) create mode 100644 core/src/components/title/test/title.spec.ts diff --git a/core/src/components/title/test/title.spec.ts b/core/src/components/title/test/title.spec.ts new file mode 100644 index 00000000000..06079570a30 --- /dev/null +++ b/core/src/components/title/test/title.spec.ts @@ -0,0 +1,86 @@ +import { newSpecPage } from '@stencil/core/testing'; + +import { ToolbarTitle } from '../title'; + +describe('title: a11y', () => { + it('should add heading level 1 attributes', async () => { + const { root } = await newSpecPage({ + components: [ToolbarTitle], + html: 'Title', + }); + + const title = root!; + + expect(title.getAttribute('role')).toBe('heading'); + expect(title.getAttribute('aria-level')).toBe('1'); + }); + it('at most one ion-title should have level 1 attributes', async () => { + const page = await newSpecPage({ + components: [ToolbarTitle], + html: ` + Title 1 + Title 2 + `, + }); + + const titles = page.body.querySelectorAll('ion-title'); + + expect(titles[0].getAttribute('role')).toBe('heading'); + expect(titles[0].getAttribute('aria-level')).toBe('1'); + + expect(titles[1].hasAttribute('role')).toBe(false); + expect(titles[1].hasAttribute('aria-level')).toBe(false); + }); + it('should not add level 1 attributes if other level 1 headings exist', async () => { + const { root } = await newSpecPage({ + components: [ToolbarTitle], + html: ` +

Title

+ Title 1 + `, + }); + + const title = root!; + + expect(title.hasAttribute('role')).toBe(false); + expect(title.hasAttribute('aria-level')).toBe(false); + }); + it('should not add level 1 attributes if other level 1 attributes exist', async () => { + const { root } = await newSpecPage({ + components: [ToolbarTitle], + html: ` +
Title
+ Title 1 + `, + }); + + const title = root!; + + expect(title.hasAttribute('role')).toBe(false); + expect(title.hasAttribute('aria-level')).toBe(false); + }); + + it('should level 1 attributes even if there is a level 1 heading on another page', async () => { + const page = await newSpecPage({ + components: [ToolbarTitle], + html: ` +
+ Title 1 +
+ +
+ Title 2 +
+ `, + }); + + const pages = page.body.querySelectorAll('.ion-page'); + + pages.forEach((page) => { + const title = page.querySelector('ion-title')!; + + expect(title.getAttribute('role')).toBe('heading'); + expect(title.getAttribute('aria-level')).toBe('1'); + }); + }); +}); diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index 82a108071ff..87e2b3bfa5e 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -1,5 +1,6 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Component, Element, Event, Host, Prop, Watch, h } from '@stencil/core'; +import { doc } from '@utils/browser'; import { createColorClasses } from '@utils/theme'; import { getIonMode } from '../../global/ionic-global'; @@ -56,11 +57,30 @@ export class ToolbarTitle implements ComponentInterface { } render() { + const { el } = this; const mode = getIonMode(this); const size = this.getSize(); + /** + * If there is already a level one heading + * within the context of the page then + * do not add another one. + */ + const root = el.closest('.ion-page') ?? doc?.body; + const hasHeading = root?.querySelector('h1, [role="heading"][aria-level="1"]'); + + /** + * The first `ion-title` on the page is considered + * the heading. This can be customized by setting + * role="heading" aria-level="1" on another element + * or by using h1. + */ + const isHeading = hasHeading === null && root?.querySelector('ion-title') === el; + return ( Date: Fri, 15 Dec 2023 13:14:19 -0500 Subject: [PATCH 2/5] ensure title is in a level 1 heading, add more tests --- .../components/title/test/a11y/title.e2e.ts | 50 +++++++++++ .../components/title/test/basic/index.html | 76 ++--------------- core/src/components/title/test/title.spec.ts | 85 ++++++++++++++----- core/src/components/title/title.tsx | 6 +- 4 files changed, 125 insertions(+), 92 deletions(-) diff --git a/core/src/components/title/test/a11y/title.e2e.ts b/core/src/components/title/test/a11y/title.e2e.ts index 7b799aa4b39..4547451ef7f 100644 --- a/core/src/components/title/test/a11y/title.e2e.ts +++ b/core/src/components/title/test/a11y/title.e2e.ts @@ -1,5 +1,6 @@ import { expect } from '@playwright/test'; import { configs, test } from '@utils/test/playwright'; +import AxeBuilder from '@axe-core/playwright'; configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { test.describe(title('title: font scaling'), () => { @@ -74,3 +75,52 @@ configs({ directions: ['ltr'], modes: ['ios'] }).forEach(({ title, screenshot, c }); }); }); + +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe.only(title('title: level 1 heading'), () => { + test('should not have accessibility violations', async ({ page }) => { + /** + * Level 1 headings must be inside of a landmark (ion-header) + */ + await page.setContent( + ` + + + My Title + + + `, + config + ); + + const results = await new AxeBuilder({ page }).analyze(); + + expect(results.violations).toEqual([]); + }); + test('should not have accessibility violations with multiple h1 elements on a hidden page', async ({ page }) => { + await page.setContent( + ` +
+ + + My Title + + +
+
+ + + My Title + + +
+ `, + config + ); + + const results = await new AxeBuilder({ page }).analyze(); + + expect(results.violations).toEqual([]); + }); + }); +}); diff --git a/core/src/components/title/test/basic/index.html b/core/src/components/title/test/basic/index.html index 0b3257888b0..049bdda823c 100644 --- a/core/src/components/title/test/basic/index.html +++ b/core/src/components/title/test/basic/index.html @@ -5,7 +5,7 @@ Title - Basic @@ -15,74 +15,10 @@ - -
- - - Settings - - - - - - Browse - - Cancel - - - - - - - - - - Title - Subtitle - - - - - - - - - - Choose a contact to add to Favorites - - - Contacts - - Cancel - - - - - - - - Choose multiple contacts to add to your Favorites so this just goes on and on and on - - - - - Groups - - Contacts - - Cancel - - - -
- - -
- - + + + My Title + + diff --git a/core/src/components/title/test/title.spec.ts b/core/src/components/title/test/title.spec.ts index 06079570a30..9ee4d6d3b3a 100644 --- a/core/src/components/title/test/title.spec.ts +++ b/core/src/components/title/test/title.spec.ts @@ -1,25 +1,54 @@ import { newSpecPage } from '@stencil/core/testing'; +import { Header } from '../../header/header'; +import { Toolbar } from '../../toolbar/toolbar'; import { ToolbarTitle } from '../title'; describe('title: a11y', () => { - it('should add heading level 1 attributes', async () => { - const { root } = await newSpecPage({ - components: [ToolbarTitle], - html: 'Title', + it('should add heading level 1 attributes when inside of a landmark', async () => { + const page = await newSpecPage({ + components: [Header, Toolbar, ToolbarTitle], + html: ` + + + Title + + + `, }); - const title = root!; + const title = page.body.querySelector('ion-title')!; expect(title.getAttribute('role')).toBe('heading'); expect(title.getAttribute('aria-level')).toBe('1'); }); + it('should not add heading level 1 attributes when outside of a landmark', async () => { + const page = await newSpecPage({ + components: [Header, Toolbar, ToolbarTitle], + html: ` + Title + `, + }); + + const title = page.body.querySelector('ion-title')!; + + expect(title.hasAttribute('role')).toBe(false); + expect(title.hasAttribute('aria-level')).toBe(false); + }); it('at most one ion-title should have level 1 attributes', async () => { const page = await newSpecPage({ - components: [ToolbarTitle], + components: [Header, Toolbar, ToolbarTitle], html: ` - Title 1 - Title 2 + + + Title 1 + + + + + Title 2 + + `, }); @@ -32,44 +61,60 @@ describe('title: a11y', () => { expect(titles[1].hasAttribute('aria-level')).toBe(false); }); it('should not add level 1 attributes if other level 1 headings exist', async () => { - const { root } = await newSpecPage({ - components: [ToolbarTitle], + const page = await newSpecPage({ + components: [Header, Toolbar, ToolbarTitle], html: `

Title

- Title 1 + + + Title 1 + + `, }); - const title = root!; + const title = page.body.querySelector('ion-title')!; expect(title.hasAttribute('role')).toBe(false); expect(title.hasAttribute('aria-level')).toBe(false); }); it('should not add level 1 attributes if other level 1 attributes exist', async () => { - const { root } = await newSpecPage({ - components: [ToolbarTitle], + const page = await newSpecPage({ + components: [Header, Toolbar, ToolbarTitle], html: `
Title
- Title 1 + + + Title 1 + + `, }); - const title = root!; + const title = page.body.querySelector('ion-title')!; expect(title.hasAttribute('role')).toBe(false); expect(title.hasAttribute('aria-level')).toBe(false); }); - it('should level 1 attributes even if there is a level 1 heading on another page', async () => { + it('should have level 1 attributes even if there is a level 1 heading on another page', async () => { const page = await newSpecPage({ - components: [ToolbarTitle], + components: [Header, Toolbar, ToolbarTitle], html: `
- Title 1 + + + Title 1 + +
- Title 2 + + + Title 2 + +
`, }); diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index 87e2b3bfa5e..e1464e6dd7c 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -74,9 +74,11 @@ export class ToolbarTitle implements ComponentInterface { * the heading. This can be customized by setting * role="heading" aria-level="1" on another element * or by using h1. + * + * Level 1 headings must be contained inside of a landmark, + * so we check for ion-header which is typically the landmark. */ - const isHeading = hasHeading === null && root?.querySelector('ion-title') === el; - + const isHeading = hasHeading === null && root?.querySelector('ion-title') === el && el?.closest('ion-header[role]') !== null; return ( Date: Fri, 15 Dec 2023 13:14:39 -0500 Subject: [PATCH 3/5] lint --- .../components/title/test/a11y/title.e2e.ts | 2 +- .../components/title/test/basic/index.html | 76 +++++++++++++++++-- core/src/components/title/title.tsx | 3 +- 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/core/src/components/title/test/a11y/title.e2e.ts b/core/src/components/title/test/a11y/title.e2e.ts index 4547451ef7f..a7684190ddc 100644 --- a/core/src/components/title/test/a11y/title.e2e.ts +++ b/core/src/components/title/test/a11y/title.e2e.ts @@ -1,6 +1,6 @@ +import AxeBuilder from '@axe-core/playwright'; import { expect } from '@playwright/test'; import { configs, test } from '@utils/test/playwright'; -import AxeBuilder from '@axe-core/playwright'; configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { test.describe(title('title: font scaling'), () => { diff --git a/core/src/components/title/test/basic/index.html b/core/src/components/title/test/basic/index.html index 049bdda823c..0b3257888b0 100644 --- a/core/src/components/title/test/basic/index.html +++ b/core/src/components/title/test/basic/index.html @@ -5,7 +5,7 @@ Title - Basic @@ -15,10 +15,74 @@ - - - My Title - - + +
+ + + Settings + + + + + + Browse + + Cancel + + + + + + + + + + Title + Subtitle + + + + + + + + + + Choose a contact to add to Favorites + + + Contacts + + Cancel + + + + + + + + Choose multiple contacts to add to your Favorites so this just goes on and on and on + + + + + Groups + + Contacts + + Cancel + + + +
+ + +
+ + diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index e1464e6dd7c..b12ae5297a7 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -78,7 +78,8 @@ export class ToolbarTitle implements ComponentInterface { * Level 1 headings must be contained inside of a landmark, * so we check for ion-header which is typically the landmark. */ - const isHeading = hasHeading === null && root?.querySelector('ion-title') === el && el?.closest('ion-header[role]') !== null; + const isHeading = + hasHeading === null && root?.querySelector('ion-title') === el && el?.closest('ion-header[role]') !== null; return ( Date: Fri, 15 Dec 2023 13:25:34 -0500 Subject: [PATCH 4/5] remove .only and lint --- core/src/components/title/test/a11y/title.e2e.ts | 2 +- core/src/components/title/test/title.spec.ts | 16 ++++++++++++++++ core/src/components/title/title.tsx | 3 ++- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/core/src/components/title/test/a11y/title.e2e.ts b/core/src/components/title/test/a11y/title.e2e.ts index a7684190ddc..22e3f44ea11 100644 --- a/core/src/components/title/test/a11y/title.e2e.ts +++ b/core/src/components/title/test/a11y/title.e2e.ts @@ -77,7 +77,7 @@ configs({ directions: ['ltr'], modes: ['ios'] }).forEach(({ title, screenshot, c }); configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { - test.describe.only(title('title: level 1 heading'), () => { + test.describe(title('title: level 1 heading'), () => { test('should not have accessibility violations', async ({ page }) => { /** * Level 1 headings must be inside of a landmark (ion-header) diff --git a/core/src/components/title/test/title.spec.ts b/core/src/components/title/test/title.spec.ts index 9ee4d6d3b3a..92513a7d889 100644 --- a/core/src/components/title/test/title.spec.ts +++ b/core/src/components/title/test/title.spec.ts @@ -22,6 +22,22 @@ describe('title: a11y', () => { expect(title.getAttribute('role')).toBe('heading'); expect(title.getAttribute('aria-level')).toBe('1'); }); + it('should not override a custom role', async () => { + const page = await newSpecPage({ + components: [Header, Toolbar, ToolbarTitle], + html: ` + + + Title 1 + + + `, + }); + const title = page.body.querySelector('ion-title')!; + + expect(title.getAttribute('role')).toBe('article'); + expect(title.hasAttribute('aria-level')).toBe(false); + }); it('should not add heading level 1 attributes when outside of a landmark', async () => { const page = await newSpecPage({ components: [Header, Toolbar, ToolbarTitle], diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index b12ae5297a7..4e392759470 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -68,6 +68,7 @@ export class ToolbarTitle implements ComponentInterface { */ const root = el.closest('.ion-page') ?? doc?.body; const hasHeading = root?.querySelector('h1, [role="heading"][aria-level="1"]'); + const hasRole = el.hasAttribute('role'); /** * The first `ion-title` on the page is considered @@ -79,7 +80,7 @@ export class ToolbarTitle implements ComponentInterface { * so we check for ion-header which is typically the landmark. */ const isHeading = - hasHeading === null && root?.querySelector('ion-title') === el && el?.closest('ion-header[role]') !== null; + hasRole === false && hasHeading === null && root?.querySelector('ion-title') === el && el?.closest('ion-header[role]') !== null; return ( Date: Fri, 15 Dec 2023 13:30:04 -0500 Subject: [PATCH 5/5] lint --- core/src/components/title/title.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index 4e392759470..55e5595cbe6 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -80,7 +80,10 @@ export class ToolbarTitle implements ComponentInterface { * so we check for ion-header which is typically the landmark. */ const isHeading = - hasRole === false && hasHeading === null && root?.querySelector('ion-title') === el && el?.closest('ion-header[role]') !== null; + hasRole === false && + hasHeading === null && + root?.querySelector('ion-title') === el && + el?.closest('ion-header[role]') !== null; return (