Press the mark where the reader can see it - #37
Conversation
`test_an_open_composer_does_not_eat_the_next_click` opened its composer on `#q`, which scrolled the commented paragraph 141px above the top of the window, and then pressed the mark there anyway. A painted range keeps its client rects while it is scrolled away — they simply go negative — so `mark_point` handed back a point outside the window and `page.mouse` pressed it. The press was heard because the old hit test asked only the rects' arithmetic; `markAt` now asks `pageWords` about the element under the point first, and `elementFromPoint` answers null outside the window, so the click reached `<html>` and the panel never opened. The runtime is right: a point over none of the page's words is not a mark, and no reader can press above the window. So the gesture becomes one a reader makes — the passage is scrolled back into view before the press, which is what the fixture puts `#q` that far down the page for, and the composer stays up across a scroll, so it is still an open composer the press has to get past. `mark_point` asserts the point is on screen, so the next caller to press off the window is told which mark and where, rather than 30 seconds later in `panel_settled` as a thread that would not open. Bug-back: with the composer's teardown made to rewrite the marked paragraph's nodes at mousedown — the swap the test exists for — it fails again at the same wait.
leaf-agent
left a comment
leaf-agent
left a comment
There was a problem hiding this comment.
The diagnosis holds up: pageWords's Boolean(node) clause is what turned the off-window press into a null hit, elementFromPoint answers for every point inside the window, and a press above the top edge is an event history no reader produces — so fixing the gesture rather than the hit test is the right level. Two things about the guard itself, neither blocking.
The same point is computed inline a few tests down. test_a_click_on_a_mark_decides_once builds mark_point's expression by hand — const r = [...CSS.highlights.get('lf-mark')][0].getClientRects()[0]; returning {x: r.left + r.width / 2, y: r.top + r.height / 2} — and presses it with page.mouse.click(spot["x"], spot["y"]), after its own scroll_into_view_if_needed. It is the same gesture on the same highlight, so it inherits none of the new assertion, and if that press ever goes off-window it fails as the same 30-second panel_settled timeout this PR set out to make legible. page.mouse.click(*mark_point(page, "lf-mark")) is behaviour-identical there. I haven't run the browser suite from this job, so that swap wants a run before it lands.
The floor lets the banner through — see the inline note. Worth weighing because the helper's second call in this test asserts an absence, so a press the banner eats reads as green.
|
The same failure came back on the next commit — run 32750967520 at 9b7a7ee, byte-identical: Reached the same diagnosis independently from that run before finding this PR, and it lands in the same two places: the press was at (413, −142) on a 1200×900 window because reaching The one thing not yet on this thread: #36 and this PR are jointly sufficient and individually not. This branch's own Local verification of current
|
Review on #37: the floor the guard put under a press was `0`, and the top of every page belongs to the banner — `.lf-ui`, fixed, over the words — so a mark scrolled under the bar passed the guard and lost its press exactly as the off-window one did. It is the worse of the two here, because the second press in `test_an_open_composer_does_not_eat_the_next_click` asserts that nothing opens. The floor is now read off the page rather than written as a number: the helper asks `elementFromPoint` what is under the point and refuses `null` — outside the window — or anything inside `.lf-ui`, which is `markAt`'s own veto. A constant would have been a second copy of `--lf-banner-h`, and wrong on any page whose banner is shorter or absent. `test_a_click_on_a_mark_decides_once` built the same point inline, character for character, and so inherited none of this while ending in the same `panel_settled` wait. It calls the helper now. Bug-backs: with the passage scrolled under the bar, "painted at (413, 30), under the layer's lf-banner"; with the scroll removed, the same sentence ending "outside the window".
|
Both taken; pushed as a second commit. The inline copy in The floor is read off the page instead of written as 48 — see the inline reply. The helper asks Verified on a Linux runner, not by inspection: On the recurrence above: the branch still applies to current |
leaf-agent
left a comment
leaf-agent
left a comment
There was a problem hiding this comment.
Both points from the last pass are settled — the inline copy in test_a_click_on_a_mark_decides_once calls the helper, and the floor is read off the page instead of copying --lf-banner-h. One thing about the replacement itself, inline with a suggestion: the veto it asks is a near-copy of pageWords's rather than the same question, and [data-lf-said] is where the two come apart. Non-blocking — nothing in the corpus reaches it today, and the failure it would produce is a named assertion at the call site rather than the silent timeout this PR set out to remove.
The helper's veto was `closest('.lf-ui')`, which parts from `inUi` on a declared label: the nearest element wins there, so a label a widget declared as the page's own words is nearer than the control it sits in and a mark painted on it is pressable. The helper refused that press and named the control. It also named the wrong class whenever chrome came from `offer()`, whose `${cls} lf-ui` puts `lf-ui` last.
|
The new part is that the pair has now been verified together, on the current head — which neither PR can claim alone: each was verified against its own base, neither against the other, and ac03b7f itself added 130 lines to The green is the fixes rather than the runner: the same machine reproduces both failures byte-identically at plain ac03b7f, the 30s What was runMerge base Run in two halves (271 + 284) because this runner has 4 cores and the suite does not fit one window; the split is by collected node id, so every test ran. Timing note: the 555-test run was against 00cb3b4, this branch's head when it started. f63cf5c landed while it ran, so the nine tests touching the two changed helpers — the three |
|
Check state at So the test this PR is for passes on the branch, and |
|
Seventh occurrence, at 20f387c — run 32763448588, same two failures as the sixth: this PR's What is new is that the caveat on the previous verification is closed. That run was the whole module against #37's head at the time (00cb3b4), with only nine targeted tests re-run once it advanced to f63cf5c. This one is the whole of The one that is not green is So the conclusion from the sixth occurrence stands two commits further out: merging this and #36 is enough to green What was runMerge base Run in three chunks at |
|
Closing as superseded: the required reader-visible mark behavior is already on main in a simpler form. This branch is now conflicting and red; any remaining helper hardening can return as a narrow follow-up. |
2 participants
Problem
ciis red onmainat 91515b5 with two failures. This PR is the new one:test_an_open_composer_does_not_eat_the_next_clicktimes out inpanel_settled— a mark it pressed did not open its thread. It was green on the run before, and 91515b5 is the commit that reaches it. (The other failure,test_a_note_sets_the_page_axis_with_its_whole_strip, is the fifth occurrence of the 7.5px scroll gutter and belongs to #36, which is open; nothing about it is in this diff.)The test opens its composer on
#q, and reaching#qscrolls the commented paragraph 141px above the top of the window. It then presses the mark up there anyway. A painted range keeps its client rects while it is scrolled away — they simply go negative — somark_pointhanded back(413, -142)on a 1200×900 window andpage.mousepressed it. Nothing above the fold is anyone's:elementFromPointanswers null outside the window, so the press arrived at<html>and the panel stayed shut.It used to be heard because the old hit test only vetoed
inUi(over), which is false fornull, and then matched the point against the rects' arithmetic — which happily contains a point off the screen. 91515b5 replaced that veto withpageWords, whose first clause isBoolean(node), somarkAtnow declines a point that is over none of the page's words.Solution
The fix is in the test. The runtime is right — a point outside the window is over none of the page's words, and a reader cannot press there;
elementFromPointreturns an element for every point inside it, so no real press is affected. What was wrong is the gesture: the suite's own rule is to drive the browser a reader gets, and a press above the top edge is an impossible event history.So the passage is scrolled back into view before the press. That is what the fixture puts
#qthat far down the page for — with the scroll, the mark (y=138) and the composer (y=750) are on screen together, which is the state the test's name describes. The composer stays up across a scroll (only a mousedown outside it stands it down), so it is still an open composer that the press has to get past.mark_pointthen asserts its point is one a press can reach, which is the durable half. It is the shared helper for every real press on a highlight, and an unreachable point out of it is undetectable at the call site: the failure surfaces 30 seconds later in whatever wait came next, as a thread that would not open. The question it asks is the runtime's own —elementFromPointat the point — so it refusesnull(outside the window) and anything inside.lf-ui(the banner holding the top of every page, the panel, a composer floating over the words it quotes), naming which of the two it was. That ismarkAt's veto rather than a copy of--lf-banner-h, so it stays right on a page whose banner is shorter or absent.test_a_click_on_a_mark_decides_oncebuilt the same point inline, character for character, and ended in the samepanel_settledwait; it calls the helper now.Testing
uv run pytest tests/test_render.py --run-nightlyat this head, on a Linux runner: 553 passed, 1 failed — the failure beingtest_a_note_sets_the_page_axis_with_its_whole_strip, which is Read the note-set axis against the page's box, not the window #36's and unreachable from this diff. Both tests touched here pass.panel_settledwait. The scroll did not make it vacuous.the lf-mark mark at index 0 is painted at (413, 30), under the layer's lf-banner — no press there reaches the page, so bring the passage into view before pressing it; with the scroll removed, the same sentence endingoutside the window. Both at the call, instead of the timeout.pre-commit run --files tests/test_render.pyclean.Automated fix for failed run