{{ message }}
Wait for the Compare nav instead of sampling for it - #9264
Merged
Conversation
#9251's Windows Chat UI run failed with [ui-extra] FAIL: Compare nav not found six milliseconds after the step began. Locator.count() does not wait. Playwright's auto-waiting covers actions and expectations, not counting, so the gate was a sample of one instant presented as a question about the app, and it sampled the instant a reload snapshot overlay was still up. #9251 paints a cloned overlay over the app and removes it on hydration (or after 5000ms), which opens a window where the composer is on screen but not yet in the accessibility tree. The report was true about that instant and false about the app, and in CI it is indistinguishable from the menu item having been deleted, which is the expensive part: it sends the next reader looking for a removed control that was never removed. wait_for_first() waits and returns None on timeout, so the callers keep their present-or-not branch, including the fallback that legitimately expects a miss and looks under the More submenu. Only a timeout is swallowed: a closed page still raises rather than being reported as a missing feature. The failure message now names which of the two controls never appeared. This is the first step after load, so it is the one that pays for anything slowing first paint. It will not be the last change to do that. Six tests, no browser needed, since a locator is a small protocol. All four mutants (shortened default, blanket except, raising, waiting on visible) were reintroduced and confirmed red.
for more information, see https://pre-commit.ci
danielhanchen
force-pushed
the
robustify-compare-nav
branch
from
August 19, 2026 07:20
8485d77 to
10b52ac
Compare
danielhanchen
added a commit
that referenced
this pull request
Aug 19, 2026
* Scroll into view before a forced click
`Compare tab: send to two panes` failed on a macOS runner with
Locator.click: Element is outside of the viewport
then a cascade of TargetClosedError as the browser came down behind it. The menu
item existed and was found. It was simply below the fold: a Mac runner's window
is shorter than a Linux one and Compare sits at the bottom of a long menu.
`click(force = True)` turns off actionability checks, which is exactly what these
call sites want against a menu whose overlay would otherwise intercept the click.
It also turns off the scroll that brings the element into the viewport, and
Playwright will not click a point it cannot reach. So the flag was doing two
things and only one of them was wanted.
The forced clicks are not new -- they arrived with the composer redesign in #5891
and have always worked on Linux. What changed is #9264, which made the Compare nav
be found reliably instead of sampled for: before that the step often gave up
earlier with "Compare nav not found" and never reached the click. That PR is
correct; it surfaced this rather than causing it, the same way #9283 was going to
surface the picker's context-pin assertion once the shard stopped dying in apt.
click_forced keeps force and adds the scroll back. The scroll is best effort: an
element that cannot be scrolled -- fixed position, zero size -- should still reach
the click and fail there with Playwright's own message, rather than here with a
scrolling one that names the wrong problem.
Applied to all 17 forced clicks across the three drivers, not just the one that
failed. They are the same hazard on the same runners, and the guard would be
worth little if the file it was written for were the only one obeying it. The
guard is a source scan for the bare shape, so a new one fails here rather than
twenty minutes into a Mac job.
Driven against a fake locator: these tests run on browserless lanes, and the
ordering is the whole contract, so a stand-in that records call order tests it
exactly -- scroll before click, force preserved, a failing scroll not swallowing
the click, and a failing click still propagating.
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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
1 participant
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.
The failure
#9251's Windows Chat UI run:Six milliseconds. That is not a probe finding a missing feature, it is a probe not looking.
Locator.count()does not wait. Playwright's auto-waiting covers actions and expectations, not counting, soif plus_btn.count() > 0:is a sample of one instant presented as a question about the app.The instant it sampled was one where a reload snapshot overlay was still up. #9251 paints a cloned overlay over the app and removes it on hydration, or after 5000ms, whichever comes first, which opens a window where the composer is on screen but not yet in the accessibility tree.
So the report was true about that instant and false about the app. In CI it is indistinguishable from the menu item having been deleted, and that is the expensive part: it sends the next reader looking for a removed control that was never removed.
The change
wait_for_first()waits and returnsNoneon timeout, so the callers keep their present-or-not branch. That matters here: one of the fallbacks legitimately expects a miss and goes looking under the "More" submenu, so raising would turn a real branch into a crash. The menu-item lookups get a short 2s wait for the same reason; only the first-paint gate gets the full one.Only a timeout is swallowed. A closed page or a bad selector still raises, rather than being quietly reported as a missing feature.
The message now names which of the two controls never appeared, instead of "Compare nav not found" for either.
This is the first step after load, so it is the one that pays for anything slowing first paint. It will not be the last change to do that, which is why the helper is shared rather than inlined -- there are 19 more
count()gates in that file.On #9251 itself
This does not paper over a product bug, and it is worth saying why I think that. The overlay window is intended behaviour: the whole point of that PR is to keep a reload from flashing blank, and content that is painted-but-not-yet-interactive for a few hundred milliseconds is what that costs. A probe that cannot tell "not yet" from "not there" would have broken on any of several reasonable implementations.
What is worth checking on #9251 separately is whether the overlay should be
inert/aria-hiddenwhile it is up, so assistive technology and automation both see one app rather than two. That is a question about the PR, not about this helper.Verification
Six tests, and no browser needed -- a locator is a small protocol (
.first,.wait_for), so the timeout, success, pass-through and error paths are all checkable directly.All four mutants were reintroduced and confirmed red:
The default wait is the one number here that is not arbitrary: it has to outlast the 5000ms the overlay gives itself, or it samples inside the window it exists to outlast.
The playwright import is deliberately local to the function, and a test asserts it stays there: this module is read by harness-contract tests on runners with no browser stack, where a module-scope import turns skips into collection errors.
tests/studio: 4462 passed, 4 skipped.