fix(nav): focus search input when mobile menu opens#523
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a client-side script in ChangesMobile Menu Focus Script
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
_includes/head_custom.html (1)
64-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider deferring the
nav-opencheck for robustness against handler ordering.The click handler checks
mainHeader.classList.contains("nav-open")synchronously, relying on just-the-docs' own click handler having already toggled the class. This works today because just-the-docs registers its handler before this one, but the ordering is implicit and fragile — a future just-the-docs update that wraps its handler registration inDOMContentLoaded(or changes include order) would silently reverse the execution order. In that scenario, the check would see stale state: on open,nav-openwouldn't be present yet (focus wouldn't move), and on close,nav-openwould still be present (focus would incorrectly move during close).Wrapping the check in
setTimeout(fn, 0)defers it until after all synchronous click handlers have run, making the behavior independent of registration order.♻️ Proposed refactor: defer the nav-open check
menuButton.addEventListener("click", function () { - if (mainHeader.classList.contains("nav-open")) { - searchInput.focus(); - } + setTimeout(function () { + if (mainHeader.classList.contains("nav-open")) { + searchInput.focus(); + } + }, 0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@_includes/head_custom.html` around lines 64 - 67, The menuButton click handler currently checks mainHeader.classList.contains("nav-open") synchronously, which depends on just-the-docs’ handler running first and is fragile. Update the click handler in the menuButton listener to defer the nav-open state check until after synchronous click handlers finish (for example by moving the focus logic into a zero-delay timeout), so the behavior in head_custom.html no longer depends on handler registration order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@_includes/head_custom.html`:
- Around line 59-61: The selector lookup in the head custom script is using IDs
that do not exist, causing the setup to abort before the focus behavior runs.
Update the DOM queries in the script around menuButton/mainHeader/searchInput to
use the theme’s real selectors from the markup and styles, specifically the
class-based header/nav elements instead of `#main-header` and `#search-input` while
keeping `#menu-button` for the toggle button. Make sure the variables used by the
focus handoff logic still resolve to the actual header, nav, and search elements
so the guard no longer exits early.
---
Nitpick comments:
In `@_includes/head_custom.html`:
- Around line 64-67: The menuButton click handler currently checks
mainHeader.classList.contains("nav-open") synchronously, which depends on
just-the-docs’ handler running first and is fragile. Update the click handler in
the menuButton listener to defer the nav-open state check until after
synchronous click handlers finish (for example by moving the focus logic into a
zero-delay timeout), so the behavior in head_custom.html no longer depends on
handler registration order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cc6c129-e878-4e73-a826-e988b2856c05
📒 Files selected for processing (1)
_includes/head_custom.html
Code review of #518 found that moving the search box to the top of the mobile menu (visually) left keyboard/screen-reader focus order unchanged: Tab still traverses the whole nav list before reaching search, since DOM order wasn't touched, only CSS position. That's now inconsistent with the new visual order. Focus the search input as soon as the mobile menu opens, matching its new position at the top. The listener runs after Just the Docs' own (head_custom.html loads after just-the-docs.js), so nav-open has already been toggled by the time it checks. Confirmed this doesn't prematurely expand the search results UI (that's gated on non-empty input, unaffected by focus alone) and doesn't affect desktop, where the menu button is hidden. Verified via headless Chromium: menu open -> focus lands on #search-input; typing still triggers results normally; desktop unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gkorland
force-pushed
the
fix/mobile-search-autofocus
branch
from
83ee56b to
0660fbe
Compare
1 participant
Summary
Follow-up to #518 (already merged). Code review flagged a focus-order inconsistency introduced by that fix: the search box now appears visually at the top of the mobile menu, but keyboard/screen-reader Tab order is unchanged (nav links first, search last), since only CSS position moved, not DOM order.
Changes
#search-inputautomatically when the mobile menu is opened, so keyboard focus lands where the search box now visually appears.head_custom.html, which loads afterjust-the-docs.js, so by the time it runs the theme's own click handler has already togglednav-open— no timing hacks (e.g.setTimeout) needed..search-activeis gated on non-empty input, unaffected by focus alone) and has no effect on desktop, where the menu button is hidden (display: noneat >= 50rem).Testing
Verified with headless Chromium (Playwright), injecting the change at its real position in
<head>to preserve script execution/registration order:#search-input.Escapestill clears the input as before.Memory / Performance Impact
N/A — small JS listener, no measurable perf impact.
Related Issues
Follow-up to #518.
Summary by CodeRabbit