[UEPR-483] "Choose a Sprite" and "Choose a Backdrop" keyboard accessibility#433
Conversation
e37597f to
f2902ce
Compare
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Improves keyboard accessibility for the “Choose a Sprite” / “Choose a Backdrop” action menus in Scratch GUI as part of the accessibility initiative (UEPR-483).
Changes:
- Refactors
ActionMenufrom a class component to a hook-based functional component and adds keyboard navigation/focus handling. - Updates
ActionMenubutton styling to show focus state similar to hover. - Minor test/comment and formatting cleanup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/scratch-gui/test/integration/sounds.test.js | Adds a clarifying comment for the keyboard shortcuts integration test. |
| packages/scratch-gui/src/components/stage-selector/stage-selector.jsx | Removes an extraneous blank line in the moreButtons config. |
| packages/scratch-gui/src/components/action-menu/action-menu.jsx | Implements keyboard navigation + refactors ActionMenu to hooks/memoization. |
| packages/scratch-gui/src/components/action-menu/action-menu.css | Adds :focus styling to match hover for action menu buttons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/scratch-gui/src/components/action-menu/action-menu.jsx
Outdated
Show resolved
Hide resolved
KManolov3
left a comment
There was a problem hiding this comment.
Looks mostly good, left a comment on an edge case. Thanks for migrating the action menu to React hook syntax!
| isOpen: true, | ||
| forceHide: false | ||
| }); | ||
|
|
There was a problem hiding this comment.
Nitpick: there's now an empty line between the comment and the code it's referring to
| }, CLOSE_DELAY); | ||
| }, []); | ||
|
|
||
| const clickDelayer = useCallback( |
There was a problem hiding this comment.
This looks like it slightly differs in behavior from production:
- on production, when we open and close the sprite modal, the action menu is closed
- with this change, when we open and close the sprite modal, the action menu is still open
I am not averse to the updated behavior, as it aligns with the "return focus to previously selected element when you navigate out of a modal".
However there are some visually unpleasant cases:
- if I open the "Upload a sprite" menu from the action button, the menu closes after a timeout, but if I navigate out of the upload dialog which it opens, it reopens the menu, but the label shows on the bottom
- same for other actions
I am open to suggestions for what the most consistent behavior (and the least effort change) would be.
As a fallback, keeping the production behavior (and possibly making sure the inner menu items are also unselected, so that no inconsistent label appears) can be an option for this case (so, blur out of the focused action menu, but keep the focus, so that TAB would return to the same / next item).
There was a problem hiding this comment.
Additionally, the behavior when clicking "Surprise" has changed here - it unfocuses the action menu entirely, while on production it blurs, but keeps the focus on the menu item.
Resolves
https://scratchfoundation.atlassian.net/browse/UEPR-483
Proposed Changes
Reason for Changes
Part of accessibility initiative for Scratch.