fix(locator): restrict loose field branches to field-like elements - #5703
Open
DavertMik wants to merge 6 commits into
Open
fix(locator): restrict loose field branches to field-like elements#5703DavertMik wants to merge 6 commits into
DavertMik wants to merge 6 commits into
Conversation
`Locator.field.labelContains` ended with three branches matching `.//*` with no tag restriction, so any container carrying `@aria-label`, `@title` or `@aria-labelledby` was returned by `findFields`. `xpathLocator.combine` joins branches with `|`, and a union is evaluated in document order, so a labelled wrapper always preceded the control it wraps. A Base UI slider (`div[role=group][aria-labelledby]` around `input[type=range]` with the same `aria-labelledby`) resolved to the `div`, and `selectOption` on a `ul[role=tablist][aria-label]` reported "Element is not a <select> element" instead of a clean not-found. Scope the three branches to a field tag or an editable ARIA role, which keeps every custom `role=combobox` / `role=textbox` / `role=listbox` widget reachable by accessible name while dropping plain containers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcwzSXPnfaig8nBZD2Vxfi
`span[role=checkbox][aria-label]` widgets are resolved through `findFields` by the WebDriver helper, so the allowlist must cover the checkable ARIA form controls too. Also drop `grabValueFrom` from the new wrapper test: the WebDriver helper resolves that action through `_locate`, not `findFields`, so it never accepted a fuzzy field name. `seeInField` / `dontSeeInField` assert the same thing across all three helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcwzSXPnfaig8nBZD2Vxfi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Locator.field.labelContainsended with three branches that had no tag restriction at all:.//*matches any element, so a wrapper carrying the accessible name was returned byfindFieldsalongside the control it wraps.
The mechanism is worth spelling out, because it explains why "just reorder the branches" would not
have worked:
xpathLocator.combinejoins the branches with|, and an XPath union is evaluated indocument order, not branch order. A labelled wrapper therefore always precedes the field inside it,
and
selectElementtakesels[0].Two measured consequences:
Slider.Rootrenders<div role="group" aria-labelledby="…-label">and thereal
<input type="range">inside carries the samearia-labelledby.findFieldsreturned[div, input], sofillField/clearFieldfailed withElement is not an <input>, <textarea>, <select> or [contenteditable].I.selectOption('Settings tabs', 'Password')matched therole="tablist"container via@aria-labeland failed withElement is not a <select> elementinstead of a clean not-found.In both cases the real field was in the result set — it just lost the race to a container.
Change
lib/locator.js— the three loose branches now require a field tag or a form-control ARIA role:Why the allowlist and not "prefer innermost"
The plan offered two options. The allowlist (a) removes the container from the node-set outright, so
document order stops mattering. Option (b) — drop any match that contains another match — would have
to fight the union's document ordering and would change the result order of every currently-passing
findFieldscall, including the custom combobox/listbox cases below. The allowlist changes only whichelement types may match, which is the property actually being asserted.
Two entries in that list are load-bearing for existing tests, not speculative:
listbox—custom_select.phphas a standalonediv[role=listbox][aria-labelledby]("Favorite Color") that
selectOptionmust resolve, plus a combobox + listbox pair sharing onearia-labelledbywhere both matches are relied on.checkbox—role_elements.phphasspan[role=checkbox][aria-label]widgets that the WebDriverhelper reaches through
findFields(proceedSeeCheckbox). A first revision of this PR omitted thecheckable roles and turned that test red in CI;
radioandswitchare included with it so theARIA form-control family stays complete.
optionis deliberately not included — no fixturecarries an accessible name on a
role=option, and options are reached through their listbox.labelEqualsandbyTextwere left alone — they have no loose branch to restrict.Locator.clickable.widehas the same three lines and is deliberately untouched: clicking an elementby its accessible name is intended behaviour there, and the tablist regression in
clickablewasalready handled separately by
clickable.self's narrowest-match guard.Regression audit
Every
aria-label/aria-labelledby/titleintest/data/was checked: each one reached by afield action sits on an
input, arole=combobox|textbox|listboxwidget or arole=checkboxwidget;the remaining ones are on
<a>/<svg>elements only reachable byclick. The role-locator and"aria selectors without role locators" suites pass unchanged — they are the guard that the allowlist
is not too tight.
Tests
test/unit/locator_test.js— newLocator.field.labelContainsblock pinning the XPath: the sliderwrapper is dropped and the
inputresolved;ul[role=tablist][aria-label]anddiv[role=group][title]match nothing;div[role=textbox][aria-label],span[role=checkbox][aria-label]andinput[aria-label]still match; a combobox + listbox sharingone
aria-labelledbystill yields both.test/data/app/view/form/field_containers.php— new fixture: a Base-UI-shapeddiv[role=group][aria-labelledby]wrappinginput[type=range][aria-labelledby], adiv[role=textbox][aria-labelledby], and aul[role=tablist][aria-label].test/helper/webapi.js—seeInField('Volume', '30')/dontSeeInField('Volume', '70')prove theinput wins over the wrapper;
seeInField('Nickname', 'Bob')proves a custom widget is stillreachable;
selectOption('Settings tabs', …)asserts a clean not-found rather than… <select> ….Read-only assertions were used for the slider on purpose — Playwright's
fill()handlesinput[type=range]but Puppeteer and WebDriver send keystrokes, which do not set a range value.grabValueFromis not used either: the WebDriver helper resolves it through_locate, notfindFields, so it has never accepted a fuzzy field name.Local runs (serially, own fixture server): WebDriver 390 passing / 0 failing, Playwright
435 passing / 3 failing (all three are pre-existing external-network tests — a
seeTrafficassertion against codecept.io and two third-party
makeApiRequestcalls), unit 777 passing,lint clean.
Not fixed here — follow-up
Puppeteer's
findFieldshas a second loose path this PR does not touch: after the XPathstrategies it falls back to
page.$$('::-p-aria(<name>)'), which resolvesul[role=tablist][aria-label="Settings tabs"]by accessible name and still producesElement is not <select>. Restricting that means changing helper code, which is outside this change'sscope, so the tablist integration test carries an
isHelper('Puppeteer')skip with the reason statedinline, and the gap is left for a follow-up. The unit test still pins the locator-level behaviour for
all helpers, and the slider fix works on Puppeteer today (verified locally).
Overlap with #5701
PR #5701 (
fix/fill-field-combobox, open, targets4.x) edits the same functions: it addsnot(./@aria-hidden = 'true')to the tag-restricted branches oflabelEquals,labelContains,byNameandbyText. That work is complementary — it filters Base UI's hidden mirror inputs, this onerestricts which element types the loose branches may match — but the edits sit on adjacent lines of
labelContains, so a textual conflict is expected when the second of the two merges. Both changesmust be kept; nothing here reverts or reflows those
aria-hiddenguards.🤖 Generated with Claude Code
https://claude.ai/code/session_01TcwzSXPnfaig8nBZD2Vxfi