Repository navigation
Conversation
scrollIntoView moves Chrome's sequential focus navigation starting point past the active tab, so the first Tab after load skipped the whole header and landed on the next tab. Scrolling the tablist itself (plain scrollLeft, also readable under jsdom) keeps the tab in view without touching that starting position.
Owner
|
Thanks so much for picking this up, and for the clear write-up and tests. We'd already opened richshaw/OpenSignup#672 for richshaw/OpenSignup#644, so we’re going to land that one and close this. One thing for next time, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On a signup's Build, Responses and Settings tabs, the first Tab after the page loads skips the whole header and lands on the next tab (Fixes #644).
Root cause
TabsNavcentered the active tab withscrollIntoView. Besides scrolling the strip,scrollIntoViewalso moves Chrome's sequential focus navigation starting position past the active tab — so the first Tab press started from the tab itself and went to whatever followed it in DOM order, skipping the header links.Fix
Scroll the tablist element directly instead:
tablist.scrollLeft = el.offsetLeft - (tablist.clientWidth - el.offsetWidth) / 2. SettingscrollLefton the strip scrolls only the strip and never touches the focus-navigation starting position, and it keeps the same centering behavior. PlainscrollLeft(rather thanscrollTo) also runs under jsdom, which implements neither scroll API.Testing
src/components/signup/TabsNav.test.tsx:scrollIntoViewis never called.pnpm lintandpnpm typecheckpass.npx vitest run: 1778 passed, 3 failed — the same 3 fail on unmodifiedmainin this environment (env.test.ts.env.examplegolden,worker-rendergolden,magic-composegolden), so they are pre-existing and unrelated.pnpm knipcould not run locally (OOM at baseline too), so it is left to CI.