Fix keyboard navigation on the Image block toolbar #25127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -12 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
This reverts commit dfde4c7.
diegohaz
requested review from
ajitbohra,
ajlende,
chrisvanpatten,
ellatrix,
nerrad,
ntwb,
Soean,
talldan and
youknowriad
as code owners
diegohaz
commented
Sep 17, 2020
| @@ -21,11 +21,16 @@ | |||
| min-height: $block-toolbar-height; | |||
| margin: 0; | |||
| border: $border-width solid $gray-900; | |||
| border-radius: $radius-block-ui; | |||
There was a problem hiding this comment.
It was used in the legacy toolbar apparently to make toolbar groups rounded, but it seems that it's not needed anymore.
|
|
||
| .wp-block-template-part__preview-dropdown-button { | ||
| border-right: $border-width solid $gray-900; | ||
| } |
There was a problem hiding this comment.
I've updated the code to use ToolbarGroup instead of just a <div>, so these styles that were simulating a toolbar group aren't needed anymore.
Comment on lines
+63
to
+75
| function AspectMenu( { | ||
| toggleProps, | ||
| isDisabled, | ||
| onClick, | ||
| value, | ||
| defaultValue, | ||
| } ) { | ||
| return ( | ||
| <DropdownMenu | ||
| icon={ aspectRatioIcon } | ||
| label={ __( 'Aspect Ratio' ) } | ||
| popoverProps={ POPOVER_PROPS } | ||
| toggleProps={ toggleProps } |
There was a problem hiding this comment.
toggleProps wasn't being passed to this component. That's why it wasn't being considered a ToolbarItem.
| & .components-toolbar.components-toolbar { | ||
| border-width: 0; | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
This snippet is already used in the new toolbar. This code also applies it to the legacy toolbar.
diegohaz
added
[a11y] Keyboard & Focus
[Type] Bug
diegohaz
added
[Package] Block library
Thanks for working on this. However, I'm not sure it's a good pattern because, besides the focus loss, the "roving tabindex" can't work this way. When the toolbar is displayed and users tab back and forth from the block to its toolbar, focus is expected to go on the last button that received focus. If some of these buttons are just removed from the DOM, the "roving tabindex" is just broken. Will comment on the issue with additional considerations. |
priethor
added
the
[Focus] Accessibility (a11y)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #24766
This PR addresses the case where replacing parts of the block toolbar "on the fly" breaks keyboard navigation when the new buttons aren't using
ToolbarItemorToolbarButtoncomponents, like in the Image block toolbar.It should've shown a deprecation warning, but this change had introduced a regression where the block toolbar stopped tracking new elements inside it. The changes in the
navigable-toolbarcomponent are related to this.It doesn't fix the focus loss issue, which is a separate problem that will be fixed in a follow-up PR. Currently, the user would have to press Shift+Tab to go back to the toolbar after clicking on the crop button.
Screenshots
How to test
Site editor
Currently, the block toolbar doesn't support text fields. In the Site Editor, the Template parts toolbar adds a text field asynchronously. Now it fallbacks to the legacy toolbar. It shows a deprecation warning that has been ignored in the tests for now.