From dd4a8ccf43689c7de92619f81c0a188bf37defc3 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Sat, 8 Aug 2026 21:20:02 +0800 Subject: [PATCH] fix(components): make spinners rotate in place instead of orbiting [risk:low] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mobile home status pill's "刷新中…" spinner did not spin about its own center; it swept a circle around a point beside it. Measured in WebKit rather than inferred. Two mechanisms reproduce it, both rooted in `transform-box` defaulting to `view-box`, so `animate-spin`'s `transform-origin: 50% 50%` is only the box center via a spec fallback that nothing in the markup asserts: - The pill is a capped-width flex row whose label truncates. Without `shrink-0` a long label compressed the glyph from 14x14 to 13.55x15.43, so the rotation swept an ellipse and the glyph wobbled. - Two sites animate an HTML wrapper instead of the glyph to keep an always-on spinner on the compositor. That is only equivalent while the wrapper's box is the glyph's box; a wrapper that also held its "Syncing" label orbited with 64px of travel. Not the cause, and verified as such before fixing: the Loader2 artwork is exactly centered in its viewBox, and CSS rotation of a root is pixel-identical to SVG-native rotation about (12,12). Fixes the pivot globally in `@layer base` (below utilities, so an explicit `origin-*` / `shrink` utility at a call site still wins) and adds `shrink-0` to the three pill spinner states, since the CSS rule corrects the pivot but cannot restore a squished box. Audited all 233 `animate-spin` sites: 231 are on the glyph and are covered by the new rule; the 2 wrapper sites are documented and constrained by test. Tests assert the structural invariants that produce the geometry, since jsdom has no layout. Verified to fail without the fix. Model: claude-opus-5[1m] Co-Authored-By: Claude Opus 5 (1M context) --- .../mobile/mobile-connection-status.tsx | 15 ++- packages/components/src/tailwind/index.css | 50 +++++++ .../tests/spinner-rotates-in-place.test.tsx | 124 ++++++++++++++++++ 3 files changed, 186 insertions(+), 3 deletions(-) create mode 100644 packages/components/tests/spinner-rotates-in-place.test.tsx diff --git a/packages/components/src/components/mobile/mobile-connection-status.tsx b/packages/components/src/components/mobile/mobile-connection-status.tsx index 176689f019..37ae707d7b 100644 --- a/packages/components/src/components/mobile/mobile-connection-status.tsx +++ b/packages/components/src/components/mobile/mobile-connection-status.tsx @@ -34,6 +34,15 @@ import type { LodyConnectionUiState } from '@/atoms/control-connection'; * - The whole pill springs in / out on appear / disappear; while it stays * mounted, its inner content cross-fades between states so e.g. the * loader → ✓ swap reads as a smooth transition rather than a jump. + * + * Every leading glyph carries `shrink-0`. The pill is a capped-width flex + * row whose label truncates, so without it a long label compresses the + * spinner to a non-square box (measured 14×14 → 13.55×15.43 on the + * reconnecting label) and `animate-spin` then sweeps an ellipse — the + * glyph visibly wobbles instead of turning in place. `index.css` also + * pins `transform-box`/`transform-origin` globally for spinners; both + * are needed, since that rule fixes the pivot but cannot restore a + * squished box. */ export type MobileConnectionStatusLabels = { @@ -114,7 +123,7 @@ function contentFor( both at once would be visual noise. */ return { key: 'refreshing', - icon: , + icon: , label: labels.refreshing ?? '刷新中…', textColor: 'text-muted-foreground', }; @@ -130,7 +139,7 @@ function contentFor( if (state === 'reconnecting') { return { key: 'reconnecting', - icon: , + icon: , label: labels.reconnecting ?? '正在重连…', textColor: 'text-muted-foreground', }; @@ -138,7 +147,7 @@ function contentFor( if (state === 'loading') { return { key: 'loading', - icon: , + icon: , label: labels.loading ?? '连接中…', textColor: 'text-muted-foreground', }; diff --git a/packages/components/src/tailwind/index.css b/packages/components/src/tailwind/index.css index 4fbb8e15ef..72ee9f62f1 100644 --- a/packages/components/src/tailwind/index.css +++ b/packages/components/src/tailwind/index.css @@ -1791,6 +1791,56 @@ pre, } } +/* + ---break--- + */ + +/* Spinners must rotate about their OWN center, not orbit a point. + `animate-spin` is `rotate(360deg)` with the default + `transform-origin: 50% 50%`, and `transform-box`'s initial value is + `view-box` — for an SVG that percentage resolves against the nearest + SVG viewport rather than the element's CSS box. A root `` has a + CSS layout box, so `view-box` is specified to act as `border-box` + there and modern engines land on the box center. But nothing in the + markup states that intent, so any element that loses its CSS layout + box, is sized asymmetrically, or is rotated by an engine resolving + the percentage against the 24-unit `viewBox` pivots off-center — the + glyph then sweeps a circle around a point beside it (measured: a + 14px icon pivoting on the viewBox center orbits with a ~14px + diameter, i.e. wider than the icon itself) instead of turning in + place. Pinning both properties states the intent once, globally. + + `flex-shrink: 0` belongs here for the same reason: in a tight flex + row (the mobile home status pill, whose label truncates) a spinner + without it is compressed to a non-square box, so the rotation + sweeps an ellipse and the glyph visibly wobbles. Measured 14×14 → + 13.55×15.43 in the reconnecting pill before this rule. + + `@layer base` keeps this below utilities, so an explicit + `origin-*` / `shrink` utility at a call site still wins. */ +@layer base { + svg.animate-spin { + transform-box: border-box; + transform-origin: 50% 50%; + flex-shrink: 0; + } + + /* Two sites animate an HTML wrapper instead of the glyph, to keep an + always-on spinner on the compositor (see `sidebar-row-shared.tsx` + and `session-syncing-indicator.tsx`). A wrapper is only equivalent + while its box matches the glyph's: the moment anything else lands + inside it (a label, a gap) its box grows, its center moves off the + glyph, and the glyph orbits — measured 64px of travel for a + wrapper that also contained its "Syncing" label. These wrappers + must therefore stay icon-only and explicitly sized; centering the + origin here makes that failure mode a wobble-free no-op rather + than an orbit. */ + span.animate-spin { + transform-box: border-box; + transform-origin: 50% 50%; + } +} + /* ---break--- */ diff --git a/packages/components/tests/spinner-rotates-in-place.test.tsx b/packages/components/tests/spinner-rotates-in-place.test.tsx new file mode 100644 index 0000000000..9a11aa7d6d --- /dev/null +++ b/packages/components/tests/spinner-rotates-in-place.test.tsx @@ -0,0 +1,124 @@ +// @vitest-environment jsdom + +/** + * A spinner must rotate about its own center. `animate-spin` is + * `rotate(360deg)` about `transform-origin: 50% 50%`, so the glyph turns in + * place only while the animated element's box is square AND centered on the + * glyph. Two ways that breaks, both of which shipped as visible bugs: + * + * 1. The animated element is an HTML *wrapper* that contains more than the + * glyph (a label, a gap). Its box grows, its center moves off the glyph, + * and the glyph orbits that offset center — a wrapper that also held its + * "Syncing" label measured 64px of travel in WebKit. + * 2. The glyph sits in a tight flex row with a truncating label and no + * `shrink-0`, so it is compressed to a non-square box and the rotation + * sweeps an ellipse (measured 14×14 → 13.55×15.43). + * + * jsdom has no layout, so these assert the *structural* invariants that make + * the geometry correct rather than re-measuring pixels: an animated wrapper + * is icon-only and explicitly sized, and a spinner in a flex row cannot be + * squished. The `transform-box`/`transform-origin` half of the fix lives in + * `src/tailwind/index.css` and is not observable here. + */ + +import React from 'react'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { flushSync } from 'react-dom'; +import { createRoot, type Root } from 'react-dom/client'; +import { SessionRowLeadingSlot } from '../src/components/sidebar-row-shared'; +import { SessionSyncingIndicator } from '../src/components/sessions/session-syncing-indicator'; +import { MobileConnectionStatus } from '../src/components/mobile/mobile-connection-status'; +import { initI18n } from '../src/i18n'; + +/** A sizing utility that pins BOTH axes, e.g. `h-3 w-3` / `size-4`. */ +function hasExplicitSquareSize(el: Element): boolean { + const classes = [...el.classList]; + const has = (prefix: string) => + classes.some((c) => new RegExp(String.raw`^-?${prefix}-\[?[\d./]`).test(c)); + return (has('h') && has('w')) || has('size'); +} + +let container: HTMLDivElement; +let root: Root | undefined; + +beforeEach(async () => { + await initI18n(); + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); +}); + +afterEach(() => { + flushSync(() => root?.unmount()); + root = undefined; + container.remove(); +}); + +function render(node: React.ReactElement) { + flushSync(() => root?.render(node)); +} + +describe('spinners rotate in place', () => { + /* The compositor-friendly pattern: animate an HTML wrapper, not the SVG. + Only equivalent while the wrapper's box IS the glyph's box. */ + const wrapperCases: ReadonlyArray React.ReactElement]> = [ + [ + 'session row working indicator', + () => + React.createElement(SessionRowLeadingSlot, { isWorking: true, menuLabel: 'More actions' }), + ], + ['session syncing indicator', () => React.createElement(SessionSyncingIndicator, {})], + ]; + + for (const [name, element] of wrapperCases) { + it(`${name}: animated wrapper is icon-only and explicitly sized`, () => { + render(element()); + + const wrappers = [...container.querySelectorAll('span.animate-spin')]; + expect(wrappers.length).toBe(1); + const wrapper = wrappers[0]!; + + // Square, explicitly sized: the wrapper's center is the glyph's center. + expect(hasExplicitSquareSize(wrapper)).toBe(true); + + // Icon-only. Any extra content (a label, a sibling) shifts the box + // center off the glyph and turns the spin into an orbit. + expect(wrapper.childElementCount).toBe(1); + expect(wrapper.firstElementChild?.tagName).toBe('svg'); + expect(wrapper.textContent).toBe(''); + + // The glyph itself must NOT also spin, or the two rotations compound. + expect(wrapper.querySelector('svg')?.classList.contains('animate-spin')).toBe(false); + }); + } + + /* The mobile home status pill: a capped-width flex row with a truncating + label, i.e. exactly the layout that squishes an unprotected spinner. */ + const pillStates = [ + { label: 'refreshing', props: { state: 'online', refreshing: true } }, + { label: 'reconnecting', props: { state: 'reconnecting' } }, + { label: 'loading', props: { state: 'loading' } }, + ] as const; + + for (const { label, props } of pillStates) { + it(`mobile status pill (${label}): spinner cannot be squished by the label`, () => { + render( + React.createElement(MobileConnectionStatus, { + ...props, + labels: { + refreshing: '正在刷新工作区，请稍候等待同步完成', + reconnecting: '正在重新连接到工作区，请稍候等待', + loading: '正在连接到工作区，请稍候等待', + }, + } as React.ComponentProps) + ); + + const spinner = container.querySelector('svg.animate-spin'); + expect(spinner).not.toBeNull(); + // Without shrink-0 the flex row compresses the glyph to a non-square + // box and the rotation sweeps an ellipse. + expect(spinner!.classList.contains('shrink-0')).toBe(true); + expect(hasExplicitSquareSize(spinner!)).toBe(true); + }); + } +});