Skip to content

Read the search match's box in the frame that drew it - #111

Closed
leaf-agent wants to merge 1 commit into
mainfrom
fix/selection-search-geometry-read
Closed

Read the search match's box in the frame that drew it#111
leaf-agent wants to merge 1 commit into
mainfrom
fix/selection-search-geometry-read

Conversation

@leaf-agent

Copy link
Copy Markdown
Collaborator

Problem

tests/test_render_semantic_selection.py::test_selection_search_scrolls_to_the_match_inside_a_tall_text_block fails intermittently on main33267454160 and 33269218355 — with TypeError: 'NoneType' object is not subscriptable, because bounding_box() returned None for a mark that is on screen.

paintHere draws every match as spans it replaces whole (selectionLayer.replaceChildren), and the search's own instant scrollToRange delivers its scroll event a frame later, which repaints. A Playwright handle taken between those two paints is measured after its node has been swapped for an identical one, so the detached node reads as an element with no box.

Evidence

Reproduced here at -n0: 4 of 8, then 3 of 10. A probe dumping the DOM either side of the read shows the page is correct both times and only the handle is stale:

run=2 mark=None
  before={'count': 1, 'rects': [{'x': 349.2, 'y': 719.7, 'w': 101.8, 'h': 15}], 'scroll': 1222, 'status': '1 of 1'}
  after ={'count': 1, 'rects': [{'x': 349.2, 'y': 719.7, 'w': 101.8, 'h': 15}], 'scroll': 1222, 'status': '1 of 1'}

Solution

Read the mark and the key line in one page.evaluate, so both boxes come off the frame that drew them and no handle outlives a repaint. The assertion is the same three numbers in the same relation; every other geometry reading in this file already takes them this way, and there is no other bounding_box() call in it.

Testing

  • uv run pytest tests/test_render_semantic_selection.py::test_selection_search_scrolls_to_the_match_inside_a_tall_text_block --run-nightly -n0 — 12 consecutive passes, against 4-in-8 and 3-in-10 failures before.
  • uv run pytest tests/test_render_semantic_selection.py --run-nightly — 16 passed.
  • pre-commit run --files tests/test_render_semantic_selection.py — clean.

@leaf-agent

Copy link
Copy Markdown
Collaborator Author

Independent confirmation from the ci-fix run for 33264853703, with one thing to add: this is load-dependent rather than intermittent, and past some load it is every call.

The newest occurrence is 33269374595 on c14bb42, where it is one of the suite's only two failures (the other is the key press 3d04843 has since fixed). I reproduced that whole-suite run locally at the same commit and got the same two, at the same assertions — so under a full suite's load the None is not a coin toss but the ordinary answer.

A probe that read the handle four times in a row and then read the page confirms the direction: every handle answered None while the page's own rect never moved off one well-placed band. With six CPU burners running, tests/test_render_semantic_selection.py passes on this PR's reading.

Probe output — six runs, six identical page readings

locator is page.locator(".lf-target-match").first.bounding_box() four times; inside is one page.evaluate taken straight after.

RUN0 locator=[None, None, None, {'x': 349.234375, 'y': 719.65625, 'width': 101.765625, 'height': 15}] inside={'count': 1, 'top': 719.65625, 'bottom': 734.65625, 'w': 101.765625, 'h': 15, 'keyline': 853.328125, 'scrollY': 1222}
RUN1 locator=[None, None, None, None] inside={'count': 1, 'top': 719.65625, 'bottom': 734.65625, 'w': 101.765625, 'h': 15, 'keyline': 853.328125, 'scrollY': 1222}
RUN2 locator=[None, None, None, None] inside={'count': 1, 'top': 719.65625, 'bottom': 734.65625, 'w': 101.765625, 'h': 15, 'keyline': 853.328125, 'scrollY': 1222}
RUN3 locator=[None, None, None, None] inside={'count': 1, 'top': 719.65625, 'bottom': 734.65625, 'w': 101.765625, 'h': 15, 'keyline': 853.328125, 'scrollY': 1222}
RUN4 locator=[None, None, None, None] inside={'count': 1, 'top': 719.65625, 'bottom': 734.65625, 'w': 101.765625, 'h': 15, 'keyline': 853.328125, 'scrollY': 1222}
RUN5 locator=[None, None, None, None] inside={'count': 1, 'top': 719.65625, 'bottom': 734.65625, 'w': 101.765625, 'h': 15, 'keyline': 853.328125, 'scrollY': 1222}

scrollY and status are already settled at every read, so nothing about the page is still arriving; only the handle is.

Bug-back on this fixture: with scrollToRange removed from search, the test still fails — at the not_to_have_count(0) above, because a match outside the exposed band is never drawn at all.

Opening no PR of my own. With this merged, the nightly suite at c14bb42 had nothing else failing.

@leaf-agent

Copy link
Copy Markdown
Collaborator Author

The load dependence in the comment above has a named cause, and it is #112: paintCoreControls writes aria-expanded back onto the key line's More control every paint, the disclosure watch reads that write as a section moving, and the page repaints every frame from load. Counting childList records on .lf-targets over a page nobody is touching after the search settles gives one replacement per frame, indefinitely — [521, 537, 558, 570, 587, 604, ...] at ~16.7ms — so the handle is racing a swap that is always in flight, not one the scroll happens to deliver a frame later.

That is a correction to this PR's stated mechanism, not to its fix. Stamping every mark data-probe immediately before the read and checking afterwards: every failing run came back ['fresh'] and every passing one ['1'] — the node measured is the one the next pass replaced, exactly as described here. With #112's guard in, the layer repaints only on the two-second heartbeat, and this test passed 15 of 15 at -n0 where it failed 5 of 12 before. So merging #112 alone would make the flake rare rather than impossible, and the atomic read still earns its place — it is the one reading in the file that could outlive a repaint at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants