ReviewOS

stacks/very-happy-dom

getByRole matches elements that are hidden, so strict mode throws where Playwright succeeds

#1601
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Problem

getByRole() matches elements a user cannot see. Playwright excludes anything hidden by display: none, visibility: hidden or aria-hidden from its role queries; Locator.byRole() applies no such filter, so hidden markup competes with the real element — and under strict mode it turns a query that would have worked into a throw.

const page = new Browser().newPage()
page.mainFrame.window.document.head.innerHTML = '<style>.closed { display: none }</style>'
page.mainFrame.window.document.body.innerHTML
  = '<div class="closed"><button>Save</button></div><button>Save</button>'

page.getByRole('button', { name: 'Save' }).click()
// Error: 2 elements match getByRole("button"); use first(), last() or nth() to choose one

A real browser has one clickable Save button here. Playwright clicks it.

Why it matters

A closed modal, a collapsed menu, a tab panel that is not the active one — all of them leave their markup in the DOM. This is the ordinary shape of an app, not an edge case, so the failure lands on exactly the queries people write most.

It fails in both directions, and the quiet one is worse:

  • Loudly, as above: two matches, strict mode throws, and the fix that suggests itself — .first() — silently binds the test to the hidden element, since it comes first in document order. The test then passes while asserting on something invisible.
  • Quietly, when only the hidden copy exists: the locator resolves, isVisible() is never consulted, and the assertion passes against markup no user could reach.

The information needed to fix it already exists. getComputedStyle() has resolved the cascade since 0.1.11, and BrowserPage._isRendered already walks the ancestor chain through it to answer exactly this question — byRole just does not ask.

Proposed scope

Filter byRole() matches to those an accessibility tree would expose:

  • not display: none on the element or any ancestor
  • not visibility: hidden or collapse
  • not aria-hidden="true" on the element or any ancestor
  • not the hidden attribute

_isRendered covers the first two and the hidden attribute already; aria-hidden is the addition. Reusing it keeps one definition of "visible" rather than a second that can drift — which is how stacksjs/very-happy-dom#1600 happened.

Worth considering in the same pass: Playwright's includeHidden option, so the hidden case stays reachable deliberately rather than by accident.

Acceptance

  • The reproduction above resolves one element and clicks it.
  • A role query matches nothing when the only candidate is hidden, rather than returning it.
  • Hiding an ancestor hides its descendants from role queries.
  • aria-hidden="true" excludes the subtree.
  • The other getBy* helpers are considered too: they have the same problem and should not be left inconsistent with byRole.
9 days ago

Fixed in 56dab61. CI green.

The reproduction from the issue now clicks the visible button instead of throwing.

All four matchers filter, not only byRole — leaving getByText, getByLabel and the attribute-based helpers inconsistent with it would have been its own bug, and the tests cover each. includeHidden keeps the hidden case reachable deliberately.

isRendered and isExposedToAria live in one module (src/aria/visibility.ts) that BrowserPage._isRendered shares, so "hidden" has a single definition rather than a second that can drift — which is exactly how #1600 happened.

They are two predicates on purpose, and the distinction is asserted in both directions:

  • isRendered — display, visibility, the hidden attribute, inherited from every ancestor. This is what isVisible() asks.
  • isExposedToAria — that, plus aria-hidden. An aria-hidden element is painted, so isVisible() still answers true for it while a role query skips it.

14 tests in test/locator-visibility.test.ts, covering ancestor inheritance, visibility: collapse, the hidden attribute, aria-hidden on the element and on an ancestor, and the quiet case the issue called out: when only the hidden copy exists, the query now matches nothing rather than resolving.

Suite is 1635 passing.

One thing worth recording

My first draft of these tests was worthless and passed nothing: Locator.count(), textContent(), elementHandle(), click() and page.isVisible() are all async, and I compared Promise to 1. They failed for the wrong reason entirely. Any future test here needs await on those, or it asserts on a promise object and tells you nothing.

Sign in to comment on this issue.