Refactor MessageActionBar w/ARIA Toolbar pattern #4827
Conversation
Signed-off-by: Mike Pennisi <[email protected]>
|
Can you do a DCO sign-off in the PR description as one of your commits lacks one, thanks :) |
| const current = buttonNames.indexOf(this.state.focused); | ||
| let newIndex = null; | ||
|
|
||
| if (key === Key.ARROW_UP || key === Key.ARROW_DOWN) { |
There was a problem hiding this comment.
Hmm, this seems to disagree with https://www.w3.org/TR/wai-aria-practices-1.1/examples/toolbar/toolbar.html
Looks like the "up" and "down" arrows should only perform a click for targets with |
|
So the codebase already has some classes to help out with keeping track of RovingTabIndex. It'd be nice to use that and make a generic Toolbar component which would handle the keypresses between all of its direct children so that we can have one backing solution for both this and #4799
Yes, that's what the aria authoring practices say |
|
Are you saying we should do that work before implementing this feature? |
|
I am saying it'd be nice to write a component which does this for all Toolbars rather than manually having to add these keyboard handlers to every toolbar component manually, means a lot more repeated work like this PR |
|
Totally! I just can't tell if you consider that improvement to be blocking this one. |
|
It'd be a replacement which would replace this one, so whilst this could land, it'd be replaced by this generic toolbar thing |
| componentDidMount() { | ||
| this.props.mxEvent.on("Event.decrypted", this.onDecrypted); | ||
| this.props.mxEvent.on("Event.beforeRedaction", this.onBeforeRedaction); | ||
| this.elRef.current.addEventListener("keydown", this.onKeyDown, true); |
There was a problem hiding this comment.
we should really be using react keydown handlers for this
(note to self mostly, as I am trying to make a generic Toolbar component now)
|
Superseded by above. Thank you for your effort though! |
This is intended to resolve element-hq/element-web#11316
Signed-off-by: Mike Pennisi [email protected]