Skip to content

Stop forced layout reads in the bottom-anchored scroll hot path #2280

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

Closed
vburojevic wants to merge 1 commit into from
Closed

Stop forced layout reads in the bottom-anchored scroll hot path #2280

Show file tree
Hide file tree
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
234 changes: 228 additions & 6 deletions apps/app/src/components/ui/bottom-anchored-scroll-body.scroll-preservation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,13 +232,18 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {
top: 80,
bottom: 180,
});
// Content lays out settled at the bottom; the ResizeObserver delivery
// refreshes the component's cached max scroll offset, exactly as a real
// browser does whenever the scroll port or content wrapper resizes.
setScrollMetrics(scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 150,
scrollTop: 300,
});
getLatestResizeObserver().trigger();

// User-intent scroll away from bottom, then a scroll event triggers capture.
scrollArea.scrollTop = 150;
fireEvent.wheel(scrollArea);
fireEvent.scroll(scrollArea);

Expand Down Expand Up @@ -271,9 +276,11 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {
setScrollMetrics(scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 150,
scrollTop: 300,
});
getLatestResizeObserver().trigger();

scrollArea.scrollTop = 150;
fireEvent.wheel(scrollArea);
fireEvent.scroll(scrollArea);

Expand Down Expand Up @@ -302,9 +309,11 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {
setScrollMetrics(scrollArea, {
scrollHeight: 1_400,
clientHeight: 100,
scrollTop: 1_000,
scrollTop: 1_300,
});
getLatestResizeObserver().trigger();

scrollArea.scrollTop = 1_000;
fireEvent.wheel(scrollArea);
fireEvent.scroll(scrollArea);

Expand All @@ -330,9 +339,11 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {
setScrollMetrics(scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 150,
scrollTop: 300,
});
getLatestResizeObserver().trigger();

scrollArea.scrollTop = 150;
fireEvent.wheel(scrollArea, { deltaY: -100 });
fireEvent.scroll(scrollArea);
fireEvent.click(getByRole("button", { name: "Capture prepend anchor" }));
Expand Down Expand Up @@ -400,6 +411,7 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {
clientHeight: 100,
scrollTop: 300,
});
getLatestResizeObserver().trigger();

fireEvent.scroll(scrollArea);

Expand Down Expand Up @@ -641,8 +653,10 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {
setScrollMetrics(a1.scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 150,
scrollTop: 300,
});
getLatestResizeObserver().trigger();
a1.scrollArea.scrollTop = 150;
fireEvent.wheel(a1.scrollArea);
fireEvent.scroll(a1.scrollArea);
a1.unmount();
Expand All @@ -664,8 +678,10 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {
setScrollMetrics(b.scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 150,
scrollTop: 300,
});
getLatestResizeObserver().trigger();
b.scrollArea.scrollTop = 150;
fireEvent.wheel(b.scrollArea);
fireEvent.scroll(b.scrollArea);
b.unmount();
Expand Down Expand Up @@ -702,4 +718,210 @@ describe("BottomAnchoredScrollBody scroll preservation", () => {

expect(a2.scrollArea.scrollTop).toBe(220);
});

it("never reads scrollHeight or clientHeight from per-scroll-event handlers", () => {
const { scrollArea, rowElements } = renderTimeline({
threadId: "thread-a",
rowIds: ["row-a", "row-b", "row-c"],
});
mockScrollAreaRect(scrollArea);
mockRowRect(requireHTMLElement(rowElements.get("row-a")!), {
top: -120,
bottom: -20,
});
mockRowRect(requireHTMLElement(rowElements.get("row-b")!), {
top: -20,
bottom: 80,
});
mockRowRect(requireHTMLElement(rowElements.get("row-c")!), {
top: 80,
bottom: 180,
});
setScrollMetrics(scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 300,
});
getLatestResizeObserver().trigger();

// From here on, every scrollHeight/clientHeight read is observable. On an
// unvirtualized timeline those getters force a full synchronous layout
// pass, so scroll/wheel handlers must run on the cached max offset alone.
const readScrollHeight = vi.fn(() => 400);
const readClientHeight = vi.fn(() => 100);
Object.defineProperty(scrollArea, "scrollHeight", {
configurable: true,
get: readScrollHeight,
});
Object.defineProperty(scrollArea, "clientHeight", {
configurable: true,
get: readClientHeight,
});

// The attach -> detach edge is allowed exactly one verification read (the
// content-shrink guard re-testing the cached off-bottom classification).
scrollArea.scrollTop = 150;
fireEvent.wheel(scrollArea);
fireEvent.scroll(scrollArea);
expect(readScrollHeight).toHaveBeenCalledTimes(1);
expect(readClientHeight).toHaveBeenCalledTimes(1);
readScrollHeight.mockClear();
readClientHeight.mockClear();

// Steady state — a mid-timeline scroll burst, a wheel-down, and a return
// to the bottom — must be entirely read-free.
for (let scrollTop = 140; scrollTop >= 50; scrollTop -= 10) {
scrollArea.scrollTop = scrollTop;
fireEvent.scroll(scrollArea);
}
fireEvent.wheel(scrollArea, { deltaY: 120 });
scrollArea.scrollTop = 300;
fireEvent.scroll(scrollArea);

expect(readScrollHeight).not.toHaveBeenCalled();
expect(readClientHeight).not.toHaveBeenCalled();
// The cached geometry still classified the burst correctly: the detach
// captured the top-most visible row mid-timeline...
expect(readAnchor("thread-a")).toEqual({
rowId: "row-b",
offsetWithinRow: 20,
atBottom: false,
});
// ...and the final scroll re-attached to the bottom, so the next growth
// (a legitimate fresh read in the resize path) restores to the new max.
setScrollMetrics(scrollArea, {
scrollHeight: 500,
clientHeight: 100,
scrollTop: 300,
});
getLatestResizeObserver().trigger();
expect(scrollArea.scrollTop).toBe(400);
});

it("stays pinned when a shrink-frame scroll event outruns the resize refresh", () => {
const { scrollArea, rowElements } = renderTimeline({
threadId: "thread-a",
rowIds: ["row-a", "row-b", "row-c"],
});
mockScrollAreaRect(scrollArea);
mockRowRect(requireHTMLElement(rowElements.get("row-a")!), {
top: -120,
bottom: -20,
});
mockRowRect(requireHTMLElement(rowElements.get("row-b")!), {
top: -20,
bottom: 80,
});
// Pinned at the bottom of settled content; the cached max offset is 300.
setScrollMetrics(scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 300,
});
getLatestResizeObserver().trigger();

// iOS tap-collapsing a long tool output while pinned: the touch marks
// user intent, the content shrinks (new max offset 100), and the browser
// clamps scrollTop and delivers the scroll event BEFORE the
// ResizeObserver refresh — the cache still says 300.
fireEvent.touchStart(scrollArea);
setScrollMetrics(scrollArea, {
scrollHeight: 200,
clientHeight: 100,
scrollTop: 100,
});
fireEvent.scroll(scrollArea);

// The stale-high cache reads 200px off-bottom with recent intent, which
// would detach for good (the bottom-restore is suppressed once
// stick-to-bottom is off) and persist a mid-timeline row anchor. The
// detach-edge verification must keep us pinned instead.
expect(readAnchor("thread-a")).toEqual({
rowId: "",
offsetWithinRow: 0,
atBottom: true,
});

// The late resize delivery finds stick-to-bottom intact, so further
// content growth keeps following the bottom.
getLatestResizeObserver().trigger();
setScrollMetrics(scrollArea, {
scrollHeight: 250,
clientHeight: 100,
scrollTop: 100,
});
getLatestResizeObserver().trigger();
expect(scrollArea.scrollTop).toBe(150);
});

it("tracks isAtBottom transitions against the cache refreshed by resizes", () => {
const { scrollArea, rowElements } = renderTimeline({
threadId: "thread-a",
rowIds: ["row-a", "row-b", "row-c"],
});
mockScrollAreaRect(scrollArea);
mockRowRect(requireHTMLElement(rowElements.get("row-a")!), {
top: -120,
bottom: -20,
});
mockRowRect(requireHTMLElement(rowElements.get("row-b")!), {
top: -20,
bottom: 80,
});
setScrollMetrics(scrollArea, {
scrollHeight: 400,
clientHeight: 100,
scrollTop: 300,
});
getLatestResizeObserver().trigger();

// Detach: against the cached max offset (300), 100 is far off the bottom.
scrollArea.scrollTop = 100;
fireEvent.wheel(scrollArea);
fireEvent.scroll(scrollArea);
expect(readAnchor("thread-a")).toEqual({
rowId: "row-b",
offsetWithinRow: 20,
atBottom: false,
});

// Content doubles while detached: the resize refreshes the cache (max
// offset 700) without yanking the detached viewport.
setScrollMetrics(scrollArea, {
scrollHeight: 800,
clientHeight: 100,
scrollTop: 100,
});
getLatestResizeObserver().trigger();
expect(scrollArea.scrollTop).toBe(100);

// 694 is 6px shy of the refreshed bottom (700), outside the 4px threshold:
// still detached. The stale pre-resize max (300) would misclassify it as
// at-bottom and the growth below would yank to the new maximum.
fireEvent.wheel(scrollArea, { deltaY: 400 });
scrollArea.scrollTop = 694;
fireEvent.scroll(scrollArea);

setScrollMetrics(scrollArea, {
scrollHeight: 900,
clientHeight: 100,
scrollTop: 694,
});
getLatestResizeObserver().trigger();
expect(scrollArea.scrollTop).toBe(694);

// 797 is 3px shy of the again-refreshed bottom (800): re-attaches...
fireEvent.wheel(scrollArea, { deltaY: 200 });
scrollArea.scrollTop = 797;
fireEvent.scroll(scrollArea);

// ...so the next content growth follows the bottom again.
setScrollMetrics(scrollArea, {
scrollHeight: 1_000,
clientHeight: 100,
scrollTop: 797,
});
getLatestResizeObserver().trigger();
expect(scrollArea.scrollTop).toBe(900);
});
});
Loading
Loading