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

Updates the behavior of the top toolbar fixed setting #47277

Closed
wants to merge 19 commits into from

Conversation

draganescu
Copy link
Contributor

@draganescu draganescu commented Jan 19, 2023

What?

Closes #40450
Updates the behavior of the top toolbar fixed setting.

Why?

How?

  • wraps the edit post header in a slot fill provider
  • implements a temporary behavior that shows document tools when no block is selected
  • updates the block tools and parent selector to appear properly in the new place
  • implements a utility component that shows the document tools when they receive focus via toolbar

Testing Instructions

  1. Enable the Top Toolbar setting from the top right dot menu
  2. Select a block
  3. Notice the block tools (toolbar) replaces the document tools (toolbar)

Testing Instructions for Keyboard

N/A

Screenshots or screencast

top-toolbar-parent-selector-focus-show.mp4

@github-actions
Copy link

github-actions bot commented Jan 19, 2023

Size Change: +1.39 kB (0%)

Total Size: 1.35 MB

Filename Size Change
build/block-editor/index.min.js 202 kB -16 B (0%)
build/block-editor/style-rtl.css 14.7 kB +65 B (0%)
build/block-editor/style.css 14.7 kB +68 B (0%)
build/edit-post/index.min.js 35.6 kB +679 B (+2%)
build/edit-post/style-rtl.css 7.89 kB +298 B (+4%)
build/edit-post/style.css 7.89 kB +300 B (+4%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/annotations/index.min.js 2.78 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 483 B
build/block-directory/index.min.js 7.2 kB
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/content-rtl.css 4.11 kB
build/block-editor/content.css 4.1 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 138 B
build/block-library/blocks/audio/theme.css 138 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 91 B
build/block-library/blocks/avatar/style.css 91 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 587 B
build/block-library/blocks/button/editor.css 587 B
build/block-library/blocks/button/style-rtl.css 628 B
build/block-library/blocks/button/style.css 627 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 409 B
build/block-library/blocks/columns/style.css 409 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 649 B
build/block-library/blocks/cover/editor.css 651 B
build/block-library/blocks/cover/style-rtl.css 1.61 kB
build/block-library/blocks/cover/style.css 1.6 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 138 B
build/block-library/blocks/embed/theme.css 138 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 269 B
build/block-library/blocks/file/style.css 270 B
build/block-library/blocks/file/view.min.js 353 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 984 B
build/block-library/blocks/gallery/editor.css 988 B
build/block-library/blocks/gallery/style-rtl.css 1.55 kB
build/block-library/blocks/gallery/style.css 1.55 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 830 B
build/block-library/blocks/image/editor.css 829 B
build/block-library/blocks/image/style-rtl.css 652 B
build/block-library/blocks/image/style.css 652 B
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 716 B
build/block-library/blocks/navigation-link/editor.css 715 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation/editor-rtl.css 2.13 kB
build/block-library/blocks/navigation/editor.css 2.14 kB
build/block-library/blocks/navigation/style-rtl.css 2.22 kB
build/block-library/blocks/navigation/style.css 2.21 kB
build/block-library/blocks/navigation/view-modal.min.js 2.81 kB
build/block-library/blocks/navigation/view.min.js 447 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 401 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 174 B
build/block-library/blocks/paragraph/editor.css 174 B
build/block-library/blocks/paragraph/style-rtl.css 279 B
build/block-library/blocks/paragraph/style.css 281 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 501 B
build/block-library/blocks/post-comments-form/style.css 501 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 588 B
build/block-library/blocks/post-featured-image/editor.css 586 B
build/block-library/blocks/post-featured-image/style-rtl.css 322 B
build/block-library/blocks/post-featured-image/style.css 322 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 281 B
build/block-library/blocks/post-template/style.css 281 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 335 B
build/block-library/blocks/pullquote/style.css 335 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 463 B
build/block-library/blocks/query/editor.css 463 B
build/block-library/blocks/quote/style-rtl.css 222 B
build/block-library/blocks/quote/style.css 222 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 408 B
build/block-library/blocks/search/style.css 406 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 489 B
build/block-library/blocks/site-logo/editor.css 489 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 359 B
build/block-library/blocks/spacer/editor.css 359 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 433 B
build/block-library/blocks/table/editor.css 433 B
build/block-library/blocks/table/style-rtl.css 651 B
build/block-library/blocks/table/style.css 650 B
build/block-library/blocks/table/theme-rtl.css 157 B
build/block-library/blocks/table/theme.css 157 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 404 B
build/block-library/blocks/template-part/editor.css 404 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 179 B
build/block-library/blocks/video/style.css 179 B
build/block-library/blocks/video/theme-rtl.css 139 B
build/block-library/blocks/video/theme.css 139 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.12 kB
build/block-library/common.css 1.12 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 11.7 kB
build/block-library/editor.css 11.7 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 203 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.8 kB
build/block-library/style.css 12.8 kB
build/block-library/theme-rtl.css 698 B
build/block-library/theme.css 703 B
build/block-serialization-default-parser/index.min.js 1.13 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 51.1 kB
build/components/index.min.js 208 kB
build/components/style-rtl.css 11.7 kB
build/components/style.css 11.7 kB
build/compose/index.min.js 12.4 kB
build/core-data/index.min.js 16.3 kB
build/customize-widgets/index.min.js 12.2 kB
build/customize-widgets/style-rtl.css 1.41 kB
build/customize-widgets/style.css 1.41 kB
build/data-controls/index.min.js 663 B
build/data/index.min.js 8.68 kB
build/date/index.min.js 40.4 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.72 kB
build/edit-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/edit-site/index.min.js 63.1 kB
build/edit-site/style-rtl.css 10.1 kB
build/edit-site/style.css 10.1 kB
build/edit-widgets/index.min.js 17.3 kB
build/edit-widgets/style-rtl.css 4.56 kB
build/edit-widgets/style.css 4.56 kB
build/editor/index.min.js 45.9 kB
build/editor/style-rtl.css 3.49 kB
build/editor/style.css 3.48 kB
build/element/index.min.js 4.95 kB
build/escape-html/index.min.js 548 B
build/format-library/index.min.js 7.26 kB
build/format-library/style-rtl.css 557 B
build/format-library/style.css 556 B
build/hooks/index.min.js 1.66 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.79 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.79 kB
build/keycodes/index.min.js 1.94 kB
build/list-reusable-blocks/index.min.js 2.14 kB
build/list-reusable-blocks/style-rtl.css 865 B
build/list-reusable-blocks/style.css 865 B
build/media-utils/index.min.js 2.99 kB
build/notices/index.min.js 977 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.23 kB
build/preferences/index.min.js 1.35 kB
build/primitives/index.min.js 960 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 937 B
build/react-i18n/index.min.js 702 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.75 kB
build/reusable-blocks/index.min.js 2.26 kB
build/reusable-blocks/style-rtl.css 265 B
build/reusable-blocks/style.css 265 B
build/rich-text/index.min.js 11.1 kB
build/server-side-render/index.min.js 2.09 kB
build/shortcode/index.min.js 1.52 kB
build/style-engine/index.min.js 1.55 kB
build/token-list/index.min.js 650 B
build/url/index.min.js 3.74 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 1.09 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.3 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@draganescu draganescu added [Type] Enhancement A suggestion for improvement. General Interface Parts of the UI which don't fall neatly under other labels. labels Jan 19, 2023
@draganescu draganescu marked this pull request as ready for review January 19, 2023 10:18
@draganescu draganescu requested a review from ellatrix as a code owner January 19, 2023 10:18
@github-actions
Copy link

github-actions bot commented Jan 19, 2023

Flaky tests detected in 161eb93.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4618849358
📝 Reported issues:

@alexstine alexstine self-requested a review January 25, 2023 16:08
@alexstine alexstine added the Needs Accessibility Feedback Need input from accessibility label Jan 25, 2023
@joedolson
Copy link
Contributor

I think my biggest concern about this change is that you lose access to a bunch of tools if a block is selected. As a workflow, a common pattern for me on long documents is "Edit Block > Document Overview > Select Block > Edit Block"; and that workflow isn't available to me with this model without first removing focus from a block. That's easy to do with a mouse, but less trivial or discoverable from the keyboard.

One thing that might help with this would be if there is a way to deselect the block while in the top toolbar edit tools, so that if you've navigated all the way back to the editing tools, you can get back to the top toolbar immediately. Alternately, the edit post header controls could be placed in a popover at the beginning of the toolbar edit tools, so that those controls are collapsed rather than removed.

@draganescu
Copy link
Contributor Author

@joedolson in the final shape of this there is a button next to the block tools on the left (an up arrow) than can be used to go back to document tools, and there is a button on the right (a button with the block icon, a paragraph in the design) to go back to block tools, all while having a block selected. This PR is just not there yet.

Do you think the toggle solution proposed by the design is good enough?

@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from d658902 to 78aebb6 Compare February 2, 2023 16:30
@joedolson
Copy link
Contributor

I feel like the toggle to switch between menus should stay in the same position. Effectively, the arrow control to switch to the document tools and the paragraph tool to switch to the block tools are just a single switch that changes between toolbars, and having one of those tools at the start of the toolset and the other at the end feels strange to me. I think it would be more predictable if the switch control was always in the same position. But yes, that would help a lot!

@aaronrobertshaw

This comment was marked as outdated.

@alexstine
Copy link
Contributor

I am not in favor of this change because of context issues. Let me take a moment and paint some thoughts.

  1. When a user opens a post/page and starts to navigate the editor, they will pass through the document tools toolbar.
  2. Once they enter the content and focus a block, pressing shift+tab will land in the block toolbar. No change yet.
  3. Pressing shift+tab again, what will happen? Will I land on an options button or will I land on a show document tools button?
  4. We'll have to take extra steps to manage focus with a button that shows a toolbar.
  5. Disappearing and appearing DOM elements are always tricky to communicate to non-sighted users. The show document tools button would need to disappear or have some type of state value.
  6. If the show document tools button persists with some type of state, that adds yet another tab stop.
  7. For screen reader users who are navigating in virtual mode, there is now no longer a way to access the document tools toolbar since it will be hidden in the DOM/conditionally removed.
  8. This further continues the problem in Gutenberg with everything needed to be overly communicated for screen reader users.
  9. There are important buttons in the toolbar such as the block inserter and document overview/list view. Hiding these should be a non-starter.

Just my thoughts without testing. Right, wrong, not even close?

Thanks.

@talldan
Copy link
Contributor

talldan commented Feb 3, 2023

Just my thoughts without testing. Right, wrong, not even close?

I think you're right on the money that it's going to be difficult to get this feature right. It isn't the default option though, so one good thing is you shouldn't ever need to enable it. (though I realise it should also work in a good way anyway even when enabled.)

One of the main thing that stands out to me is that the tab order is going to be hard to get right. Currently users expect to be able to shift tab from the block to the block toolbar directly without passing through Options, Settings, Publish and Preview buttons. But this change will put those buttons in the way, and that won't be very convenient.

It's worth noting that plugins can also add buttons to this area, which means there could be quite a lot of tab stops.

@draganescu draganescu self-assigned this Feb 3, 2023
@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from 64099ef to 6c7c8e8 Compare February 3, 2023 13:40
@alexstine
Copy link
Contributor

Here is an idea. How about you visually hide the document tools toolbar and then add events to show it when focus passes through it? That way the experience does not change for keyboard users but you still get the appear/disappear functionality. I simply cannot support adding another tab stop to this area. I was also unaware that there was a slot fill in that area. That really could turn messy one day.

@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from 6c7c8e8 to 861590b Compare February 6, 2023 13:42
@draganescu
Copy link
Contributor Author

visually hide the document tools toolbar and then add events to show it when focus passes through it

@alexstine I don't grasp how that would work? Does it mean for keyboard navigation both toolbars will be available at the same time?

@alexstine
Copy link
Contributor

@draganescu Correct. Then when the toolbar receives focus, it will be shown visually. Completely removing or hiding the first toolbar from the DOM I do not think is a good idea.

@annezazu annezazu mentioned this pull request Feb 7, 2023
57 tasks
@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from 861590b to 06e9f82 Compare February 20, 2023 13:22
@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from 06e9f82 to 4d42882 Compare March 7, 2023 13:47
@draganescu

This comment was marked as outdated.

@draganescu draganescu changed the title Updates the behavior of the top toolbar fixed setting: Updates the behavior of the top toolbar fixed setting Mar 8, 2023
@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from 35cd696 to 5cb233f Compare March 8, 2023 15:51
@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from 4050a07 to 3421026 Compare March 16, 2023 07:47
@draganescu

This comment was marked as outdated.

@afercia
Copy link
Contributor

afercia commented Mar 31, 2023

@draganescu thanks for the ping.

It's important to note that this PR breaks the expected keyboard interaction within the two toolbars. These are based on the ARIA toolbar design pattern. Behind the scenes, there are two NavigableToolbar components that provide the expected semantics and interaction. For more details on the breakage reasons, see a very similar case here: #49479. In short: the toolbar buttons can't be hidden with CSS. Also rendering some of the buttons dynamically may break keyboard interaction. When the hidden/removed button happens to be the only focusable button expected within the toolbar, keyboard interaction is broken.

I also share all the accessibility concerns already reported by @alexstine and @joedolson so I'll avoid to repeat the same concerns.

More general considerations

I went through all the feedback and comments in this PR and in the original issue #40450. Lots of interesting explorations and thoughts there ❤️

However, seems to me this PR is going a bit too far. The original issues to address were:

  1. The lack of parent selector for nested blocks
  2. The overall increase in editor UI footprint

The initial suggested changes were:

  1. Add direct parent selector on the left. Makes totally sense.
  2. Try center layout and/or floating docked toolbar. OK, some better alignment can be surely tried.

Then, starting from #40450 (comment) the conversation evolved into something entirely different: morphing the document tools into block tools on selection. Which basically means a unified toolbar.

I'd like to remind that before WordPress 5.6 the top bar used to be a 'unified' toolbar already. Screenshot from WordPress 5.5:

wp55

It was than changes to two separate toolbars, and for good reasons. The split in two was discussed by the design team, see the original G2 issue, and having the two toolbars in two separate rows was the preferred option. See also some a11y considerations here: #20592 (comment). Other important considerations were pointed out in the related pull requests, see #31134 and #30234.

This PR would bring us back to a unified toolbar, re-introducing most of the problems addressed two years ago.

On top of that, this PR aims to automatically toggle the toolbars visibility depending on the selection state and keyboard interaction. As already pointed out in this PR and in several issues and PRs across the history of this project, 'disappearing and appearing elements' are always a barrier to non-sighted users. I'd add they're always confusing also for sighted users, as they add cognitive load, trigger loss of visual context, and potentially hide important tools thus breaking the user's flow.

To me, that's already a good reason to rethink the impact of this PR and limit the changes to the ones that were originally proposed:

  • add the parent selector
  • try a better alignment

There's a lot of additional usability / accessibility considerations that make me think the proposed unified toolbar is less than ideal. Trying to avoid a TL;DR, here's some of the main concerns:

  • As said previously, the expected keyboard interaction of the ARIA toolbar pattern is now broken.
  • Re: this comment: I'm hesitant to bury the List view control. That's a very good point. I'd say burying the Undo and Redo buttons is even more impactful though. As pointed out, the List view is typically ued only a few times in a typical user flow. However, Undo and Redo are likely used way more often. I'm not sure forcing users to perform and extra click each time they need to Undo or Redo is an improvement. Yes, there are keyboard shortcuts but also yes: most users will look for the Undo and Redo buttons.
  • The 'Up' and 'selected block' buttons to switch toolbars are very unclear to me. Not intuitive at all. Aside: right now these two buttons are unlabelled and miss a tooltip.
  • The 'selected block' button performs always the same action but its icon changes depending on the selected block. By its ever changing look, I'm not sure I would be able to understand what this button does. It always confuses me.
  • I'd argue to whole concept of 'levels', where the Document tools is the 'top level' toolbar and the Block tools is the 'second level' toolbar is too much to be easily understandable for users. As a user, I'd just want the toolbar I need back into view. I wouldn't want to be forced to think each time at where I am now and whether I need to go 'up' or 'down'.
  • Pressing Enter on the 'Up' and 'selected block' buttons triggers a focus loss, as they get removed from the DOM while they are focused.
  • As pointed out by @alexstine, For screen reader users who are navigating in virtual mode, there is now no longer a way to access the document tools toolbar since it will be hidden in the DOM/conditionally removed. This is a serious barrier.
  • Minor but still a problem: when the Block tools toolbar is shown, the Top bar will show two 'vertical ellipsis' buttons that are visually placed in the same row, both labelled 'Options'. Arguably a good UI. Screenshot:

two options same row

With the current split toolbars, at least it's more clear they belong to two different contexts:

Screenshot 2023-03-31 at 17 14 19

  • Last but not least: in the Site Editor there's way less space for a unified toolbar because of the DocumentActions component displayed at the center of the top bar,

Conclusion

I'm not sure this PR would be an actual improvement, as is. To me, the disappearing / appearing elements and hiding important tools are a no starter IMHO.

However, I wouldn't be opposed to try the unified toolbar as long as the Document tools stay always visible. No disappearing / appearing. No hidden tools. From there, I'm pretty sure we could refine and address all the remaining concerns.

I do realize that would lead us back to the lack of space issue though. As I see it, that's more a design issue. The fixed height ot the top bae and its absolute positioning are part of the problem. The very nature of content on the web is that it should dreflow as necessary depending on the viewport and the device in use. Trying to 'squeeze' or hide the content just to make it fit into an UI that doesn't provide enough space just proves the UI isn't design around the content.

@alexstine
Copy link
Contributor

Yeah, I am not a fan of the appearing/disappearing DOM elements either but it seems to be the way of the future with React/Vue/other frameworks. Not the largest issue in the world but things would be less complicated if things did not disappear from the DOM. Something a lot of times sighted people and designers take for granted how hard these interactions can be for people without visual indicators. I totally support conditional rendering for dialogs but it is way over used in Gutenberg even outside of this PR.

@draganescu
Copy link
Contributor Author

I have tried but not succeeded in a different approach, yet I still tink it's possible:

  • make the block toolbar in fixed mode render in a place where:
    • it is possible that we position it absolutely on top of the editor header, including z-index
    • it still is the nearest toolbar (Alt+f10 and shift+tab work)
  • use an observer to hide it if the focus is not in canvas

Everything else stays the same for mouse users with buttons that can show and hide document and block tools.

My challenge was where to place the block toolbar in fixed mode to respect the bullet requirements above.

draganescu and others added 18 commits April 4, 2023 17:42
- wraps the edit post header in a slot fill provider
- implements a temporary behavior that shows document tools when no block is selected
- updates the block tools and parent selector to appear properly in the new place
- restores fixed toolbar on mobile viewports
- adds hover states to toolbar toggles
- adds the correct level up icon
- moves CSS to the right files

Co-authored-by: Joen A. <1204802+jasmussen@users.noreply.github.com>
Co-authored-by: James Koster <846565+jameskoster@users.noreply.github.com>
@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from b85ff40 to 0988ac3 Compare April 4, 2023 14:43
@draganescu
Copy link
Contributor Author

Ok, here is the current state: playing around with the DOM position and some heavy focus management, I can achieve the state in the video.

toolbar-focus-aria.mp4

There still is work to do:

  • I have an effect that is too eager and moves the focus to the toolbar when selection changes between document and block
  • For some reason Alt+F10 focuses document tools which hides the block toolbar. But if I don't listen to the focus event Alt+F10 focuses block tools, I swear it's playing quantum jokes on me.

@alexstine
Copy link
Contributor

@draganescu It is working much better. Keep going, you are close.

Thanks.

@draganescu draganescu force-pushed the try/block-tools-marry-document-tools branch from 15a38d0 to 161eb93 Compare April 5, 2023 13:10
@draganescu
Copy link
Contributor Author

draganescu commented Apr 5, 2023

TL;DR; there appears to be a way to not break the keyboard interaction at all but it has sent me basically to square one.

With the help of @MaggieCabrera and @jeryj I have found a way to reuse the fixed block toolbar and not have to change where it is rendered. Instead, I can simply reposition it on top of the document tools and style it to fit visually there. This has the great advantage that all the keyboard interaction remains the same! No more tricks.

However it has some challenges which I am looking into:

  • the buttons that toggle the document tools and the block tools can no longer use a simple state to toggle between toolbars because one toolbar is in header and the other one is in a popover somewhere in content. It is dubious both to use a store and to drill down props (because the button that toggles the document tools is in the block toolbar component which is way too many layers to drill down props and it's also in another package).
    • to this a solution may be to put both the toggling buttons in the header and use CSS to make a nice puzzle, and use the edit post store to store toolbar visibility, but it's still weird.
  • there'll be a lot of CSS to guard small viewports from the desktop changes
  • I still have to use the :focus-within trick on document tools to hide block tools, but, not sure I can via CSS so it'll probably be a focus listener.

As far as the approach in this PR, Alt+F10 is handled by a hook that manages focus when this keyboard shortcut is used. This hook is not specific to block toolbar but to all navigable toolbars. Because this PR puts two navigable toolbars in the same container Alt+F10 always selects the 1st which is document tools. The other thing I still have to see if I can change this in a reasonable way (unlikely).

@draganescu
Copy link
Contributor Author

Closing in favor of #49634

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. Needs Accessibility Feedback Need input from accessibility [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Top Toolbar improvements
10 participants