-
Notifications
You must be signed in to change notification settings - Fork 0
feature: Add Keyboard shortcuts and Focus management to uni-dom #403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add two complementary features for building accessible, keyboard-navigable UIs: Keyboard Shortcuts: - Global shortcuts with Keyboard.bind() and Keyboard.bindAll() - Scoped shortcuts with Keyboard.scoped() for element-specific bindings - Modifier key tracking with Keyboard.modifiers and Keyboard.isPressed() - KeyCombination parsing for flexible shortcut strings (ctrl+s, cmd+k, etc.) Focus Management: - Focus trapping with Focus.trap() for modals and dialogs - Auto-focus on mount with Focus.onMount and Focus.onMountDelayed() - Focus restoration with Focus.saveAndRestore() and Focus.withRestoration() - Active element tracking with Focus.active reactive stream - Programmatic focus via Focus.focusById() and Focus.blur() - Utility for finding focusable elements with Focus.getFocusableElements() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @xerial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces valuable keyboard shortcut and focus management features to uni-dom. The code is well-structured, follows existing patterns, and includes a good set of tests. I've provided a couple of suggestions to enhance the robustness of the focus management logic by considering element visibility and to strengthen a related test case. Overall, this is an excellent contribution.
| ( | ||
| dom.document.activeElement == dom.document.body || dom.document.activeElement == testInput | ||
| ) shouldBe true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test assertion is a bit weak. If Focus.blur() has no effect, dom.document.activeElement would remain testInput, and the overall condition ... || dom.document.activeElement == testInput would be true, causing the test to pass incorrectly.
A stronger test would explicitly assert that the focus is no longer on the input element and has moved to the body element.
(dom.document.activeElement == testInput) shouldBe false
// After blur, activeElement should be body (or null in some browsers)
dom.document.activeElement shouldBe dom.document.body| "button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), " + | ||
| "textarea:not([disabled]), [tabindex]:not([tabindex=\"-1\"]):not([disabled])" | ||
| val nodeList = container.querySelectorAll(selector) | ||
| (0 until nodeList.length).map(nodeList(_)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getFocusableElements doesn't account for element visibility. Hidden elements (e.g., with display: none or visibility: hidden) will be included in the focusable list, but users cannot interact with them. This can cause Focus.trap to attempt to focus an invisible element, leading to a poor user experience.
To make this more robust, you should filter out elements that are not visible. A common technique is to check if the element has a positive offsetWidth or offsetHeight.
(0 until nodeList.length).map(nodeList(_)).filter {
case elem: dom.HTMLElement =>
elem.offsetWidth > 0 || elem.offsetHeight > 0 || elem.getClientRects().length > 0
case _ =>
// For non-HTMLElements (like SVG), assume they are visible if selected.
true
}- Fix RxElement.empty infinite recursion by returning Embedded(DomNode.empty) - Focus trap now focuses container when no focusable children exist Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Strengthen Focus.blur test to verify focus actually moves away from input - Filter invisible elements (display:none, visibility:hidden) from focusable list Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Keyboardobject for global and scoped keyboard shortcut handling with modifier key trackingFocusobject for accessibility features: focus trapping, auto-focus, focus restoration, and active element trackingRxElement.emptyfor convenience when creating empty reactive elementswvlet.uni.dom.allFeatures
Keyboard Shortcuts
Focus Management
Test plan
🤖 Generated with Claude Code