Skip to content

Commit b4e2ab7

Browse files
committed
fix(conversation): end unseen map jumps and keep the tick in view
A tick clicked mid-glide pinned its turn as current until the turn had been seen. If ConversationContent went away first, or the turn was removed, the pin was never released and a stale tick stayed current. The pin now ends with the view it was gliding, and with its item. Keeping the current tick in view ran only when the current turn changed, so a rail that shrank could clip it. A ResizeObserver on the rail now places it again at once. The tick's data-current attribute is dropped: nothing read it, and aria-current already carries the state.
1 parent 1254444 commit b4e2ab7

4 files changed

Lines changed: 122 additions & 26 deletions

File tree

‎e2e/conversation-map.spec.ts‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,22 @@ const wheel = async (page: Page, deltaY: number) => {
6363
await settled(page);
6464
};
6565

66+
/** Whether the rail overflows, has scrolled, and shows the current tick. */
67+
const railState = (page: Page) =>
68+
page.locator("[data-slot='conversation-map-rail']").evaluate((element) => {
69+
const tick = element.querySelector("[aria-current='location']");
70+
const view = element.getBoundingClientRect();
71+
const box = tick?.getBoundingClientRect();
72+
return {
73+
overflows: element.scrollHeight > element.clientHeight,
74+
scrolled: element.scrollTop > 0,
75+
showsCurrent:
76+
box !== undefined &&
77+
box.top >= view.top - 1 &&
78+
box.bottom <= view.bottom + 1,
79+
};
80+
});
81+
6682
const open = async (page: Page) => {
6783
await page.goto("/conversation-map");
6884
await expect(page.getByTestId("t19a")).toBeAttached();
@@ -150,24 +166,10 @@ test.describe("conversation map", () => {
150166
await page.goto("/conversation-map-tall-page");
151167
await expect(page.getByTestId("t79a")).toBeAttached();
152168
await expect(current(page)).toHaveAccessibleName("Question 79");
153-
const rail = page.locator("[data-slot='conversation-map-rail']");
154169
// The rail overflows, and has scrolled to bring the last tick into view.
155170
await expect
156-
.poll(() =>
157-
rail.evaluate((element) => {
158-
const tick = element.querySelector("[aria-current='location']");
159-
if (tick === null) return false;
160-
const view = element.getBoundingClientRect();
161-
const box = tick.getBoundingClientRect();
162-
return (
163-
element.scrollHeight > element.clientHeight &&
164-
element.scrollTop > 0 &&
165-
box.top >= view.top - 1 &&
166-
box.bottom <= view.bottom + 1
167-
);
168-
}),
169-
)
170-
.toBe(true);
171+
.poll(() => railState(page))
172+
.toEqual({ overflows: true, scrolled: true, showsCurrent: true });
171173
// Give any late scroll a few frames to land.
172174
await page.evaluate(
173175
() =>
@@ -184,6 +186,28 @@ test.describe("conversation map", () => {
184186
expect(await page.evaluate(() => window.scrollY)).toBe(0);
185187
});
186188

189+
test("a rail that shrinks keeps the current tick in view", async ({
190+
page,
191+
}) => {
192+
await open(page);
193+
await expect(current(page)).toHaveAccessibleName("Question 19");
194+
expect(await railState(page)).toEqual({
195+
overflows: false,
196+
scrolled: false,
197+
showsCurrent: true,
198+
});
199+
// The host's layout shrinks the map's box below the rail's height.
200+
await page
201+
.getByRole("navigation", { name: "Conversation map" })
202+
.evaluate((element) => {
203+
element.style.height = "100px";
204+
});
205+
await expect(current(page)).toHaveAccessibleName("Question 19");
206+
await expect
207+
.poll(() => railState(page))
208+
.toEqual({ overflows: true, scrolled: true, showsCurrent: true });
209+
});
210+
187211
test("moving along an overflowing rail keeps the preview open", async ({
188212
page,
189213
}) => {

‎src/hooks/use-reading-position.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,8 @@ export function useReadingPosition(
146146
observed.clear();
147147
idOf.clear();
148148
visible.clear();
149+
// A jump does not outlive the view it was gliding, seen or not.
150+
pinRef.current = null;
149151
};
150152

151153
/** Follows the scroll container (`null` while `ConversationContent` is
@@ -179,6 +181,11 @@ export function useReadingPosition(
179181
observer.observe(element);
180182
}
181183
}
184+
// A jumped-to turn removed before it was seen can never release.
185+
const pin = pinRef.current;
186+
if (pin !== null && items.get(pin.id) === undefined) {
187+
pinRef.current = null;
188+
}
182189
};
183190

184191
measureRef.current = measure;

‎src/ui/conversation-map.test.tsx‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,51 @@ describe("useReadingPosition", () => {
581581
expect(removed).toEqual(added);
582582
});
583583

584+
test("a jump not yet seen ends when ConversationContent goes away", async () => {
585+
let setShow!: (show: boolean) => void;
586+
function Host() {
587+
const [show, set] = useState(true);
588+
setShow = set;
589+
return <Chat showContent={show} />;
590+
}
591+
const mounted = render(<Host />);
592+
act(() => onlyObserver().report(["q4", "q4-reply"]));
593+
await nextFrame();
594+
act(() => tick(mounted.container, "First question").click());
595+
await nextFrame();
596+
expect(states(mounted.container)[0]).toBe("current");
597+
// Gone before the glide brought q1 on screen, then back showing q4.
598+
act(() => setShow(false));
599+
await nextFrame();
600+
act(() => setShow(true));
601+
await nextFrame();
602+
act(() => onlyObserver().report(["q4", "q4-reply"]));
603+
await nextFrame();
604+
expect(states(mounted.container)[0]).toBe("faint");
605+
expect(states(mounted.container)[3]).toBe("current");
606+
mounted.unmount();
607+
});
608+
609+
test("a jump not yet seen ends when its turn is removed", async () => {
610+
let setItems!: (items: readonly string[]) => void;
611+
function Host() {
612+
const [items, set] = useState<readonly string[]>(ITEMS);
613+
setItems = set;
614+
return <Chat items={items} />;
615+
}
616+
const mounted = render(<Host />);
617+
act(() => onlyObserver().report(["q4", "q4-reply"]));
618+
await nextFrame();
619+
act(() => tick(mounted.container, "First question").click());
620+
await nextFrame();
621+
expect(states(mounted.container)[0]).toBe("current");
622+
act(() => setItems(ITEMS.filter((id) => !id.startsWith("q1"))));
623+
await nextFrame();
624+
expect(states(mounted.container)[0]).toBe("faint");
625+
expect(states(mounted.container)[3]).toBe("current");
626+
mounted.unmount();
627+
});
628+
584629
test("a jumped turn stays current until seen, then until it leaves", async () => {
585630
const mounted = render(<Chat />);
586631
act(() => onlyObserver().report(["q4", "q4-reply"]));

‎src/ui/conversation-map.tsx‎

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,18 @@ function nearestTop(rail: HTMLElement, tick: HTMLElement): number | null {
5555
return null;
5656
}
5757

58+
/** Scrolls the rail alone to bring `tick` into view: scrollIntoView would
59+
* also scroll every ancestor, pulling the page to a conversation below the
60+
* fold. */
61+
function keepInView(
62+
rail: HTMLElement,
63+
tick: HTMLElement,
64+
behavior: ScrollBehavior,
65+
): void {
66+
const top = nearestTop(rail, tick);
67+
if (top !== null) rail.scrollTo({ top, behavior });
68+
}
69+
5870
/**
5971
* A rail of ticks, one per turn, for finding your way around a long
6072
* conversation. Ticks are faint, longer and darker for turns on screen, and
@@ -110,8 +122,6 @@ export function ConversationMap({
110122
card.style.top = `${top}px`;
111123
}, [preview]);
112124

113-
// Scrolls the rail alone: scrollIntoView would also scroll every ancestor,
114-
// pulling the page to a conversation below the fold.
115125
useEffect(() => {
116126
const rail = railRef.current;
117127
const tick = currentRef.current;
@@ -120,17 +130,28 @@ export function ConversationMap({
120130
return;
121131
}
122132
if (tick === null) return;
123-
const top = nearestTop(rail, tick);
124133
// The first placement is instant; later ones glide unless reduced.
125-
if (top !== null) {
126-
rail.scrollTo({
127-
top,
128-
behavior: placedRef.current && !reducedMotion ? "smooth" : "auto",
129-
});
130-
}
134+
keepInView(
135+
rail,
136+
tick,
137+
placedRef.current && !reducedMotion ? "smooth" : "auto",
138+
);
131139
placedRef.current = true;
132140
}, [currentId, hidden, reducedMotion]);
133141

142+
// A rail that shrinks (the host's layout, the window) can clip the current
143+
// tick: bring it back at once.
144+
useEffect(() => {
145+
const rail = railRef.current;
146+
if (rail === null) return;
147+
const observer = new ResizeObserver(() => {
148+
const tick = currentRef.current;
149+
if (tick !== null) keepInView(rail, tick, "auto");
150+
});
151+
observer.observe(rail);
152+
return () => observer.disconnect();
153+
}, [hidden]);
154+
134155
if (hidden) return null;
135156

136157
const visible = new Set(visibleIds);
@@ -247,7 +268,6 @@ export function ConversationMap({
247268
data-slot="conversation-map-tick"
248269
data-turn-id={turn.id}
249270
data-visible={onScreen ? "" : undefined}
250-
data-current={current ? "" : undefined}
251271
aria-label={turn.label}
252272
aria-current={current ? "location" : undefined}
253273
aria-describedby={described ? previewTextId : undefined}

0 commit comments

Comments
 (0)