Skip to content

fix: switch bug#989

Open
sotomaque wants to merge 3 commits into
mainfrom
fix/switch-bug
Open

fix: switch bug#989
sotomaque wants to merge 3 commits into
mainfrom
fix/switch-bug

Conversation

@sotomaque
Copy link
Copy Markdown
Contributor

Bug:

  • Toggling a <Switch> inside a scrollable container causes the container to auto-scroll, jumping content out from under the user.

Root cause:

  • Switch renders a <input role="switch" type="checkbox"> that's visually hidden via position: absolute; clip-path: inset(50%); width: 1px; height: 1px; while the visible UI sits in sibling spans.
  • When the user clicks the visible label, react-aria's press handler calls inputRef.current.focus() to move keyboard focus onto the hidden input.
  • The browser's default behavior on focus() is to run scrollIntoView({block: 'nearest'}) on the focused element — so it tries to bring a 1×1 pixel element into view, scrolling the nearest scrollable ancestor by however much the input's position differs from the viewport.

Fix:

  • Apply scroll-margin: 100vh to the hidden input inside .switch, .checkbox, and .radio. This convinces scrollIntoView({ block: 'nearest' }) that the input is already visible, so the scroll is skipped.
  • Visible inputs are unaffected — auto-scroll-on-focus was already a no-op for them.
  • Scoped per component (no global selector), so the rule only touches the hidden a11y inputs these components render.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
design-toolkit Ready Ready Preview, Comment May 8, 2026 5:44am
map-toolkit Ready Ready Preview, Comment May 8, 2026 5:44am

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

🧠 Memory Leak Test Results

Status: ✅ All tests passed

Component Leaks Retained Size Status
accordion-group 0 0 B
accordion 0 0 B
actionbar 0 0 B
avatar 6 132.45 KB
badge 0 0 B
button 0 0 B
dialog 0 0 B
drawer 0 0 B
floating-card 0 0 B
floatingcard 0 0 B
intentional-leak 0 0 B
notice 0 0 B
tooltip 2 44.23 KB
📋 Test Details
  • Components tested: 13
  • Total leaks detected: 8
  • Workflow run: View details

🤖 Generated by MemLab + Playwright

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

🖼️ Visual Regression Test Results

Status: ✅ All tests passed

Metric Count
✅ Passed 2726
❌ Failed 0
Total 2726

Component Coverage

45 / 54 design-toolkit components have VRT tests (83%)

Missing VRT tests (9 components) - audio - carousel - deferred-collection - floating-card - gantt - lines - media-controls - status-indicator - video
> 4 components excluded: hotkey, icon, skeleton, view-stack --- 🤖 Generated by Vitest Browser + Playwright

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

📊 Coverage Reports

Coverage Changes by Package

Click to expand 29 package details

apps/next (No diff)

packages/bus (No diff)

packages/constants (No diff)

packages/converters (No diff)

packages/core (No diff)

packages/dataset (No diff)

packages/design-foundation (No diff)

packages/design-toolkit (No diff)

packages/formatters (No diff)

packages/geo (No diff)

packages/hotkey-manager (No diff)

packages/icons (No diff)

packages/logger (No diff)

packages/map-toolkit (No diff)

packages/math (No diff)

packages/ntds (No diff)

packages/postcss-tailwind-css-modules (No diff)

packages/predicates (No diff)

packages/temporal (No diff)

packages/web-worker (No diff)

packages/websocket (No diff)

tooling/biome-config (No diff)

tooling/constellation-tracker (No diff)

tooling/eslint-config (No diff)

tooling/prettier-config (No diff)

tooling/smeegl (No diff)

tooling/turbo-filter (No diff)

tooling/typescript-config (No diff)

tooling/vitest-config (No diff)

Coverage data collected from all packages in the monorepo.

@sotomaque
Copy link
Copy Markdown
Contributor Author

Screen.Recording.2026-05-05.at.6.39.52.PM.mov

PR with story that proves out bug here: #991

@switzerb
Copy link
Copy Markdown
Contributor

switzerb commented May 6, 2026

Screen.Recording.2026-05-05.at.6.39.52.PM.mov
PR with story that proves out bug here: #991

I think I am a little confused about the expectations around this fix. You want the container to scroll out of view with that jump? What is being fixed here? More directly -- on this branch with the style changes, I don't see any difference in the scroll behavior in the reproduction case. I feel like I am missing something. My apologies if I am being really dense...just trying to validate the fix.

@sotomaque
Copy link
Copy Markdown
Contributor Author

Screen.Recording.2026-05-05.at.6.39.52.PM.mov
PR with story that proves out bug here: #991

I think I am a little confused about the expectations around this fix. You want the container to scroll out of view with that jump? What is being fixed here? More directly -- on this branch with the style changes, I don't see any difference in the scroll behavior in the reproduction case. I feel like I am missing something. My apologies if I am being really dense...just trying to validate the fix.

this video is not for this branch; this video is how the switch behaves today demonstrating the bug

@switzerb
Copy link
Copy Markdown
Contributor

switzerb commented May 6, 2026

Screen.Recording.2026-05-05.at.6.39.52.PM.mov
PR with story that proves out bug here: #991

I think I am a little confused about the expectations around this fix. You want the container to scroll out of view with that jump? What is being fixed here? More directly -- on this branch with the style changes, I don't see any difference in the scroll behavior in the reproduction case. I feel like I am missing something. My apologies if I am being really dense...just trying to validate the fix.

this video is not for this branch; this video is how the switch behaves today demonstrating the bug

yes! that part makes sense -- but I am trying to say that running the same story on this branch does not appear to fix the bug and I feel like I am missing something. i.e. -- the style.module.css changes don't change behavior for me when I run it.

@shadyendless
Copy link
Copy Markdown
Contributor

shadyendless commented May 6, 2026

Hey @sotomaque, could you update your PR description or add in a comment here the code that you wrote up for the repro story shown in the video that demonstrates the issue? Would like to paste it in locally and run it to verify the results.

EDIT: Disregard, just saw #991.

input. scroll-margin makes scrollIntoView({block:'nearest'}) treat it as
already visible. */
& input {
scroll-margin: 100vh;
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.

This does not fix the problem for switches. I did not check for the other components that this PR updates.

CleanShot.2026-05-06.at.14.50.34.mp4

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants