Improve arrow nav (horizontal only)

[ellatrix]

Same as #2296, but without repositioning vertically.

Fixes various browser issues with arrow key navigation. #2124
Fixes link boundaries. #2104

[codecov]

Codecov Report

Merging #2431 into master will decrease coverage by 0.75%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2431      +/-   ##
==========================================
- Coverage   26.44%   25.69%   -0.76%     
==========================================
  Files         157      157              
  Lines        4851     5079     +228     
  Branches      819      894      +75     
==========================================
+ Hits         1283     1305      +22     
- Misses       3015     3152     +137     
- Partials      553      622      +69

Impacted Files	Coverage Δ
editor/modes/visual-editor/block.js	0% <0%> (ø)	⬆️
blocks/library/paragraph/index.js	39.28% <0%> (-7.78%)	⬇️
blocks/api/paste.js	90.24% <0%> (-7.13%)	⬇️
blocks/library/heading/index.js	22.85% <0%> (-0.96%)	⬇️
blocks/editable/index.js	10.36% <0%> (-0.52%)	⬇️
blocks/library/list/index.js	6.55% <0%> (-0.42%)	⬇️
blocks/color-palette/index.js	0% <0%> (ø)	⬆️
blocks/library/image/index.js	15.38% <0%> (+0.38%)	⬆️
blocks/editable/format-toolbar/index.js	8.33% <0%> (+0.83%)	⬆️
blocks/library/cover-text/index.js	36% <0%> (+1%)	⬆️

---

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 c3a1fbd...56bbbcf. Read the comment docs.

[ellatrix]

I wonder if it's a good idea to add some static methods to Editable for Editable specific logic, and getting the next editor etc.

[youknowriad]

Several notes here:

• What if a block contains an input, an editable and a textarea for example, It looks like the code is ignoring the inputs and textareas right?
• What bothers me is that sometimes we're using onFocus to navigate and in other cases we use DOM or TinyMCE API. Can't we be consistent and always use DOM Api (or TinyMCE since I see it as a helper over DOM)?
• Also, I'm seeing some inconsistent caret placemenet. Say I have a separator between two text blocks. Depending on whether the first text block is empty/full the caret won't be on the same position if I navigate down to the text block after the tabbable.

Basically, What I'm proposing is something like this:

if ( isEdge( currentTabbable ) ) {
   const nextTabbable = getNextTabbable( currentTabbable );
   focusBeginingOrEndOfNextTabbbable( nextTabbable );
}

with TinyMCE as a special case of "tabbable", and handled like a special case inside these functions. Is it possible this way?

[ellatrix]

What bothers me is that sometimes we're using onFocus to navigate and in other cases we use DOM or TinyMCE API. Can't we be consistent and always use DOM Api (or TinyMCE since I see it as a helper over DOM)?

That's what it's doing, using the TinyMCE API. But we have to fall back to onFocus if there is no editable (or nothing at all) in the next block, no?

Also, I'm seeing some inconsistent caret placemenet. Say I have a separator between two text blocks. Depending on whether the first text block is empty/full the caret won't be on the same position if I navigate down to the text block after the tabbable.

Hm, I'm not sure how to reproduce.

Basically, What I'm proposing is something like this:

Sounds good to me. Sounds like it's the same thing but slightly abstracted.

[ellatrix]

Do you think we should do #2424 first?

[ellatrix]

I do feel like we should try to handle keys at the source. So Editable or a modified textarea would signal when to handle focus. Instead of a wrapper around all of it which tries to handle the keys without context.

[youknowriad]

That's what it's doing, using the TinyMCE API. But we have to fall back to onFocus if there is no editable (or nothing at all) in the next block, no?

Understood, I was thinking we could consider the block's wrapper as a "tabbable" but the question would be how we decide what's tabbable and what's not? I also raise this concern because the component in #2424 would be less "generic", it will be aware of blocks and state... Maybe we could resolve this by providing a prop extraTabbableSelector or something.

Hm, I'm not sure how to reproduce.

Me neither :) I don't know what happened

Do you think we should do #2424 first?

I'm fine either way :) we could also just extract the behavior in this PR

[ellatrix]

What if we use HOCs and force everything "tabbable" in the editor to be wrapped? FlowedTextarea, FlowedInput, FlowedEditableList etc. (I'm also bad with naming.) Then we know exactly what we have. If the block doesn't have anything, we use the wrapper of the block.

[aduth]

In Firefox, when moving to previous or next editable by arrow key, the focused block changes, but the editable is not focused, so I lose the text caret.

[aduth]

What does this do? Can we comment as such?

[aduth]

Is the reason we get TinyMCE instance primarily to have access to its APIs for selecting the end of the next block? Native APIs aren't quite as nice, but not absurd:

const range = document.createRange();
range.selectNodeContents( followingEditor );
range.collapse( false );
const selection = window.getSelection();
selection.removeAllRanges();
selection.addRange( range );

I think I might be more okay with this when we pull out and isolate these behaviors, as in #2424.

[aduth]

Pyramid of if could be improved if we delegated this to some helper methods that had early returns (as previously with handleArrowKey). See below comment too where we have early return that will inadvertently skip other onKeyDown logic.

[aduth]

A return here makes me wary since we don't isolate this logic from other onKeyDown handlers. Currently the only remaining logic which would be skipped is the ENTER handling, that would never coincide with this, but seems easy to overlook in future maintenance.

[ellatrix]

An alternative PR was merged.