Improve area menu#590
Conversation
📝 WalkthroughWalkthroughRelocates the accessibility.js module import from index.html to main/main.js. Refactors keyboard activation into an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/accessibility.js`:
- Around line 76-80: When no badge is focused
(badges.indexOf(document.activeElement) returns -1) and the user presses
Shift+Tab the current calculation yields (badIndex - 1 + badges.length) %
badges.length which produces badges.length-2; to fix this, detect currentIndex
=== -1 and, before computing nextIndex, set currentIndex to 0 when e.shiftKey is
true (so (0 - 1 + badges.length) % badges.length becomes badges.length-1),
otherwise leave it as -1 for forward Tab; then compute nextIndex exactly as
currently done and call badges[nextIndex].focus(); this change touches the
variables currentIndex, nextIndex, document.activeElement and the e.shiftKey
branch.
🪄 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: 16d05f67-52bf-400c-ab44-7f745ace6d0f
📒 Files selected for processing (4)
index.htmlmain/accessibility.jsmain/main.jsstyle.css
💤 Files with no reviewable changes (1)
- index.html
| const currentIndex = badges.indexOf(document.activeElement); | ||
| const nextIndex = e.shiftKey | ||
| ? (currentIndex - 1 + badges.length) % badges.length | ||
| : (currentIndex + 1) % badges.length; | ||
| badges[nextIndex].focus(); |
There was a problem hiding this comment.
Fix reverse-tab wrap when no badge is currently focused.
At Line 77–79, currentIndex === -1 with Shift+Tab resolves to badges.length - 2, so focus skips the last badge.
Suggested fix
- const currentIndex = badges.indexOf(document.activeElement);
- const nextIndex = e.shiftKey
- ? (currentIndex - 1 + badges.length) % badges.length
- : (currentIndex + 1) % badges.length;
+ const currentIndex = badges.indexOf(document.activeElement);
+ const hasFocusedBadge = currentIndex !== -1;
+ const nextIndex = e.shiftKey
+ ? hasFocusedBadge
+ ? (currentIndex - 1 + badges.length) % badges.length
+ : badges.length - 1
+ : hasFocusedBadge
+ ? (currentIndex + 1) % badges.length
+ : 0;
badges[nextIndex].focus();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/accessibility.js` around lines 76 - 80, When no badge is focused
(badges.indexOf(document.activeElement) returns -1) and the user presses
Shift+Tab the current calculation yields (badIndex - 1 + badges.length) %
badges.length which produces badges.length-2; to fix this, detect currentIndex
=== -1 and, before computing nextIndex, set currentIndex to 0 when e.shiftKey is
true (so (0 - 1 + badges.length) % badges.length becomes badges.length-1),
otherwise leave it as -1 for forward Tab; then compute nextIndex exactly as
currently done and call badges[nextIndex].focus(); this change touches the
variables currentIndex, nextIndex, document.activeElement and the e.shiftKey
branch.
There was a problem hiding this comment.
It's not possible for no badge to be focused because the first badge is auto focused and Esc exits the overlay.
There was a problem hiding this comment.
@lawsie, you're right — since the first badge is auto-focused immediately when the overlay opens and Escape (as well as number keys and Enter) always closes the overlay, the currentIndex === -1 state is unreachable during normal Tab navigation. My concern doesn't apply here. Thanks for the clear explanation!
Summary
You can now tab through the areas in the area menu and press enter to select an area, as well as using the number keys.
main.jsfor consistency.AI usage
Planned by me, mostly executed by me, with Claude Sonnet 4.6 to advise on a few fiddly bits.
Summary by CodeRabbit
Release Notes
New Features
Style