Skip to content

Stop ToggleSwitch jumping when the loading spinner is shown - #4152

Merged
gabrielhl merged 4 commits into
mainfrom
fix-toggle-switch-jump-on-loading
Jul 31, 2026
Merged

Stop ToggleSwitch jumping when the loading spinner is shown#4152
gabrielhl merged 4 commits into
mainfrom
fix-toggle-switch-jump-on-loading

Conversation

@gabrielhl

@gabrielhl gabrielhl commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

What are you trying to accomplish?

Fixes the long-standing bug where a ToggleSwitch jumps up 4px the moment its loading spinner appears, then drops back down when the request finishes. Only size: :small is affected, which is what #2623 reported in 2024. It resurfaced this week on GitHub's Code scanning settings page, but every ToggleSwitch with a src is affected.

Root cause. .ToggleSwitch-statusIcon reserves width but not height:

.ToggleSwitch-statusIcon {
  display: flex;
  width: var(--base-size-16);
  margin-top: 0.063rem;
}

With both icons hidden the box has no in-flow children, so it's 0px tall. Unhiding the spinner makes it 21px, not 16px, because Primer::Beta::Spinner hangs hidden on a wrapper <span> that also carries sr-only "Loading" text, and that inline formatting context adds the font strut descender.

.ToggleSwitch is inline-flex, so it sits on its line by its baseline, which comes from its first flex item, this box. 0px to 21px moves the baseline and the browser repositions the whole switch. At the default size the 32px track stays the tallest item so nothing moves. At small the track is 24px and the 21px slot wins, hence -4px.

The fix reserves the height the same way the width is already reserved, and centers the contents.

Screenshots

No visual change at rest. The change is that the switch stops moving while loading.

Measured on this PR's preview deployment, .ToggleSwitch-track top before and after unhiding each icon. Right column is the same page with the new rule overridden back to what ships today:

browser slot height shift, with fix shift, reverted
Chrome 150 16 → 16 0px -3.5px
Edge 150 16 → 16 0px -3.5px
Firefox 150 16 → 16 0px -3.5px
WebKit 26.4 (Safari) 16 → 16 0px -3.28px

Spinner and error icon give identical numbers in each.

Integration

CSS only, no API or markup change. Consumers pick it up on the next release.

List the issues that this change affects.

Closes #2623

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.

One rule, one component. The switch's own box height is unchanged (24px small, 32px default) so nothing around it reflows.

What approach did you choose and why?

The slot already reserves width so the switch can't move horizontally. This makes it reserve height for the same reason.

Two alternatives I measured and discarded:

  • line-height: 0 on the slot. Collapses the spinner wrapper 21px to 16px, but the box still goes 0px to 16px, so the baseline still moves and you still get the full -4px.
  • Fixing Primer::Beta::Spinner by giving its wrapper display: inline-flex. Worth doing on its own merits, but it doesn't fix this bug for the same reason. Happy to open that separately.

height alone gets to 0px. flex: none and the centering are belt and braces so the 21px wrapper can't stretch the box when the row is tight.

Anything you want to highlight for special attention from reviewers?

The tests unhide the icons directly rather than clicking and racing the in-flight request, so they assert the CSS invariant instead of flaking on timing. Both fail without the CSS change, per the right column above.

They use removeAttribute('hidden') rather than .hidden = false because hidden is an HTMLElement IDL property and the error icon is an <svg>, so assigning to it silently does nothing. CI caught that on my first attempt.

Accessibility

  • No new axe scan violation - This change does not introduce any new axe scan violations.

CSS only, no change to markup, roles, or the sr-only loading text.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Docs and previews aren't affected, the existing small preview already reproduces the bug.

… jump when loading

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 14f13505-d338-494c-a7df-96616e27c88f
@changeset-bot

changeset-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f90fc6a

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

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

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

… no hidden IDL property

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 14f13505-d338-494c-a7df-96616e27c88f

Copilot AI 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

Fixes a layout-shift bug in Primer::Alpha::ToggleSwitch (small variant) where revealing the loading/error status icon changes the inline-flex baseline and causes the switch to “jump” vertically.

Changes:

  • Reserve vertical space for .ToggleSwitch-statusIcon (height + centering) to prevent baseline changes when icons appear.
  • Add system tests that assert the switch track’s getBoundingClientRect().top is stable when the spinner or error icon is unhidden.
  • Add a patch changeset documenting the fix.
Show a summary per file
File Description
test/system/alpha/toggle_switch_test.rb Adds system regression tests ensuring the small ToggleSwitch doesn’t shift when status icons are shown.
app/components/primer/alpha/toggle_switch.pcss Reserves height and centers contents in the status icon slot to eliminate vertical jumping.
.changeset/hungry-poems-shout.md Publishes a patch-level changeset describing the CSS fix.

Review details

Suppressed comments (2)

test/system/alpha/toggle_switch_test.rb:138

  • Like the spinner test, this execute_script will fail with an unhelpful JS error if the element can’t be found. Adding an explicit existence assertion (visible: :all) and a clearer error improves debuggability.
      page.execute_script(
        "document.querySelector(\"[data-target='toggle-switch.errorIcon']\").removeAttribute('hidden')"
      )

test/system/alpha/toggle_switch_test.rb:148

  • track_top will currently raise a JS TypeError if .ToggleSwitch-track is not found, which makes failures harder to interpret. Throwing a targeted error produces clearer test output if the preview structure changes.
    def track_top
      evaluate_script("document.querySelector('.ToggleSwitch-track').getBoundingClientRect().top")
    end
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread test/system/alpha/toggle_switch_test.rb Outdated
…nt fails clearly

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 14f13505-d338-494c-a7df-96616e27c88f

Copilot AI 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.

Review details

  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@gabrielhl
gabrielhl marked this pull request as ready for review July 31, 2026 13:31
@gabrielhl
gabrielhl requested a review from a team as a code owner July 31, 2026 13:31
@gabrielhl
gabrielhl requested a review from liuliu-dev July 31, 2026 13:31

@llastflowers llastflowers 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.

@liuliu-dev liuliu-dev 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.

nitpick: consider adding line-height: var(--base-size-16) here as well.

Screen.Recording.2026-07-31.at.10.09.57.AM.mov

…sits on centre

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 14f13505-d338-494c-a7df-96616e27c88f
@gabrielhl

gabrielhl commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Good catch! Went with line-height: 0 instead, it lands the spinner dead centre where var(--base-size-16) leaves it ~1px too high.

0 makes the wrapper collapse to the icon's 16px. It's also what this file already does for .ToggleSwitch-lineIcon and .ToggleSwitch-circleIcon.

@gabrielhl

Copy link
Copy Markdown
Contributor Author

@llastflowers just in case you're curious, in the wild it happens on every click – at least in this pattern inside a control group

Screen.Recording.2026-07-31.at.21.56.03.mov

@gabrielhl
gabrielhl merged commit 0c0462d into main Jul 31, 2026
32 checks passed
@gabrielhl
gabrielhl deleted the fix-toggle-switch-jump-on-loading branch July 31, 2026 20:25
@primer primer Bot mentioned this pull request Jul 31, 2026
myabc added a commit to opf/primer_view_components that referenced this pull request Aug 14, 2026
Merges primer/view_components up to 18aa9c4, the batch released
upstream as v0.53.2.

Component change: reserves height as well as width in
.ToggleSwitch-statusIcon so the switch no longer jumps when the
loading spinner appears (primer#4152).

Keeps the fork's workflow files unchanged: the upstream conflicts are
GitHub Action version bumps (the fork pins by SHA through its own
Dependabot) and the migration of gem publishing to RubyGems Trusted
Publishing, which does not apply to the fork's release flow.

Replays the rails-html-sanitizer 1.7.1 and loofah 2.25.2 bumps that
merge=ours dropped from both Gemfile.locks. Remaining upstream
lockfile bumps are left to the fork's weekly Dependabot.

Regenerates static/classnames.* from a clean tree, which also picks up
class names the fork's own sources already carried.
myabc added a commit to opf/primer_view_components that referenced this pull request Aug 14, 2026
Merges primer/view_components up to 18aa9c4, the parent of upstream
Release Tracking primer#4154.

Component change: reserves height as well as width in
.ToggleSwitch-statusIcon so the switch no longer jumps when the
loading spinner appears (primer#4152).

Keeps the fork's workflow files unchanged: the upstream conflicts are
GitHub Action version bumps (the fork pins by SHA through its own
Dependabot) and the migration of gem publishing to RubyGems Trusted
Publishing, which does not apply to the fork's release flow.

Replays the rails-html-sanitizer 1.7.1 and loofah 2.25.2 bumps that
merge=ours dropped from both Gemfile.locks. Remaining upstream
lockfile bumps are left to the fork's weekly Dependabot.

Regenerates static/classnames.* from a clean tree, which also picks up
class names the fork's own sources already carried.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Primer::Alpha::ToggleSwitch jumps in the small variant

4 participants