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

Block toolbar: split switcher from mover and simplify styles. #23971

Merged
merged 3 commits into from
Aug 6, 2020

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Jul 15, 2020

Description

Following the discussion starting here, this PR simplifies the block toolbar styles so that the switcher and movers are shown in separate toolbar groups, with the standard spacing between the two. They were already technically in separate toolbar groups, but the styles obscured this to try and show them closer together.

To ensure that the parent block selector button and block outline only show when the block icon is hovered, I had to move some elements around a bit. As it turns out, this allowed me to remove even more special-case CSS.

I initially tried to create a PR that simply increased the space between the switcher and mover, following this comment by @diegohaz. However, this proved to be really, really complicated to try and implement. As it turns out, going the next step and just using the standard styles was a drastically simpler solution.

As the accessibility report found the combined switcher/mover area to be a regression, accessibility-wise, I'm tagging this with the "Backport to WP Core" label in the hope that it will be included in WP 5.5, as suggested by @afercia.

Screenshots

image
image

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ZebulanStanphill ZebulanStanphill added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback. Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jul 15, 2020
@github-actions
Copy link

github-actions bot commented Jul 15, 2020

Size Change: -361 B (0%)

Total Size: 1.15 MB

Filename Size Change
build/block-editor/index.js 125 kB -3 B (0%)
build/block-editor/style-rtl.css 10.6 kB -180 B (1%)
build/block-editor/style.css 10.6 kB -178 B (1%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.97 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-library/editor-rtl.css 7.59 kB 0 B
build/block-library/editor.css 7.59 kB 0 B
build/block-library/index.js 132 kB 0 B
build/block-library/style-rtl.css 7.76 kB 0 B
build/block-library/style.css 7.77 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.3 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 11.8 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.9 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 9.38 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.33 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill force-pushed the update/divide-switcher-from-movers branch 3 times, most recently from 1d8e1c1 to 5cead2b Compare July 15, 2020 23:42
@ZebulanStanphill ZebulanStanphill marked this pull request as ready for review July 16, 2020 01:26
@ZebulanStanphill ZebulanStanphill changed the title Block toolbar: simplify switcher/mover styles. Block toolbar: split switcher from mover and simplify styles. Jul 16, 2020
@ZebulanStanphill
Copy link
Member Author

It turns out the bug I fixed in a669dc3 was actually present in master, too. I've created #23980 to fix the bug in a separate PR, assuming this one doesn't get merged first.

@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 16, 2020
@youknowriad
Copy link
Contributor

There's still a lot of discussions about that and different opinions. I removed the backport label but if it an agreement is reached and it makes it before 5.5, we'll add it back.

@ZebulanStanphill ZebulanStanphill force-pushed the update/divide-switcher-from-movers branch from 36dd4f1 to b57c1e7 Compare July 16, 2020 13:00
@ZebulanStanphill
Copy link
Member Author

Rebased since #23980 got merged.

@ZebulanStanphill

This comment has been minimized.

@ZebulanStanphill ZebulanStanphill force-pushed the update/divide-switcher-from-movers branch from b57c1e7 to 682d66c Compare July 16, 2020 13:50
@ZebulanStanphill

This comment has been minimized.

@ZebulanStanphill ZebulanStanphill force-pushed the update/divide-switcher-from-movers branch from 682d66c to 2b14435 Compare July 17, 2020 01:40
@ZebulanStanphill

This comment has been minimized.

@ZebulanStanphill ZebulanStanphill force-pushed the update/divide-switcher-from-movers branch 3 times, most recently from df0847c to e7a9dcb Compare August 3, 2020 20:33
@ZebulanStanphill
Copy link
Member Author

Rebased again.

@joedolson
Copy link
Contributor

The accessibility team would definitely like to see this happen in WP 5.5; it would help a lot in mitigating some of the challenges with this part of the control.

Only show parent block selector button and block outline when hovering the block icon.
@ZebulanStanphill ZebulanStanphill force-pushed the update/divide-switcher-from-movers branch from e7a9dcb to 9672373 Compare August 4, 2020 19:01
@shaunandrews
Copy link
Contributor

This makes the movers a little less visually obvious; With the added divider between the block icon and the movers, the context feels different. My instinct tells me that the movers should be somewhat special in comparison to the rest of the buttons in the block toolbar.

Looking back at @diegohaz's suggestion, I like the extra spacing instead of a more explicit divider; The space helps connect the movers to the block in question, and feels overall visually simpler.

image

I wonder if this PR could accomplish this by hiding the divider between the block icon and the movers.

@paaljoachim
Copy link
Contributor

paaljoachim commented Aug 5, 2020

I am pinging a few folks who added a comment to the issue, so they have a chance to take a look at this PR.
@pablohoneyhoney @jasmussen @ellatrix @mtias @enriquesanchez

@ZebulanStanphill
Copy link
Member Author

Semantically, though, the controls should be divided by a line because they're in separate ToolbarGroups. Also, the dividing line helps make it clear that the movers look are not part of the switcher. The a11y team has pointed out that the current design's movers look like dropdown indicators for a <select>.

This also ties in another issue (that hopefully someone else can solve): the movers use the same icons as the accordion panels, <select> dropdowns (not just in general but in our own UI style), and inline formatting tool overflow, which is very bad for comprehension since the same icons are being used in different ways across the UI.

Another issue is that the mover buttons being stacked in the first place makes it a lot more difficult to where one starts and ends. The icons are so close together they appear like a single button.

There's yet another issue tying into all this: we need to have a visible drag handle. Showing a drag-hand cursor when hovering the mover buttons was deemed bad for a11y (all buttons should use the standard button cursor), so it was reverted. But now there's no indicator of any way to drag blocks. There ought to be a clearly defined drag handle somewhere. And to pull that off, the movers will ultimately have to be split apart into 2 full-size buttons... probably with a drag handle in-between.

These all have to be addressed in order to undo a11y regressions that WP 5.5 is introducing.

I realize that fitting 3 full-size buttons into the block toolbar is asking a lot. But I think that speaks to a larger issue: we need a way to show controls more contextually. We can't always show the block movement controls, because there simply isn't enough space to do that. In general, there's simply too many toolbar controls to show all at once.

I've been thinking about recent explorations like #24021. Assuming it can be done accessibly, I think we should apply this concept to the movement controls: by default, a movement controls button would appear in the toolbar. Clicking it would swap the currently visible controls with all the movement-related ones. This would allow us to not only show up, down, and a drag handle, but also additional controls like "move to top" or "move to bottom".

Anyway, this PR was intended to be a compromise to improve the situation for now by removing some of the weird special-casing that makes the current situation bad for a11y. Trying to step back a bit closer to the current design in master makes the CSS really complicated again or ends up looking inconsistent with the rest of the toolbar, while barely addressing the usability/a11y problems.

If this PR isn't a good enough compromise for everyone, I suggest we look into the full-toolbar-replacement idea and try to get it done in an accessible way in time for WP 5.6.

@pablohoneyhoney
Copy link

pablohoneyhoney commented Aug 6, 2020

In my opinion, we can move affirmatively with this. And I think we’ll need to revise soon how the dividers and grouping work in the anatomy of the block toolbar and its different cases. Because this change introduces complexity in the hierarchy of the controls, their taxonomy, and also establishes a busier toolbar.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! This is what I see:

movers

High level, this seems like a good enough compromise that keeps intact the benefits of having the mover control in the toolbar (no lateral overlap in nested contexts, consistency across mobile and desktop, works identically for vertical and lateral movement). Closes #23760.

Keeping Pablo's comments in mind, let's try it.

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks!

@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Aug 6, 2020
@ZebulanStanphill ZebulanStanphill removed the Needs Design Feedback Needs general design feedback. label Aug 6, 2020
@ZebulanStanphill
Copy link
Member Author

Thanks, everyone! 🎉

@ZebulanStanphill ZebulanStanphill merged commit ba80605 into master Aug 6, 2020
@ZebulanStanphill ZebulanStanphill deleted the update/divide-switcher-from-movers branch August 6, 2020 16:07
@github-actions github-actions bot added this to the Gutenberg 8.8 milestone Aug 6, 2020
@mapk
Copy link
Contributor

mapk commented Aug 6, 2020

It's great to see this happened! 🎉 Thanks @ZebulanStanphill

@mapk mapk added the Needs Figma Update Needs an update to Figma for design purposes label Aug 6, 2020
whyisjake pushed a commit that referenced this pull request Aug 9, 2020
* Block toolbar: simplify switcher/mover styles.

* Move mover out of block-switcher-container

Only show parent block selector button and block outline when hovering the block icon.

* Reduce style specificity and add missing comment.
@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Aug 10, 2020
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). General Interface Parts of the UI which don't fall neatly under other labels. Needs Figma Update Needs an update to Figma for design purposes [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants