feat(menu): add hotkey prop for items & improve keyboard navigation#3978
feat(menu): add hotkey prop for items & improve keyboard navigation#3978adrianschmidt merged 1 commit intomainfrom
hotkey prop for items & improve keyboard navigation#3978Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds first-class hotkey support across the menu system: new Changes
Sequence DiagramsequenceDiagram
participant User
participant Document as Document (global)
participant Menu as limel-menu
participant Utils as HotkeyUtils
participant Item as MenuItem (visible)
User->>Document: keydown
Document->>Menu: deliver keydown
activate Menu
Menu->>Menu: exit if closed / defaultPrevented / repeat
Menu->>Menu: ignore if typing in input and no Ctrl/Meta
Menu->>Utils: normalize hotkey from event
Utils-->>Menu: normalizedHotkey
Menu->>Item: compare normalizedHotkey to visible, enabled items (cached)
alt matched
Menu->>Document: preventDefault & stopPropagation
Menu->>Menu: handleSelect(matched item)
Menu-->>User: emit select, close menu
else no match
Menu-->>User: no action
end
deactivate Menu
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3978/ |
adrianschmidt
force-pushed
the
split/2-hotkey-component
branch
4 times, most recently
from
d0d36ff to
66462bb
Compare
adrianschmidt
force-pushed
the
split/2-hotkey-component
branch
2 times, most recently
from
2db801c to
bb97913
Compare
adrianschmidt
force-pushed
the
split/3-menu-hotkeys
branch
from
3509a13 to
d792051
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/list-item/menu-item-meta/menu-item-meta.tsx`:
- Around line 59-63: The hotkey string is passed raw into renderCommandText and
into <limel-hotkey>, so rendered shortcut hints can be incorrect; update
renderCommandText to normalize this.hotkey before rendering by calling the same
normalization utility used in src/components/menu/menu.tsx (or the normalization
function documented in menu.types.ts) and pass the normalized value to
<limel-hotkey> (keep the check on this.hotkey but use the normalizedHotkey when
rendering and consider handling empty/invalid results).
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Around line 18-20: The comment notes the documentation in menu-hotkeys.tsx
wrongly states arrow keys are "always reserved"; update the text to match the
implementation in menu.tsx which only rejects unmodified arrow keys and allows
modifier combinations (e.g. alt+ArrowDown) as verified by menu.e2e.tsx. Edit the
explanatory bullet to say that Tab and unmodified arrow keys are reserved, and
that Enter, Escape, and Space are reserved unless used with a modifier (and
explicitly mention that arrows can be used with modifiers like alt+arrowdown);
also add or update a short example in this file demonstrating a modifier+arrow
hotkey to follow the guideline for documenting new prop/behavior.
In `@src/components/menu/menu.tsx`:
- Around line 328-329: The hotkey interception currently treats any modifier
(including Alt/AltGraph) as blocking typing; update the modifier check used by
the keyboard handler (the call sites using this.isFromTextInput(event) &&
!this.hasModifier(event)) so that hasModifier only returns true for
event.ctrlKey or event.metaKey and explicitly rejects
event.getModifierState('AltGraph') and event.altKey; adjust the implementation
of hasModifier (or replace its use) to allow Alt/Option/AltGraph for text input
and ensure the same change is applied at the second occurrence around the other
keyboard handler (the block that mirrors the check at the later occurrence).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6dee05e4-adbb-48a2-9ac4-fa015f13cbee
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (7)
src/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/examples/menu-searchable-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.ts
| if (this.isFromTextInput(event) && !this.hasModifier(event)) { | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/menu/menu.tsx | sed -n '320,340p'Repository: Lundalogik/lime-elements
Length of output: 736
🏁 Script executed:
cat -n src/components/menu/menu.tsx | sed -n '375,390p'Repository: Lundalogik/lime-elements
Length of output: 564
🏁 Script executed:
cat -n src/components/menu/menu.tsx | sed -n '320,345p'Repository: Lundalogik/lime-elements
Length of output: 900
🏁 Script executed:
# Find the hasModifier method definition
rg -n "hasModifier" src/components/menu/menu.tsx -A 3Repository: Lundalogik/lime-elements
Length of output: 467
🌐 Web query:
macOS Alt Option key text input special characters how it works
💡 Result:
On macOS, the Option key (also labeled Alt on some keyboards, symbolized as ⌥) acts as a modifier key similar to AltGr on other systems. It enables access to special characters, symbols, and diacritical marks when combined with other keys. This works primarily through direct character mappings and dead key functionality, varying by keyboard layout (e.g., US English ABC layout). Key mechanisms: - Direct symbols: Option + key produces a special character (e.g., Option + 2 = ™, Option + 8 = •, Option + - = –, Option + G = ©). - Option + Shift often produces variants (e.g., Option + Shift + 8 = °, Option + Shift + 2 = €, Option + Shift + - = —). - Dead keys for accents: Option + accent key (e.g., Option + e = acute ´), then letter (e.g., a → á). Orange-highlighted keys in Keyboard Viewer indicate dead keys. - Unicode Hex Input (optional): Add via System Settings > Keyboard > Text Input > Input Sources > + > Unicode Hex Input. Switch to it, then Option + hex code (e.g., Option + 00A9 = ©). To view mappings: 1. Enable Input menu: System Settings > Keyboard > Text Input > Edit > Show Input menu in menu bar. 2. Click Input menu icon > Show Keyboard Viewer. 3. Press Option (and/or Shift) on physical keyboard; viewer shows changed characters/orange dead keys. Examples (US layout): - Currency: Option + 3 = £, Option + Y = ¥, Option + Shift + 2 = €. - Punctuation/Math: Option + ; = …, Option + / = ÷, Option + X = ≈. - Accents: Option + then e = è, Option + i then a = â. Note: Exact output depends on input source/layout. Press-and-hold letters shows accent popups separately (not Option-specific). Use Character Viewer (Control + Command + Space) for any symbol. This is consistent across recent macOS versions including Tahoe 26.
Citations:
- 1: https://howtotypeanything.com/alt-codes-on-mac
- 2: https://en.wikipedia.org/wiki/Option_key
- 3: https://support.apple.com/gu-in/guide/mac-help/mh27474/mac
- 4: https://support.apple.com/en-lb/guide/mac-help/mh27474/mac
- 5: https://support.apple.com/guide/mac-help/use-the-keyboard-viewer-on-mac-mchlp1015/mac
- 6: https://support.apple.com/en-tj/guide/mac-help/mchlp1015/mac
🌐 Web query:
AltGraph keyboard event how it reports in JavaScript ctrlKey altKey
💡 Result:
In JavaScript KeyboardEvent objects, the AltGraph (AltGr) key's reporting on ctrlKey and altKey varies by browser and platform but follows consistent patterns in modern browsers. When the AltGraph key is pressed alone or as a modifier: - event.key === "AltGraph" (key code 225, code: "AltRight", location: 2) - event.altKey is true across all major browsers (Chrome, Firefox, Safari, Edge) - event.ctrlKey is typically true in modern browsers (Chrome/WebKit, Firefox, Edge/IE10+), but was false in older Firefox and IE8 This is because AltGr is often implemented as a right Alt key that logically combines Alt + Ctrl effects for character input (e.g., € from AltGr+E), making it indistinguishable from simultaneous Ctrl+Alt in many cases. Implementations are encouraged to expose AltGraph separately via getModifierState("AltGraph"), which returns true specifically for AltGr (supported in Chrome 48+, Firefox 15+, Safari 10.1+). To detect AltGr reliably: - Check event.key === "AltGraph" for the key itself - Use event.ctrlKey && event.altKey && !event.getModifierState("Control") && !event.getModifierState("Alt") && event.getModifierState("AltGraph") for AltGr-modified keys (platform-dependent) - Note: No reliable way to distinguish physical AltGr from Ctrl+Alt in all cases due to OS-level synthesis (Windows treats AltGr as Ctrl+Alt). Example event dump for AltGraph press: {key: "AltGraph", code: "AltRight", location: 2, altKey: true, ctrlKey: false (varies), getModifierState("AltGraph"): true}
Citations:
- 1: https://stackoverflow.com/questions/10657346/detect-alt-gr-alt-graph-modifier-on-key-press
- 2: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/altKey
- 3: https://www.toptal.com/developers/keycode/alt-graph
- 4: https://stackoverflow.com/questions/10657346/detect-alt-gr-alt-graph-modifier-on-key-press/18570096
- 5: Add specification for AltGraph key & modifier behaviour w3c/uievents#147
- 6: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent
- 7: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
- 8: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.getModifierState
- 9: https://www.w3.org/TR/uievents/
- 10: https://w3c.github.io/uievents/
🌐 Web query:
international keyboard layouts Alt modifier text input
💡 Result:
In international keyboard layouts, the Alt modifier (often the right Alt key, functioning as AltGr) enables text input of accented characters, special symbols, and diacritics, particularly for Latin-based languages. This is distinct from the left Alt used for shortcuts. Key mechanisms: - Right Alt/AltGr + key: Direct input of characters like € (AltGr+5), æ (AltGr+a in US-Intl), ç (AltGr+, in some layouts). - Dead keys: AltGr activates diacritics (e.g., AltGr+' then e → é), which combine with following letters without inserting the accent alone unless followed by space. Platform specifics: Windows (US-International layout): - Right Alt (AltGr) for symbols: AltGr+2=², AltGr+E=€, AltGr+N=ñ. - Dead keys on ', , ^, ~, " for accents (e.g., ' then e → é). Press twice or +space for literal accent. - Official: Microsoft docs confirm layout supports European languages via right Alt and dead keys. Linux (us variant altgr-intl or intl with AltGr dead keys): - setxkbmap us(altgr-intl): Hides dead keys behind AltGr (AltGr+' then e → é; normal ' types apostrophe). - Preferred for programmers: Avoids dead key interference in code. macOS (Option key ≡ Alt): - Option + key for dead keys: Option+e then a → é; Option+u then a → ä. - ABC - Extended layout: Comprehensive diacritics. - Press-and-hold vowels for accent menus. Common issues: - Shortcuts using right Alt may fail as it's AltGr. - Ctrl+Alt often emulates AltGr on Windows/Linux. To enable: - Windows: Settings > Time & Language > Language > Add English (US) > Options > Add keyboard > United States-International. - Linux: Settings or setxkbmap -layout us -variant altgr-intl. - macOS: System Settings > Keyboard > Input Sources > Add ABC Extended or target language.
Citations:
- 1: https://en.wikipedia.org/wiki/AltGr_key
- 2: https://learn.microsoft.com/en-us/globalization/keyboards/kbdusx
- 3: https://ftp.zx.net.nz/pub/Patches/ftp.microsoft.com/MISC/KB/en-us/97/738.HTM
- 4: https://askubuntu.com/questions/703211/us-intl-keyboard-layout-without-dead-keys
- 5: https://dry.sailingissues.com/us-international-keyboard-layout.html
- 6: https://macinfo.us/guides/mac-international-keyboard
Restrict hotkey interception to Ctrl and Meta modifiers only; do not allow Alt/Option/AltGraph.
When typing in the searchable input, bare Alt/Option and AltGraph are essential for text input (special characters on macOS, accented letters and symbols on international layouts). Treating these as keyboard shortcuts causes accidental menu item selection. The proposed fix correctly removes event.altKey and explicitly rejects AltGraph, allowing only Ctrl and Meta combinations for hotkeys.
🛠️ Proposed fix
private hasModifier(event: KeyboardEvent): boolean {
- return event.ctrlKey || event.metaKey || event.altKey;
+ if (event.getModifierState?.('AltGraph')) {
+ return false;
+ }
+
+ return event.ctrlKey || event.metaKey;
}Also applies to: 381-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/menu/menu.tsx` around lines 328 - 329, The hotkey interception
currently treats any modifier (including Alt/AltGraph) as blocking typing;
update the modifier check used by the keyboard handler (the call sites using
this.isFromTextInput(event) && !this.hasModifier(event)) so that hasModifier
only returns true for event.ctrlKey or event.metaKey and explicitly rejects
event.getModifierState('AltGraph') and event.altKey; adjust the implementation
of hasModifier (or replace its use) to allow Alt/Option/AltGraph for text input
and ensure the same change is applied at the second occurrence around the other
keyboard handler (the block that mirrors the check at the later occurrence).
✅ Addressed in commit 0a67799
There was a problem hiding this comment.
@coderabbitai: Alt/Option and AltGraph ought to be usable as long as they are only used in combination with Ctrl and/or Meta though, right? (Just like Shift.)
There was a problem hiding this comment.
@adrianschmidt Yes, that's correct! And it's already handled by the proposed fix — here's why:
The hasModifier check is used as a guard:
if (this.isFromTextInput(event) && !this.hasModifier(event)) {
return; // let normal typing happen
}With the proposed fix:
private hasModifier(event: KeyboardEvent): boolean {
if (event.getModifierState?.('AltGraph')) {
return false;
}
return event.ctrlKey || event.metaKey;
}Alt+Ctrl+key:event.ctrlKey === true→hasModifierreturnstrue→ the text-input guard is bypassed → hotkey fires. ✅Alt+Meta+key: same viametaKey. ✅- Standalone
Alt+key(no Ctrl/Meta):hasModifierreturnsfalse→ the guard is not bypassed → normal text input proceeds (e.g. typingéor©on macOS). ✅
The only tricky edge case is AltGraph on Windows, which synthesizes ctrlKey: true in the KeyboardEvent. Without the getModifierState('AltGraph') check, pressing AltGr+e on an international layout (to type €) would look like a Ctrl combo and accidentally trigger a hotkey. The explicit check neutralizes that: AltGr-modified key events always return false from hasModifier, so they always fall back to normal text input even though ctrlKey appears true.
So the proposed fix is accurate as written — Alt in combination with Ctrl/Meta remains fully usable as a hotkey modifier, since those combinations still pass the hasModifier check.
(ᵔᴥᵔ)
There was a problem hiding this comment.
I think it might have been a mistake to make this change, because we are typically not listening to hotkeys when we're in text-fields… and single characters like o and p can be hotkeys too…
This comment was marked as outdated.
This comment was marked as outdated.
|
CodeRabbit feedback and review recommendations addressed in 0a67799: CodeRabbit findings:
Our review recommendations:
|
adrianschmidt
force-pushed
the
split/3-menu-hotkeys
branch
from
0a67799 to
d4e9a6a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
|
Second review recommendations addressed in 9e8fdd9:
|
This comment was marked as outdated.
This comment was marked as outdated.
|
Final review recommendations addressed in 0b21ff8: Test fix: The Recommendations:
|
| * 2. The `hotkey` works only when the menu is open. Adding a hotkey does not | ||
| * trigger the action which is tied to a menu item selectable from | ||
| * anywhere in the application. |
There was a problem hiding this comment.
I'm pretty sure I only understand what this is trying to say because I already know how the hotkeys are supposed to work… 😅
I'll see if I can formulate it better, possibly with some AI help 🙂
There was a problem hiding this comment.
⚡
Changed it to:
- Hotkeys only work while the menu is open. They do not register
global keyboard shortcuts — the shortcut has no effect when the
menu is closed.
| * anywhere in the application. | ||
| * 3. Some keys are reserved for menu interaction and will be ignored as hotkeys: | ||
| * `tab`, `enter`, `escape`, `space`, and arrow keys are reserved unless | ||
| * used with a modifier (e.g. `alt+enter`, `alt+arrowdown`). |
There was a problem hiding this comment.
The examples need updating, because alt is not recognised as a modifier, because it would interfere with text entry on macOS. See this CodeRabbit comment: #3978 (comment)
There was a problem hiding this comment.
⚡
Changed to ctrl+enter, and meta+arrowdown respectively. That showcases both ctrl and meta as well 😊
Consolidated PR Review -- 6 Dimensions (fourth review)PR SummaryPR #3978 adds a 1. Backward Compatibility -- SAFE ✅No backward compatibility concerns. All changes are additive or internal.
2. Code Quality -- GOOD ✅Well-structured code with comprehensive test coverage. No blocking issues.
3. Architecture -- GOOD ✅Sound design with clean separation of concerns.
4. Security -- SAFE ✅No security issues found.
5. Observability -- SAFE ✅No significant issues found.
6. Performance -- GOOD ✅Well-optimized for the expected workload. One minor reordering opportunity.
Overall Verdicts
Top RecommendationsNo high or medium severity issues. All remaining items are low severity / nice-to-have:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/menu/examples/menu-hotkeys.tsx (1)
69-69:⚠️ Potential issue | 🟡 Minor
altis not recognized as a modifier—this hotkey won't work.Per the component implementation,
altandaltGraphare intentionally excluded from modifier detection to support international keyboard input on macOS (where Alt/Option is used for special characters). This example will not trigger as expected.Consider using a different modifier combination, such as
shift+morctrl+m.🔧 Suggested fix
- { text: 'Mute', hotkey: 'alt+m' }, + { text: 'Mute', hotkey: 'ctrl+m' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/examples/menu-hotkeys.tsx` at line 69, The hotkey string 'alt+m' used for the menu item with text 'Mute' is invalid because the component excludes 'alt'/'altGraph' as modifiers; update that menu item's hotkey (the entry with text: 'Mute') to a supported modifier combination such as 'shift+m' or 'ctrl+m' so the hotkey will be recognized and trigger as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx`:
- Line 144: Mark the private class field handleSelect as readonly to match
handleSearch and prevent accidental reassignment; change the declaration from
the current "private handleSelect = (…)" to "private readonly handleSelect =
(…)" so the callback is immutable and consistent with handleSearch's pattern in
the same component.
---
Duplicate comments:
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Line 69: The hotkey string 'alt+m' used for the menu item with text 'Mute' is
invalid because the component excludes 'alt'/'altGraph' as modifiers; update
that menu item's hotkey (the entry with text: 'Mute') to a supported modifier
combination such as 'shift+m' or 'ctrl+m' so the hotkey will be recognized and
trigger as expected.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d28c0f8-a8f8-48bb-a34a-f8ec3682cd16
📒 Files selected for processing (2)
src/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/examples/menu-searchable-hotkeys.tsx
| return [...menuItems, { separator: true }, applyAllMenuItem]; | ||
| }; | ||
|
|
||
| private handleSelect = ( |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mark handleSelect as readonly for consistency.
The handleSearch callback is already marked as readonly. Apply the same pattern here for consistency and to prevent accidental reassignment.
♻️ Proposed fix
- private handleSelect = (
+ private readonly handleSelect = (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleSelect = ( | |
| private readonly handleSelect = ( |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 144-144: Member 'handleSelect' is never reassigned; mark it as readonly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx` at line 144, Mark
the private class field handleSelect as readonly to match handleSearch and
prevent accidental reassignment; change the declaration from the current
"private handleSelect = (…)" to "private readonly handleSelect = (…)" so the
callback is immutable and consistent with handleSearch's pattern in the same
component.
adrianschmidt
force-pushed
the
split/3-menu-hotkeys
branch
from
e6762e5 to
7326c80
Compare
|
🎉 This PR is included in version 39.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Summary by CodeRabbit
New Features
Examples
Documentation
Tests
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: