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.

Sign up for GitHub

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

Jump to bottom

Persist focus when tabbing back to the block toolbar #25760

Merged
merged 6 commits into from
Oct 30, 2020
Merged

Persist focus when tabbing back to the block toolbar #25760

merged 6 commits into from
Oct 30, 2020

Conversation

diegohaz
Copy link
Member

@diegohaz diegohaz commented Oct 1, 2020 •
edited
Loading

Fixes #24469

This PR adds a new experimental feature to the block toolbar so it "remembers" the last focused toolbar item when the user Tabs back to the toolbar after it is unmounted.

Tabbing in and out the block toolbar with focus persistence

How to test

  1. Create any block
  2. Press Shift+Tab to move focus to the block toolbar
  3. Press arrow keys to move focus to a different toolbar button
  4. Press Tab to move focus to the block contents
  5. Do some action so as to hide the toolbar (eg. typing on the Paragraph block)
  6. Press Shift+Tab to move focus back to the block toolbar
  7. Check if the focus is persisted

Also, see the added e2e test.

What's not covered by this PR

  • The alternative discussed in Improve the toolbars roving tabindex #24469 (comment) to make the toolbar only visually hidden isn't implemented in this PR. This would require a lot of changes as the app as a whole seems to rely on this mounting behavior.
  • Alt+F10 behavior is unchanged.

@github-actions
Copy link

github-actions bot commented Oct 1, 2020 •
edited
Loading

Size Change: +206 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/block-editor/index.js 130 kB +206 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.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.98 kB 0 B
build/block-library/editor.css 8.98 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/style-rtl.css 7.83 kB 0 B
build/block-library/style.css 7.84 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.38 kB 0 B
build/edit-post/style.css 6.37 kB 0 B
build/edit-site/index.js 22.1 kB 0 B
build/edit-site/style-rtl.css 3.86 kB 0 B
build/edit-site/style.css 3.85 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/edit-widgets/style-rtl.css 3.1 kB 0 B
build/edit-widgets/style.css 3.1 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@diegohaz diegohaz changed the title Initial implementation for persisting block toolbar focus Persist block toolbar focus between re-mounts Oct 1, 2020
@diegohaz diegohaz changed the title Persist block toolbar focus between re-mounts Persist focus when tabbing back to the block toolbar Oct 10, 2020
@diegohaz diegohaz added [a11y] Keyboard & Focus [Package] Block editor /packages/block-editor [Type] Experimental Experimental feature or API. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Oct 10, 2020
@diegohaz diegohaz self-assigned this Oct 10, 2020
@diegohaz diegohaz marked this pull request as ready for review October 10, 2020 01:50
@diegohaz diegohaz requested a review from afercia October 10, 2020 01:50
@gziolo gziolo self-requested a review October 20, 2020 08:22
@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

I found one edge case when testing:

toolbar-focus

The issue is that the index is persisted even when you move to another block. It wouldn't be a bad idea if those blocks would have the same set of toolbar items, but they differ a lot.

Would it be feasible to use at least the block type name to ensure that it doesn't produce unexpected behaviors? Well, it might be even necessary to use the block client id as a way to determine if the persisted initial index should be used.

@diegohaz
Copy link
Member Author

Thanks, @gziolo! I confirmed the issue and it should be fixed now (hopefully, if e2e tests pass).

I think we have some options to persist the active toolbar item index:

  • Persist only for the currently active block and reset when clientId changes (current solution).
  • Persist per clientId (using a map) and don't reset.
  • Persist only while navigating through blocks of the same type. The problem here is that same-type blocks may also have different toolbars, which I guess would make this much more complicated.
  • Persist per block type (using a map) and don't reset. The same problem above.

For now, I think the first one is the simplest option, but we can explore the others if it's not enough.

@gziolo
Copy link
Member

gziolo commented Oct 30, 2020

I see some failing e2e tests on node 1 but they exists in master branch as well.

I'm fine with the simplest solution proposed. In fact, it might be the most expected behavior if you think about it from the user's perspective. When you edit a block then you are in the "Edit mode", so going back and forth between the toolbar and content is simplified when the last position is persisted. As soon as you move focus to another block you start fresh and might be easier to navigate starting from the beginning of the toolbar, especially if the number and type of items differ between blocks. Again, when you move back to the block you edited previously, you probably won't remember what was your action, so having the focus on the start of the block is the most reasonable in my opinion.

gziolo
gziolo approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It works as advertised with the last changes applied. Nice work on this improvement 👍

@diegohaz diegohaz merged commit 5209577 into master Oct 30, 2020
@diegohaz diegohaz deleted the try/persist-toolbar-focus branch October 30, 2020 12:20
@github-actions github-actions bot added this to the Gutenberg 9.3 milestone Oct 30, 2020
@youknowriad youknowriad added [Type] Enhancement A suggestion for improvement. and removed [Type] Experimental Experimental feature or API. labels Nov 3, 2020
@jeryj jeryj mentioned this pull request May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@ellatrix ellatrix Awaiting requested review from ellatrix

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

@afercia afercia Awaiting requested review from afercia

Assignees

@diegohaz diegohaz

Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 9.3
Development

Successfully merging this pull request may close these issues.

Improve the toolbars roving tabindex
3 participants
@diegohaz @gziolo @youknowriad