fix(aria): process children of hidden elements #36316
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses processing of hidden elements for ARIA snapshots in response to issue #36296. Key changes include:
- Adding a new test to verify that visible children of hidden elements are correctly captured.
- Introducing the helper function isVisible in the ARIA snapshot generation, and modifying text node and element handling accordingly.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/page/page-aria-snapshot.spec.ts | Adds a test verifying that visible children within hidden elements are processed. |
| packages/injected/src/ariaSnapshot.ts | Introduces a new isVisible helper function and updates element processing logic to incorporate it. |
Comments suppressed due to low confidence (2)
packages/injected/src/ariaSnapshot.ts:50
- Consider adding a brief comment above the isVisible function to explain its purpose in determining element visibility for ARIA processing.
function isVisible(element: Element, options?: { forAI?: boolean }): boolean {
tests/page/page-aria-snapshot.spec.ts:662
- [nitpick] For consistency with other test names, consider renaming the test title to begin with 'should', for example: "should show visible children of hidden elements".
it('show visible children of hidden elements', { annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/36296' } }, async ({ page }) => {
This comment has been minimized.
This comment has been minimized.
| if (!isVisible) | ||
| if (!isVisible(element, options)) { | ||
| // skip this element, but still process its children: https://github.com/w3c/aria/issues/1055 | ||
| processElement(ariaNode, element, []); |
There was a problem hiding this comment.
I am not sure how this makes sense. Also, you can't process children as if they are visible. And we probably only want this for non-aria visibility.
There was a problem hiding this comment.
I am not sure how this makes sense
How so? As in, you're surprised how Chromium and this issue interpret the ARIA spec, or as in this code is weird?
Also, you can't process children as if they are visible.
I think we can, because the isVisible method takes parent elements into account when determining visibility.
we probably only want this for non-aria visibility.
It already does so, based on my reading of isElementHiddenForAria. I added a test in 9775c4a to ensure that.
This comment has been minimized.
This comment has been minimized.
|
As discussed yesterday, I scoped down the change to the AI snapshot. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -252,3 +252,16 @@ it('should auto-wait for blocking CSS', async ({ page, server }) => { | |||
| `, { waitUntil: 'commit' }); | |||
| expect(await snapshotForAI(page)).toContainYaml('Hello World'); | |||
| }); | |||
|
|
|||
| it('should show visible children of hidden elements', { annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/36296' } }, async ({ page }) => { | |||
| await page.setContent(` | |||
There was a problem hiding this comment.
Let's make this test a bit more involved:
- hidden
- visible
- neutral
- hidden
- visible
- neutral (no visibility change)
- visible
- hidden
- neutral
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 8 flaky47019 passed, 983 skipped Merge workflow run. |
| @@ -61,6 +69,9 @@ export function generateAriaTree(rootElement: Element, options?: { forAI?: boole | |||
| visited.add(node); | |||
|
|
|||
| if (node.nodeType === Node.TEXT_NODE && node.nodeValue) { | |||
| if (node.parentElement && !isVisible(node.parentElement, options)) | |||
There was a problem hiding this comment.
Would be nice to solve it via a parameter boolean, that way you don't need to check parent visibility for every text child.
| if (!isVisible(element, options)) { | ||
| if (options?.forAI) | ||
| // skip this element, but still process its children https://github.com/microsoft/playwright/issues/36296 | ||
| processElement(ariaNode, element, []); |
There was a problem hiding this comment.
We should be consistent with the aria-owns children. We either respect them in forAI or we don't. Your change makes it inconsistent.
There was a problem hiding this comment.
I'd say all we care about is that toAriaNode considers element as role === 'presentation' || role === 'none', then the right things will happen below.
Closes #36296