From 0087bfac23fe3a47a666bdb9c6a228f125c66ff1 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 27 Dec 2022 09:06:46 -0800 Subject: [PATCH] fix(role): update allowsNameFromContent to closer align with blink/gecko (#19692) --- .../src/server/injected/roleUtils.ts | 19 ++++- tests/assets/axe-core/accessible-text.js | 5 +- tests/library/role-utils.spec.ts | 77 ++++++++++++++++++- tests/page/selectors-role.spec.ts | 60 --------------- 4 files changed, 94 insertions(+), 67 deletions(-) diff --git a/packages/playwright-core/src/server/injected/roleUtils.ts b/packages/playwright-core/src/server/injected/roleUtils.ts index 2fbd272756..cc3820a94d 100644 --- a/packages/playwright-core/src/server/injected/roleUtils.ts +++ b/packages/playwright-core/src/server/injected/roleUtils.ts @@ -316,6 +316,19 @@ export function getAriaLabelledByElements(element: Element): Element[] | null { return getIdRefs(element, ref); } +function allowsNameFromContent(role: string, targetDescendant: boolean) { + // SPEC: https://w3c.github.io/aria/#namefromcontent + // + // Note: there is a spec proposal https://github.com/w3c/aria/issues/1821 that + // is roughly aligned with what Chrome/Firefox do, and we follow that. + // + // See chromium implementation here: + // https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.cc;l=6338;drc=3decef66bc4c08b142a19db9628e9efe68973e64;bpv=0;bpt=1 + const alwaysAllowsNameFromContent = ['button', 'cell', 'checkbox', 'columnheader', 'gridcell', 'heading', 'link', 'menuitem', 'menuitemcheckbox', 'menuitemradio', 'option', 'radio', 'row', 'rowheader', 'switch', 'tab', 'tooltip', 'treeitem'].includes(role); + const descendantAllowsNameFromContent = targetDescendant && ['', 'caption', 'code', 'contentinfo', 'definition', 'deletion', 'emphasis', 'insertion', 'list', 'listitem', 'mark', 'none', 'paragraph', 'presentation', 'region', 'row', 'rowgroup', 'section', 'strong', 'subscript', 'superscript', 'table', 'term', 'time'].includes(role); + return alwaysAllowsNameFromContent || descendantAllowsNameFromContent; +} + export function getElementAccessibleName(element: Element, includeHidden: boolean, hiddenCache: Map): string { // https://w3c.github.io/accname/#computation-steps @@ -581,9 +594,9 @@ function getElementAccessibleNameInternal(element: Element, options: AccessibleN } // step 2f + step 2h. - // https://w3c.github.io/aria/#namefromcontent - const allowsNameFromContent = ['button', 'cell', 'checkbox', 'columnheader', 'gridcell', 'heading', 'link', 'menuitem', 'menuitemcheckbox', 'menuitemradio', 'option', 'radio', 'row', 'rowheader', 'switch', 'tab', 'tooltip', 'treeitem'].includes(role); - if (allowsNameFromContent || options.embeddedInLabelledBy !== 'none' || options.embeddedInLabel !== 'none' || options.embeddedInTextAlternativeElement || options.embeddedInTargetElement === 'descendant') { + if (allowsNameFromContent(role, options.embeddedInTargetElement === 'descendant') || + options.embeddedInLabelledBy !== 'none' || options.embeddedInLabel !== 'none' || + options.embeddedInTextAlternativeElement) { options.visitedElements.add(element); const tokens: string[] = []; const visit = (node: Node, skipSlotted: boolean) => { diff --git a/tests/assets/axe-core/accessible-text.js b/tests/assets/axe-core/accessible-text.js index a951b7bd30..9c84cd1cea 100644 --- a/tests/assets/axe-core/accessible-text.js +++ b/tests/assets/axe-core/accessible-text.js @@ -169,7 +169,10 @@ module.exports = [ '' + '', target: '#t2label', - accessibleText: 'This is This is a label of everything', + // accessibleText: 'This is This is a label of everything', + // Chrome and axe-core disagree, we follow Chrome and spec proposal + // https://github.com/w3c/aria/issues/1821. + accessibleText: 'This is This is a label of', }, { diff --git a/tests/library/role-utils.spec.ts b/tests/library/role-utils.spec.ts index 245b6f9e1a..0f4061245d 100644 --- a/tests/library/role-utils.spec.ts +++ b/tests/library/role-utils.spec.ts @@ -84,7 +84,7 @@ for (let range = 0; range <= ranges.length; range++) { return result; }); for (const { selector, expected, received } of result) - expect(received, `checking "${selector}"`).toBe(expected); + expect.soft(received, `checking "${selector}" in ${testFile}`).toBe(expected); }); } }); @@ -107,7 +107,7 @@ test('axe-core implicit-role', async ({ page, asset, server }) => { throw new Error(`Unable to resolve "${selector}"`); return (window as any).__injectedScript.getAriaRole(element); }, testCase.target); - expect(received, `checking ${JSON.stringify(testCase)}`).toBe(testCase.role); + expect.soft(received, `checking ${JSON.stringify(testCase)}`).toBe(testCase.role); }); } }); @@ -141,11 +141,82 @@ test('axe-core accessible-text', async ({ page, asset, server }) => { return injected.getElementAccessibleName(element); }); }, targets); - expect(received, `checking ${JSON.stringify(testCase)}`).toEqual(expected); + expect.soft(received, `checking ${JSON.stringify(testCase)}`).toEqual(expected); }); } }); +test('accessible name with slots', async ({ page }) => { + // Text "foo" is assigned to the slot, should not be used twice. + await page.setContent(` + + + `); + const name1 = await page.$eval('button', e => (window as any).__injectedScript.getElementAccessibleName(e)); + expect.soft(name1).toBe('foo'); + + // Text "foo" is assigned to the slot, should be used instead of slot content. + await page.setContent(` +
foo
+ + `); + const name2 = await page.$eval('button', e => (window as any).__injectedScript.getElementAccessibleName(e)); + expect.soft(name2).toBe('foo'); + + // Nothing is assigned to the slot, should use slot content. + await page.setContent(` +
+ + `); + const name3 = await page.$eval('button', e => (window as any).__injectedScript.getElementAccessibleName(e)); + expect.soft(name3).toBe('pre'); +}); + +test('accessible name nested treeitem', async ({ page }) => { + await page.setContent(` +
+ Top-level +
+
Nested 1
+
Nested 2
+
+
+ `); + const name = await page.$eval('#target', e => (window as any).__injectedScript.getElementAccessibleName(e)); + expect.soft(name).toBe('Top-level'); +}); + function toArray(x: any): any[] { return Array.isArray(x) ? x : [x]; } diff --git a/tests/page/selectors-role.spec.ts b/tests/page/selectors-role.spec.ts index 0e45304a09..253afd9475 100644 --- a/tests/page/selectors-role.spec.ts +++ b/tests/page/selectors-role.spec.ts @@ -423,63 +423,3 @@ test('errors', async ({ page }) => { const e8 = await page.$('role=treeitem[expanded="none"]').catch(e => e); expect(e8.message).toContain(`"expanded" must be one of true, false`); }); - -test('should detect accessible name with slots', async ({ page }) => { - // Text "foo" is assigned to the slot, should not be used twice. - await page.setContent(` - - - `); - expect(await page.locator(`role=button[name="foo"]`).evaluateAll(els => els.map(e => e.outerHTML))).toEqual([ - ``, - ]); - - // Text "foo" is assigned to the slot, should be used instead of slot content. - await page.setContent(` -
foo
- - `); - expect(await page.locator(`role=button[name="foo"]`).evaluateAll(els => els.map(e => e.outerHTML))).toEqual([ - ``, - ]); - - // Nothing is assigned to the slot, should use slot content. - await page.setContent(` -
- - `); - expect(await page.locator(`role=button[name="pre"]`).evaluateAll(els => els.map(e => e.outerHTML))).toEqual([ - ``, - ]); -});