fix: focus the search input through the owning component instead of a DOM query - #3252
Chirag6722 wants to merge 2 commits into
Conversation
… DOM query The '/' shortcut in app.vue and the ArrowUp handler on the search page located the search input with a selector that depended on markup in Header/SearchBox.vue and pages/index.vue. Add a useSearchInputFocus composable: app.vue provides a registry, both search inputs register their focus method, and the shortcut handlers call it. resolves npmx-dev#3216
Thanks for opening this pull request! 🎉We really appreciate you taking the time to contribute, @Chirag6722. A maintainer will take a look as soon as they can. In the meantime, please make sure that:
If anything needs adjusting we'll leave comments here. Thanks again! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe PR adds a scoped search-input focus context. The homepage and header register focus targets. Keyboard handlers use the context instead of DOM selectors. Unit and end-to-end tests cover focus routing, cleanup, route changes, and the ChangesSearch input focus
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Search focus routing preserves the navigation fallback when no target is available and includes lifecycle and shortcut coverage. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
graphieros
left a comment
graphieros
left a comment
There was a problem hiding this comment.
@Chirag6722
As you know, if you read our contribution guide, the use of AI to assist with coding is perfectly acceptable, as long as it is human driven.
However, using your own voice is required when communicating with npmx contributors.
Please consider writing the PR description with your own words. It can be in your native language if it is more practical.
Thank you^^
2 participants
🔗 Linked issue
resolves #3216
🧭 Context
The
/shortcut inapp.vueand the ArrowUp-from-first-result handler inpages/search.vueboth found the search input withdocument.querySelector('input[type="search"], input[name="q"]'). That selector depends on markup that lives in two other components (Header/SearchBox.vueandpages/index.vue), so a change to either input's attributes would silently break the shortcuts. Review on #3214 asked for the focus method to come from the component that owns the input instead.📚 Description
Adds a small
useSearchInputFocuscomposable with three parts:provideSearchInputFocus()is called once inapp.vue. It keeps a set of registered focus targets and exposesfocus(), which tries them in order and returnstrueat the first one that focused something.useSearchInputFocusTarget(fn)registers a target for the lifetime of the calling component scope (unregistered viaonScopeDispose).Header/SearchBox.vueregisters its existingfocus()(which now returnsfalsewhile the input is hidden byv-if, i.e. on the homepage), andpages/index.vueregisters the homepage search input through a template ref.useSearchInputFocus()returns the injectedfocus()for consumers;pages/search.vueuses it for ArrowUp.app.vue's/handler becomesif (focusSearchInput()) returnbefore falling back torouter.push({ name: 'search' }), which is the same control flow as before with the DOM query removed.Behaviour is intended to be unchanged: on the homepage the header box is not rendered, so the homepage input gets focus; elsewhere the header box does. One pre-existing edge is preserved rather than fixed: on mobile the header input exists but is hidden with CSS, so
focus()is a no-op there, exactly as the old querySelector path was. Happy to follow up on that separately if wanted.Tests:
test/nuxt/composables/use-search-input-focus.spec.tscovers focusing a registered target, the no-target/no-provider cases, first-success short-circuit, unregistration on unmount, andHeaderSearchBoxregistering itself and reportingfalseon the homepage. I checked that the last two fail when the registration line and theonScopeDisposecleanup are removed.test/e2e/interactions.spec.tsgets one new case: on/, move focus off the autofocused input, press/, expect#home-searchfocused. That is the homepage registration path, which no existing e2e test exercised. The existing/and ArrowUp e2e tests on/searchstill pass against aTEST=1build, and the new case fails when theuseSearchInputFocusTargetcall inpages/index.vueis removed.vp lint,vp fmt --check,vp test --project nuxtfor the new spec, andplaywright test test/e2e/interactions.spec.ts -g "search|homepage"pass locally. I could not runtest:typeson this machine because thetypescript-native-bridgebinary is blocked by a local application-control policy, so please rely on CI for that.AI help was taken for this change.