From 484bdc2e3ba0c1875f0d074c5dd4d90004fdd4fd Mon Sep 17 00:00:00 2001 From: Kun Date: Thu, 8 Oct 2026 23:44:49 +0800 Subject: [PATCH 1/4] fix(ui): stop the collapsible content flashing on collapse `tw-animate-css`'s `animate-out` runs with `animation-fill-mode: none`, so the moment the exit animation ends the content snaps back to its resting style (full opacity, no slide). The unmount only landed in a later task, after that frame had painted, so every collapse showed one fully visible frame of the content that was just folded away. Hold the exit's last keyframe (`animation-fill-mode: forwards`) until the node is removed, and remove it synchronously from the `animationend` handler so it is gone before the next paint. A reopen during the exit restores the node's own fill mode. Co-Authored-By: Claude Sonnet 5.5 --- src/components/ui/instant-collapsible.tsx | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/components/ui/instant-collapsible.tsx b/src/components/ui/instant-collapsible.tsx index ee9b158f18..fb4c20ac2f 100644 --- a/src/components/ui/instant-collapsible.tsx +++ b/src/components/ui/instant-collapsible.tsx @@ -11,6 +11,7 @@ import { useState, type ComponentProps, } from "react" +import { flushSync } from "react-dom" import { useControllableState } from "@radix-ui/react-use-controllable-state" // Drop-in replacement for ui/collapsible (Radix) used on the message hot @@ -161,11 +162,20 @@ function CollapsibleContent({ children, ...props }: ComponentProps<"div">) { setPresent(false) return } + // tw-animate-css's `animate-out` runs with `animation-fill-mode: none`, so + // the moment the exit finishes the content snaps back to its resting + // style (opacity 1, no slide) — and a plain setPresent lands in a later + // task, after that frame paints. That one fully visible frame is the flash + // on every collapse. Hold the exit's last keyframe until removal, and + // remove synchronously in the animationend handler, which runs before + // the frame paints (the same pair Radix Presence uses). + const prevFillMode = node.style.animationFillMode + node.style.animationFillMode = "forwards" let done = false const finish = () => { if (done) return done = true - setPresent(false) + flushSync(() => setPresent(false)) } const onAnimationDone = (event: AnimationEvent) => { // Child animations bubble; only the content's own exit counts. @@ -178,6 +188,9 @@ function CollapsibleContent({ children, ...props }: ComponentProps<"div">) { node.removeEventListener("animationend", onAnimationDone) node.removeEventListener("animationcancel", onAnimationDone) window.clearTimeout(timeout) + // Reopened mid-exit: the node stays mounted, so hand back its own + // fill mode for the enter animation. + node.style.animationFillMode = prevFillMode } }, [open]) From 7984e25b53e275ff67002bbb1fa6a3d390bda815 Mon Sep 17 00:00:00 2001 From: Kun Date: Thu, 8 Oct 2026 23:44:49 +0800 Subject: [PATCH 2/4] fix(chat): keep the transcript focus ring hidden after a click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking the transcript marks the scroll viewport as pointer-focused (`data-focus-origin="pointer"`) so the `focus-visible` ring stays off. Two things dropped that marker while the viewport was still focused, and the ring then appeared around the whole transcript: - the `keydown` listener, so any key — Esc included — brought the ring back (a key press makes the browser match `:focus-visible`); - the `blur` listener, which also fires when the whole window loses focus. The viewport stays the document's active element and the browser re-focuses it when the window returns (e.g. on a resize), now without the marker. Drop the keydown listener, and on blur wait for it to settle and only clear the marker when the viewport is really no longer the active element. Tabbing into the viewport still shows the ring: that focus never sets the marker. Co-Authored-By: Claude Sonnet 5.5 --- .../virtualized-message-thread.test.tsx | 66 ++++++++++++++----- .../message/virtualized-message-thread.tsx | 30 ++++++--- 2 files changed, 69 insertions(+), 27 deletions(-) diff --git a/src/components/message/virtualized-message-thread.test.tsx b/src/components/message/virtualized-message-thread.test.tsx index 82a53c6232..dfc7d55b32 100644 --- a/src/components/message/virtualized-message-thread.test.tsx +++ b/src/components/message/virtualized-message-thread.test.tsx @@ -61,34 +61,64 @@ beforeEach(() => { }) describe("VirtualizedMessageThread focus origin", () => { - it("marks pointer-origin focus and clears it on blur", () => { - renderThread() - const viewport = screen.getByTestId("viewport") - - pointerDown(screen.getByTestId("content"), 0) - - expect(document.activeElement).toBe(viewport) - expect(viewport).toHaveAttribute("data-focus-origin", "pointer") - expect(viewport.className).toContain( - "data-[focus-origin=pointer]:focus-visible:ring-0" - ) + it("marks pointer-origin focus and clears it once focus moves away", () => { + vi.useFakeTimers() + try { + renderThread() + const viewport = screen.getByTestId("viewport") + const other = document.createElement("button") + document.body.appendChild(other) + + pointerDown(screen.getByTestId("content"), 0) + + expect(document.activeElement).toBe(viewport) + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + expect(viewport.className).toContain( + "data-[focus-origin=pointer]:focus-visible:ring-0" + ) + + other.focus() + vi.runAllTimers() + expect(viewport).not.toHaveAttribute("data-focus-origin") + other.remove() + } finally { + vi.useRealTimers() + } + }) - fireEvent.blur(viewport) - expect(viewport).not.toHaveAttribute("data-focus-origin") + it("keeps the pointer marker when only the window loses focus", () => { + vi.useFakeTimers() + try { + renderThread() + const viewport = screen.getByTestId("viewport") + + pointerDown(screen.getByTestId("content"), 0) + + // Switching apps blurs the viewport but leaves it the active element; + // the browser re-focuses it when the window comes back (e.g. on a + // resize), and without the marker the ring would show then. + fireEvent.blur(viewport) + vi.runAllTimers() + expect(document.activeElement).toBe(viewport) + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + } finally { + vi.useRealTimers() + } }) - it("clears the pointer marker on keyboard input so the ring returns", () => { + it("keeps the pointer marker through keyboard input so no ring appears", () => { renderThread() const viewport = screen.getByTestId("viewport") pointerDown(screen.getByTestId("content"), 0) expect(viewport).toHaveAttribute("data-focus-origin", "pointer") - // Switching to keyboard scrolling drops the marker, so the suppressing - // `data-[focus-origin=pointer]` selector no longer matches and the - // keyboard focus ring becomes visible again. + // Any key press makes the browser match :focus-visible; the marker must + // survive it, or Esc (or keyboard scrolling) after a click would ring the + // whole transcript. + keyDown(viewport, "Escape") keyDown(viewport, "ArrowDown") - expect(viewport).not.toHaveAttribute("data-focus-origin") + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") expect(document.activeElement).toBe(viewport) }) diff --git a/src/components/message/virtualized-message-thread.tsx b/src/components/message/virtualized-message-thread.tsx index 4f937a02a6..f828cfd7b7 100644 --- a/src/components/message/virtualized-message-thread.tsx +++ b/src/components/message/virtualized-message-thread.tsx @@ -229,7 +229,9 @@ function VirtualizedMessageThreadImpl({ const el = scrollRef.current if (!el) return el.tabIndex = 0 + let blurCheck: number | undefined const clearPointerFocus = () => { + window.clearTimeout(blurCheck) el.removeAttribute("data-focus-origin") } const onPointerDown = (e: PointerEvent) => { @@ -249,18 +251,28 @@ function VirtualizedMessageThreadImpl({ el.setAttribute("data-focus-origin", "pointer") el.focus({ preventScroll: true }) } + // The pointer-origin marker holds until focus really leaves the viewport, + // so a clicked transcript never grows the ring — not even once a key is + // pressed (pressing any key makes the browser match :focus-visible, so + // clearing the marker on keydown put a ring around the whole transcript on + // a mere Esc). Tabbing in still shows the ring: that focus never sets it. + // + // A blur alone isn't "really leaves": the whole window losing focus (switch + // apps, then resize the window to come back) blurs the viewport too, yet it + // stays the document's active element and the browser re-focuses it on the + // way back — without the marker, the ring appeared on return. So look after + // the blur settles: only a viewport that is no longer active drops it. + const onBlur = () => { + window.clearTimeout(blurCheck) + blurCheck = window.setTimeout(() => { + if (document.activeElement !== el) clearPointerFocus() + }) + } el.addEventListener("pointerdown", onPointerDown) - el.addEventListener("blur", clearPointerFocus) - // Once the user drives the viewport with the keyboard (Arrow/Page/Home/End - // to scroll), drop the pointer-origin marker so the focus ring reappears — - // keeping the keyboard focus indicator visible per WCAG 2.4.7. The ring is - // only suppressed for the mouse click that focused the viewport, not for - // subsequent keyboard use. - el.addEventListener("keydown", clearPointerFocus) + el.addEventListener("blur", onBlur) return () => { el.removeEventListener("pointerdown", onPointerDown) - el.removeEventListener("blur", clearPointerFocus) - el.removeEventListener("keydown", clearPointerFocus) + el.removeEventListener("blur", onBlur) clearPointerFocus() } }, [scrollRef]) From e5dd75d8a6d494094cad44dea7b4209957a2ddff Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 9 Oct 2026 15:57:20 +0800 Subject: [PATCH 3/4] fix(ui): keep the collapsible's onAnimationEnd firing on collapse The collapse flash fix unmounted the content with flushSync from its native animationend listener. That removed the node before React dispatched the same event, so a CollapsibleContent onAnimationEnd never saw the exit animation end. Holding the exit's last keyframe (animation-fill-mode: forwards) stops the flash on its own: the frame after animationend paints the held keyframe, and the plain state update unmounts the node in a later task. Radix Presence relies on the same hold. A cancelled exit has no keyframe left to hold, so that path still unmounts synchronously; React has no onAnimationCancel to lose. Cover the fill-mode hold, its restore when the content reopens mid-exit, the onAnimationEnd delivery, and the synchronous unmount on a cancelled exit. --- .../ui/instant-collapsible.test.tsx | 116 ++++++++++++++++++ src/components/ui/instant-collapsible.tsx | 22 ++-- 2 files changed, 130 insertions(+), 8 deletions(-) diff --git a/src/components/ui/instant-collapsible.test.tsx b/src/components/ui/instant-collapsible.test.tsx index 7021b8a701..431247c737 100644 --- a/src/components/ui/instant-collapsible.test.tsx +++ b/src/components/ui/instant-collapsible.test.tsx @@ -184,6 +184,122 @@ describe("InstantCollapsible", () => { expect(screen.getByTestId("body")).toBeInTheDocument() }) + it("holds the exit's last keyframe until the content unmounts", () => { + mockExitAnimation() + + render( + + toggle + +
+ + + ) + + const content = screen.getByTestId("body").parentElement as HTMLElement + expect(content.style.animationFillMode).toBe("") + + // `animate-out` ends with fill-mode none, which would snap the content + // back to full opacity for a frame before it unmounts. + fireEvent.click(screen.getByRole("button", { name: "toggle" })) + expect(content.style.animationFillMode).toBe("forwards") + + // The unmount lands after the event, so the hold must still be in place + // when the event has gone all the way through. + const seen: Array<{ connected: boolean; fillMode: string }> = [] + const record = () => + seen.push({ + connected: content.isConnected, + fillMode: content.style.animationFillMode, + }) + document.addEventListener("animationend", record) + try { + fireEvent.animationEnd(content) + } finally { + document.removeEventListener("animationend", record) + } + expect(seen).toEqual([{ connected: true, fillMode: "forwards" }]) + expect(screen.queryByTestId("body")).not.toBeInTheDocument() + }) + + it("unmounts at once when the exit animation is cancelled", () => { + mockExitAnimation() + + render( + + toggle + +
+ + + ) + + fireEvent.click(screen.getByRole("button", { name: "toggle" })) + const content = screen.getByTestId("body").parentElement as HTMLElement + + // A cancelled animation leaves no keyframe to hold, so the content must + // be gone before the event finishes dispatching, not a task later. + const seen: boolean[] = [] + const record = () => seen.push(content.isConnected) + document.addEventListener("animationcancel", record) + try { + fireEvent(content, new Event("animationcancel", { bubbles: true })) + } finally { + document.removeEventListener("animationcancel", record) + } + expect(seen).toEqual([false]) + expect(screen.queryByTestId("body")).not.toBeInTheDocument() + }) + + it("hands the content its own fill mode back when reopened mid-exit", () => { + mockExitAnimation() + + render( + + toggle + +
+ + + ) + + const trigger = screen.getByRole("button", { name: "toggle" }) + const content = screen.getByTestId("body").parentElement as HTMLElement + + fireEvent.click(trigger) + expect(content.style.animationFillMode).toBe("forwards") + + // React leaves the unchanged style prop alone, so the enter animation + // only gets "backwards" back because the exit restores it. + fireEvent.click(trigger) + expect(content).toHaveAttribute("data-state", "open") + expect(content.style.animationFillMode).toBe("backwards") + }) + + it("still delivers the exit's animationend to the content's onAnimationEnd", () => { + mockExitAnimation() + const onAnimationEnd = vi.fn() + + render( + + toggle + +
+ + + ) + + fireEvent.click(screen.getByRole("button", { name: "toggle" })) + const content = screen.getByTestId("body").parentElement as HTMLElement + + // The unmount must not land inside the native listener: React dispatches + // the same event to onAnimationEnd afterwards, from the root, and finds + // nothing there once the node is gone. + fireEvent.animationEnd(content) + expect(onAnimationEnd).toHaveBeenCalledTimes(1) + expect(screen.queryByTestId("body")).not.toBeInTheDocument() + }) + it("ignores bubbling child animation ends while exiting", () => { mockExitAnimation() diff --git a/src/components/ui/instant-collapsible.tsx b/src/components/ui/instant-collapsible.tsx index fb4c20ac2f..357805e722 100644 --- a/src/components/ui/instant-collapsible.tsx +++ b/src/components/ui/instant-collapsible.tsx @@ -162,24 +162,30 @@ function CollapsibleContent({ children, ...props }: ComponentProps<"div">) { setPresent(false) return } - // tw-animate-css's `animate-out` runs with `animation-fill-mode: none`, so - // the moment the exit finishes the content snaps back to its resting - // style (opacity 1, no slide) — and a plain setPresent lands in a later + // tw-animate-css's `animate-out` defaults to `animation-fill-mode: none`, + // so the moment the exit finishes the content snaps back to its resting + // style (opacity 1, no slide) — and the unmount below lands in a later // task, after that frame paints. That one fully visible frame is the flash - // on every collapse. Hold the exit's last keyframe until removal, and - // remove synchronously in the animationend handler, which runs before - // the frame paints (the same pair Radix Presence uses). + // on collapse. Hold the exit's last keyframe until removal instead, as + // Radix Presence does. Don't unmount synchronously from the animationend + // listener (flushSync): the node would be gone before React dispatches + // the same event, so the content's own onAnimationEnd would never fire. const prevFillMode = node.style.animationFillMode node.style.animationFillMode = "forwards" let done = false const finish = () => { if (done) return done = true - flushSync(() => setPresent(false)) + setPresent(false) } const onAnimationDone = (event: AnimationEvent) => { // Child animations bubble; only the content's own exit counts. - if (event.target === node) finish() + if (event.target !== node) return + // A cancelled exit leaves no keyframe for `forwards` to hold, so the + // content is already back at its resting style: unmount before that + // paints. React has no onAnimationCancel for this to starve. + if (event.type === "animationcancel") flushSync(finish) + else finish() } node.addEventListener("animationend", onAnimationDone) node.addEventListener("animationcancel", onAnimationDone) From 1a9cf42d4d6732e815bea699b285ac5a2977284c Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 9 Oct 2026 15:41:27 +0800 Subject: [PATCH 4/4] fix(chat): bring the transcript focus ring back for keyboard scrolling After a click on the transcript, scrolling it from the keyboard (arrow keys, Page Up/Down, Home/End, Space, with any modifier) drops the pointer-origin marker again, so the focus ring returns for keyboard use as WCAG 2.4.7 asks. Other keys keep the marker: Esc, or Ctrl/Cmd+C to copy a selection, no longer ring the whole transcript. Also re-check the marker when the window regains focus. Focus moved by a script while the window was in the background never blurs the viewport a second time, so the marker went stale and Tabbing back into the transcript later showed no ring. --- .../virtualized-message-thread.test.tsx | 98 ++++++++++++++++--- .../message/virtualized-message-thread.tsx | 52 ++++++++-- 2 files changed, 126 insertions(+), 24 deletions(-) diff --git a/src/components/message/virtualized-message-thread.test.tsx b/src/components/message/virtualized-message-thread.test.tsx index dfc7d55b32..1a0a27c3d5 100644 --- a/src/components/message/virtualized-message-thread.test.tsx +++ b/src/components/message/virtualized-message-thread.test.tsx @@ -52,8 +52,12 @@ function pointerDown(element: HTMLElement, button: number) { fireEvent(element, new MouseEvent("pointerdown", { bubbles: true, button })) } -function keyDown(element: HTMLElement, key: string) { - fireEvent.keyDown(element, { key }) +function keyDown( + element: HTMLElement, + key: string, + modifiers: Omit = {} +) { + fireEvent.keyDown(element, { key, ...modifiers }) } beforeEach(() => { @@ -101,27 +105,93 @@ describe("VirtualizedMessageThread focus origin", () => { vi.runAllTimers() expect(document.activeElement).toBe(viewport) expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + + fireEvent.focus(window) + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") } finally { vi.useRealTimers() } }) - it("keeps the pointer marker through keyboard input so no ring appears", () => { - renderThread() - const viewport = screen.getByTestId("viewport") + it("drops the pointer marker when focus moved on while the window was away", () => { + vi.useFakeTimers() + const other = document.createElement("button") + document.body.appendChild(other) + try { + renderThread() + const viewport = screen.getByTestId("viewport") - pointerDown(screen.getByTestId("content"), 0) - expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + pointerDown(screen.getByTestId("content"), 0) + // The window loses focus: the viewport blurs but stays active. + fireEvent.blur(viewport) + vi.runAllTimers() + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") - // Any key press makes the browser match :focus-visible; the marker must - // survive it, or Esc (or keyboard scrolling) after a click would ring the - // whole transcript. - keyDown(viewport, "Escape") - keyDown(viewport, "ArrowDown") - expect(viewport).toHaveAttribute("data-focus-origin", "pointer") - expect(document.activeElement).toBe(viewport) + // A script focuses another control while the window is in the + // background: the active element changes, but the viewport sees no + // second blur (its blur went out with the window's). Only the window + // coming back can notice. + const activeElement = vi + .spyOn(document, "activeElement", "get") + .mockReturnValue(other) + try { + fireEvent.focus(window) + } finally { + activeElement.mockRestore() + } + expect(viewport).not.toHaveAttribute("data-focus-origin") + } finally { + other.remove() + vi.useRealTimers() + } }) + it.each([ + ["ArrowDown", "ArrowDown", {}], + ["ArrowUp", "ArrowUp", {}], + ["PageDown", "PageDown", {}], + ["PageUp", "PageUp", {}], + ["Space", " ", {}], + ["Shift+Space", " ", { shiftKey: true }], + ["Home", "Home", {}], + ["Ctrl+End", "End", { ctrlKey: true }], + ["Cmd+ArrowDown", "ArrowDown", { metaKey: true }], + ])( + "clears the pointer marker on %s, a scroll key, so the ring can return", + (_label, key, modifiers) => { + renderThread() + const viewport = screen.getByTestId("viewport") + + pointerDown(screen.getByTestId("content"), 0) + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + + keyDown(viewport, key, modifiers) + expect(viewport).not.toHaveAttribute("data-focus-origin") + expect(document.activeElement).toBe(viewport) + } + ) + + it.each([ + ["Escape", "Escape", {}], + ["Ctrl+C", "c", { ctrlKey: true }], + ["Cmd+C", "c", { metaKey: true }], + ["Shift", "Shift", { shiftKey: true }], + ])( + "keeps the pointer marker through %s, which doesn't scroll", + (_label, key, modifiers) => { + renderThread() + const viewport = screen.getByTestId("viewport") + + pointerDown(screen.getByTestId("content"), 0) + + // In Chromium the click's script focus already matches :focus-visible, + // so only the marker keeps the whole transcript from being ringed here. + keyDown(viewport, key, modifiers) + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + expect(document.activeElement).toBe(viewport) + } + ) + it("keeps keyboard-origin focus distinguishable", () => { renderThread() const viewport = screen.getByTestId("viewport") diff --git a/src/components/message/virtualized-message-thread.tsx b/src/components/message/virtualized-message-thread.tsx index f828cfd7b7..094979131c 100644 --- a/src/components/message/virtualized-message-thread.tsx +++ b/src/components/message/virtualized-message-thread.tsx @@ -24,6 +24,22 @@ import { */ const LOAD_OLDER_THRESHOLD_PX = 240 +/** + * Keys the browser scrolls a focused viewport with. Modifiers don't matter: + * Cmd+ArrowDown and Ctrl+End jump to the end, Shift+Space pages back up. + */ +const SCROLL_KEYS = new Set([ + "ArrowUp", + "ArrowDown", + "ArrowLeft", + "ArrowRight", + "PageUp", + "PageDown", + "Home", + "End", + " ", +]) + interface VirtualizedMessageThreadProps { /** Data to virtualise — each entry becomes one virtual row. */ items: T[] @@ -251,16 +267,21 @@ function VirtualizedMessageThreadImpl({ el.setAttribute("data-focus-origin", "pointer") el.focus({ preventScroll: true }) } - // The pointer-origin marker holds until focus really leaves the viewport, - // so a clicked transcript never grows the ring — not even once a key is - // pressed (pressing any key makes the browser match :focus-visible, so - // clearing the marker on keydown put a ring around the whole transcript on - // a mere Esc). Tabbing in still shows the ring: that focus never sets it. - // - // A blur alone isn't "really leaves": the whole window losing focus (switch - // apps, then resize the window to come back) blurs the viewport too, yet it - // stays the document's active element and the browser re-focuses it on the - // way back — without the marker, the ring appeared on return. So look after + // The pointer-origin marker only hides the ring for the click that + // focused the viewport. Once the user scrolls it with the keyboard, drop + // the marker so the ring reappears — keeping the keyboard focus indicator + // visible per WCAG 2.4.7. Other keys keep it: Chromium already matches + // :focus-visible after that script focus, so clearing on any key ringed + // the whole transcript on a mere Esc, or on Ctrl/Cmd+C to copy a + // selection. Tabbing in still shows the ring: that focus never sets it. + const onKeyDown = (e: KeyboardEvent) => { + if (SCROLL_KEYS.has(e.key)) clearPointerFocus() + } + // Otherwise the marker holds until focus really leaves the viewport. A + // blur alone isn't that: the whole window losing focus (switch apps, then + // resize the window to come back) blurs the viewport too, yet it stays the + // document's active element and the browser re-focuses it on the way + // back — without the marker, the ring appeared on return. So look after // the blur settles: only a viewport that is no longer active drops it. const onBlur = () => { window.clearTimeout(blurCheck) @@ -268,11 +289,22 @@ function VirtualizedMessageThreadImpl({ if (document.activeElement !== el) clearPointerFocus() }) } + // Focus can also move on while the window is away (a script focuses + // another control). The viewport gets no second blur for that — its blur + // already went out with the window's — so look again when the window + // comes back, or Tabbing back in later would bring no ring. + const onWindowFocus = () => { + if (document.activeElement !== el) clearPointerFocus() + } el.addEventListener("pointerdown", onPointerDown) + el.addEventListener("keydown", onKeyDown) el.addEventListener("blur", onBlur) + window.addEventListener("focus", onWindowFocus) return () => { el.removeEventListener("pointerdown", onPointerDown) + el.removeEventListener("keydown", onKeyDown) el.removeEventListener("blur", onBlur) + window.removeEventListener("focus", onWindowFocus) clearPointerFocus() } }, [scrollRef])