Reveal the Continue Writing controls also on focus #1941
Conversation
afercia
added
the
[Focus] Accessibility (a11y)
There was a problem hiding this comment.
Question: Should we be rendering the Text and Image buttons at all when showContinueWritingControls is false? Currently for example when shift-tabbing from "Document" in the Settings sidebar, it focuses Image... but prior to tab the controls weren't visible, so I might have expected the "+" button to be focused instead.
I think we may also want to break out "Continue writing" behavior into its own subcomponent, maybe here or in a subsequent pull request.
| className={ continueWritingClassname } | ||
| onFocus={ this.onContinueWritingFocus } | ||
| onBlur={ this.onContinueWritingBlur } | ||
| > | ||
| <Inserter position="top right" /> | ||
| <button |
There was a problem hiding this comment.
Aside: This should probably just be using the <IconButton /> component.
There was a problem hiding this comment.
Tried that, and seems the IconButton uses some default focus style that maybe is not appropriate here. Instead, it should maybe use the same focus style of the items in the inserter menu?
There was a problem hiding this comment.
We could override the outline style in this specific case, or if it's something that would be common, perhaps a prop on IconButton (focusOutline={ false }).
| @@ -214,6 +231,9 @@ class VisualEditorBlockList extends Component { | |||
| ...blocks.slice( insertionPointIndex + 1 ), | |||
| ] | |||
| : blocks; | |||
| const continueWritingClassname = classnames( 'editor-visual-editor__continue-writing', { | |||
| 'show-controls': this.state.showContinueWritingControls, | |||
There was a problem hiding this comment.
Minor: To reduce chance for naming conflicts, might be good to prefix the modifier class with something distinctive for modifiers, like is-showing-controls.
| <div className="editor-visual-editor__continue-writing"> | ||
| <div | ||
| className={ continueWritingClassname } | ||
| onFocus={ this.onContinueWritingFocus } |
There was a problem hiding this comment.
Minor: While it has the downside of not being a shared function reference (bypassing benefit if we ever adopted PureComponent), we could consolidate onFocus and onBlur toggle to a single helper function:
toggleContinueWritingControls( showContinueWritingControls ) {
return () => this.setState( { showContinueWritingControls } );
}
// ...
<div
className={ continueWritingClassname }
onFocus={ this.toggleContinueWritingControls( true ) }
onBlur={ this.toggleContinueWritingControls( false ) }
>
|
Well if you ask me, I'd make these controls always visible. Not just for a11y reasons but also because I'm not sure revealing UI controls on hover is a good idea in the first place. Hover is going to die (touch devices...). |
|
Sure, thanks! |
|
Looks good to me, just one small thing: since now the buttons are |
This PR tries to make the "Continue Writing" controls at the bottom of the editor...
revealed also on focus when navigating content with a keyboard.
Also, adds an
aria-labelto the Text and Image button since they're out of the context of the inserter menu, they're announced by screen readers just asbutton Textandbutton Imageand need to be clarified a bit.Fixes #1939