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

List View and Block List: Update hover, focus, highlight, select borders #29636

Merged
merged 12 commits into from
Mar 16, 2021

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Mar 8, 2021

Description

Fixes #29467

The initial implementation of the Persistent List View had a pretty big usability issue caused by the fact that the component wasn't designed to stick around after an item was selected.
The persistence required a visual clue to indicate that, on item selection, the focus shifted from the list to the block.

This PR subtly updates the outlines of both the List View items, and the blocks in the canvas, to make their relationship clearer.

  • In canvas, the primary selection (selected, hovered, highlighted) outline changed from 2px --wp-admin-theme-color (typically blue) to 1px $gray-900.
  • In canvas, the Template Part block's outline becomes dotted when one of its children is selected.
  • In the List View, focused items gain a white halo inside their blue outline.

This PR also highlight the entire children branch of a selected item.
This required the introduction of some additional logic in the BlockNavigation component.
To avoid regressions and incompatibilities, like the other Persistent List View-only features, this too is gated behind the __experimentalPersistentListViewFeatures flag.

How has this been tested?

  • Activate an FSE theme such as TT1 Blocks.
  • Enter the Site Editor, and open the List View.
  • Play around with it, making sure the focus position is clear enough at all times.

Screenshots

Grey outline of a hovered block:

Screenshot 2021-03-15 at 16 58 39

Dotted outline of a template part with a child selected:
Screenshot 2021-03-15 at 16 58 19

List View item's focus halo:

Screenshot 2021-03-15 at 16 59 13

List View's selected branch:

Screenshot 2021-03-15 at 16 57 51

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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.

@Copons Copons added [Type] Enhancement A suggestion for improvement. [Status] In Progress Tracking issues with work in progress Needs Design Feedback Needs general design feedback. [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Package] Block editor /packages/block-editor labels Mar 8, 2021
@Copons Copons requested a review from ellatrix as a code owner March 8, 2021 17:35
@Copons Copons self-assigned this Mar 8, 2021
@Copons Copons requested a review from jameskoster March 8, 2021 17:37
@github-actions
Copy link

github-actions bot commented Mar 8, 2021

Size Change: +260 B (0%)

Total Size: 1.4 MB

Filename Size Change
build/block-editor/index.js 125 kB +140 B (0%)
build/block-editor/style-rtl.css 12.4 kB +67 B (+1%)
build/block-editor/style.css 12.4 kB +69 B (+1%)
build/block-library/blocks/template-part/editor-rtl.css 552 B -5 B (-1%)
build/block-library/blocks/template-part/editor.css 551 B -5 B (-1%)
build/block-library/editor-rtl.css 9.57 kB -3 B (0%)
build/block-library/editor.css 9.57 kB -3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.63 kB 0 B
build/block-directory/style-rtl.css 1 kB 0 B
build/block-directory/style.css 1.01 kB 0 B
build/block-library/blocks/archives/editor-rtl.css 61 B 0 B
build/block-library/blocks/archives/editor.css 60 B 0 B
build/block-library/blocks/audio/editor-rtl.css 58 B 0 B
build/block-library/blocks/audio/editor.css 58 B 0 B
build/block-library/blocks/audio/style-rtl.css 112 B 0 B
build/block-library/blocks/audio/style.css 112 B 0 B
build/block-library/blocks/block/editor-rtl.css 161 B 0 B
build/block-library/blocks/block/editor.css 161 B 0 B
build/block-library/blocks/button/editor-rtl.css 475 B 0 B
build/block-library/blocks/button/editor.css 474 B 0 B
build/block-library/blocks/button/style-rtl.css 479 B 0 B
build/block-library/blocks/button/style.css 479 B 0 B
build/block-library/blocks/buttons/editor-rtl.css 315 B 0 B
build/block-library/blocks/buttons/editor.css 315 B 0 B
build/block-library/blocks/buttons/style-rtl.css 364 B 0 B
build/block-library/blocks/buttons/style.css 363 B 0 B
build/block-library/blocks/calendar/style-rtl.css 208 B 0 B
build/block-library/blocks/calendar/style.css 208 B 0 B
build/block-library/blocks/categories/editor-rtl.css 84 B 0 B
build/block-library/blocks/categories/editor.css 83 B 0 B
build/block-library/blocks/categories/style-rtl.css 79 B 0 B
build/block-library/blocks/categories/style.css 79 B 0 B
build/block-library/blocks/code/style-rtl.css 90 B 0 B
build/block-library/blocks/code/style.css 90 B 0 B
build/block-library/blocks/columns/editor-rtl.css 190 B 0 B
build/block-library/blocks/columns/editor.css 190 B 0 B
build/block-library/blocks/columns/style-rtl.css 421 B 0 B
build/block-library/blocks/columns/style.css 421 B 0 B
build/block-library/blocks/cover/editor-rtl.css 599 B 0 B
build/block-library/blocks/cover/editor.css 599 B 0 B
build/block-library/blocks/cover/style-rtl.css 1.24 kB 0 B
build/block-library/blocks/cover/style.css 1.24 kB 0 B
build/block-library/blocks/embed/editor-rtl.css 486 B 0 B
build/block-library/blocks/embed/editor.css 486 B 0 B
build/block-library/blocks/embed/style-rtl.css 401 B 0 B
build/block-library/blocks/embed/style.css 400 B 0 B
build/block-library/blocks/file/editor-rtl.css 199 B 0 B
build/block-library/blocks/file/editor.css 198 B 0 B
build/block-library/blocks/file/style-rtl.css 248 B 0 B
build/block-library/blocks/file/style.css 248 B 0 B
build/block-library/blocks/freeform/editor-rtl.css 2.45 kB 0 B
build/block-library/blocks/freeform/editor.css 2.45 kB 0 B
build/block-library/blocks/gallery/editor-rtl.css 689 B 0 B
build/block-library/blocks/gallery/editor.css 690 B 0 B
build/block-library/blocks/gallery/style-rtl.css 1.11 kB 0 B
build/block-library/blocks/gallery/style.css 1.1 kB 0 B
build/block-library/blocks/group/editor-rtl.css 318 B 0 B
build/block-library/blocks/group/editor.css 317 B 0 B
build/block-library/blocks/group/style-rtl.css 57 B 0 B
build/block-library/blocks/group/style.css 57 B 0 B
build/block-library/blocks/heading/editor-rtl.css 129 B 0 B
build/block-library/blocks/heading/editor.css 129 B 0 B
build/block-library/blocks/heading/style-rtl.css 76 B 0 B
build/block-library/blocks/heading/style.css 76 B 0 B
build/block-library/blocks/html/editor-rtl.css 281 B 0 B
build/block-library/blocks/html/editor.css 281 B 0 B
build/block-library/blocks/image/editor-rtl.css 717 B 0 B
build/block-library/blocks/image/editor.css 716 B 0 B
build/block-library/blocks/image/style-rtl.css 476 B 0 B
build/block-library/blocks/image/style.css 478 B 0 B
build/block-library/blocks/latest-comments/editor-rtl.css 159 B 0 B
build/block-library/blocks/latest-comments/editor.css 158 B 0 B
build/block-library/blocks/latest-comments/style-rtl.css 269 B 0 B
build/block-library/blocks/latest-comments/style.css 269 B 0 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B 0 B
build/block-library/blocks/latest-posts/editor.css 137 B 0 B
build/block-library/blocks/latest-posts/style-rtl.css 523 B 0 B
build/block-library/blocks/latest-posts/style.css 522 B 0 B
build/block-library/blocks/list/editor-rtl.css 65 B 0 B
build/block-library/blocks/list/editor.css 65 B 0 B
build/block-library/blocks/list/style-rtl.css 63 B 0 B
build/block-library/blocks/list/style.css 63 B 0 B
build/block-library/blocks/media-text/editor-rtl.css 191 B 0 B
build/block-library/blocks/media-text/editor.css 191 B 0 B
build/block-library/blocks/media-text/style-rtl.css 535 B 0 B
build/block-library/blocks/media-text/style.css 532 B 0 B
build/block-library/blocks/more/editor-rtl.css 434 B 0 B
build/block-library/blocks/more/editor.css 434 B 0 B
build/block-library/blocks/navigation-link/editor-rtl.css 620 B 0 B
build/block-library/blocks/navigation-link/editor.css 621 B 0 B
build/block-library/blocks/navigation-link/style-rtl.css 671 B 0 B
build/block-library/blocks/navigation-link/style.css 668 B 0 B
build/block-library/blocks/navigation/editor-rtl.css 1.09 kB 0 B
build/block-library/blocks/navigation/editor.css 1.09 kB 0 B
build/block-library/blocks/navigation/style-rtl.css 204 B 0 B
build/block-library/blocks/navigation/style.css 205 B 0 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B 0 B
build/block-library/blocks/nextpage/editor.css 395 B 0 B
build/block-library/blocks/page-list/editor-rtl.css 215 B 0 B
build/block-library/blocks/page-list/editor.css 215 B 0 B
build/block-library/blocks/page-list/style-rtl.css 527 B 0 B
build/block-library/blocks/page-list/style.css 526 B 0 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B 0 B
build/block-library/blocks/paragraph/editor.css 157 B 0 B
build/block-library/blocks/paragraph/style-rtl.css 247 B 0 B
build/block-library/blocks/paragraph/style.css 248 B 0 B
build/block-library/blocks/post-author/editor-rtl.css 209 B 0 B
build/block-library/blocks/post-author/editor.css 209 B 0 B
build/block-library/blocks/post-author/style-rtl.css 183 B 0 B
build/block-library/blocks/post-author/style.css 184 B 0 B
build/block-library/blocks/post-comments-form/style-rtl.css 250 B 0 B
build/block-library/blocks/post-comments-form/style.css 250 B 0 B
build/block-library/blocks/post-content/editor-rtl.css 139 B 0 B
build/block-library/blocks/post-content/editor.css 139 B 0 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B 0 B
build/block-library/blocks/post-excerpt/editor.css 73 B 0 B
build/block-library/blocks/post-featured-image/editor-rtl.css 338 B 0 B
build/block-library/blocks/post-featured-image/editor.css 338 B 0 B
build/block-library/blocks/post-featured-image/style-rtl.css 100 B 0 B
build/block-library/blocks/post-featured-image/style.css 100 B 0 B
build/block-library/blocks/preformatted/style-rtl.css 63 B 0 B
build/block-library/blocks/preformatted/style.css 63 B 0 B
build/block-library/blocks/pullquote/editor-rtl.css 183 B 0 B
build/block-library/blocks/pullquote/editor.css 183 B 0 B
build/block-library/blocks/pullquote/style-rtl.css 318 B 0 B
build/block-library/blocks/pullquote/style.css 318 B 0 B
build/block-library/blocks/query-loop/editor-rtl.css 90 B 0 B
build/block-library/blocks/query-loop/editor.css 89 B 0 B
build/block-library/blocks/query-loop/style-rtl.css 315 B 0 B
build/block-library/blocks/query-loop/style.css 317 B 0 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B 0 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B 0 B
build/block-library/blocks/query-pagination/editor-rtl.css 270 B 0 B
build/block-library/blocks/query-pagination/editor.css 262 B 0 B
build/block-library/blocks/query-pagination/style-rtl.css 168 B 0 B
build/block-library/blocks/query-pagination/style.css 168 B 0 B
build/block-library/blocks/query-title/editor-rtl.css 86 B 0 B
build/block-library/blocks/query-title/editor.css 86 B 0 B
build/block-library/blocks/query/editor-rtl.css 814 B 0 B
build/block-library/blocks/query/editor.css 812 B 0 B
build/block-library/blocks/quote/editor-rtl.css 61 B 0 B
build/block-library/blocks/quote/editor.css 61 B 0 B
build/block-library/blocks/quote/style-rtl.css 169 B 0 B
build/block-library/blocks/quote/style.css 169 B 0 B
build/block-library/blocks/rss/editor-rtl.css 201 B 0 B
build/block-library/blocks/rss/editor.css 202 B 0 B
build/block-library/blocks/rss/style-rtl.css 290 B 0 B
build/block-library/blocks/rss/style.css 290 B 0 B
build/block-library/blocks/search/editor-rtl.css 165 B 0 B
build/block-library/blocks/search/editor.css 165 B 0 B
build/block-library/blocks/search/style-rtl.css 342 B 0 B
build/block-library/blocks/search/style.css 344 B 0 B
build/block-library/blocks/separator/editor-rtl.css 99 B 0 B
build/block-library/blocks/separator/editor.css 99 B 0 B
build/block-library/blocks/separator/style-rtl.css 236 B 0 B
build/block-library/blocks/separator/style.css 236 B 0 B
build/block-library/blocks/shortcode/editor-rtl.css 504 B 0 B
build/block-library/blocks/shortcode/editor.css 504 B 0 B
build/block-library/blocks/site-logo/editor-rtl.css 201 B 0 B
build/block-library/blocks/site-logo/editor.css 201 B 0 B
build/block-library/blocks/site-logo/style-rtl.css 115 B 0 B
build/block-library/blocks/site-logo/style.css 115 B 0 B
build/block-library/blocks/social-link/editor-rtl.css 164 B 0 B
build/block-library/blocks/social-link/editor.css 165 B 0 B
build/block-library/blocks/social-links/editor-rtl.css 769 B 0 B
build/block-library/blocks/social-links/editor.css 769 B 0 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB 0 B
build/block-library/blocks/social-links/style.css 1.32 kB 0 B
build/block-library/blocks/spacer/editor-rtl.css 317 B 0 B
build/block-library/blocks/spacer/editor.css 317 B 0 B
build/block-library/blocks/spacer/style-rtl.css 48 B 0 B
build/block-library/blocks/spacer/style.css 48 B 0 B
build/block-library/blocks/table/editor-rtl.css 478 B 0 B
build/block-library/blocks/table/editor.css 478 B 0 B
build/block-library/blocks/table/style-rtl.css 402 B 0 B
build/block-library/blocks/table/style.css 402 B 0 B
build/block-library/blocks/tag-cloud/editor-rtl.css 118 B 0 B
build/block-library/blocks/tag-cloud/editor.css 118 B 0 B
build/block-library/blocks/tag-cloud/style-rtl.css 94 B 0 B
build/block-library/blocks/tag-cloud/style.css 94 B 0 B
build/block-library/blocks/term-description/editor-rtl.css 90 B 0 B
build/block-library/blocks/term-description/editor.css 90 B 0 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B 0 B
build/block-library/blocks/text-columns/editor.css 95 B 0 B
build/block-library/blocks/text-columns/style-rtl.css 166 B 0 B
build/block-library/blocks/text-columns/style.css 166 B 0 B
build/block-library/blocks/verse/editor-rtl.css 62 B 0 B
build/block-library/blocks/verse/editor.css 62 B 0 B
build/block-library/blocks/verse/style-rtl.css 87 B 0 B
build/block-library/blocks/verse/style.css 87 B 0 B
build/block-library/blocks/video/editor-rtl.css 504 B 0 B
build/block-library/blocks/video/editor.css 503 B 0 B
build/block-library/blocks/video/style-rtl.css 187 B 0 B
build/block-library/blocks/video/style.css 187 B 0 B
build/block-library/common-rtl.css 1.1 kB 0 B
build/block-library/common.css 1.1 kB 0 B
build/block-library/index.js 147 kB 0 B
build/block-library/style-rtl.css 8.85 kB 0 B
build/block-library/style.css 8.85 kB 0 B
build/block-library/theme-rtl.css 700 B 0 B
build/block-library/theme.css 701 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.3 kB 0 B
build/components/index.js 283 kB 0 B
build/components/style-rtl.css 16.2 kB 0 B
build/components/style.css 16.2 kB 0 B
build/compose/index.js 11.1 kB 0 B
build/core-data/index.js 16.7 kB 0 B
build/customize-widgets/index.js 3.95 kB 0 B
build/customize-widgets/style-rtl.css 168 B 0 B
build/customize-widgets/style.css 168 B 0 B
build/data-controls/index.js 829 B 0 B
build/data/index.js 8.87 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 769 B 0 B
build/dom-ready/index.js 576 B 0 B
build/dom/index.js 4.93 kB 0 B
build/edit-navigation/index.js 11.9 kB 0 B
build/edit-navigation/style-rtl.css 1.31 kB 0 B
build/edit-navigation/style.css 1.31 kB 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.83 kB 0 B
build/edit-post/style.css 6.82 kB 0 B
build/edit-site/index.js 27.2 kB 0 B
build/edit-site/style-rtl.css 4.49 kB 0 B
build/edit-site/style.css 4.48 kB 0 B
build/edit-widgets/index.js 20.1 kB 0 B
build/edit-widgets/style-rtl.css 3.2 kB 0 B
build/edit-widgets/style.css 3.2 kB 0 B
build/editor/editor-styles-rtl.css 347 B 0 B
build/editor/editor-styles.css 347 B 0 B
build/editor/index.js 41.8 kB 0 B
build/editor/style-rtl.css 3.9 kB 0 B
build/editor/style.css 3.9 kB 0 B
build/element/index.js 4.61 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.75 kB 0 B
build/format-library/style-rtl.css 637 B 0 B
build/format-library/style.css 639 B 0 B
build/hooks/index.js 2.28 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 4.01 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.53 kB 0 B
build/keycodes/index.js 1.95 kB 0 B
build/list-reusable-blocks/index.js 3.14 kB 0 B
build/list-reusable-blocks/style-rtl.css 629 B 0 B
build/list-reusable-blocks/style.css 628 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.85 kB 0 B
build/nux/index.js 3.41 kB 0 B
build/nux/style-rtl.css 731 B 0 B
build/nux/style.css 727 B 0 B
build/plugins/index.js 2.89 kB 0 B
build/primitives/index.js 1.42 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/react-i18n/index.js 1.46 kB 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 3.78 kB 0 B
build/reusable-blocks/style-rtl.css 225 B 0 B
build/reusable-blocks/style.css 225 B 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.58 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 3.02 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@jameskoster
Copy link
Contributor

jameskoster commented Mar 8, 2021

This is awesome to see :)

Before getting in to the style of everything (which will certainly need some extra thought and refinement) I noticed one small issue:

If you select a block on the canvas then open list view, focus transfers to the list view as expected 👍 If you then close list view using the keyboard shortcut focus seems to vanish. Should it return to the selected block on the canvas? Video:

focus.mp4

@jameskoster
Copy link
Contributor

Another small issue that I suspect is related – if you select a text-based block from list view, focus transfers to the canvas as expected, but for some reason I do not see the text cursor, and if I start typing characters appear at the beginning of the value:

text.mp4

@Copons
Copy link
Contributor Author

Copons commented Mar 8, 2021

Before getting in to the style of everything (which will certainly need some extra thought and refinement)

Yeah, there are a couple of minor things in the current code, but most importantly, I literally typed random block list classnames until they worked well enough.
There are a lot of combinations, and many of them are obscure to me: navigation mode, edit mode, outline mode, focus mode, is selected, is highlighted, is hovered, :focus, is typing, is editing — and I'm pretty sure I'm missing more. 😅
I was mostly interested in having something to play around with.

If you select a block on the canvas then open list view, focus transfers to the list view as expected 👍 If you then close list view using the keyboard shortcut focus seems to vanish. Should it return to the selected block on the canvas? Video:

Blurgh.
Keyboard navigation is too hard, we should just skip ahead to brain interfaces.

Another small issue that I suspect is related – if you select a text-based block from list view, focus transfers to the canvas as expected, but for some reason I do not see the text cursor, and if I start typing characters appear at the beginning of the value:

I think the cursor at the beginning is unrelated to the Persistent List View, and it's just how the selectBlock action works (which is not to say it's correct 🤔 ). The normal List View does the same, for example.

The missing cursor might be hidden under one of the three outlines of the selected/focused block.
Although, the block shouldn't have that outline when .is-typing, but it's likely conflicting with other rules.

@paaljoachim
Copy link
Contributor

Here is an overview:

List-view-FSE.mp4

I really like what I see. It gives a nice overview.

@jameskoster jameskoster added the Needs Accessibility Feedback Need input from accessibility label Mar 10, 2021
@Copons
Copy link
Contributor Author

Copons commented Mar 10, 2021

@jameskoster I... I really can't see the difference with d772416, but I'm happy it saves a few characters! 😅

Anyway, I think the changes introduced in this PR make things 100% clearer, at least in relation to the Persistent List View.

We'd be pushing major visual changes to the block editor itself though, so I'd expect more discussions from designers before committing to an approach.

Otherwise, I could limit the outline changes to only the Persistent version of the List View and only the Site Editor's canvas (so, basically, limiting all the changes to the Site Editor).
It would allow more people to check it out IRL and see how it feels.
Should we get some backlash, we can just simply revert.

@jameskoster
Copy link
Contributor

Exploring ways to eliminate the double-border on the canvas which looks kind of ugly. Perhaps when a block is selected + focussed we only show the focus treatment? I believe this still satisfies the accessibility requirements as the border width and position both change.

single-border.mp4

@priethor priethor mentioned this pull request Mar 10, 2021
13 tasks
@jameskoster
Copy link
Contributor

Having got some feedback from the design team, I think what we're doing in this PR may be overkill :)

I think the chief concern for now should be addressing the issue in the following screenshot:

Screenshot 2021-03-10 at 19 53 38

Focus is in list view, but since blocks on the canvas with the isHighlighted (and isHovered for that matter) state receive the same style treatment, that is not clear and obvious.

The remedy for this seems fairly straight-forward – reserve the blue treatment for focus, and use another color (likely $gray-900) for hover and highlight states. The only drawback I can see to this is that template parts will then receive the same treatment (when a child is selected), as when they are hovered. So we may need to adjust that style as well.

I'll push some changes for us to test soon.

@Copons Copons requested a review from ajitbohra as a code owner March 11, 2021 11:43
@jameskoster
Copy link
Contributor

Pushed the changes suggested in my last comment. Here's a video demo:

focus.mp4

I suspect the dotted outline on the template part will be the most controversial change. There are perhaps other options to explore there, light setting an opacity instead of the different border-style.

@jasmussen
Copy link
Contributor

I haven't fully tested this PR, as I'm in the middle of a separate one, but thank for all the work!

I suspect the dotted outline on the template part will be the most controversial change.

I'm generally skeptical of dotted and dashed lines, as they tend to add noise. However with borders and boundaries and hover states in the site editor still in need of a fair bit of holistic thinking and love, if we are to try a dotted line, this is as good a time as any. If it doesn't work, we can explore a semi-transparent border instead.

@Copons
Copy link
Contributor Author

Copons commented Mar 11, 2021

@jameskoster your latest changes help a lot with the list/canvas focus confusion!
I'm still slightly tricked by the black background of selected list items, but at least seeing the blue border moving from list to canvas makes things pretty clear to me.

I don't have a strong opinion about the dotted template part border.
Dots and dashes have a bit of a nostalgic web 1.0 flavour, but I like that there's a distinction (even if subtle) between template parts and other blocks.


I've noticed two unrelated issues that become more obvious with the Persistent List View.
(I'll open issues for these shortly)

  • The Post Title block's outline is bugged, and it's rendered as a 0x0 shape, which can be seen as a little dot in the top-left corner of the block (the dot should be the outset outline I think).

  • The Query block plays a little wonky with the Persistent List View.
    In the list, it only shows the nested blocks for the most recent post in the loop, whereas in the canvas there might be several posts.
    Selecting one of the "other" posts updates the list view: the Query will show the nested blocks for the selected post in the loop.
    I'd probably expect to see the whole loop "unfurled" in the list view. 🤔

Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

I've noticed those issues as well.

I think it is worth merging this. There will no doubt be subsequent adjustments to further refine these interactions, but this feels like a step forwards overall.

@Copons
Copy link
Contributor Author

Copons commented Mar 11, 2021

Let me clean up the code a little before merging 🙂

@Copons
Copy link
Contributor Author

Copons commented Mar 11, 2021

Just FYI in 3f7b4eb I've:

  • Restored the selected branch background color (light gray).
  • Changed the list item hover from a light gray background to a dark gray outline. (Note: this also affects non-persistent List Views)
  • Stop marking the list item focus' double border as "experimental". (Note: this also affects non-persistent List Views)
  • Removed an inline comment that's now incorrect.

Later I'll update the PR description and proceed with the merge. :)

@jasmussen
Copy link
Contributor

I don't think this one is ready.

Here's before:

before site editor

before post editor

Here's after:

after post editor

after site editor editor

  • The hover was better when gray, the bordered hover is not used anywhere else in the editor
  • The selected state halo inside the blue box shadow is not necessary, the focus style on a selected item is already a shape change
  • I'm not convinced that showing the child blocks with a gray background actually helps legibility, and it's not something we use elsewhere.

Thank you for all the work, but I would not make any changes to how hover and focus works inside the list view, I'd keep the changes solely to inside the canvas.

@jameskoster
Copy link
Contributor

Two more very small details to address @Copons (sorry).

  • The last item in a branch should have a rounded bottom.
  • When an item inside a branch is selected but has no children of its own, it should be fully rounded.

@Copons
Copy link
Contributor Author

Copons commented Mar 11, 2021

@jameskoster @jasmussen If it's all right with you, I think it's probably easier to move the Block List changes into a different PR, merge that one, then rebase this and keep discussing/playing around with the List View style.

I'm totally cool with opening it first thing tomorrow, or @jameskoster could do it if you want to keep full authorship of your changes. 🙂

@jameskoster
Copy link
Contributor

jameskoster commented Mar 11, 2021

@Copons I think we can continue, this is close :D

The hover was better when gray, the bordered hover is not used anywhere else in the editor

I'll revert this and we can potentially revisit in the future.

The selected state halo inside the blue box shadow is not necessary, the focus style on a selected item is already a shape change

We spoke about this on Slack and found some consensus based on the fact that the white halo effect is used on a few other situations like selected + focussed Icon Buttons:

Screenshot 2021-03-11 at 17 44 19

I'm not convinced that showing the child blocks with a gray background actually helps legibility, and it's not something we use elsewhere.

I think this is particularly useful in this list view. In heavily nested blocks, when you scroll so that the parent is hidden (and indentation consequently becomes less intuitive), it helps to understand where the container ends.

Screenshot 2021-03-11 at 18 23 55

We could perhaps restrict this to list view only? I believe this is the final sticking point.

@jasmussen
Copy link
Contributor

I think we can continue, this is close :D

I think so too. I think the hover change is the one that needs further thought, as the additional borders feel like they detract from focus:

Screenshot 2021-03-11 at 18 46 31

Changing that back to a gray background hover addresses the biggest point. The rest we can look at.

jameskoster and others added 8 commits March 15, 2021 13:51
Hovered and highlighted blocks on the canvas now received a 1px dark grey outline rather than a 1.5px blue one. Blue is reserved for focus.

When a child is selected, the associated template part now receives a dotted border to differentiate from its hover and highlighted states.
Also adjusts the background color on children of the selected parent
@Copons Copons removed Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress labels Mar 15, 2021
Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

Testing

Requirements

  • In canvas, the Template Part block's outline becomes dotted when one of its children is selected.
  • In the List View, focused items gain a white halo inside their blue outline.
  • In canvas, the primary selection (selected, hovered, highlighted) outline changed to gray

@Copons I had a question about the canvas primary selection selected outline. I might be missing something, but could you elaborate on how to arrive in the selected state? I'm clicking on canvas blocks, and the outlines still appear to be blue?

Browsers

  • Firefox
  • Edge
  • Safari
  • Chrome
  • IE11

Note:

  • Didn't test IE11 (see here). It looks like a fix is currently in the works, so hopefully (or maybe not so hopefully? 😅) we can test it again soon.
  • There seems to be a noticeable delay when editing canvas text content with the persistent list view open. (Open the list view, click on a paragraph block, and start typing). Wondering if anyone else can reproduce this as well.
  • I saw some general navigation block select state weirdness in Safari, but I could replicate it on the master branch as well. It appears unrelated to the changes made here. I'll look for an existing issue and file one if it doesn't already exist first thing tomorrow morning.

Selecting and unselecting nav links unexpectedly modifies link opacity

2021-03-16 00 26 51

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

The code itself looks reasonable to me 👍

+1 from me when testing question is answered 🚀

@Copons
Copy link
Contributor Author

Copons commented Mar 16, 2021

@Copons I had a question about the canvas primary selection selected outline. I might be missing something, but could you elaborate on how to arrive in the selected state? I'm clicking on canvas blocks, and the outlines still appear to be blue?

@jeyip The blue outline is because the block is :focused.
It's super complicated to wrap the head around, but easy to test:

  • Open the List View and click on an item.
  • The corresponding block will be selected (grey outline) and focused (overriding blue outline).
  • Click on some white space (e.g. the header toolbar, or the List View title rectangle) to draw the focus away from the block, while leaving it selected.
  • The block is now just selected (grey outline).

Note that not all blocks work in the same way. Sometimes the blue outline show up when you start editing their text content. Sometimes they never have a blue outline somehow (the Navigation block, for example, has the blue outline, but the Navigation Link blocks inside it don't).

I'm not sure about the root of these inconsistencies, or if there's an intentional rule behind them.
Though, the PR doesn't really change their behaviours (as in, changing when a block is selected or focused), it only adjusts the states colors.

cc @jameskoster in case there's more to this. 🙂

@Copons
Copy link
Contributor Author

Copons commented Mar 16, 2021

There seems to be a noticeable delay when editing canvas text content with the persistent list view open. (Open the list view, click on a paragraph block, and start typing). Wondering if anyone else can reproduce this as well.

Seing this as well. 🤔
It's unrelated to this PR though, so it's a bug of the Persistent List View itself.
Going to investigate. Thanks for reporting!

I saw some general navigation block select state weirdness in Safari, but I could replicate it on the master branch as well. It appears unrelated to the changes made here. I'll look for an existing issue and file one if it doesn't already exist first thing tomorrow morning.

Yeah apparently there are several issues related to blocks states in Safari, as if a block state "sticks around" after the state changed.
Definitely happening in trunk as well, and anyway it's not something that would be affected by this PR.

@jasmussen
Copy link
Contributor

Yeah apparently there are several issues related to blocks states in Safari, as if a block state "sticks around" after the state changed.

Is that related to #29677 and/or #29678 ?

@Copons
Copy link
Contributor Author

Copons commented Mar 16, 2021

Yeah apparently there are several issues related to blocks states in Safari, as if a block state "sticks around" after the state changed.

Is that related to #29677 and/or #29678 ?

@jasmussen #29677!

@jeyip
Copy link
Contributor

jeyip commented Mar 16, 2021

The corresponding block will be selected (grey outline) and focused (overriding blue outline).

Oh I see! There's a difference between select and focus, and the focus border styles will override the select border styles. Gotcha 👍

@jameskoster Out of curiosity, has there been a discussion about the difference between select and focus states in Gutenberg outlined anywhere? Is it similar to what this external article describes?

  • A focused object is the one object in the entire operating system that receives keyboard input. The object with the keyboard focus is either the active window or a child object of the active window.
  • A selected object is marked to participate in some type of group operation.

Is that related to #29677 and/or #29678 ?

Thanks @jasmussen 🙏

I'm still unsure about the background for children, and I think our focus styles could use a holistic looking at so we don't write 100 different focus style rules. But that seems easy to revisit.

Is it worthwhile to make an issue for the background for children, or will we revisit it naturally in the course of developing the full site editor?

@jameskoster
Copy link
Contributor

@jeyip good question! Not that I am aware of. The descriptions you shared seem quite closely aligned with what we have in the block editor though.

Clicking an object (an on-canvas block in this case) will select and focus it. A selected and focussed block can be directly manipulated, IE when you click a paragraph block you can begin typing immediately.

The focus state can move independently of the select state to a variety of different elements such as the block toolbar, the list view, or the inspector. In all of these cases the on-canvas block remains selected (and we need to indicate this visually) so that users are able to comprehend that it is the target of operations contained within the aforementioned elements – so things like grouping, transforms, text formatting, layout adjustments, and so on.

I know y'all know this. But it can be easy to forget if you aren't performing these actions keyboard-only on a day to day basis 😅

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

This is a very nice improvement overall! 🚀

I noticed two issues while testing, but both of them seem to be edge cases that can be addressed later in my opinion.

Mar-16-2021.20-04-51.mp4
  1. After opening the navigator with keyboard shortcuts and pressing tab a few times the footer TP is selected, but its contents are not rendered properly and gray background is shown instead.

  2. After selecting the header for example and using down arrow to focus other top level groups, the focused item is not scrolled into view and goes outside of the viewable area.

@Copons
Copy link
Contributor Author

Copons commented Mar 16, 2021

  1. After opening the navigator with keyboard shortcuts and pressing tab a few times the footer TP is selected, but its contents are not rendered properly and gray background is shown instead.

@vindl The TreeGrid component (which powers BlockNavigation) is navigable with arrows instead of tab, so when you tab, you are actually moving out of the list and, in this case, into the canvas, selecting the top level blocks.
For some reason, the content below the fold doesn't render when navigating via tab.
I'd say it's not related to the List View at all, though; most likely it's a Site Editor issue. 🤔

  1. After selecting the header for example and using down arrow to focus other top level groups, the focused item is not scrolled into view and goes outside of the viewable area.

That's because list scroll and canvas scroll are not "in sync".
After you tab away from the List View and into the canvas, you end up navigating through the canvas.
Eventually you find your way to the Footer. The canvas scrolls appropriately (even though the content is not rendered); but the List doesn't scroll at all.

It's something that bugs me as well, to be honest, and I've raised it in the past.
We eventually decided to not do anything about it, as it might be counterproductive.
For example, you might want to compare the list at a scroll point with the canvas at another scroll point, or (in the future) drag a block from the list to the canvas.
Locked scrolls might get in the way, or even make it impossible altogether.

@Copons Copons merged commit 3e88a6a into trunk Mar 16, 2021
@Copons Copons deleted the try/hover-focus-select branch March 16, 2021 19:32
@github-actions github-actions bot added this to the Gutenberg 10.3 milestone Mar 16, 2021
@vindl
Copy link
Member

vindl commented Mar 17, 2021

@vindl The TreeGrid component (which powers BlockNavigation) is navigable with arrows instead of tab, so when you tab, you are actually moving out of the list and, in this case, into the canvas, selecting the top level blocks.

I tested the arrow navigation and tried this after - with the current state/styles it appears as though it's working with tab too, even though under the hood it's focusing things in the canvas, so I got confused by it. :)

For example, you might want to compare the list at a scroll point with the canvas at another scroll point, or (in the future) drag a block from the list to the canvas. Locked scrolls might get in the way, or even make it impossible altogether.

Thanks for clarifying, makes sense now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persistent List View: Update hover, focus, select styles to make the focus clear in relation to the canvas
7 participants