[LUM-654] Implement M3 keyboard focus system for VMenu components#23277
Conversation
- Convert VMenuCoordinator to @observable for reactive focus state tracking - Add UUID-based item registration system (VMenuItemRegistrationKey preference key) - Implement keyboard activation (Enter/Space) and submenu opening (right arrow) - Add VoiceOver bridge: post focusedUIElementChanged on keyboard focus moves - Install mouse move monitor to clear keyboard focus (matches native NSMenu) - Add visual keyboard focus highlight to VMenuItem, VSubMenuItem, and VNavItem - Add VMenuItemNSViewCapture (NSViewRepresentable) for VoiceOver NSView tracking - Inject panel level via environment for multi-level keyboard navigation - Add isKeyboardFocused parameter to VNavItem for .regular size menu items Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
LUM-654 Accessibility for VMenu and VSubmenu
Ensure that the new VMenu and VSubMenu and VMenuItem components are following best accessibility practices |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
|
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
5 similar comments
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Swift does not support conditional compilation directives inside function parameter lists or call sites. Make isKeyboardFocused available on all platforms (defaults to false, no-op on iOS) instead of conditionally compiling it. Use an immediately-invoked closure for the VMenu call site to conditionally pass the value. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
Three fixes for arrow keys not highlighting menu items: 1. Mouse move monitor debounce: Ignore mouse movements within 200ms of a keyboard event to prevent trackpad/mouse micro-jitter from clearing the focus highlight before SwiftUI renders it. 2. Belt-and-suspenders item registration: registerItemAction() now also appends the item to itemOrder/itemCounts as a fallback, ensuring moveFocus() has a non-zero count even if onPreferenceChange hasn't fired yet. 3. Timestamp tracking: handleKeyDown() records lastKeyboardEventTime before processing, used by the mouse move debounce guard. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
The panel's keyDown override was not receiving key events — likely because NSHostingView intercepts them in the responder chain, or addChildWindow changes key window status away from the panel. Fix: Install NSEvent.addLocalMonitorForEvents(matching: .keyDown) on the coordinator when the root panel opens. This intercepts key events BEFORE the responder chain, the same approach native NSMenu uses. The monitor returns nil to consume handled events (arrow keys, Enter, Space) and passes through all other keys. Reference: https://developer.apple.com/documentation/appkit/nsevent/addlocalmonitorforevents(matching:handler:) Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
…vigation Root cause: previous attempts used AppKit NSEvent monitors to catch key events then tried to bridge state changes back to SwiftUI via @observable or Combine. This AppKit→SwiftUI bridge is fundamentally fragile — SwiftUI may not re-render when coordinator properties change through external event monitors. Fix: Use Apple's recommended approach from WWDC23 'The SwiftUI cookbook for focus' (https://developer.apple.com/videos/play/wwdc2023/10162/): 1. VMenu's VStack is .focusable() with .onKeyPress() handlers — key events are handled entirely within SwiftUI's rendering cycle. 2. Focus state is VMenu's @State focusedItemID — guaranteed re-renders. 3. focusedItemID flows to children via environment (vMenuFocusedItemID). 4. VMenuItem/VSubMenuItem read from environment — no Combine needed. 5. NSEvent keyboard monitor only tracks timing (for mouse-move debounce), does NOT consume events. 6. VMenuPanel.keyDown remains as fallback if VMenu loses focus — it calls coordinator.handleKeyDown which publishes via Combine, and VMenu subscribes to update its @State. References: - https://developer.apple.com/videos/play/wwdc2023/10162/ - https://developer.apple.com/documentation/swiftui/view/focusable(_:interactions:) - https://developer.apple.com/documentation/swiftui/view/onkeypress(_:action:)-14rhi Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…nAppear Root cause: VMenuPanel.show() calls registerRootPanel() AFTER NSHostingView triggers SwiftUI layout, which means .onAppear has already fired and registered item actions. registerRootPanel() then clears itemActions/submenuActions/itemNSViews, permanently wiping the registered actions (.onAppear only fires once). Up/Down worked because VMenu uses its own @State registeredIDs (populated by async onPreferenceChange which fires after registerRootPanel). But Enter/Space/Right/Left depend on coordinator.itemActions which was being wiped. Fix: Only reset transient navigation state (focusedIndex) in registerRootPanel. The coordinator is freshly created per panel tree, so action dictionaries start empty and don't need clearing. Also: Change keyboard focus highlight from Color.accentColor (blue) to VColor.systemPositiveWeak (light green design system token). Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ld auto-focus, remove debug prints - Block keyboard activation (Enter/Space) for disabled menu items by tracking isEnabled state per item in the coordinator (#1, #2) - Include panel level in focusChangeSubject so fallback-path focus changes only affect the correct VMenu instance, preventing parent corruption (#4) - Replace synchronous moveFocus into child (which raced SwiftUI preference propagation) with a pendingChildFocus flag consumed by the child VMenu's .task after layout settles (#3, #6) - Remove all print("[VMenuKeyboard]...") debug statements (#5) - Forward isKeyboardFocused in VNavItemTrailingIcon convenience init (#7) Co-Authored-By: Claude <noreply@anthropic.com>
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.
…m enabled state, fix mouse monitor - Add isItemEnabled check to right-arrow paths (both primary openMenuSubmenu and fallback handleKeyDown case 124) so disabled VSubMenuItems can't be opened via keyboard - Register VSubMenuItem enabled state with coordinator on appear and onChange(of: isEnabled), matching VMenuItem's existing pattern - Set acceptsMouseMovedEvents = true on VMenuPanel so the coordinator's local mouse-move monitor fires when the cursor is over the panel, allowing keyboard focus to be properly cleared Co-Authored-By: Claude <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
VMenuItem and VSubMenuItem icons now use VColor.contentDisabled when the item is disabled, matching the text color behavior. Previously icons kept their normal color (primaryBase/primaryActive) even when disabled, making it harder to visually identify disabled items. Co-Authored-By: Claude <noreply@anthropic.com>
Add `guard !Task.isCancelled else { return }` after `Task.sleep` to
prevent stale state mutation when the view is dismissed before the
50ms delay completes. Without this, a cancelled task could consume
the pendingChildFocus flag meant for a subsequent child menu.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4349584b95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard !Task.isCancelled else { return } | ||
| isMenuFocused = true | ||
| // If this child menu was opened via keyboard (→ arrow), auto-focus the first item. | ||
| if panelLevel > 0, coordinator?.consumePendingChildFocus() == true, !registeredIDs.isEmpty { |
There was a problem hiding this comment.
consumePendingChildFocus() is evaluated before the !registeredIDs.isEmpty guard, so the pending flag is cleared even when the submenu has no registered rows yet. If a submenu opens from keyboard navigation while its items register later (for example, delayed/conditional content), the first-item auto-focus is permanently skipped and arrow/enter handling starts with no focused row. Check registeredIDs first (or retry on registration change) before consuming the pending focus flag.
Useful? React with 👍 / 👎.
1 participant
Summary
Completes the partially scaffolded keyboard navigation system in
VMenuCoordinatorso that VMenu panels support full arrow-key navigation, Enter/Space activation, right-arrow submenu opening, and VoiceOver bridging — matching nativeNSMenukeyboard behavior.Architecture: SwiftUI-native focus (WWDC23)
After multiple iterations where AppKit-level event interception (
NSEventmonitors) failed to reliably bridge state changes back to SwiftUI, this PR adopts Apple's recommended approach from WWDC23 "The SwiftUI cookbook for focus":VMenu's VStack is.focusable()with.onKeyPress()handlers. Key events are processed entirely within SwiftUI's rendering cycle, and focus state is a@State focusedItemID: UUID?— guaranteed to trigger re-renders.VMenuPanel.keyDowncatches events via the responder chain and publishes via Combine (focusChangeSubject/clearFocusSubject), which VMenu subscribes to with.onReceive.focusedItemIDflows from VMenu to children via\.vMenuFocusedItemIDenvironment key.VMenuItemandVSubMenuItemderiveisKeyboardFocusedas a computed property (menuFocusedItemID == itemID), so highlighting is reactive without any Combine subscriptions in items.Key changes by file:
VMenuCoordinator: Adds UUID-based item registration (itemOrder,itemActions,submenuActions,itemNSViews), Combine subjects for the fallback focus-change path, keyboard activation for Enter/Space/→, a mouse-move monitor with 200ms debounce after key events, and a VoiceOver bridge viaNSAccessibility.post(element:notification:). The local keyboard monitor tracks event timing only — it does not consume events.VMenu.swift: AddsVMenuPanelLevelKeyandVMenuFocusedItemIDKeyenvironment keys,VMenuItemRegistrationKeypreference key for item ordering,VMenuItemNSViewCapture(NSViewRepresentable) for VoiceOver NSView tracking, and the.focusable()+.onKeyPress()handler with navigation helpers.VMenuItemandVSubMenuItemread focus state from environment and register themselves with the coordinator on appear.VNavItem.swift: AddsisKeyboardFocusedparameter (cross-platform, defaults tofalse) for keyboard focus highlight. On iOS the parameter is a no-op. Extracts background color into anavItemBackgroundcomputed property.VMenuPanel.swift: InjectsvMenuPanelLevelenvironment (0 for root,panels.countfor child) so items know their nesting depth.All changes are additive — existing mouse interaction, hover behavior, and visual appearance are unchanged.
Review & Testing Checklist for Human
#if os(macOS)boundary correctness,@FocusStateavailability..focusable()+.onKeyPress()works inside NSPanel. The core bet is that SwiftUI's focus system works when hosted in anNSHostingViewthat is thecontentViewof anNSPanel(not a standardNSWindow). The.tasksetsisMenuFocused = trueafter 50ms — confirm this actually grants focus..onAppearand never re-registered. If an action closure captures state that changes after appear (e.g., a binding), the registered action may be stale. Verify activation triggers the correct/current action.findAccessibleElement(from:)walks up the NSView hierarchy to find the accessible element; verify it targets the correct element.print("[VMenuKeyboard] ...")diagnostic statements. These should be removed or gated behind aDEBUGflag before shipping.Notes
VMenuItemNSViewCaptureis a zero-frame invisibleNSViewembedded viaNSViewRepresentable. Its sole purpose is to capture a view reference for VoiceOver'sNSAccessibility.post(element:notification:). It is markedsetAccessibilityElement(false)to stay out of the accessibility tree.NSEvent.addLocalMonitorForEvents(matching: .mouseMoved)) fires for all mouse movement in the app, not just over the menu panel. A 200ms debounce after the last keyboard event prevents trackpad micro-jitter from immediately clearing focus. This matches native NSMenu behavior where mouse movement exits keyboard mode.WeakNSViewRefwraps NSView references to avoid retain cycles between the coordinator and the view hierarchy.isKeyboardFocusedis a public property onVNavItemon all platforms (not conditionally compiled) because Swift does not support#ifdirectives inside function parameter lists. On iOS it defaults tofalseand has no effect.VMenuFocusedItemIDKeyenvironment key hasinternalaccess (notpublic), which is sufficient since all consumers are within the same DesignSystem module.Link to Devin session: https://app.devin.ai/sessions/f5b6760008ac4fa7867c752eeb3bf0e7
Requested by: @Jasonnnz