[UEPR-445] Ensure topbar is navigable via tab navigation #403
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| import {MenuRefContext} from '../../contexts/menu-ref-context'; | ||
| import React from 'react'; | ||
| import bindAll from 'lodash.bindall'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| /* Subclasses must implement (some optionally): | ||
|
Contributor
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.
Contributor
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 |
||
| _______________________________________________ | ||
| render | ||
| define this.itemRefs | ||
| add onKeyDown={this.handleKeyPress} | ||
| and onParentKeyPress={this.handleKeyPressOpenMenu} for MenuItem elements | ||
|
|
||
| and replace isOpenMenu-like props with this.isExpanded() checks | ||
|
|
||
| They should also receive: | ||
| ______________________ | ||
| onOpen, | ||
| onClose, | ||
| menuRef, | ||
| depth | ||
| */ | ||
| class BaseMenu extends React.PureComponent { | ||
|
Contributor
There was a problem hiding this comment. I’d like to hear @KManolov3's thoughts on this as well, but I'd suggest avoiding class inheritance here (and overall using class components). React’s design encourages composition over subclassing, so shared behavior can be handled with hooks, generic components, etc. This reduces the risk of lifecycle conflicts and makes the code easier to maintain. |
||
| constructor (props) { | ||
| super(props); | ||
| bindAll(this, [ | ||
| 'handleKeyPress', | ||
| 'handleKeyPressOpenMenu', | ||
| 'handleMove', | ||
| 'handleOnOpen', | ||
| 'handleOnClose', | ||
| 'refocusRef', | ||
| 'refocusItemByIndex', | ||
| 'isExpanded' | ||
| ]); | ||
|
|
||
| this.state = {focusedIndex: -1}; | ||
| this.menuRef = props.menuRef; | ||
|
Contributor
Author
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
Contributor
There was a problem hiding this comment. If |
||
| } | ||
|
|
||
| static contextType = MenuRefContext; | ||
|
|
||
| refocusRef (ref) { | ||
| if (ref && ref.current) { | ||
| ref.current.focus(); | ||
| } | ||
| } | ||
|
|
||
| refocusItemByIndex (index) { | ||
| this.setState({focusedIndex: index}, () => { | ||
| this.refocusRef(this.itemRefs[index]); | ||
| }); | ||
| } | ||
|
|
||
| handleKeyPress (e) { | ||
| if (this.props.depth === 1) { | ||
| if (e.key === 'Tab') { | ||
|
Contributor
There was a problem hiding this comment. I think we also want to handle "Shift + Tab" - it should move to the previous top level element |
||
| this.handleOnClose(); | ||
| this.context.clear(); | ||
| } | ||
| } | ||
|
|
||
| if (this.context.isTopMenu(this.menuRef)) { | ||
| this.handleKeyPressOpenMenu(e); | ||
| } else if (!this.isExpanded() && (e.key === ' ' || (e.key === 'ArrowRight' && this.props.depth !== 1))) { | ||
|
Contributor
There was a problem hiding this comment. Why
Contributor
There was a problem hiding this comment. Let's extract the relevant keys in a named enum and use them from there for better maintainability |
||
| e.preventDefault(); | ||
| this.handleOnOpen(); | ||
| } | ||
| } | ||
|
|
||
| handleKeyPressOpenMenu (e) { | ||
| if (e.key === 'ArrowDown') { | ||
|
Contributor
There was a problem hiding this comment. nitpick: Maybe we can extract the keys into an
Contributor
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. |
||
| e.preventDefault(); | ||
| this.handleMove(1); | ||
| } | ||
| if (e.key === 'ArrowUp') { | ||
| e.preventDefault(); | ||
| this.handleMove(-1); | ||
| } | ||
| if (e.key === 'Enter' && this.props.clearOnItemSelect) { | ||
| this.context.clear(); | ||
| } | ||
| if (e.key === 'ArrowLeft' || e.key === 'Escape') { | ||
| e.preventDefault(); | ||
| this.handleOnClose(); | ||
| } | ||
| } | ||
|
|
||
| handleOnOpen () { | ||
| if (this.context.isOpenMenu(this.menuRef)) return; | ||
|
|
||
| this.props.onOpen(); | ||
| this.refocusItemByIndex(0); | ||
|
|
||
| this.context.push(this.menuRef, this.props.depth); | ||
| } | ||
|
|
||
| handleMove (direction) { | ||
| const newIndex = (this.state.focusedIndex + direction + this.itemRefs.length) % this.itemRefs.length; | ||
|
Contributor
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. |
||
| this.setState({focusedIndex: newIndex}, () => { | ||
| this.refocusRef(this.itemRefs[newIndex]); | ||
| }); | ||
| } | ||
|
|
||
| handleOnClose () { | ||
|
Contributor
Author
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.
Contributor
There was a problem hiding this comment. I'm not entirely sure whether I understand the question, but we can discuss this further |
||
| this.context.cut(this.menuRef); | ||
| this.setState({focusedIndex: -1}, () => { | ||
| this.refocusRef(this.menuRef); | ||
| }); | ||
|
|
||
| this.props.onClose(); | ||
| } | ||
|
|
||
| isExpanded () { | ||
| return this.context.isOpenMenu(this.menuRef); | ||
| } | ||
| } | ||
|
|
||
| BaseMenu.propTypes = { | ||
| menuRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}), | ||
|
Contributor
Author
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 |
||
| depth: PropTypes.number, | ||
| onOpen: PropTypes.func, | ||
| onClose: PropTypes.func, | ||
| clearOnItemSelect: PropTypes.bool | ||
| }; | ||
|
|
||
| BaseMenu.defaultProps = { | ||
| onClose: () => {}, | ||
| clearOnItemSelect: false | ||
|
|
||
| }; | ||
|
|
||
| export default BaseMenu; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import React from 'react'; | ||
| import styles from './menu-bar.css'; | ||
| import classNames from 'classnames'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| import editIcon from './icon--edit.svg'; | ||
| import {FormattedMessage, defineMessage} from 'react-intl'; | ||
| import MenuBarMenu from './menu-bar-menu.jsx'; | ||
| import {MenuItem, MenuSection} from '../menu/menu.jsx'; | ||
| import BaseMenu from './base-menu'; | ||
| import dropdownCaret from './dropdown-caret.svg'; | ||
| import DeletionRestorer from '../../containers/deletion-restorer.jsx'; | ||
| import TurboMode from '../../containers/turbo-mode.jsx'; | ||
| import intlShape from '../../lib/intlShape.js'; | ||
|
|
||
| const editMenu = defineMessage({ | ||
| id: 'editMenu.aria.editMenu', | ||
|
Contributor
Author
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. |
||
| defaultMessage: 'Edit menu', | ||
| description: 'ARIA label for edit menu' | ||
| }); | ||
|
|
||
| export class EditMenu extends BaseMenu { | ||
|
Contributor
There was a problem hiding this comment. Since we're adding this file, let's make use of the newer, functional-component react syntax. |
||
| constructor (props) { | ||
| super(props); | ||
|
|
||
| this.restoreRef = React.createRef(); | ||
| this.turboRef = React.createRef(); | ||
|
|
||
| this.itemRefs = [ | ||
| this.restoreRef, | ||
| this.turboRef | ||
| ]; | ||
| } | ||
|
|
||
| render () { | ||
| return ( | ||
| <div | ||
| className={classNames(styles.menuBarItem, styles.hoverable, { | ||
| [styles.active]: this.isExpanded() | ||
| })} | ||
| onClick={this.handleOnOpen} | ||
| role="button" | ||
|
Contributor
There was a problem hiding this comment. nitpick: Does it make sense to use an actual |
||
| aria-label={this.props.intl.formatMessage(editMenu)} | ||
| aria-expanded={this.isExpanded()} | ||
| tabIndex={0} | ||
| onKeyDown={this.handleKeyPress} | ||
| > | ||
| <img src={editIcon} /> | ||
| <span className={styles.collapsibleLabel}> | ||
| <FormattedMessage | ||
| defaultMessage="Edit" | ||
| description="Text for edit dropdown menu" | ||
| id="gui.menuBar.edit" | ||
| /> | ||
| </span> | ||
| <img src={dropdownCaret} /> | ||
| <MenuBarMenu | ||
| className={classNames(styles.menuBarMenu)} | ||
| open={this.isExpanded()} | ||
| place={this.props.isRtl ? 'left' : 'right'} | ||
| onRequestClose={this.handleOnClose} | ||
| > | ||
| <DeletionRestorer>{(handleRestore, {restorable, deletedItem}) => ( | ||
| <MenuItem | ||
| className={classNames({[styles.disabled]: !restorable})} | ||
| onClick={this.props.onRestoreOption(handleRestore)} | ||
| menuRef={this.restoreRef} | ||
| onParentKeyPress={this.handleKeyPressOpenMenu} | ||
| isDisabled={!restorable} | ||
|
Contributor
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? |
||
| > | ||
| {this.props.restoreOptionMessage(deletedItem)} | ||
| </MenuItem> | ||
| )}</DeletionRestorer> | ||
| <MenuSection> | ||
| <TurboMode>{(toggleTurboMode, {turboMode}) => ( | ||
| <MenuItem | ||
| onClick={toggleTurboMode} | ||
| menuRef={this.turboRef} | ||
| onParentKeyPress={this.handleKeyPressOpenMenu} | ||
| > | ||
| {turboMode ? ( | ||
| <FormattedMessage | ||
| defaultMessage="Turn off Turbo Mode" | ||
| description="Menu bar item for turning off turbo mode" | ||
| id="gui.menuBar.turboModeOff" | ||
| /> | ||
| ) : ( | ||
| <FormattedMessage | ||
| defaultMessage="Turn on Turbo Mode" | ||
| description="Menu bar item for turning on turbo mode" | ||
| id="gui.menuBar.turboModeOn" | ||
| /> | ||
| )} | ||
| </MenuItem> | ||
| )}</TurboMode> | ||
| </MenuSection> | ||
| </MenuBarMenu> | ||
| </div> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| EditMenu.propTypes = { | ||
| intl: intlShape.isRequired, | ||
|
Contributor
There was a problem hiding this comment. In order to use Same comment for the other menus where 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 |
||
| menuRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}), | ||
| isRtl: PropTypes.bool, | ||
| restoreOptionMessage: PropTypes.func, | ||
| onRestoreOption: PropTypes.func | ||
| }; | ||
|
|
||
| export default EditMenu; | ||
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.
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.