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

Components: Make Toolbar accessible #17875

Closed
wants to merge 86 commits into from

Conversation

@diegohaz
Copy link
Member

diegohaz commented Oct 10, 2019

Description

Fixes #15331.
Fixes #3383.
Related to #16514.

This PR fixes Toolbar transforming it into an actual toolbar, addressing the accessibility concerns discussed on #15331, and leveraging the design discussions on #16514.

I tried to touch the API as little as possible. But we have a few changes, all backward compatible:

  1. Current Toolbar has been renamed to ToolbarGroup.
  2. New Toolbar needs an accessibilityLabel prop.
  3. New Toolbar requires that its buttons are rendered using @wordpress/components.

1. Current Toolbar has been renamed to ToolbarGroup

The current Toolbar on master still works, but now it will emit a deprecation warning with a suggestion to use ToolbarGroup instead, which name better describes its semantics.

ToolbarGroup has the same implementation as the current Toolbar on master. In fact, most of the code was copied and pasted to the new location.

import { ToolbarGroup } from '@wordpress/components';

<ToolbarGroup>
  ...
</ToolbarGroup>

// or
<ToolbarGroup controls={...} />

// or
<ToolbarGroup isCollapsed controls={...} />

Just like today, <ToolbarGroup isCollapsed /> only accepts controls, not children.

2. New Toolbar needs an accessibilityLabel prop.

To use the new Toolbar, it's necessary to pass an accessibilityLabel prop to the component. Other than that, the API remains the same:

import { Toolbar, ToolbarButton, ToolbarGroup } from '@wordpress/components';

<Toolbar accessibilityLabel="Block options">
  <ToolbarButton icon="thumbs-up" title="Thumbs Up" onClick={...} />
  <ToolbarButton icon="thumbs-down" title="Thumbs Down" onClick={...} />
  <ToolbarGroup>
    <ToolbarButton ... />
    <ToolbarButton ... />
  </ToolbarGroup>
   ...
</Toolbar>

When using Toolbar without accessibilityLabel, the component will fallback to ToolbarGroup and emit a warning.

3. New Toolbar requires that its buttons are rendered using @wordpress/components.

Like Button, IconButton, ToolbarButton etc.

Ideally, only ToolbarButton would be accepted, and it would be flexible enough to render all kinds of toolbar items. Unfortunately, that's not the case for ToolbarButton, and even Gutenberg uses different components as toolbar items to achieve different results.

This means that block authors that aren't using @wordpress/components for toolbar items will see a warning.

import {
  BlockControls,
  Button,
  IconButton,
  ToolbarButton,
  DropdownMenu,
} from '@wordpress/components';

// still works, but will fallback to the old navigable menu
// less accessible, but without breaking changes
<BlockControls>
  <button />
  <button />
  <button />
  <button />
</BlockControls>

// after
<BlockControls>
  <Button />
  <IconButton />
  <ToolbarButton />
  <DropdownMenu />
</BlockControls>

Other stuff

How is this different from NavigableToolbar

NavigableToolbar finds all the tabbable elements within the toolbar whenever the user presses left and arrow keys. It determines which one is active and focuses the previous/next one. To conform with the WAI-ARIA recommendations, it still needs to:

  • set tabIndex="-1" on inactive items so the toolbar has only one tab stop.
  • keep record of the last active item so focus will move back to it the user tabs into the toolbar.
  • listen to PageUp PageDown Home End keys.

The new Toolbar solves all these issues. Also, it registers Buttons when they're mounted and unregisters them when they're unmounted. Because of that, it doesn't need to run a query on the DOM every time an arrow key is pressed, which leads to a better performance.

Reakit

This PR introduces a new dependency: Reakit. It's an open source library that provides low level components and hooks to address common accessibility aspects of design systems. It has 1000s of lines of tests to make sure accessible behaviors work. It's being used by projects like CodeSandbox and Gatsby. And is used here to add the roving tabindex method to Toolbar.

reakit/Toolbar adds around 3.9 kB gzipped to the bundle. My plan is to address higher-level cases like Toolbar, Popover or whatever is more urgent first. I want to see how well Reakit fits into the current @wordpress/components implementation while trying to touch the components' API as little as possible.

Once we have a few high-level components working with proper accessibility and less dependent of @wordpress/components' abstractions, we can go deeper and determine what to do with things like NavigableContainer.

I think that's the path that will give us more flexibility to change things (even removing Reakit later, if needed) at the same time we provide more accessible ready-to-use components faster.

Next steps

As I said, I tried to touch the Toolbar API as little as possible. But this should be improved. ToolbarButton must be more flexible and work for more cases besides only IconButton. So, next steps would be improving Toolbar API. Some discussions are happening on #16514.

How has this been tested?

Checked the header toolbar and the block toolbar of many (if not all) blocks. Many end-to-end tests failed on the first iterations, so I went through each of them and fixed the issues.

Screenshots

Storybook

Oct-26-2019 22-52-06

Playground

Oct-26-2019 23-53-13

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@diegohaz diegohaz changed the title Make Toolbar accessible Components: Make Toolbar accessible Oct 10, 2019
@gziolo gziolo self-requested a review Oct 10, 2019
@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Nov 12, 2019

I see this when setting the full width for the Table block when in the mobile viewport, but this might be present in master

Checked it and it was introduced by this PR. Working on a fix.

Nevermind, looks like it's on master indeed. Trying to find the commit that changed this.

Found it: #17877 (comment)

@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Nov 12, 2019

Can you summarize in the comment what's left so we could decide if there any remaining blockers for this PR?

Marked the comments that were addressed as resolved. The ones missing are generally discussions.

Future tasks I can think of:

  • Improve ToolbarButton so it can be used for more cases
  • Refactor SlotFill
jasmussen added a commit that referenced this pull request Nov 13, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 13, 2019

I see this when setting the full width for the Table block when in the mobile viewport, but this might be present in master:

Hey, Diego is right, #17877 is responsible for this. I created #18469 to fix it.

Copy link
Contributor

ItsJonQ left a comment

@diegohaz Amazing job with this PR!

I tested this locally in Gutenberg with keyboard + voiceover. It works as expected. There were a couple of hiccups, but those interactions already exist in Gutenberg today (unfortunately. We'll make it better!).

Based on the convo with @gziolo , I think a great fast follow PR would be to improve the SlotFIll related forceUpdate work around.

Thank you so much for all of your hard work.
Double thank you for your efforts and advocacy for Reakit and a11y!

Very excited to see how we can systematically improve the experience of Gutenberg as we start refactoring with Reakit within the components.

❤️

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 13, 2019

It's not clear to me yet how this impacts our existing public APIs and what's the potential of breakage of existing usage. Anyone able to write a summary?

@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Nov 13, 2019

@youknowriad I wrote a summary here: #17875 (comment)

It shouldn't break the current usage unless they're doing things within <BlockControls> that diverge a lot from what Gutenberg does with built-in blocks.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 13, 2019

To be completely honest, I don't feel confident merging this PR as is. My concerns are:

1- Maybe it's just me but I find it too big, so hard to understand. I wonder if we can split it to increase confidence in the changes we're making.
2- Things like that:

It shouldn't break the current usage unless they're doing things within that diverge a lot from what Gutenberg does with built-in blocks.

This statement is a little bit unprecise to me. We can expect that plugin authors do all kind of weird things and Backwards compatibility is really important for WordPress, we can't break existing APIs.

Some exceptions are possible when we know exactly what we're breaking, how we're breaking it and if the breakage is small enough. In that case, we can write a dev note for plugin authors to let them know how they should update their code to avoid breakage.

@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Nov 13, 2019

@youknowriad
I think this PR could be simplified if all the new features are made experimental and no deprecation is made. Gutenberg itself wouldn't change (it would still use the old Toolbar without deprecation warnings) and the accessible Toolbar adoption could be broken into more PRs.

In the meanwhile, other projects using @wordpress/components could still benefit from this (knowing that it's experimental) and we would have room to try and change things.

My statement is so just because I don't have enough experience with how people create custom blocks, but maybe @gziolo may have a better perception.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 14, 2019

I think this PR could be simplified if all the new features are made experimental and no deprecation is made. Gutenberg itself wouldn't change (it would still use the old Toolbar without deprecation warnings) and the accessible Toolbar adoption could be broken into more PRs.

If we make it experimental, when do you think we could get out of the experimental phase? What concerns me the most is not the deprecations. Deprecations are good because they let developpers know what they should do.

It seems like you're saying that we can add the new components without touching the existing Core blocks. That is reassuring. So aside deprecations which can potentially break e2e tests, if I'm a block author and I don't touch my code, is there something that can break?

@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Nov 14, 2019

If we make it experimental, when do you think we could get out of the experimental phase? What concerns me the most is not the deprecations. Deprecations are good because they let developers know what they should do.

How is the process that is currently used to take things out of the experimental phase?

The removal of the deprecation would allow us to change between both implementations and even revert this one entirely if it doesn't work out. One thing that I learned while working on this PR is that making things __experimental and not deprecating stuff in the first step is the best way to introduce new features on Gutenberg (as long as you continue working on that).

Steps I would propose (they could be separate PRs):

  1. Simplify this PR so only packages/components/src/toolbar-* is touched.

    The accessible toolbar would be enabled by passing an __experimentalAccessibilityLabel prop to Toolbar. Only ToolbarButton would be accepted as a toolbar item (with the current state of this PR, it's Button, but only because ToolbarButton is still not flexible enough to serve all use cases).

    At this point, other projects using @wordpress/components could start experimenting it for simple toolbars.

  2. Improve ToolbarButton so that it serves more use cases than just being an IconButton.

    At this point, the Gutenberg header toolbar (without the fixed block toolbar) could use it.

  3. Refactor SlotFill so that the workarounds introduced by the current state of this PR (forcing fills to re-render when fillProps change, for example) are not necessary.

    At this point, block toolbars could start using it.

  4. Convert the fixed toolbar buttons (block switcher, more tools etc.) to ToolbarButton.

    At this point, custom blocks could start using it, as long as they use ToolbarButton inside BlockControls or BlockFormatControls.

  5. Convert core block toolbars.

    We could convert one per PR (for example, start with the Paragraph toolbar).

  6. Let it be for a while.

  7. If it proves to be useful and works out, deprecate the old usage and remove the __experimental prefix. Otherwise, revert it altogether, which would break the code for people using __experimentalAccessibilityLabel explicitly, but custom blocks would seamlessly rollback to the old usage.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 14, 2019

An iterative process like that sounds good to me 👍

@diegohaz diegohaz mentioned this pull request Nov 14, 2019
6 of 6 tasks complete
@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Nov 15, 2019

Closing in favor of #18534

@diegohaz diegohaz closed this Nov 15, 2019
jasmussen added a commit that referenced this pull request Nov 15, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
jasmussen added a commit that referenced this pull request Nov 18, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
jasmussen added a commit that referenced this pull request Nov 18, 2019
This is a followup to a regression found in #17875, specifically #17877 (comment).

The PR refactored horizontal margins, including for full-wide blocks. As a result, full-wide blocks would, on the mboile breakpoint, receive too much negative margin.

This PR addresses that.
@diegohaz diegohaz deleted the diegohaz:try/accessible-toolbar branch Nov 20, 2019
@diegohaz diegohaz mentioned this pull request Nov 20, 2019
1 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.