feat: add keyboard shortcut for search and accessibility label - #2
feat: add keyboard shortcut for search and accessibility label#2itzMRZ wants to merge 4 commits into
Conversation
- Added `/` and `Ctrl+K` keyboard shortcuts to focus search input - Added visually hidden label for search input for better accessibility - Added visual keyboard hint to search bar on desktop - Added `.sr-only` utility class
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds search accessibility enhancements: screen-reader label and visible hint, keyboard shortcuts (/, Ctrl/Cmd+K) to focus the search input and Escape to blur, new CSS utilities for visually hidden content and shortcut hint, plus a new Markdown note documenting these choices. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- Potential crash in
js/uiController.jsbecausesearchInput.focus()is called without the usual null guard; if the element is absent the UI could throw before initializing search. - Misleading "Clear search" comment in
js/uiController.jswhile the code only blurs the input, which may confuse future maintenance decisions. - Pay close attention to
js/uiController.js- add the missing null check and correct the comment to reflect actual behavior.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="js/uiController.js">
<violation number="1" location="js/uiController.js:295">
P2: Missing null check for `searchInput`. Other code in this file guards with `if (this.elements.searchInput)` before use. If the element doesn't exist, calling `.focus()` will throw an error.</violation>
<violation number="2" location="js/uiController.js:298">
P3: Misleading comment: says "Clear search" but code only blurs (unfocuses) the input without clearing the search text.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🤖 Fix all issues with AI agents
In @js/uiController.js:
- Around line 282-302: The keydown handler registered via
document.addEventListener('keydown', (e) => ...) uses this.elements.searchInput
multiple times without verifying it exists, which can cause runtime errors; fix
this by adding a defensive null check for this.elements.searchInput at the start
of the handler (or before each use) and early-return if it's missing, and also
guard the Escape branch so it only compares document.activeElement ===
this.elements.searchInput when this.elements.searchInput is non-null; keep the
rest of the shortcut logic unchanged.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.Jules/palette.mdindex.htmljs/uiController.jsstyles.css
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (4)
styles.css (2)
148-159: LGTM! Standard accessibility pattern correctly implemented.The
.sr-onlyutility follows the well-established pattern for visually hiding content while keeping it accessible to screen readers.
467-492: LGTM! Well-designed keyboard hint with responsive behavior.The
.shortcut-hintstyling correctly implements:
- Desktop-only display via media query
- Smart hiding when input is focused or has content
- Proper positioning within the search container
The general sibling combinator (
~) on lines 489-491 works well with the HTML structure inindex.html.index.html (1)
68-74: Excellent accessibility implementation!The search input markup demonstrates best practices:
- Properly associated label via
for="searchInput"(Line 69)- Screen-reader-only label using
.sr-onlyclass- Decorative hint correctly marked with
aria-hidden="true"(Line 73)- Clear button includes
aria-labelfor screen readers (Line 72)This ensures the search functionality is accessible to all users while maintaining the intended visual design.
.Jules/palette.md (1)
1-3: Great practice documenting accessibility learnings!This note captures valuable insights about keyboard shortcuts and accessibility patterns that can guide future feature development.
|
@jules |
![cubic-dev-ai[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Fix misleading comment in keyboard shortcut handler
- Added `/` and `Ctrl+K` keyboard shortcuts to focus search input - Added visually hidden label for search input for better accessibility - Added visual keyboard hint to search bar on desktop - Added `.sr-only` utility class - Updated comment for keyboard listener logic - Added null check for search input in keyboard listener
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/uiController.js (1)
282-305: Keyboard shortcuts: don’t steal focus during modals; broaden “typing” guards; handleKcaseNice improvement and the
searchInputnull guard fixes the reported risk. A couple UX/a11y edge cases remain:
- If the help modal is open,
/or Ctrl/Cmd+K can focus the search input behind the modal (confusing and breaks modal keyboard flow).- Guarding only
INPUT/TEXTAREAmissesSELECTand contenteditable targets.e.key === 'k'won’t fire for uppercaseK(Shift+Cmd/Ctrl+K).Proposed tweak (localized to this block)
document.addEventListener('keydown', (e) => { // Guard clause if search input doesn't exist if (!this.elements.searchInput) return; + if (e.defaultPrevented) return; + + // Don't steal focus when the help modal is open + if (this.elements.infoModal?.classList.contains('visible')) return; + + const target = e.target; + const tag = target?.tagName; + const isTypingContext = + target?.isContentEditable || + tag === 'INPUT' || + tag === 'TEXTAREA' || + tag === 'SELECT'; // Focus search on '/' or Ctrl+K (Cmd+K) - if ((e.key === '/' || (e.key === 'k' && (e.metaKey || e.ctrlKey))) && + const key = (e.key || '').toLowerCase(); + if ((key === '/' || (key === 'k' && (e.metaKey || e.ctrlKey))) && document.activeElement !== this.elements.searchInput) { - // Don't trigger if user is typing in another input (if any existed) - if (document.activeElement.tagName === 'INPUT' || - document.activeElement.tagName === 'TEXTAREA') { + // Don't trigger if user is typing in another control + if (isTypingContext) { return; } e.preventDefault(); this.elements.searchInput.focus(); } // Remove focus from search on Escape if (e.key === 'Escape' && document.activeElement === this.elements.searchInput) { this.elements.searchInput.blur(); } });
1 participant
/andCtrl+Kkeyboard shortcuts to focus search input.sr-onlyutility classSummary by cubic
Adds standard search shortcuts and an accessible label to make search faster and screen-reader friendly. Addresses Linear palette-search-ux-1375227574019156193 by adding keyboard focus and a discoverable desktop hint.
Written for commit cc5dc7a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.