Skip to content

test(browser): scroll past tall rows before asserting a new reading row #110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wesbillman merged 1 commit into from
Sep 21, 2026
Merged

test(browser): scroll past tall rows before asserting a new reading row #110

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 20 additions & 27 deletions tests/browser/layout.spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { test, expect } from "./fixture.mjs";
import { settle, upper, expectAnchor } from "./timeline.mjs";
import { anchor, settle, upper, expectAnchor } from "./timeline.mjs";

const scroll = test.extend({ historyCounts: { alpha: 20, beta: 1 } });
// Resize tests must not enter the fixture’s deliberately held paging path.
Expand Down Expand Up @@ -606,33 +606,26 @@ readingTest(
const history = page.getByRole("region", {
name: "Channel message history",
});
const before = await history.evaluate((el) => el.scrollTop);
await history.hover();
await page.mouse.wheel(0, -300);
await expect
.poll(() => history.evaluate((el) => el.scrollTop))
.toBeLessThan(before);
await settle(page);
// A tall paragraph need not fit wholly in the narrowed viewport. Capture
// the visible reading row, including the production clipped-row fallback.
const reading = await history.evaluate((el) => {
const bounds = el.getBoundingClientRect();
const rows = Array.from(el.querySelectorAll("[data-message-id]"));
const row =
rows.find((row) => {
const p = row.querySelector("p").getBoundingClientRect();
return p.top >= bounds.top && p.bottom <= bounds.bottom;
}) ??
rows.find((row) => {
const rect = row.getBoundingClientRect();
return rect.bottom > bounds.top && rect.top < bounds.bottom;
});
if (!row) throw new Error("No visible post-gesture reading row");
return {
id: row.dataset.messageId,
y: row.querySelector("p").getBoundingClientRect().top - bounds.top,
};
});
// A tall row above the anchor can exceed one wheel step, and a tall
// paragraph need not fit wholly in the narrowed viewport. Keep making real
// progress until the visible reading row (including the production
// clipped-row fallback) belongs to another message; never repeat a read
// until an immobile timeline happens to pass.
let reading = original;
for (
let gesture = 0;
gesture < 6 && reading.id === original.id;

@wesbillman wesbillman Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

gesture++
) {
const before = await history.evaluate((el) => el.scrollTop);
await page.mouse.wheel(0, -300);
await expect
.poll(() => history.evaluate((el) => el.scrollTop))
.toBeLessThan(before);
await settle(page);
reading = await anchor(page);
}
expect(reading.id).not.toBe(original.id);
await button(page, "Close Bestie panel").click();
await settle(page);
Expand Down
Loading