Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toolbar: consider to use a roving tabindex and standardize navigation #3383

Closed
afercia opened this issue Nov 8, 2017 · 11 comments
Closed

Toolbar: consider to use a roving tabindex and standardize navigation #3383

afercia opened this issue Nov 8, 2017 · 11 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Milestone

Comments

@afercia
Copy link
Contributor

afercia commented Nov 8, 2017

On current master (eab4b64) keyboard navigation on the top toolbar is broken. Since tabbing is now constrained within the toolbar, there's no way to tab away from it.

[Edit: this has been fixed while posting this issue, see comments below]

Constraining tabbing may make sense when editing a block and then moving focus to the toolbar using Alt+F10. Then, pressing Escape moves focus back to the edited block and there's no keyboard trap. However, when normally tabbing through the page content, this is actually a keyboard trap and should be fixed.

There have been a few experiments trying to improve keyboard navigation and also some progress, but I think there's no a standardized, simple, solid, pattern yet.

There are also some pending issues, see for example:

The first 2 issues refer to the previous floating toolbars but the keyboard navigation issue still applies. Please let me know if you prefer to close the old issues and handle improvements here.

Wondering what an interaction model that is discoverable, predictable, and standard across toolbars, menus, etc. could be. On #3281 the Inserter is going to use the "roving tabindex" technique suggested in the ARIA Authoring Practices. That seems to work pretty well.

See https://www.w3.org/TR/wai-aria-practices/#toolbar
The technique is better described in the example
https://www.w3.org/TR/wai-aria-practices/examples/toolbar/toolbar.html

Basically, there would be just one tab stop in the toolbar. This would be the control that previously had focus or the first control within the toolbar. Any other control has tabindex=-1 and can be navigated with arrow keys only.

This technique is standard and would have some benefits:

  • it reduces the amount of tab stops (there are way too many in Gutenberg)
  • it avoids keyboard traps
  • no need to programmatically handle the Tab key: it should handle just arrow keys and the "selected" item
  • implementing only arrow navigation in this kind of "widgets" would introduce a standardized interaction model: Tab to focus the widget, arrows to focus its controls

The top toolbar content is now "dynamic" and changes based on the selected block, for example:

screen shot 2017-11-08 at 10 13 20

Under the hood, it's made of multiple "toolbar" components. However, arrow navigation should work on the main toolbar as a whole (the highlighted section in the screenshots above).
This also means there should be just one element with role="toolbar" and it could be a wrapper around the toolbar components.

Not sure about the technical side, but I feel the current NavigableMenu (which is already used in the NavigableToolbar) could be adjusted to use just arrow keys and maybe renamed in something like KeyboardNavigation.

Thoughts and discussion very welcome!

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended labels Nov 8, 2017
@youknowriad
Copy link
Contributor

youknowriad commented Nov 8, 2017

@afercia Actually, I just merged a PR improving this. It might not answer everything but there's no tab trap anymore #3303

@afercia
Copy link
Contributor Author

afercia commented Nov 8, 2017

@youknowriad ah I see, thanks! Then, going to remove the bug label, update the title, and keep this open for discussion (will also close the old issues).

@afercia afercia removed the [Type] Bug An existing feature does not function as intended label Nov 8, 2017
@afercia afercia changed the title Toolbar: consider to use a roving tabindex and fix keyboard navigation Toolbar: consider to use a roving tabindex and standardize navigation Nov 8, 2017
@afercia
Copy link
Contributor Author

afercia commented Jan 8, 2018

About this point:

Under the hood, it's made of multiple "toolbar" components. However, arrow navigation should work on the main toolbar as a whole (the highlighted section in the screenshots above).
This also means there should be just one element with role="toolbar" and it could be a wrapper around the toolbar components.

Now that Gutenberg is using React 16, how much work would be refactoring all the toolbars in a way that they return just the buttons/controls without a wrapper and then all the buttons/controls are placed in just one wrapper?
/cc @aduth @youknowriad

@aduth
Copy link
Member

aduth commented Jan 16, 2018

Now that Gutenberg is using React 16, how much work would be refactoring all the toolbars in a way that they return just the buttons/controls without a wrapper and then all the buttons/controls are placed in just one wrapper?

The main blocker would be that we apply a class to the wrapper and assign styles to elements within (including a default border):

<div className={ classnames( 'components-toolbar', className ) }>

.components-toolbar {
margin: 0;
border: 1px solid $light-gray-500;
background-color: $white;
display: inline-flex;
}
div.components-toolbar {
&> div {
display: inline-flex;
margin: 0;
}
&> div + div {
margin-left: -3px;
&.has-left-divider {
margin-left: 6px;
position: relative;
overflow: visible;
}
&.has-left-divider:before {
display: inline-block;
content: '';
box-sizing: content-box;
background-color: $light-gray-500;
position: absolute;
top: 8px;
left: -3px;
width: 1px;
height: $icon-button-size - 16px;
}
}
}

But to the point itself, yes, it is technically possible for the Toolbar component to simply return an array of the control elements.

@afercia
Copy link
Contributor Author

afercia commented Jan 16, 2018

@aduth thanks. If I'm not wrong, I've seen sometimes toolbars have a children? Would that be a blocker?

@aduth
Copy link
Member

aduth commented Jan 16, 2018

@aduth thanks. If I'm not wrong, I've seen sometimes toolbars have a children? Would that be a blocker?

No, should be fine, particularly with React 16.2.0's addition of Fragment, it should work to do:

return (
	<Fragment>
		{ flatMap( controlSets, /* ... */ ) }
		{ children }
	</Fragment>
);

https://reactjs.org/blog/2017/11/28/react-v16.2.0-fragment-support.html

@afercia
Copy link
Contributor Author

afercia commented Jul 16, 2018

Discussed during today's accessibility bug-scrub and agreed to move this issue to Projects, as an enhancement to evaluate for the future.

@mtias mtias added this to the Future: 5.1 Onwards milestone Oct 12, 2018
@mtias mtias added the [Type] Enhancement A suggestion for improvement. label Oct 12, 2018
@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Oct 18, 2019
@gziolo
Copy link
Member

gziolo commented Oct 18, 2019

@diegohaz works on this issue in #17875.

@diegohaz
Copy link
Member

All toolbar blocks are now working with roving tabindex on master. Should we close this?

@gziolo
Copy link
Member

gziolo commented Jun 25, 2020

Yes, let's close. If there are any specific issues, let's tackle them through follow-up issues. Stellar work @diegohaz 🎉

@gziolo gziolo closed this as completed Jun 25, 2020
@afercia
Copy link
Contributor Author

afercia commented Jun 25, 2020

Thanks everyone for the effort made here ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants