[UEPR-445] Ensure topbar is navigable via tab navigation #403
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements keyboard navigation and accessibility improvements for the top navigation bar, enabling users to navigate menu items using Tab and arrow keys. The changes introduce a new context-based system for tracking open menus and managing focus states across nested menu hierarchies.
Key Changes:
- Added
tabIndexattributes and ARIA properties to make menu bar elements keyboard-navigable - Implemented arrow key navigation within dropdown menus (Settings, Language, Theme)
- Created
MenuRefContextto manage the state of open menus and focus tracking across the menu hierarchy
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/scratch-gui/src/components/context-menu/menu-path-context.jsx | New context provider for tracking open menu references and navigation state |
| packages/scratch-gui/src/components/menu-bar/settings-menu.jsx | Converted to class component with keyboard navigation handlers |
| packages/scratch-gui/src/components/menu-bar/language-menu.jsx | Added keyboard navigation with arrow keys and Enter/Escape handlers |
| packages/scratch-gui/src/components/menu-bar/theme-menu.jsx | Added keyboard navigation similar to language menu |
| packages/scratch-gui/src/components/menu/menu.jsx | Added accessibility props (focusedRef, aria attributes, keyboard handlers) |
| packages/scratch-gui/src/components/menu-bar/menu-bar.jsx | Added tabIndex and ARIA attributes to menu bar items for keyboard access |
| packages/scratch-gui/src/components/gui/gui.jsx | Wrapped MenuBar with MenuRefProvider context |
| packages/scratch-gui/src/containers/gui.jsx | Contains commented test code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/scratch-gui/src/components/context-menu/menu-path-context.jsx
Outdated
Show resolved
Hide resolved
| this.context.print(); | ||
| } | ||
|
|
||
| handleOnClose () { |
There was a problem hiding this comment.
uncertain whether the menus should collapse when they are clicked and which ones. For now they just keep their current behaviour.
There was a problem hiding this comment.
I'm not entirely sure whether I understand the question, but we can discuss this further
| ]); | ||
|
|
||
| this.state = {focusedIndex: -1}; | ||
| this.menuRef = props.menuRef; |
There was a problem hiding this comment.
I think it's fine to leave it like this for shorter lines on usage down below. Idk
There was a problem hiding this comment.
If menuRef changes from the props, wouldn't this.menuRef remain the same? Anyway, I think if we switch to function component and just destruct the props, we wouldn't have to do that.
| ref={menuRef} | ||
| aria-label={ariaLabel} | ||
| aria-selected={isSelected ?? null} | ||
| aria-disabled={isDisabled ?? null} |
There was a problem hiding this comment.
So this applies for both aria-selected and aria-disabled.
They seem to require specific roles for both itself and its container, such as role="option" inside role="listbox", or role="tab" inside role="tablist". For some reason I couldn't really get it to work. But it might be due to my limited screen reader. I'll try to figure out why that is.
| ); | ||
|
|
||
| MenuItem.propTypes = { | ||
| menuRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}), |
There was a problem hiding this comment.
This repeats quite a few times. I should probably define it somewhere but I'm not really sure where and whether we have such a file.
There was a problem hiding this comment.
What do you think about creating a prop-types.js file in src/lib ? It seems a bit of an overkill for a single definition, but the usecase doesn't seem that uncommon to me.
There was a problem hiding this comment.
Not a single definition, will do
| id="gui.menuBar.saveAsCopy" | ||
| /> | ||
| ); | ||
| const remixMessage = ( |
There was a problem hiding this comment.
there's a duplicate definition so this can't be the final version of the PR. But I wonder if I should pass it down as a prop?
There was a problem hiding this comment.
Passing it down should be fine
| this.createRef = React.createRef(); | ||
| this.remixRef = React.createRef(); | ||
| this.loadFromComputerRef = React.createRef(); | ||
| this.saveToComputerRef = React.createRef(); |
There was a problem hiding this comment.
The following question applies for the other constructors of custom menus as well.
Should the logic be this manual? Seems fine to me, since we're talking about menu items with specific behavior. Can't really think of a better alternative.
There was a problem hiding this comment.
This is not ideal, but it has the benefit of being very explicit. I'll think a bit on another solution, but it seems good enough for now to me.
| // If we are using hover rather than clicks for submenus, scroll the selected option into view | ||
| if (!this.props.menuOpen && this.selectedRef) { | ||
| this.selectedRef.scrollIntoView({block: 'center'}); | ||
| } |
There was a problem hiding this comment.
Comment is regarding the handleMouseOver method. Should I also focus on the selected language menu item when just hovering? I, personally, don't think that's a good idea.
| this.colorRef = React.createRef(); | ||
| this.itemRefs = [ | ||
| ...(this.props.canChangeLanguage ? [this.languageRef] : []), | ||
| ...(this.props.canChangeTheme && this.props.availableThemesLength > 1 ? [this.themeRef] : []), |
There was a problem hiding this comment.
this logic repeats a lot with item refs, so it is typed twice in every such menu - once here and once in the render method. Perhaps it could be cleaned up with a sort of condition variable, because someone making a change might forget about accessibility.
| onRequestOpen, | ||
| settingsMenuOpen | ||
| }) => { | ||
| const enabledColorModesMap = useMemo(() => Object.keys(colorModeMap).reduce((acc, colorMode) => { |
There was a problem hiding this comment.
I have removed the useMemo usage. It will probably rerender too often this way. It was just not allowed in a render() method, I was just not sure what workaround is available here. I thought it'd be easier to ask and resolve it quickly afterwards.
|
Ok. Also the tests currently fail. I am not sure I understand why, even with Copilot's help. |
| } | ||
|
|
||
| BaseMenu.propTypes = { | ||
| menuRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}), |
There was a problem hiding this comment.
Also it's probably a good idea for me to add .isRequired. I will probably do that after reviews
|
Additionally, the aria-labels don't seem to change after language switch, should be tested with a real screen reader. |
| canChangeColorMode: PropTypes.bool, | ||
| canChangeTheme: PropTypes.bool, | ||
| hasActiveMembership: PropTypes.bool, | ||
| isRtl: PropTypes.bool, |
There was a problem hiding this comment.
some more default values might be good too, I'm guessing.
| import intlShape from '../../lib/intlShape.js'; | ||
|
|
||
| const editMenu = defineMessage({ | ||
| id: 'editMenu.aria.editMenu', |
There was a problem hiding this comment.
the id should probably start with gui.aria. instead of the way it is now. As I said, I will probably change all that after review.
| // eslint-disable-next-line react/jsx-no-bind | ||
| onClick={() => this.props.onChangeLanguage(locale)} | ||
| onClick={() => onChangeLanguage(locale)} | ||
| menuRef={this.itemRefs[index]} |
There was a problem hiding this comment.
I also realize ariaRef is a way more appropriate name here than menuRef since menu items are not menus
|
|
||
| pop () { | ||
| this.setState(prev => ({ | ||
| stack: prev.refStack.slice(0, prev.refStack.length - 1) |
There was a problem hiding this comment.
Shouldn't this be refStack?
|
|
||
| export const MenuRefContext = React.createContext(null); | ||
|
|
||
| export class MenuRefProvider extends React.Component { |
There was a problem hiding this comment.
Since this is a new component, it makes sense to use a function component instead of a class.
| ]); | ||
| } | ||
|
|
||
| push (ref, depth) { |
There was a problem hiding this comment.
I think it would be very helpful for maintainers if you could add comments explaining the intent of each of the push, pop, cut, etc. functions, since it’s currently not easy to infer.
| if (depth <= this.state.refStack.length) { | ||
| this.cut(this.state.refStack[depth - 1]); | ||
| } | ||
|
|
||
| this.setState(prev => ({ | ||
| refStack: [...prev.refStack, ref] | ||
| })); | ||
| } |
There was a problem hiding this comment.
I think the current implementation is a bit unsafe because it assumes that cut completes first and that state updates happen sequentially. React doesn’t guarantee this. Maybe we could create a pure cut function that performs the trimming without updating state, and then use it where needed so each operation results in a single state update?
| } | ||
|
|
||
| handleKeyPressOpenMenu (e) { | ||
| if (e.key === 'ArrowDown') { |
There was a problem hiding this comment.
nitpick: Maybe we can extract the keys into an enum?
| } | ||
|
|
||
| handleMove (direction) { | ||
| const newIndex = (this.state.focusedIndex + direction + this.itemRefs.length) % this.itemRefs.length; |
There was a problem hiding this comment.
nitpick: I think a comment here explaining that this line calculates the next focused item index, moving up or down, and wraps around the list so you never go out of bounds would be helpful for future maintainers.
| [styles.active]: this.isExpanded() | ||
| })} | ||
| onClick={this.handleOnOpen} | ||
| role="button" |
There was a problem hiding this comment.
nitpick: Does it make sense to use an actual button element? Same question for the other menu components.
| } | ||
|
|
||
| EditMenu.propTypes = { | ||
| intl: intlShape.isRequired, |
There was a problem hiding this comment.
In order to use intl from the props you need to call injectIntl to the component, for example:
EditMenuIntl = injectIntl(EditMenu);
export default EditMenuIntl;
Same comment for the other menus where intl is used from the props.
Edit: I just noticed that you're passing it from the MenuBar component. I still think it's better to inject it here rather than passing it from the parent. By the way, if we switch to function components we can use intl from the useIntl hook.
| const SettingsMenu = ({ | ||
| canChangeLanguage, | ||
| canChangeColorMode, | ||
| canChangeTheme, | ||
| hasActiveMembership, | ||
| isRtl, | ||
| isColorModeMenuOpen, | ||
| isThemeMenuOpen, | ||
| activeColorMode, | ||
| onChangeColorMode, | ||
| onRequestOpenColorMode, | ||
| onRequestOpenTheme, | ||
| activeTheme, | ||
| onChangeTheme, | ||
| onRequestClose, | ||
| onRequestOpen, | ||
| settingsMenuOpen | ||
| }) => { |
There was a problem hiding this comment.
I already mentioned this above, but I don’t think we should refactor this component into a class just to fit our inheritance-based approach. It would be better to adopt a more React-idiomatic pattern, such as using function components with composition/hooks.
| import PropTypes from 'prop-types'; | ||
| import bindAll from 'lodash.bindall'; | ||
|
|
||
| export const MenuRefContext = React.createContext(null); |
There was a problem hiding this comment.
Can we leave a comment for how this is used, e.g. "Context for manipulating and accessing active menu element. Ensures there is only one open menu stack at a given time. ... "
package-lock.json
Outdated
| "integrity": "sha512-NLhDfH/h4O6UOy+0LSso42xvYypClINuMNBVVzX4vX98TmTaTUxwRbXdhucbFMd2qLaCTcLq/PdYrvi8onw90w==", | ||
| "dev": true, | ||
| "license": "MIT", | ||
| "peer": true, |
There was a problem hiding this comment.
I'd avoid committing these package-lock changes, given that they aren't any package.json changes.
| username={username} | ||
| accountMenuOptions={accountMenuOptions} | ||
| />} | ||
| {!menuBarHidden && <MenuRefProvider> |
There was a problem hiding this comment.
Do we only need to manage menu refs on the Menu Bar level? If there are other cases where we'll be managing menu refs outside of here, we'd want to move this to an outer level - otherwise it's fine to stay as-is.
There was a problem hiding this comment.
I suppose I should have done it here but I do move it in my next task. Although it is illogical in the long term, I think it's fine to stay there in this PR.
| import bindAll from 'lodash.bindall'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| /* Subclasses must implement (some optionally): |
There was a problem hiding this comment.
This comment is hard to maintain (if a method is added or renamed) so I'd prefer not having it. Instead, maybe the base menu-specific logic (as this component contains just logic, without any presentation) can be extracted in a hook and reused in the different menu components.
See https://react.dev/learn/reusing-logic-with-custom-hooks
There was a problem hiding this comment.
Also let's ensure the name of this hook is related to the accessibility/tab navigation logic it encapsulates
| @@ -0,0 +1,131 @@ | |||
| import {MenuRefContext} from '../context-menu/menu-ref-context'; | |||
There was a problem hiding this comment.
General note - after selecting an item in a menu, and the menu closes, we'd want to keep the focus on the top-level menu element (e.g. keep focus on the "Settings" element after selecting a language
| } | ||
|
|
||
| handleKeyPressOpenMenu (e) { | ||
| if (e.key === 'ArrowDown') { |
There was a problem hiding this comment.
I imagine this would depend on whether the menu is horizontal or vertical? I am fine with not adding support for horizontal menus, given we don't have them, but maybe it'd still be useful to add a comment.
| this.context.print(); | ||
| } | ||
|
|
||
| handleOnClose () { |
There was a problem hiding this comment.
I'm not entirely sure whether I understand the question, but we can discuss this further
|
|
||
| BaseMenu.defaultProps = { | ||
| onClose: () => {}, | ||
| clearOnItemSelect: false |
| onClick={this.props.onRestoreOption(handleRestore)} | ||
| menuRef={this.restoreRef} | ||
| onParentKeyPress={this.handleKeyPressOpenMenu} | ||
| isDisabled={!restorable} |
There was a problem hiding this comment.
Why was this needed to be added? Because the element was still focusable even if it was disabled? Can we do it either entirely true css styles or through the prop - or do we really need both?
There was a problem hiding this comment.
No I just wanted to add aria-disabled and aria-expanded for the menus, I did mention I had problems with doing so in a comment of my own, will try to figure it out.
| this.createRef = React.createRef(); | ||
| this.remixRef = React.createRef(); | ||
| this.loadFromComputerRef = React.createRef(); | ||
| this.saveToComputerRef = React.createRef(); |
There was a problem hiding this comment.
This is not ideal, but it has the benefit of being very explicit. I'll think a bit on another solution, but it seems good enough for now to me.
| id="gui.menuBar.saveAsCopy" | ||
| /> | ||
| ); | ||
| const remixMessage = ( |
There was a problem hiding this comment.
Passing it down should be fine
| ); | ||
|
|
||
| MenuItem.propTypes = { | ||
| menuRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}), |
There was a problem hiding this comment.
What do you think about creating a prop-types.js file in src/lib ? It seems a bit of an overkill for a single definition, but the usecase doesn't seem that uncommon to me.
| // If we are using hover rather than clicks for submenus, scroll the selected option into view | ||
| if (!this.props.menuOpen && this.selectedRef) { | ||
| this.selectedRef.scrollIntoView({block: 'center'}); | ||
| } |
| // eslint-disable-next-line react/jsx-no-bind | ||
| onClick={() => this.props.onChangeLanguage(locale)} | ||
| onClick={() => onChangeLanguage(locale)} | ||
| menuRef={this.itemRefs[index]} |
| {(this.props.canChangeColorMode || this.props.canChangeLanguage || this.props.canChangeTheme) && | ||
| (<SettingsMenu | ||
| menuRef={this.settingsRef} | ||
| depth={1} |
There was a problem hiding this comment.
Would there be any way to avoid passing the depth explicitly, but calculating it based on what's in the context when it's opened (and exposing a method from the context to access it)?
| openThemeMenu, | ||
| closeThemeMenu, | ||
| themeMenuOpen | ||
| closeThemeMenu |
There was a problem hiding this comment.
Are the XMenuOpen reducers redundant now? If that's the case, is there any reason to continue supporting this file at all?
There was a problem hiding this comment.
Yes, they seem to be redundant. I'll remove all the old logic for the ones in the menu bar. We'll eventually delete the file but for now there are still some menus there outside of the scope of the PR
| ]); | ||
| } | ||
|
|
||
| push (ref, depth) { |
There was a problem hiding this comment.
What do you think about naming these closer to how they're used, rather than toward a data structure namespace - so e.g. open, closeCurrent, closeFrom or something with similar semantics?
| > | ||
| <MenuSection> | ||
| <MenuItem | ||
| onClick={onSetMode('NOW')} |
There was a problem hiding this comment.
Since we're not refering these in several files, let's extract them in an enum
| description: 'ARIA label for mode menu' | ||
| }); | ||
|
|
||
| export class ModeMenu extends BaseMenu { |
There was a problem hiding this comment.
Let's make sure that we don't change any of the previous behaviour here.
| this.colorRef = React.createRef(); | ||
| this.itemRefs = [ | ||
| ...(this.props.canChangeLanguage ? [this.languageRef] : []), | ||
| ...(this.props.canChangeTheme && this.props.availableThemesLength > 1 ? [this.themeRef] : []), |
There was a problem hiding this comment.
I noticed that the themes menu is not navigable through the keyboard. This is most likely caused by the fact that availableThemesLength is 1 on the initial render, because the membership data has not loaded from the session yet. Since we are only setting the itemRefs in the constructor, the themes menu never becomes a part of the items. This most likely won't be an issue if we switch to a function component, so that's another reason for doing so 😅
I think the test failures are because |
kbangelov
force-pushed
the
task/uepr-445-ensure-navigable-toolbar
branch
from
081c531 to
d2fd053
Compare
3 participants
Aims to resolve UEPR-445, not before clarifying things.
https://scratchfoundation.atlassian.net/browse/UEPR-445
Proposed Changes
To be discussed additionally
Tip for reviewing this PR
Bugs
Missing