Editor: Add KeybindScope complement to NavigableToolbar

[aduth]

Supersedes #10529
Fixes #6165

This pull request seeks to revise the behavior of NavigableToolbar from implementing a global keybind to instead observe key combinations within a specific scope. Further, in restoring focus after an Escape press from the toolbar, it considers the element which had focus before focus had shifted to the toolbar programmatically. The scoping also allows for "stepping" behavior, where Alt+F10/Escape respectively work their way up/down to/from the nearest navigable toolbar scope.

Implementation notes:

• To accommodate the lack of a visible toolbar while "typing", the typing observer now interprets the Escape key press as intent to cancel typing (thus displaying toolbar).
  • I had also considered refactoring ObserveTyping to always toggle the typing flag on/off if a key pressed is not a "typing start". This turned out to have too many edge cases to consider here.
• Most of the included commits can (should?) be split out to their own pull requests in order to ease review

Remaining tasks:

• End-to-end case(s)

Testing instructions

Verify that, with toolbars present, Alt+F10 sequentially steps upward to the "closest" toolbar. Conversely, Escape steps back down.

[youknowriad]

My ideal solution would be to avoid this entirely and "show" the toolbar when alt + f10 is hit. I don't know if that's possible though.

[aduth]

I'd agree, and see this more as an interim step in the right direction (i.e. at least making it possible by keyboard-only). I'd put some thought toward what would be required to focus the non-visible contextual toolbar and I think it'll require some non-trivial state/lifecycle wizardry in BlockListBlock to have it be forced visible / focused.

Roughly, I imagine it looking like:

• NavigableToolbar.KeybindScope has a onShortcutPressed callback
• BlockContextualToolbar / BlockToolbar accepts an autoFocus prop which behaves similarly to the equivalently-named input attribute
• BlockListBlock assigns a callback for this which somehow incurs a new render which triggers consideration in shouldShowContextualToolbar to force it to be shown, and somehow assigns the autoFocus prop.

I'd like to reserve this for a subsequent pull request.

[youknowriad]

Question: what happens when you tab into the toolbar (you didn't access it using alt+f10) and you hit Escape. I guess nothing? Is it possible to make it so we focus the first tabbable element in the block's content. This would be handy for the "image" block for instance in the demo content test, where we'd be able to hit this "block content shortcut" to focus the buttons to upload media.

[aduth]

Is it possible to make it so we focus the first tabbable element in the block's content.

As implemented, the relationship is not two-way except in tracking the element from which focus had transitioned when Alt+F10 was pressed, so it's not very compatible with this suggestion.

I guess this depends whether we consider this more a transient state of shifted focus, or a strict relationship exists between two elements. For something like the header toolbar, is there a singular element it's expected to return to when Escape is pressed? The more I think on it, the more I think the answer is "no".

[youknowriad]

I've been thinking about this lately too, I was trying to think what would be an alternative and I came up with this. Let me know what you think:

• We do have an EditorGlobalShortcuts component or something like that to handle most of the editor shortcuts.
• What if NavigableMenu is kept only for arrow navigation and we move the alt+f10 handling into EditorGlobalShortcuts
• The potential shortcut to handle navigation to the block's content could be handled there as well.
• Actually, I was even thinking about a shortcut for the block's inspector too. (These three items: toobar, content and inspector are the three elements where we edit the block, and it seems that we should treat them similarily).
• For this to work, the EditorGlobalShortcuts needs to access the block's toolbar container node and the block's content container node: Maybe these could be set using context?

I'm mostly trying to think how I'd approach it to make it as simple as possible but now that I'm writing this, it does feel a bit complex too.

Anyway, I'll plan to dive deeper into this PR to understand your proposal better.

Edit: I wonder if alt+f10 should actually move to the toolbar, then the inspector and then the content (kind of like the region navigation)

[aduth]

Importantly, this supports more than just a block toolbar, but also considers:

• The inline formatting bar
• The header toolbar
• Toolbars in a nested context (or at least this is intended, tests pending)

[aduth]

To simplify this some, I extracted ObserveTyping's handling of Escape to #10906

[aduth]

Okay, I think this one should be ready for a proper review.

I'd like to have authored the end-to-end tests to verify the behavior in an inline toolbar, but between embeds mocking and potential for focus loss in embeds, I'm feeling blocked at the moment. I can create a follow-up issue for it, if it's acceptable to let the current included tests serve as a basis.

[aduth]

#11237 messed with this one a fair bit. I had to undo the addition there of focusing the wrapper. The alternative would be to move the NavigableToolbar.KeybindScope to wrap the entire block, but this would interfere with toolbar behavior as well. The Alt+F10 keybind still works with that removal. (cc @youknowriad)

However, there is a bug which remains, affecting both this branch and master, which is the behavior of "Escape" in clearing the selected block. This has some strange effects in causing an error and the selection to remain in first multi-selected block (if applicable, e.g. paragraphs) but no longer technically being selected (proceeding to press Shift+ArrowDown does nothing). I think we'll need to have a more durable handling of the propagation of the Escape press between the block toolbar, multi-selection, and top-level block selection clearing.

[chrisvanpatten]

I can't speak to the code but just want to observe that, as depicted in the screen capture, it feels strange that esc moves you down into a "deeper" nested toolbar context. I would actually expect the reverse, that esc brought me up to the higher up block level.

[aduth]

@chrisvanpatten Maybe true. The concern of the pull request is to normalize current behaviors though of contextual toolbars.

[gziolo]

This PR needs to be refreshed. It's over 3 months old so I'm marking as Stale to make triaging easier. Feel free to remove it when PR gets updated and ready for review.

[afercia]

Would it be possible to give this some higher priority please? The JS error still happens in Safari and IE11. The Escape key moving focus back from the toolbar to the editable area is standard interaction in TinyMCE and it's the behavior users expect.

[afercia]

This sounded like a sensible approach. Would need some refresh 🙂

[aduth]

I'm hoping to set aside time this week to refresh this.

[aduth]

I've force-pushed a refreshed branch. Most all remains the same as before. There were a few updates needed to accommodate @jorgefilipecosta 's work in #11607. I also (hopefully) improved the behavior with tracking focus leaving a toolbar as the result of Escape press vs. other user interaction, which previously had some room for error.

It works fairly well for me, though it's still not quite fully polished:

• I noticed some flakey behavior when using "Top Toolbar" mode, specifically it not working correctly until the next refresh, and some incorrect Escape behavior when trying to "move down" from the top-most toolbar
• I haven't checked yet if any tests need updates

[youknowriad]

Triaging PRs today. I feel there's still a need for this PR but with all the changes that happened, we may want to just close it and revisit it when we have more time? Thoughts @aduth

[aduth]

To me, it seems like we would want something like in the original approach of stepping and and down the ancestry of toolbars (or some way to avoid an ancestry altogether). However, going through the history of this pull request and the associated issue #6165, it's not entirely clear to me if the main problem was more the error, which apparently was "resolved" in some form or another by the navigation mode.

All being said: Yes, it's too far out of date at this point to hope to rebase it. The fact that navigation mode now occupies the Escape keybind introduces some additional challenges here. It would be good to track whatever effort (if any) we want to follow-up on here. I don't feel entirely confident in writing this, since at this point I don't know exactly how that should be implemented.

In any case, let's close this one out for the time being.