Skip to content

perf(Spinner): replace Web Animations API with CSS animation-delay sync - #7550

Merged
hectahertz merged 4 commits into
mainfrom
hectahertz/perf-spinner-css-animation-sync
Mar 3, 2026
Merged

perf(Spinner): replace Web Animations API with CSS animation-delay sync#7550
hectahertz merged 4 commits into
mainfrom
hectahertz/perf-spinner-css-animation-sync

Conversation

@hectahertz

@hectahertzhectahertz commented Feb 15, 2026

Copy link
Copy Markdown
Contributor

Closes #

Replaces the Web Animations API-based spinner synchronization with a pure CSS animation-delay approach, eliminating a Safari/WebKit performance bottleneck.

The previous sync mechanism called three Web Animations API methods per Spinner mount: element.getAnimations() (3-5x slower in WebKit than Chromium), CSSAnimation.pause() + element.animate() (replaces native CSS animation with a JS-driven one, often falling back to main-thread compositing in WebKit), and Animation.startTime (forces WebKit to recalculate timing for all document animations). This resulted in 2 animations per SVG and a global useSyncExternalStore store that re-rendered all mounted Spinners when the first one set its startTime.

The new approach computes a negative CSS animation-delay from performance.now() at visibility time:

animationDelay=-(performance.now()%1000)ms

Since all instances reference the same monotonic clock, they land at the same rotation angle regardless of mount time. The CSS animation engine handles everything natively (GPU-composited), with 1 animation per SVG and zero JS animation overhead. The delay is computed at visibility time (not mount time), so spinners using the delay prop don't flash at the wrong angle on the first frame.

Measurements (5 spinners, staggered mount)

Flag OFFFlag ON
main (Web Animations API)276° spread, 1 anim/SVG0° spread, 2 anim/SVG
This PR (CSS animation-delay)276° spread, 1 anim/SVG4° spread, 1 anim/SVG

The 4° spread is floating-point rounding from performance.now(), visually imperceptible.

Changelog

New

N/A

Changed

  • Spinner animation synchronization now uses CSS animation-delay instead of the Web Animations API

Removed

N/A

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

  1. Enable primer_react_spinner_synchronize_animations feature flag in Storybook toolbar
  2. Open the "Synchronized Spinners" example story
  3. Verify all spinners rotate in sync as they appear
  4. Toggle prefers-reduced-motion: reduce in OS settings, confirm sync is skipped
  5. Test in both Safari and Chrome

Merge checklist

@changeset-bot

changeset-botBot commented Feb 15, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b8cc0c2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR replaces the Web Animations API-based spinner synchronization mechanism with a CSS animation-delay approach to eliminate a Safari/WebKit performance bottleneck. The previous implementation used multiple Web Animations API calls per Spinner mount (getAnimations(), pause(), element.animate(), and startTime manipulation) along with a global useSyncExternalStore that caused all mounted Spinners to re-render. The new approach computes a negative CSS animation-delay from performance.now() at visibility time, allowing the CSS animation engine to handle synchronization natively with GPU compositing.

Changes:

  • Removed the complex Web Animations API synchronization logic including the global animationTimingStore and useSpinnerAnimation hook
  • Implemented CSS-based animation sync using computed animation-delay based on performance.now() % 1000
  • Updated state management to track both visibility and sync delay, computing the delay at the moment the spinner becomes visible (either immediately or after the delay prop timeout)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
packages/react/src/Spinner/Spinner.tsxReplaced ~120 lines of Web Animations API code with a simple 4-line computeSyncDelay function; updated state management to compute sync delay at visibility time; applied computed delay as inline animationDelay style when feature flag is enabled and user has no motion preference
.changeset/spinner-css-animation-sync.mdAdded patch-level changeset describing the performance improvement

@primer-integration

Copy link
Copy Markdown

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14142

@primer-integration

Copy link
Copy Markdown

Integration test results from github/github-ui:

Passed CI  Passed
Passed VRT  Passed
Passed Projects  Passed

All checks passed!

@hectahertz

Copy link
Copy Markdown
ContributorAuthor

Friendly ping for review: @siddharthkp@TylerJDev 🙏

@francineluccafrancinelucca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siddharthkp

siddharthkp commented Mar 3, 2026

Copy link
Copy Markdown
Member

Hi!

@joshblack had tried to implement this with pure css first, but it never synced properly.

Worth verifying if this PR has the same issues not:

@hectahertz
hectahertz added this pull request to the merge queueMar 3, 2026
Merged via the queue into main with commit 9585669Mar 3, 2026
54 of 55 checks passed
@hectahertz
hectahertz deleted the hectahertz/perf-spinner-css-animation-sync branch March 3, 2026 16:21
@primerprimerBot mentioned this pull request Mar 3, 2026
@hectahertz

hectahertz commented Mar 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@siddharthkp Checked both threads. The two concerns were:

  1. Josh saw spinners "off a bit" when slowing down a CSS animation-delay prototype
  2. MDN says "it is impossible to sync two separate animations with CSS animations"

Neither applies here. The MDN quote is about Animation.startTime manipulation, not animation-delay. This approach uses a different mechanism: each spinner computes -(performance.now() % 1000)ms at visibility time, so at any wall-clock time N its position resolves to N % 1000 regardless of mount time.

The github-ui #6120 implementation used Date.now() instead of performance.now(). CSS animations run on the document timeline which is performance.now()-based, so using a different clock introduces drift. This PR matches the correct clock.

The ~4° spread I measured comes from the render-to-paint gap (up to 16ms at 60fps), visually imperceptible at 1 rotation/sec.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommendedThis change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@hectahertz@siddharthkp@francinelucca