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: Try directing focus to the list view toggle button when closing the list view #54175

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Sep 5, 2023

What?

This is an exploration into #54165 to see if we can manually direct focus to the list view toggle button when the list view is closed.

For reference, the keyboard shortcut for closing the list view is alt+shift+o on Windows, and control+option+o on Mac.

Why?

As raised in #54165, it is confusing for the user that sometimes when closing the list view, focus is lost completely. This happens most often when the list view was opened via the keyboard shortcut and is then closed after blocks have been deselected. There could be circumstances that exist not covered by this PR, but the hope is that this PR will improve things when closing the list view.

How?

Try using state in editor's layout, and pass down a setter to the heading in order to set a ref for the toggle button. Then, pass down the state that's keeping track of the toggle button element to the list view sidebar.

This requires a bit of prop drilling, as well as duplication between the editors. Because the editors are fairly different, I couldn't quite think of a better way to do it. I'm very happy to try out ideas if folks have ideas for a better way to do this!

Note: this PR previously only shifted focus if no blocks are selected. Based on discussion in the PR comments, I've since updated it to always shift focus when the list view is closed, to hopefully achieve more consistency.

Testing Instructions

  1. Open the post editor and add at least one block. With the block selected in the canvas, open the list view.
  2. Press the Escape key once to deselect blocks.
  3. Press the Escape key again to close the list view. The focus should now be on the list view button in the header.
  4. Select a block in the canvas and open the list view via the keyboard shortcut (alt+shift+o on Windows, and control+option+o on Mac)
  5. Press the Escape key once to deselect blocks.
  6. Use the keyboard shortcut to close the list view again. Focus should now be on the list view button in the header.
  7. Repeat the above in the site editor.

Note: if closing the list view via the keyboard shortcut while blocks are still selected, then the list view should close and focus won't be explicitly moved to the list view toggle button (but it might go there anywhere depending on other logic already in place).

Screenshots or screencast

The below screengrab demos selecting a block in the list view and using the escape key to deselect and then close the list view, with focus ending up on the list view toggle button. Then, the list view is opened using the keyboard shortcut, with the blocks deselected using the escape key, and then finally the list view is closed via the keyboard shortcut again, with focus ending up on the list view toggle button.

2023-09-18.16.41.16.mp4

@andrewserong andrewserong added [Type] Enhancement A suggestion for improvement. [Feature] List View Menu item in the top toolbar to select blocks from a list of links. labels Sep 5, 2023
@andrewserong andrewserong self-assigned this Sep 5, 2023
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

Size Change: +147 B (0%)

Total Size: 1.62 MB

Filename Size Change
build/edit-post/index.min.js 35.5 kB +42 B (0%)
build/edit-site/index.min.js 181 kB +68 B (0%)
build/edit-widgets/index.min.js 17 kB +37 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 955 B
build/annotations/index.min.js 2.69 kB
build/api-fetch/index.min.js 2.28 kB
build/autop/index.min.js 2.1 kB
build/blob/index.min.js 451 B
build/block-directory/index.min.js 7.03 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 kB
build/block-editor/content-rtl.css 4.25 kB
build/block-editor/content.css 4.24 kB
build/block-editor/default-editor-styles-rtl.css 381 B
build/block-editor/default-editor-styles.css 381 B
build/block-editor/index.min.js 217 kB
build/block-editor/style-rtl.css 15.4 kB
build/block-editor/style.css 15.4 kB
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 126 B
build/block-library/blocks/audio/theme.css 126 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 104 B
build/block-library/blocks/avatar/style.css 104 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 584 B
build/block-library/blocks/button/editor.css 582 B
build/block-library/blocks/button/style-rtl.css 629 B
build/block-library/blocks/button/style.css 628 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 421 B
build/block-library/blocks/columns/style.css 421 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 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.69 kB
build/block-library/blocks/cover/style.css 1.68 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 98 B
build/block-library/blocks/details/style.css 98 B
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 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/style-rtl.css 311 B
build/block-library/blocks/file/style.css 312 B
build/block-library/blocks/file/view.min.js 318 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/freeform/editor-rtl.css 2.61 kB
build/block-library/blocks/freeform/editor.css 2.61 kB
build/block-library/blocks/gallery/editor-rtl.css 947 B
build/block-library/blocks/gallery/editor.css 952 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 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 336 B
build/block-library/blocks/html/editor.css 337 B
build/block-library/blocks/image/editor-rtl.css 834 B
build/block-library/blocks/image/editor.css 833 B
build/block-library/blocks/image/style-rtl.css 1.42 kB
build/block-library/blocks/image/style.css 1.41 kB
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/image/view.min.js 1.83 kB
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 505 B
build/block-library/blocks/media-text/style.css 503 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 668 B
build/block-library/blocks/navigation-link/editor.css 669 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 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation/editor-rtl.css 2.26 kB
build/block-library/blocks/navigation/editor.css 2.26 kB
build/block-library/blocks/navigation/style-rtl.css 2.23 kB
build/block-library/blocks/navigation/style.css 2.22 kB
build/block-library/blocks/navigation/view.min.js 984 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 235 B
build/block-library/blocks/paragraph/editor.css 235 B
build/block-library/blocks/paragraph/style-rtl.css 335 B
build/block-library/blocks/paragraph/style.css 335 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 508 B
build/block-library/blocks/post-comments-form/style.css 508 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 319 B
build/block-library/blocks/post-featured-image/style.css 319 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 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 314 B
build/block-library/blocks/post-template/style.css 314 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 125 B
build/block-library/blocks/preformatted/style.css 125 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 168 B
build/block-library/blocks/pullquote/theme.css 168 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 302 B
build/block-library/blocks/query-pagination/style.css 299 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 486 B
build/block-library/blocks/query/editor.css 486 B
build/block-library/blocks/query/style-rtl.css 375 B
build/block-library/blocks/query/style.css 372 B
build/block-library/blocks/query/view.min.js 555 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 178 B
build/block-library/blocks/search/editor.css 178 B
build/block-library/blocks/search/style-rtl.css 607 B
build/block-library/blocks/search/style.css 607 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/search/view.min.js 468 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 323 B
build/block-library/blocks/shortcode/editor.css 323 B
build/block-library/blocks/site-logo/editor-rtl.css 754 B
build/block-library/blocks/site-logo/editor.css 754 B
build/block-library/blocks/site-logo/style-rtl.css 204 B
build/block-library/blocks/site-logo/style.css 204 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 682 B
build/block-library/blocks/social-links/editor.css 681 B
build/block-library/blocks/social-links/style-rtl.css 1.44 kB
build/block-library/blocks/social-links/style.css 1.43 kB
build/block-library/blocks/spacer/editor-rtl.css 348 B
build/block-library/blocks/spacer/editor.css 348 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 432 B
build/block-library/blocks/table/editor.css 432 B
build/block-library/blocks/table/style-rtl.css 639 B
build/block-library/blocks/table/style.css 639 B
build/block-library/blocks/table/theme-rtl.css 146 B
build/block-library/blocks/table/theme.css 146 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 403 B
build/block-library/blocks/template-part/editor.css 403 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/term-description/style-rtl.css 111 B
build/block-library/blocks/term-description/style.css 111 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 185 B
build/block-library/blocks/video/style.css 185 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.1 kB
build/block-library/common.css 1.1 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12.2 kB
build/block-library/editor.css 12.1 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 205 kB
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/style-rtl.css 13.9 kB
build/block-library/style.css 13.9 kB
build/block-library/theme-rtl.css 688 B
build/block-library/theme.css 693 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/blocks/index.min.js 51.3 kB
build/commands/index.min.js 15.5 kB
build/commands/style-rtl.css 921 B
build/commands/style.css 918 B
build/components/index.min.js 255 kB
build/components/style-rtl.css 11.7 kB
build/components/style.css 11.7 kB
build/compose/index.min.js 12.7 kB
build/core-commands/index.min.js 2.6 kB
build/core-data/index.min.js 17 kB
build/customize-widgets/index.min.js 12 kB
build/customize-widgets/style-rtl.css 1.48 kB
build/customize-widgets/style.css 1.48 kB
build/data-controls/index.min.js 640 B
build/data/index.min.js 8.84 kB
build/date/index.min.js 17.8 kB
build/deprecated/index.min.js 451 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.64 kB
build/edit-post/classic-rtl.css 544 B
build/edit-post/classic.css 545 B
build/edit-post/style-rtl.css 7.84 kB
build/edit-post/style.css 7.83 kB
build/edit-site/style-rtl.css 13.8 kB
build/edit-site/style.css 13.8 kB
build/edit-widgets/style-rtl.css 4.8 kB
build/edit-widgets/style.css 4.79 kB
build/editor/index.min.js 45.6 kB
build/editor/style-rtl.css 3.53 kB
build/editor/style.css 3.52 kB
build/element/index.min.js 4.82 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 7.74 kB
build/format-library/style-rtl.css 554 B
build/format-library/style.css 553 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.58 kB
build/interactivity/index.min.js 11.3 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.72 kB
build/keycodes/index.min.js 1.87 kB
build/list-reusable-blocks/index.min.js 2.2 kB
build/list-reusable-blocks/style-rtl.css 836 B
build/list-reusable-blocks/style.css 836 B
build/media-utils/index.min.js 2.9 kB
build/notices/index.min.js 948 B
build/nux/index.min.js 1.99 kB
build/nux/style-rtl.css 735 B
build/nux/style.css 732 B
build/patterns/index.min.js 3.35 kB
build/patterns/style-rtl.css 302 B
build/patterns/style.css 302 B
build/plugins/index.min.js 1.79 kB
build/preferences-persistence/index.min.js 1.84 kB
build/preferences/index.min.js 1.24 kB
build/primitives/index.min.js 975 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 958 B
build/react-i18n/index.min.js 615 B
build/react-refresh-entry/index.min.js 9.47 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.7 kB
build/reusable-blocks/index.min.js 2.7 kB
build/reusable-blocks/style-rtl.css 243 B
build/reusable-blocks/style.css 243 B
build/rich-text/index.min.js 10.2 kB
build/router/index.min.js 1.78 kB
build/server-side-render/index.min.js 1.94 kB
build/shortcode/index.min.js 1.39 kB
build/style-engine/index.min.js 1.97 kB
build/sync/index.min.js 53.8 kB
build/token-list/index.min.js 582 B
build/url/index.min.js 3.73 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 958 B
build/warning/index.min.js 249 B
build/widgets/index.min.js 7.16 kB
build/widgets/style-rtl.css 1.15 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.02 kB

compressed-size-action

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

Flaky tests detected in 22f6851.
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/6244591400
📝 Reported issues:

@alexstine
Copy link
Contributor

This approach is messy but makes sense. I cannot think of a better way to do this either. Do you have the ability to check if a block is not selected from edit-post package? Is there any cross-over between block-editor/edit-post stores?

@andrewserong
Copy link
Contributor Author

Oh, thanks for taking an early look, Alex! My plan is to add a check to see if there are no blocks selected before attempting to shift focus, which I think we should be able to do. My next step is to see if there's a slightly neater way to handle it, but in principle I think we'll need to find some way to pass the reference to the element along to the list view sidebar as in this PR. Whichever way we go with, we'll likely then want to duplicate the approach in the site editor.

Will let you know how I go!

@andrewserong andrewserong force-pushed the try/list-view-set-focus-on-toggle-button-when-closed-by-escape-key branch from 6e32ac2 to 0d5d016 Compare September 18, 2023 05:48
@andrewserong andrewserong marked this pull request as ready for review September 18, 2023 06:45
@andrewserong
Copy link
Contributor Author

Update: I think this PR is in a good place for testing / feedback now. I think it's working pretty well, but unfortunately I haven't been able to figure out a way to do this without a fair bit of prop drilling in order to get a reference to the list view toggle button. Very happy for any feedback if folks have other ideas for how to implement this!

Currently this PR just applies to the post and site editors. If the approach seems okay, then I can also implement it in the widget editor, too, for consistency.

@@ -259,6 +259,7 @@ export default function HeaderEditMode() {
/* translators: button label text should, if possible, be under 16 characters. */
label={ __( 'List View' ) }
onClick={ toggleListView }
ref={ setListViewToggleElement }
Copy link
Member

Choose a reason for hiding this comment

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

Passing a useState callback seems fine to me. It's a stable reference.

I was just wondering if it would cause some cognitive confusion for anyone in the future trying to access current.

Another question might be: is listViewToggleElement (the state value) a value that’s needed for rendering the component in which it lives, and would it trigger an unnecessary re-render if set?

This is all purely academic, and I'm only raising because I don't know myself! Something like this might be an alternative to useState in layout for example.

	const listViewToggleElementRef = useRef( null );
	const setListViewToggleElementRef = useCallback( ( node ) => {
		if ( node !== null ) {
			listViewToggleElementRef.current = node;
		}
	}, [] );

Copy link
Contributor Author

@andrewserong andrewserong Sep 19, 2023

Choose a reason for hiding this comment

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

Thanks for taking a look @ramonjd, and good question! I mostly went with useState for consistency with some of the existing places where we're using ref callbacks to store elements in state (e.g. setDropZoneElement here). In terms of trying to avoid confusion and whether or not current is available, I think so long as the variable is named with Element as its suffix instead of Ref, it'll hopefully be clear enough — or at least consistent enough with the other usage in the repo. Did you have any instances in mind where folks might reach for current and get tripped up?

In terms of re-renders, one of the benefits of going with state is that the consumer (list-view-sidebar.js) is fairly straightforward in that we can place listViewToggleElement in the dependency arrays of useCallbacks and know that those callbacks will always be using the appropriate current value, without having to do any further introspection.

Some of the feedback I've gotten on previous PRs that looked at using refs with elements were to prefer using state... looking the references for that up now (if you'll excuse the pun 😄), I think the linked docs were in the floating UI library, so was more to do with working with popovers (docs, and talked about in #43335), so might not be strictly speaking applicable here.

In any case, from the perspective of the list view sidebars, it feels a tiny bit neater to me if we're accessing the element directly in the component's props, rather than using a ref there, but happy to look into changing things if folks would prefer, especially if I've accidentally introduced re-renders in this PR where I shouldn't have!

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the follow up explanation. Good to know there's a precedent. I've seen it in other libraries too, so TIL it's a thing for Gutenberg too. 🙇🏻

Copy link
Member

@ramonjd ramonjd 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 working well for me, and I think satisfies the lack of focus. Thanks!

The test steps work as described in post and site editors 👍🏻 I'll defer to @alexstine for an expert opinion.

In my demo videos below I've focussed on some random element (the preview button).

Before

In trunk, opening the list view leaves the focus on the preview button after toggling it closed.

2023-09-19.12.37.34.mp4

After

In this PR, opening the list view moves the focus to the list view button after toggling it closed.

2023-09-19.12.38.22.mp4

I left a question about useState as a ref callback.

@alexstine
Copy link
Contributor

@andrewserong

Note: if closing the list view via the keyboard shortcut while blocks are still selected, then the list view should close and focus won't be explicitly moved to the list view toggle button (but it might go there anywhere depending on other logic already in place).

This is indeed a bug. If a block is selected and you close the list view with the shortcut, focus moves to the toggle instead of the selected block. If this is too hard to fix, let's not try and we'll just ensure that closing the list view always results in placing focus on the trigger.

Thanks.

Copy link
Contributor

@apeatling apeatling left a comment

Choose a reason for hiding this comment

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

This worked exactly as the steps describe in both the post and site editors. Assuming everything else is good, I think this LGTM!

@andrewserong
Copy link
Contributor Author

If this is too hard to fix, let's not try and we'll just ensure that closing the list view always results in placing focus on the trigger.

Thanks for the feedback @alexstine! From a bit of manual testing, I think the focus loss when closing the list view via the keyboard shortcut is to do with the iframed editor canvas. This appears to cause the focus to be directed to the iframe instead of the element that the user was in within the editor canvas. If I force the editor to no longer be iframed by enabling custom fields in the editor preferences, I see that the focus is working as it used to.

It does seem like it could be tricky to try to fix that for the iframed editor. Just to make sure I'm following along, is your idea that we could remove the existing calls to useFocusReturn and instead make the keyboard shortcut to close the list view always redirect focus to the toggle button? I think that could simplify things a little, too.

I'm happy to give that a try, but let me know if that's not quite what you meant and I can backtrack or revert any changes along the way.

@andrewserong
Copy link
Contributor Author

andrewserong commented Sep 20, 2023

Update: in 8f25ba0 I've had a go at removing the useFocusReturn calls and instead explicitly direct focus back to the list view toggle button. This appears to be more consistent in usage to me, and simplifies things a little, but I'm very happy to revert the change if this feels broken to anyone.

Thanks again for the feedback and reviews, everyone! I've also rolled out the change to the widgets editor since there hasn't been any real negative feedback about the prop drilling so far.

As always, happy to keep iterating 🙂

@andrewserong
Copy link
Contributor Author

Second update: I've updated the e2e tests to reflect focus returning to the list view toggle button.

@andrewserong andrewserong force-pushed the try/list-view-set-focus-on-toggle-button-when-closed-by-escape-key branch from c35e4ef to 22f6851 Compare September 20, 2023 05:59
@draganescu
Copy link
Contributor

This PR works great 👏🏻

It's perfect when using the ESC shortcut - once to deselect all blocks, once to close the list view, focus lands on the list view toggle, magic 🌟

Nevertheless, I was a bit annoyed on losing the block focus when using the ctrl option O shortcut. In trunk it's worse than this PR tho, the focus being .. nowhere.

Can we make it that the keyboard shortcut returns the focs to where it was before? CC @jeryj ?

@jeryj
Copy link
Contributor

jeryj commented Sep 20, 2023

Can we make it that the keyboard shortcut returns the focus to where it was before?

I'm fine with whatever! I have no reference of what is "standard." I really like having a global shortcut to move focus back to whatever was focused within the block editor , and it worked really well as part of this exploration.

@alexstine and I are looking at adding the last focused element + fallback to the block editor redux store, which would make a global shortcut easy to add. We'd love some feedback on the approach there!

@alexstine
Copy link
Contributor

I am fine with either. If it is too hard to focus the last position, let's not. In a perfect world, we would only focus the trigger if no blocks were selected but happy to wait until later to implement that. For now, as long as focus is consistent, it will be a big improvement over focus loss.

@andrewserong
Copy link
Contributor Author

Thanks for the reviews, testing, and feedback, folks!

In trunk it's worse than this PR tho, the focus being .. nowhere.

Yes, unfortunately I found it a bit of a rabbithole to investigate due to the presence of the iframed editor. Given the timing for the WP 6.4 feature freeze, I think it'd be better to merge this PR as-is for now as it improves the consistency of focus, and it's preferable to have the focus be somewhere useful than nowhere at all, as you mention.

@alexstine and I are looking at adding the #54443, which would make a global shortcut easy to add.

That approach does look promising. Let's merge this PR in for now, and we can see if an approach using the last focus state from #54443 could be used in a follow-up.

Thanks again! 🙇

@andrewserong andrewserong merged commit aa8ec5b into trunk Sep 20, 2023
50 checks passed
@andrewserong andrewserong deleted the try/list-view-set-focus-on-toggle-button-when-closed-by-escape-key branch September 20, 2023 23:48
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 20, 2023
@andrewserong andrewserong added the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 20, 2023
mikachan pushed a commit that referenced this pull request Sep 22, 2023
…osing the list view (#54175)

* List View: Try directing focus to the list view toggle button when closing the list view

* Only focus if no blocks are selected

* Also use focus logic for when the list view is closed via the keyboard shortcut

* Try implementing in the site editor, too

* Always return focus to the toggle whenever the list view is closed, roll out to widgets editor

* Update e2e tests
@mikachan
Copy link
Member

I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 49b9cff

@mikachan mikachan removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 22, 2023
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. [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

List View: Ensure consistent focus when closing the list view when no blocks are selected
7 participants