test(browser): scroll past tall rows before asserting a new reading row - #110
Conversation
Since the reading fixture shrank to 20 tall messages (#83), the row above the anchored one can be taller than a single 300px wheel step, so the first wholly visible paragraph stayed the same message and "panel restoration yields to a new wheel reading position" failed on main in both engines. Keep making bounded, verified wheel progress until the visible reading row belongs to another message, reusing the shared anchor reader. Signed-off-by: Thomas Petersen <thomasp@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Thanks @thomaspblock !!!
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 regression-coverage defect. Reviewed head 664b27ec1ff058d25d5b0ce6057a199c248e96e9 against base 42984601ebe1327006526cb03d5335264b377537.
The shared anchor() extraction is equivalent to the old inline reader, and the progress checks/final tolerance are retained. However, the extra gestures make the test insensitive to the precise stale-restoration regression it should catch; see inline finding.
Validation: the complete layout file passes 16/16 locally in Chromium/WebKit on the unmodified head. A review-only build mutation deleting just the gesture handler’s restoredAnchor.current = undefined also passes the revised case in both engines. Using 24 tall messages instead makes that same mutation fail both engines by 239.8125px, while the healthy 24-message variant passes the complete layout file 16/16. All 12 hosted checks are successful. No production edits or commits were made; native WebViews/touch and a broad local suite were not exercised.
Exit criterion: keep the fixture compact, but preserve a healthy-pass / missing-gesture-reset-fail distinction in both engines. The verified 24-message alternative is sufficient; restoring 640 rows or adding machinery is unnecessary.
| let reading = original; | ||
| for ( | ||
| let gesture = 0; | ||
| gesture < 6 && reading.id === original.id; |
There was a problem hiding this comment.
[P2] Keep the restored row visible when establishing the replacement anchor
With this 20-row fixture, both Chromium and WebKit need three 300px gestures before reading.id changes. At that point the original row’s top is 884.72px in a 700px viewport: it has moved completely offscreen. positionAt() (ChannelTimeline.tsx:34-43) already ignores an offscreen restoredAnchor, independently of whether reader input clears it. Consequently the test now passes even if the gesture handler’s restoredAnchor.current = undefined is deleted, so a regression that retains the old reading anchor through wheel input is no longer caught.
Reproduced on this head in both engines using a test-build-only mutation removing that one assignment. The former single-gesture/640-row setup rejects the same mutation at expectAnchor by 239.8125px. A compact alternative is enough: changing the tall Alpha history from 20 to 24 makes the revised case reject the mutation by the same amount, while healthy production code passes the complete layout file 16/16 across both engines.
Please establish the new reading anchor while the previous restored row still intersects the viewport, and preserve that fail/pass distinction. Using the verified 24-row fixture (plus an explicit old-row-intersection precondition to document the geometry) avoids a large history or more scrolling machinery.
Summary
mainhas been red since Reduce browser fixture histories and keep last message rows reachable #83 landed:tests/browser/layout.spec.mjs› "panel restoration yields to a new wheel reading position" fails in both Chromium and WebKit atexpect(reading.id).not.toBe(original.id)(run on 4776dda, run on 4298460).scrollTop, then settle) until the visible reading row belongs to another message, reusing the sharedanchor()reader instead of the inline copy. No sleeps, retries, or relaxed assertions; the rest of the test is unchanged.Test plan
origin/mainmerged into a feature branch in both engines with the same assertion; with this changelayout.spec.mjspasses 16/16 in Chromium and WebKit, and the fixed case passes 8/8 with--repeat-each 4.origin/main+ this patch): 2/2 across both engines.