mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): bottom-align short channels via row offset, not spacer padding
The short-channel topPad bottom-align was doubly broken. First, the pad was set as the spacer's paddingTop, but an absolutely-positioned virtual row resolves top:0 against the padding box inner edge — padding inflated the box without moving the rows, so they pinned to the top with dead space below (the inverse of bottom-align). Fold topPad into each row's translateY instead. Second, the pad ignored the non-row scroll chrome (the channel-header padding above the spacer and composer padding below), so the spacer over-padded by exactly that chrome and a short channel became scrollable. Subtract the measured chrome (scrollHeight minus spacer height, which is pad-invariant). Adds a fail-first e2e asserting a short channel bottom-aligns against the content-box floor and is not scrollable. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
co-authored by
Will Pfleger
parent
6cf3fde90a
commit
dff0511dca
@@ -49,6 +49,7 @@ export default defineConfig({
|
||||
"**/animated-avatar-screenshots.spec.ts",
|
||||
"**/reminders-screenshots.spec.ts",
|
||||
"**/virtualization-screenshots.spec.ts",
|
||||
"**/timeline-scroll-anchor.spec.ts",
|
||||
],
|
||||
use: {
|
||||
...devices["Desktop Chrome"],
|
||||
|
||||
@@ -226,6 +226,7 @@ export const MessageTimeline = React.memo(function MessageTimeline({
|
||||
|
||||
const {
|
||||
virtualizer,
|
||||
spacerRef,
|
||||
topPad,
|
||||
isAtBottom,
|
||||
newMessageCount,
|
||||
@@ -619,6 +620,7 @@ export const MessageTimeline = React.memo(function MessageTimeline({
|
||||
searchQuery={searchQuery}
|
||||
threadUnreadCounts={threadUnreadCounts}
|
||||
topPad={topPad}
|
||||
spacerRef={spacerRef}
|
||||
unfollowThreadById={unfollowThreadById}
|
||||
virtualizer={virtualizer}
|
||||
/>
|
||||
|
||||
@@ -85,6 +85,8 @@ type TimelineMessageListProps = {
|
||||
* pushes the absolute rows down without entering the measured total.
|
||||
*/
|
||||
topPad: number;
|
||||
/** Ref the hook reads to measure chrome around the spacer for `topPad`. */
|
||||
spacerRef: React.RefObject<HTMLElement | null>;
|
||||
};
|
||||
|
||||
export const TimelineMessageList = React.memo(function TimelineMessageList({
|
||||
@@ -116,6 +118,7 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({
|
||||
virtualizer,
|
||||
items,
|
||||
topPad,
|
||||
spacerRef,
|
||||
}: TimelineMessageListProps) {
|
||||
const reviewCommentsByRootId = React.useMemo(
|
||||
() => buildVideoReviewCommentsByRootId(messages),
|
||||
@@ -302,18 +305,18 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({
|
||||
}
|
||||
};
|
||||
|
||||
// The spacer is `box-sizing: border-box` (Tailwind global), so `topPad` is
|
||||
// folded into `height` AND set as `paddingTop`: the height keeps the full
|
||||
// scroll range while the padding pushes the absolute rows (positioned against
|
||||
// the padding box, `top:0` = inner edge) down to bottom-align a short channel.
|
||||
// `translateY` then uses the virtualizer's raw `start` — no double offset.
|
||||
// Bottom-align a short channel by shifting the absolute rows down by
|
||||
// `topPad`. The pad CANNOT be a `paddingTop` on the spacer: an
|
||||
// absolutely-positioned child resolves `top:0` against the padding box's
|
||||
// inner edge, so padding inflates the box without moving the rows — they
|
||||
// would pin to the top with dead space below. Folding `topPad` into each
|
||||
// row's `translateY` (and into the spacer height for the matching scroll
|
||||
// range) is what actually pushes the rows to the floor.
|
||||
return (
|
||||
<div
|
||||
className="relative w-full"
|
||||
style={{
|
||||
height: `${virtualizer.getTotalSize() + topPad}px`,
|
||||
paddingTop: topPad > 0 ? `${topPad}px` : undefined,
|
||||
}}
|
||||
ref={spacerRef as React.Ref<HTMLDivElement>}
|
||||
style={{ height: `${virtualizer.getTotalSize() + topPad}px` }}
|
||||
>
|
||||
{virtualizer.getVirtualItems().map((virtualRow) => (
|
||||
<div
|
||||
@@ -325,7 +328,7 @@ export const TimelineMessageList = React.memo(function TimelineMessageList({
|
||||
top: 0,
|
||||
left: 0,
|
||||
width: "100%",
|
||||
transform: `translateY(${virtualRow.start}px)`,
|
||||
transform: `translateY(${virtualRow.start + topPad}px)`,
|
||||
}}
|
||||
>
|
||||
{renderItem(items[virtualRow.index])}
|
||||
|
||||
@@ -31,9 +31,11 @@ const HIGHLIGHT_DURATION_MS = 2_000;
|
||||
*
|
||||
* - **Short-channel bottom-align pad.** A 2-3 message channel must sit at the
|
||||
* bottom of the viewport. The virtualizer lays rows from the top, so we add
|
||||
* a top pad of `max(0, viewportHeight - totalSize)`. It is recomputed off
|
||||
* the LIVE `getTotalSize()` on every measurement pass (via `onChange`) so
|
||||
* it collapses to 0 the instant content exceeds the viewport — see the
|
||||
* a top pad of `max(0, viewportHeight - totalSize - chrome)`, where `chrome`
|
||||
* is the non-row scroll content around the spacer (header/overlay padding
|
||||
* above, composer padding below). It is recomputed off the LIVE
|
||||
* `getTotalSize()` on every measurement pass (via `onChange`) so it
|
||||
* collapses to 0 the instant content exceeds the viewport — see the
|
||||
* ordering note below.
|
||||
* - **The "at bottom" / new-message-count UI state** the scroll-to-latest
|
||||
* pill reads. The library knows the geometry (`isAtEnd`); this hook lifts
|
||||
@@ -68,6 +70,12 @@ export type ChatScrollVirtualizerOptions = {
|
||||
|
||||
export type ChatScrollVirtualizer = {
|
||||
virtualizer: ChatVirtualizer;
|
||||
/**
|
||||
* Ref the surface attaches to the row spacer (the element whose `paddingTop`
|
||||
* carries `topPad`). The pad math measures the chrome around the spacer off
|
||||
* it — see `topPad`.
|
||||
*/
|
||||
spacerRef: React.RefObject<HTMLElement | null>;
|
||||
/**
|
||||
* Top padding (px) that bottom-aligns a channel whose content is shorter
|
||||
* than the viewport. Apply it to the row spacer's `paddingTop`. Always `0`
|
||||
@@ -115,6 +123,16 @@ export function useChatScrollVirtualizer({
|
||||
string | null
|
||||
>(null);
|
||||
|
||||
// The spacer carries `topPad` as its `paddingTop`, so non-row chrome that
|
||||
// also lives in the scroll content — the channel-header padding above the
|
||||
// spacer (`contentPadding`, reserving room for the pinned day overlay) and
|
||||
// the composer padding below it — must be subtracted from the pad, or a
|
||||
// short channel over-pads by exactly that chrome and becomes scrollable.
|
||||
// `scrollHeight - spacer.offsetHeight` is that chrome and is pad-invariant:
|
||||
// both grow by the same delta when the pad changes, so it converges in one
|
||||
// pass.
|
||||
const spacerRef = React.useRef<HTMLElement | null>(null);
|
||||
|
||||
// The bottom-state and pad recompute both read live geometry off the
|
||||
// virtualizer on each `onChange`. virtual-core fires `onChange` directly from
|
||||
// `resizeItem` on any size delta (not only on a visible-range change), which
|
||||
@@ -130,7 +148,14 @@ export function useChatScrollVirtualizer({
|
||||
if (!scrollEl) {
|
||||
return;
|
||||
}
|
||||
const pad = Math.max(0, scrollEl.clientHeight - instance.getTotalSize());
|
||||
const spacerEl = spacerRef.current;
|
||||
const chrome = spacerEl
|
||||
? Math.max(0, scrollEl.scrollHeight - spacerEl.offsetHeight)
|
||||
: 0;
|
||||
const pad = Math.max(
|
||||
0,
|
||||
scrollEl.clientHeight - instance.getTotalSize() - chrome,
|
||||
);
|
||||
setTopPad((prev) => (prev === pad ? prev : pad));
|
||||
|
||||
// The app's "at bottom" rule is looser than the library's 1px default;
|
||||
@@ -228,6 +253,7 @@ export function useChatScrollVirtualizer({
|
||||
|
||||
return {
|
||||
virtualizer,
|
||||
spacerRef,
|
||||
topPad,
|
||||
isAtBottom,
|
||||
newMessageCount,
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
import { expect, test } from "@playwright/test";
|
||||
|
||||
import { installMockBridge } from "../helpers/bridge";
|
||||
|
||||
// Scroll-anchoring guards for the virtualized main timeline (PR: virtualize
|
||||
// timeline). The channel-jump bug was the timeline yanking the viewport when
|
||||
// the message set changed. These assert geometry via getBoundingClientRect,
|
||||
// not scrollTop — the bug surfaced as the anchored row visibly jumping even
|
||||
// while scrollTop looked plausible, so position-on-screen is the real contract.
|
||||
|
||||
test.beforeEach(async ({ page }) => {
|
||||
await installMockBridge(page);
|
||||
});
|
||||
|
||||
// A short channel (#general seeds four backdated messages, far below a full
|
||||
// viewport) must bottom-align: the rows sit against the bottom of the scroll
|
||||
// area with a top pad above them, exactly like the legacy spacer. If topPad
|
||||
// were dropped, the rows would float at the top with empty space below.
|
||||
test("short channel bottom-aligns its messages against the viewport floor", async ({
|
||||
page,
|
||||
}) => {
|
||||
await page.goto("/");
|
||||
await page.getByTestId("channel-general").click();
|
||||
await expect(page.getByTestId("chat-title")).toHaveText("general");
|
||||
|
||||
const timeline = page.getByTestId("message-timeline");
|
||||
const rows = page.getByTestId("message-row");
|
||||
await expect(rows.first()).toBeVisible();
|
||||
|
||||
// Far fewer rows than fill the viewport — the precondition for bottom-align.
|
||||
const metrics = await timeline.evaluate((element) => ({
|
||||
clientHeight: element.clientHeight,
|
||||
scrollHeight: element.scrollHeight,
|
||||
}));
|
||||
expect(metrics.scrollHeight).toBeLessThanOrEqual(metrics.clientHeight + 1);
|
||||
|
||||
// The content bottom-aligns against the scroll container's content-box floor
|
||||
// (the inner edge above the composer-reserved bottom padding), and the first
|
||||
// row is pushed well below the top by the pad above it. "Last row" is the
|
||||
// lowest of any rendered row kind — #general's final seed is a system join
|
||||
// row, which sits below the last message-row, so anchoring on message-row
|
||||
// alone would misread the floor.
|
||||
const geometry = await timeline.evaluate((element) => {
|
||||
const style = getComputedStyle(element);
|
||||
const paddingBottom = Number.parseFloat(style.paddingBottom);
|
||||
const timelineRect = element.getBoundingClientRect();
|
||||
const contentRows = Array.from(
|
||||
element.querySelectorAll(
|
||||
'[data-testid="message-row"], [data-testid="system-message-row"]',
|
||||
),
|
||||
).map((row) => row.getBoundingClientRect());
|
||||
const firstTop = contentRows[0]?.top ?? Number.NaN;
|
||||
const lastBottom = Math.max(...contentRows.map((rect) => rect.bottom));
|
||||
return {
|
||||
timelineTop: timelineRect.top,
|
||||
contentFloor: timelineRect.bottom - paddingBottom,
|
||||
firstTop,
|
||||
lastBottom,
|
||||
};
|
||||
});
|
||||
|
||||
// Bottom-aligned: the last row ends within a small margin of the content
|
||||
// floor (one row gap of slack).
|
||||
expect(geometry.contentFloor - geometry.lastBottom).toBeLessThanOrEqual(24);
|
||||
// Padded from the top: the first row is pushed down, not floating at the top.
|
||||
expect(geometry.firstTop - geometry.timelineTop).toBeGreaterThan(96);
|
||||
});
|
||||
Reference in New Issue
Block a user