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.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| 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.
There was a problem hiding this comment.
Good point, didn't think about aria-owns. Moving this new logic into toAriaNode made this a lot better.
| 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.
Good point, didn't think about aria-owns. Moving this new logic into toAriaNode made this a lot better.
| const role = roleUtils.getAriaRole(element) ?? defaultRole; | ||
| let role: AriaRole | null = null; | ||
| if (options?.forAI) { | ||
| const isVisible = !roleUtils.isElementHiddenForAria(element) || isElementVisible(element); |
There was a problem hiding this comment.
Are we making these checks twice for each node? Looking at parentVisible = isElementVisible(element); and if (!options?.forAI && roleUtils.isElementHiddenForAria(element)) above.
There was a problem hiding this comment.
The isElementHiddenForAria check is only made once, since one of the checks is behind forAI and the other is behind !forAI.
isElementVisible is indeed called twice for the same element. Even worse, it is called parentVisible = isElementVisible(element) is called for each tree node, but only used for a subset of leaf nodes. That's what the parameter argument did, I suppose. Let me see to it.
There was a problem hiding this comment.
Alright, after molding this change around, I arrived at something good.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 6 flaky47224 passed, 980 skipped Merge workflow run. |
Closes #36296