Components: Make Toolbar accessible by diegohaz · Pull Request #17875 · WordPress/gutenberg

diegohaz · 2019-10-10T03:31:11Z

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:

Current Toolbar has been renamed to ToolbarGroup.
New Toolbar needs an accessibilityLabel prop.
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

Playground

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.

This should be done upstream

packages/components/src/toolbar-button/accessible-toolbar-button.js

…le group

diegohaz · 2019-11-12T12:35:58Z

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 · 2019-11-12T13:14:20Z

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

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 · 2019-11-13T08:17:21Z

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.

ItsJonQ

@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 · 2019-11-13T17:43:15Z

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 · 2019-11-13T19:28:44Z

@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 · 2019-11-13T19:36:41Z

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 · 2019-11-13T23:03:14Z

@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 · 2019-11-14T07:42:42Z

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 · 2019-11-14T12:27:34Z

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):

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.
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.
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.
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.
Convert core block toolbars.

We could convert one per PR (for example, start with the Paragraph toolbar).
Let it be for a while.
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 · 2019-11-14T13:10:04Z

An iterative process like that sounds good to me 👍

diegohaz · 2019-11-15T00:06:59Z

Closing in favor of #18534

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 added 2 commits October 9, 2019 21:21

Add initial accessible toolbar implementation

a406f95

Merge branch 'master' into try/accessible-toolbar

968ed0b

diegohaz changed the title ~~Make Toolbar accessible~~ Components: Make Toolbar accessible Oct 10, 2019

gziolo self-requested a review October 10, 2019 04:44

diegohaz added 17 commits October 10, 2019 13:08

Fix ToolbarButton style by putting box-sizing on the component styles

85792cb

Support current ToolbarButton usage

b39d864

Remove useMemo

f47de25

This should be done upstream

Re-export current native versions

3fe4c5a

Fix AccessibleToolbarButton overwriting existing IconButton props

73226d1

Add label to Toolbar

adef315

Create stories

0b81861

Upgrade reakit dependency

f7aaa2e

Fix ToolbarContext value

1ab4179

Update package-lock.json

98d1f25

Use accessibilityLabel instead of label

d180767

Merge toggleProps event handlers in DropdownMenu

f3152cd

Add ToolbarGroup component

544f5b9

Use ToolbarGroup on Toolbar legacy

7161889

Update Toolbar stories

60d67a8

Update Toolbar README.md

ef73ced

Fix ToolbarGroup not being included in the tab order on the legacy usage

8dca51c

jsnajdr reviewed Oct 15, 2019

View reviewed changes

packages/components/src/toolbar-button/accessible-toolbar-button.js Outdated Show resolved Hide resolved

diegohaz added 4 commits October 16, 2019 23:31

Update AccessibleToolbarButton

72ce182

Refactor ToolbarContext and deprecate Toolbar being used as collapsib…

8a9e381

…le group

Update stories

064a47c

Refactor ToolbarGroup

f9492bb

diegohaz added 3 commits November 12, 2019 13:06

Add missing JSDocs to ToolbarGroup

5e033bc

Fix ref merging on useIsWithinToolbar

4133551

Add missing JSDoc descriptions

3605055

Merge branch 'master' into try/accessible-toolbar

8d47333

Update stories

d422d7e

jasmussen mentioned this pull request Nov 13, 2019

Fix full-wide on mobile regression. #18469

Merged

getdave mentioned this pull request Nov 13, 2019

Experimental: allow parent Block to consume child Block's toolbar #18440

Merged

6 tasks

Merge branch 'master' into try/accessible-toolbar

6170843

ItsJonQ approved these changes Nov 13, 2019

View reviewed changes

diegohaz mentioned this pull request Nov 13, 2019

Storybook: Add StoryShots integration to generate unit tests #18031

Merged

ItsJonQ mentioned this pull request Nov 14, 2019

Components: Add a PageHeader / ActionBar component #18141

Closed

diegohaz mentioned this pull request Nov 14, 2019

Components: Add accessible Toolbar #18534

Merged

6 tasks

diegohaz closed this Nov 15, 2019

diegohaz deleted the try/accessible-toolbar branch November 20, 2019 04:37

diegohaz mentioned this pull request Nov 20, 2019

Components: Improve Toolbar #18619

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components: Make Toolbar accessible#17875

Components: Make Toolbar accessible#17875
diegohaz wants to merge 86 commits intoWordPress:masterfrom
diegohaz:try/accessible-toolbar

diegohaz commented Oct 10, 2019 •

edited

Loading

Uh oh!

Uh oh!

diegohaz commented Nov 12, 2019 •

edited

Loading

Uh oh!

diegohaz commented Nov 12, 2019

Uh oh!

jasmussen commented Nov 13, 2019

Uh oh!

ItsJonQ left a comment

Uh oh!

youknowriad commented Nov 13, 2019

Uh oh!

diegohaz commented Nov 13, 2019

Uh oh!

youknowriad commented Nov 13, 2019 •

edited

Loading

Uh oh!

diegohaz commented Nov 13, 2019 •

edited

Loading

Uh oh!

youknowriad commented Nov 14, 2019

Uh oh!

diegohaz commented Nov 14, 2019

Uh oh!

youknowriad commented Nov 14, 2019

Uh oh!

diegohaz commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Conversation

diegohaz commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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.

Other stuff

How is this different from NavigableToolbar

Reakit

Next steps

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

Uh oh!

diegohaz commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegohaz commented Nov 12, 2019

Uh oh!

jasmussen commented Nov 13, 2019

Uh oh!

ItsJonQ left a comment

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Nov 13, 2019

Uh oh!

diegohaz commented Nov 13, 2019

Uh oh!

youknowriad commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegohaz commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Nov 14, 2019

Uh oh!

diegohaz commented Nov 14, 2019

Uh oh!

youknowriad commented Nov 14, 2019

Uh oh!

diegohaz commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

diegohaz commented Oct 10, 2019 •

edited

Loading

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`.

How is this different from `NavigableToolbar`

diegohaz commented Nov 12, 2019 •

edited

Loading

youknowriad commented Nov 13, 2019 •

edited

Loading

diegohaz commented Nov 13, 2019 •

edited

Loading