diff --git a/src/components/message/virtualized-message-thread.test.tsx b/src/components/message/virtualized-message-thread.test.tsx index 82a53c6232..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(() => { @@ -61,36 +65,132 @@ 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") + + fireEvent.focus(window) + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + } finally { + vi.useRealTimers() + } }) - it("clears the pointer marker on keyboard input so the ring returns", () => { - 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) + // The window loses focus: the viewport blurs but stays active. + fireEvent.blur(viewport) + vi.runAllTimers() + expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + + // 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() + } + }) - pointerDown(screen.getByTestId("content"), 0) - expect(viewport).toHaveAttribute("data-focus-origin", "pointer") + 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) + } + ) - // 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. - keyDown(viewport, "ArrowDown") - 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() diff --git a/src/components/message/virtualized-message-thread.tsx b/src/components/message/virtualized-message-thread.tsx index 4f937a02a6..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[] @@ -229,7 +245,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 +267,44 @@ function VirtualizedMessageThreadImpl({ el.setAttribute("data-focus-origin", "pointer") el.focus({ preventScroll: true }) } + // 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) + blurCheck = window.setTimeout(() => { + 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("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("keydown", onKeyDown) + el.addEventListener("blur", onBlur) + window.addEventListener("focus", onWindowFocus) return () => { el.removeEventListener("pointerdown", onPointerDown) - el.removeEventListener("blur", clearPointerFocus) - el.removeEventListener("keydown", clearPointerFocus) + el.removeEventListener("keydown", onKeyDown) + el.removeEventListener("blur", onBlur) + window.removeEventListener("focus", onWindowFocus) clearPointerFocus() } }, [scrollRef]) 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 ee9b158f18..357805e722 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,6 +162,16 @@ function CollapsibleContent({ children, ...props }: ComponentProps<"div">) { setPresent(false) return } + // 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 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 @@ -169,7 +180,12 @@ function CollapsibleContent({ children, ...props }: ComponentProps<"div">) { } 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) @@ -178,6 +194,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])