[UEPR-471] Ensure Library modals are navigable through tab navigation #416
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances keyboard accessibility in the Scratch Editor's library modals by implementing comprehensive tab navigation and focus management. The changes support the broader initiative to make the Scratch Editor fully navigable via keyboard.
Changes:
- Added
ModalFocusContextto capture and restore focus when modals open/close - Implemented keyboard event handlers (Enter/Space) for library items
- Made all modal elements keyboard-accessible with proper
tabIndexand ARIA attributes
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/scratch-gui/src/lib/navigation-keys.js | Defines keyboard key constants for consistent navigation handling |
| packages/scratch-gui/src/contexts/modal-focus-context.jsx | Implements focus restoration context for modal interactions |
| packages/scratch-gui/src/containers/target-pane.jsx | Captures focus when opening sprite library |
| packages/scratch-gui/src/containers/stage-selector.jsx | Captures focus when opening backdrop library |
| packages/scratch-gui/src/containers/sound-tab.jsx | Wraps sound library click handler to capture focus |
| packages/scratch-gui/src/containers/library-item.jsx | Changes from onKeyPress to onKeyDown with Enter/Space handling |
| packages/scratch-gui/src/containers/costume-tab.jsx | Captures focus when opening costume/backdrop libraries |
| packages/scratch-gui/src/components/tag-button/tag-button.css | Adds focus-visible outline with offset to prevent cropping |
| packages/scratch-gui/src/components/library/library.jsx | Restores focus when closing library modal |
| packages/scratch-gui/src/components/library-item/library-item.jsx | Updates event handlers and adds keyboard accessibility attributes |
| packages/scratch-gui/src/components/gui/gui.jsx | Wraps main UI with ModalFocusProvider |
| packages/scratch-gui/src/components/filter/filter.jsx | Converts clear button to focusable element with accessibility improvements |
| packages/scratch-gui/src/components/filter/filter.css | Removes default button styling for clear button |
| packages/scratch-gui/src/components/extension-button/extension-button.jsx | Captures focus when opening extensions modal |
| packages/scratch-gui/src/components/extension-button/extension-button.css | Adds focus-visible outline styling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/scratch-gui/src/components/extension-button/extension-button.jsx
Outdated
Show resolved
Hide resolved
|
I like the UX |
packages/scratch-gui/src/components/library-item/library-item.jsx
Outdated
Show resolved
Hide resolved
|
|
||
| const captureFocus = useCallback(() => { | ||
| lastFocusedElement.current = document.activeElement; | ||
| }, []); |
There was a problem hiding this comment.
Nitpick: Isn't lastFocusedElement still technically a dependency?
There was a problem hiding this comment.
Technically, yes, but lastFocusedElement is a ref, and changing .current wouldn’t trigger a re-render. And the ref object itself doesn’t change, so it shouldn’t affect anything. Or am I missing something? 😅
| const initAudioContext = () => { | ||
| document.removeEventListener('mousedown', initAudioContext); | ||
| document.removeEventListener('touchstart', initAudioContext); | ||
| document.removeEventListener('keydown', initAudioContext); |
There was a problem hiding this comment.
You can now navigate to the Sound Editor without either mousedown or touchstart firing
KManolov3
left a comment
KManolov3
left a comment
There was a problem hiding this comment.
The latest changes make sense to me. It's unfortunate that the logic is as custom for every feature callout, but addressing that will be a larger effort.
| if (!isExtensionButtonVisible) return; | ||
|
|
||
| if (e.type === 'keydown') { | ||
| // Prevent focus from jumping to the next element in the tab order after keydown finishes |
3 participants
Resolves
UEPR-471
Proposed Changes
TabnavigationTODOs:
Reason for Changes