Remove focus on multi-select stop

[ellatrix]

Description

Fixes #3191. See also #3194 and #3222.

This PR removes the focus state after a selection is finished. This results in the editable blurring, and therefore fixes deleting the blocks.

The focus implicitly ends up on the body, which is something we may not want.

Additionally this this breaks shift+arrow keys, but it already seems broken in master otherwise too. It seems to rely on a focus in an editable.

How Has This Been Tested?

1. Make a multi-block selection.
2. Press backspace.
3. Blocks are gone.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows has proper inline documentation.

[ellatrix]

The problem is that WritingFlow only listens for key presses on the element, not globally. Should it maybe? cc @mcsf This is a problem in master too if you make a selection, then place the focus somewhere else and then try to expand the selection with shift+arrow keys. The visual selection is still there, but nothing will happen. I think either WritingFlow needs to listen for all key presses, or the selection needs to disappear if you click out of the editor area (e.g. in the inspector).

[ellatrix]

Don't know about 1efd3cc, but that would fix it. Otherwise we need to move the focus somewhere inside writing flow at least...

[codecov]

Codecov Report

Merging #3253 into master will increase coverage by 1.11%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3253      +/-   ##
==========================================
+ Coverage    31.2%   32.32%   +1.11%     
==========================================
  Files         229      229              
  Lines        6428     6562     +134     
  Branches     1145     1183      +38     
==========================================
+ Hits         2006     2121     +115     
- Misses       3711     3727      +16     
- Partials      711      714       +3

Impacted Files	Coverage Δ
editor/reducer.js	90.05% <ø> (ø)	⬆️
editor/writing-flow/index.js	0% <0%> (ø)	⬆️
editor/sidebar/last-revision/index.js	0% <0%> (ø)	⬆️
editor/selectors.js	95.79% <0%> (+0.52%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcca1f2...1efd3cc. Read the comment docs.

[mcsf]

Don't know about 1efd3cc, but that would fix it. Otherwise we need to move the focus somewhere inside writing flow at least...

I don't know if we're missing something obvious that this could affect, but it seems like something we can try. We could refine later. If you feel it's good for the 1.6 merge, then 🚢 !

[aduth]

Do we need these events to be monitored for the whole document? Or just this.container ?

Do we risk bugs with having onKeyDown now running for any keypress in the document?

[ephox-mogran]

Do we risk bugs with having onKeyDown now running for any keypress in the document?

Based on testing master, Yes.

Due to this change, there are massive issues with keyboard navigation. Essentially, pressing up or down anywhere will activate this code (even in the toolbar). That is because the react events are being bound to the document as well, so this is firing even if the component handlers themselves call stopPropagation.

There is a post explaining how this works:
https://stackoverflow.com/questions/24415631/reactjs-syntheticevent-stoppropagation-only-works-with-react-events

This is forcing component developers to write event.nativeEvent.stopImmediatePropagation, otherwise their event handler AND this handler will fire. And WritingFlow is pretty much always on the screen, so this handler interferes with almost everything.

Now that we have UI testing, we need to have some basic user navigation tests setup so this kind of thing will be easier to catch in the future.

[ellatrix]

😱

[ephox-mogran]

Just my two cents: (assuming you didn't like the solution proposed by #3194 which was to focus the first editor's outer container, rather than its editable)

1. It would make much more sense to focus something inside the writing flow, than focus the generic body. Having global key listeners can cause very erratic behaviour. We already see a ton of issues at the moment with things bubbling up to WritingFlow when you press up and down in Menus (Keyboard Navigation with Arrows does not work in Block Settings #3223), let alone if those listeners were on the global document. Especially as WritingFlow shifts focus and in trying to be generic, doesn't really have enough information to completely diagnose when it should shift the focus. The only use case presently for document key listeners that I can think of is focusing the toolbar, and only if you want that focusing of the toolbar to be able to happen from anywhere inside Gutenberg (i.e. the sidepanel also). If you only want it to happen from within post creation, then the key listeners should be bound at the root container of post creation.

2. If you move the focus to the body, you are no longer going to get any kind of useful screen reader output. What about if we created an extra focusable container or something near the top of the selection when doing a multi-select which received focus and had some aria properties which meant that screen readers read how many blocks were selected. I've never thought of doing that before, but it would avoid the issue that you have with an editor still having focus.

This is a problem in master too if you make a selection, then place the focus somewhere else and then try to expand the selection with shift+arrow keys.

My (potentially naive) understanding of all of our keyboard navigation work in the past is that the keys that you press are only supposed to relate to your current focus. Your focus is identifying what should receive keyboard input. If you have shifted focus away from the blocks, then pressing shift + down probably shouldn't do anything, because that is no longer the action that the user is expecting. The screen reader is only going to read what your new focused element is. However, if you're after a different experience from that, it's completely your choice. But beware of making actions respond regardless of where your key position is --- that's where you get conflicts and unexpected behaviour.

[ephox-mogran]

When playing around with #3275 , it highlighted a problem with this approach. It's fine if you want to keep doing a multi-selection and then operate on that selection. But if you want to go back to your single selection (with Shift+Up) it doesn't seem like you can because you've lost the selection and typing does nothing because the editor is no longer focused (you just lose the multi-selection view). I really think we need to find something better than focusing the body. Especially when you consider that you can so easily get into a state with just the keyboard where you can't get back to where you were.

[ellatrix]

I agree, we need to move focus somewhere in writing flow. This PR made sure that the editable gets blurred, in addition to that something will have to pick up multi-selection and move focus?

[ellatrix]

Ideally multi selection has it's own container wrapping the selected blocks, but that opens its own can of worms. As far as I see, the best thing is still to move it to one of the actions. Or we try what you suggested.

What about if we created an extra focusable container or something near the top of the selection when doing a multi-select which received focus and had some aria properties which meant that screen readers read how many blocks were selected. I've never thought of doing that before, but it would avoid the issue that you have with an editor still having focus.

@afercia Any ideas where the focus is best moved when there is a block selection? :)