fix: set the focus starting point without a fragment navigation - #16992
fix: set the focus starting point without a fragment navigation#16992Nic-Polumeyv wants to merge 3 commits into
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/942509eee2372428e7452642821c780b9764e8a1Open in |
🦋 Changeset detectedLatest commit: 942509e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
📝 WalkthroughWalkthroughSvelteKit now sets the sequential focus starting point directly on the hash target or document body. The focus helper preserves the target’s original Merge Risk: 🔵 Low · up to Direct focus reset avoids fragment-navigation side effects, but application focus handlers cannot reliably retain a tabindex change made during focus. This can affect keyboard-navigation behavior and should be corrected before merge. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9a1d481 to
39d10d1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
packages/kit/src/runtime/client/focus.js-32-35 (1)
32-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
tabindexchanges made by focus handlers.
element.focus()runs applicationfocushandlers before Line 32. If a handler changestabindex, this branch restores the captured value and discards the handler change. For example, a handler that addstabindex="0"to a target that initially has no attribute will have the attribute removed.Restore the original value only if the temporary
tabindex="-1"is still present. Add a regression test with a synchronous focus handler.Proposed fix
- if (tabindex !== null) { - element.setAttribute('tabindex', tabindex); - } else { - element.removeAttribute('tabindex'); + if (element.getAttribute('tabindex') === '-1') { + if (tabindex !== null) { + element.setAttribute('tabindex', tabindex); + } else { + element.removeAttribute('tabindex'); + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: QUIET
Plan: Advanced
Run ID: fa011ccf-7701-40e4-acc5-7637b41897e5
📒 Files selected for processing (8)
.changeset/focus-starting-point.mdpackages/kit/src/runtime/client/client.jspackages/kit/src/runtime/client/focus.jspackages/kit/src/runtime/client/focus.spec.jspackages/kit/src/runtime/client/scroll.jspackages/kit/src/runtime/client/scroll.spec.jspackages/kit/test/apps/basics/test/cross-platform/client.test.jspackages/kit/test/apps/hash-based-routing/test/test.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
sveltejs/vite-plugin-svelte(manual)vitejs/vite(manual)sveltejs/svelte(manual)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
39d10d1 to
942509e
Compare
1 participant
Fixes #16982.
Firefox and WebKit drop the focus a frame after
tabindexis removed rather than synchronously, so the two starting-point tests poll for<body>instead of readingactiveElementonce. The target now receives afocus/blurpair it never got from a fragment navigation, which is the trade-off teemingc named in #10856.