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

Editor: Move block focus restore to Block Toolbar #10529

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 11, 2018

Fixes #6165

This pull request seeks to resolve an issue where pressing Escape while within the block toolbar would not predictably return focus to the block in all browsers. This is because the text selection is lost when focus shifts in some but not all browsers. The revised approach here tracks the active element at the time focus is shifted into the toolbar, on which focus is called when Escape is pressed.

In making these changes, I also observed we had two NavigableToolbar on the page when the contextual (non-Unified) block toolbar is present, both competing for handling of the Alt+F10 keybind, each with different behavior (focusing to the first header control vs. first block control). These changes move the keybind handling into BlockToolbar to ensure it is executed once, consistently shifting focus to the first block control. This has the impact that if a user wants to access the first header control by keyboard, they now use Ctrl + ~ navigable region or Shift + Tab reverse tab from the Unified Toolbar.

Open question: If focus is not programmatically shifted into the toolbar (e.g. click on button) and the user proceeds to press Escape, we don't (and have not) directed focus back into the block. Would this be desirable?

Testing instructions:

Verify that you can Alt+F10 into the selected block's toolbar and Escape to return focus to the block. Notably, ensure this works in Safari and IE11.

Note that in Safari, since the selection is lost when focus shifts, the caret may be reset to the beginning of the block's text field when focus is returned.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Browser Issues Issues or PRs that are related to browser specific problems labels Oct 11, 2018
@aduth aduth added this to the 4.1 milestone Oct 11, 2018
@aduth aduth requested a review from afercia October 11, 2018 17:35
@afercia
Copy link
Contributor

afercia commented Oct 12, 2018

In making these changes, I also observed we had two NavigableToolbar on the page when the contextual (non-Unified) block toolbar is present, both competing for handling of the Alt+F10 keybind, each with different behavior (focusing to the first header control vs. first block control).

In this regard, I think there can be up to 3 toolbars at the same time because the inline toolbars should work like the block toolbars (and with nested blocks even more?) all competing for Alt+F10. See #6302 whichi highlighted this issue (PR is a bit old, from April).

@aduth
Copy link
Member Author

aduth commented Oct 12, 2018

Ah, I missed this one.

This pull request fixes the issue by making the keybind work only for the block's toolbar, but in that sense regresses the keyboard availability of the inline formatting toolbar.

Considering what would be the desired behavior, maybe Alt+F10 should step upward to the nearest (virtual, React hierarchy) navigable toolbar? Repeated presses to go from Inline Formatting > Block > Editor header.

@afercia
Copy link
Contributor

afercia commented Oct 12, 2018

Alt+F10 should step upward to the nearest (virtual, React hierarchy) navigable toolbar?

I've considered that, though I don't have the skills to implement it. I'd say if there's no other way to make an RichText have some knowledge of the associated toolbar, that could be an option?

@aduth
Copy link
Member Author

aduth commented Oct 12, 2018

Alt+F10 should step upward to the nearest (virtual, React hierarchy) navigable toolbar?

A challenge here is that there is no relationship which can be inferred (e.g. no ancestry) between the active element and its "closest" toolbar.

I'd think to pull this off, we'd need some combination of:

  • A React Context Provider or KeyboardShortcuts with children which acts as a scope in which Alt+F10 presses are associated to a specific toolbar
  • A pattern to associate the above with its NavigableToolbar
    • Just a simple unique string identifier? Works okay for e.g. edit-post-header, block, formatting, but becomes more difficult if we have to start dealing with nested blocks
    • A factory function which produces a pair of components, NavigableToolbar and NavigableToolbarScope, which are internally associated?

@aduth
Copy link
Member Author

aduth commented Oct 17, 2018

Closing as superseded by #10699

@aduth aduth closed this Oct 17, 2018
@aduth aduth deleted the fix/safari-toolbar-esc branch October 17, 2018 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS TypeError: Safari and IE11: Escape doesn't move focus back from the toolbar to the block content
2 participants