Writing Flow: Move selection to block's focus handler

[aduth]

This pull request seeks to resolve conflicts between the WritingFlow's selection transitioning and nested blocks. Prior to these changes, when attempting to press "Down" from the first block in a column, the new block would be added, but focus would be moved back to the first block in the column. This was caused by two related issues:

• In WritingFlow's handling of vertically moving from one block to the next, we explicitly call to select the parent block. In both nested and unnested contexts, when this occurs as a result of moving into the default block appender, the closestTabbable is the default block appender itself. For non-nested blocks, attempting to select parent block has no effect because there is no "closest block" for the appender. In a nested context, however, the closest block is the wrapper block (Columns), so therefore it becomes selected and focuses its first tabbable which is the first block's wrapper.
  • This is resolved by moving selection handling in response to any focus which occurs within the selected block, not just the current fallback logic for testing against this.node (i.e. clicking at the edge of a block, or a block without its own focusable elements).
• The previous resolution introduces another issue, which is that we rely on the IgnoreNestedEvents component to flag events as handled so that ancestor blocks don't attempt to handle them. With 8bd012a, since any focus event would cause the block to select itself if not already selected, we need to ensure that focus events from nested blocks are appropriately flagged. However, the previously described behavior of focusing the default block appender occurs outside the block itself, so it will never apply the expected _blockHandled flag.
  • This is resolved by wrapping DefaultBlockAppender with its own IgnoreNestedEvents, capturing and applying the handled flag to events managed in the appender.
  • I really don't like this approach, because it introduces a dependency from the parent BlockListLayout to a child's rendering behavior (the DefaultBlockAppender). I had unsuccessfully explored other options, including a IgnoreNestedEvents.Barrier component sitting at the top of a nested context and flagging every event. Besides being wasteful (not all events are monitored, and some events dispatch very often like mousemove), it introduced more problems of its own where expected events were not being handled correctly (like a block dismissing its hover effects).

Testing instructions:

Verify that there are no regressions in the behavior of selecting a block via focus, both for blocks with their own inputs (paragraph) and those without (image placeholder).

Verify that you can add a default block to the end of a block list, both in a nested and non-nested context, by pressing "Down" from the last non-empty block.

[youknowriad]

There's a small issue with these changes.

• create multiple text blocks
• Navigate between these blocks using arrows
• click shift + left/right arrows to select text
• The toolbar shows up in the wrong block, because the wrong block is selected in the state.

[aduth]

@youknowriad I'm not able to reproduce this, and it doesn't seem like something that ought to be happening, if the block is selecting itself in response to the focus event.

[aduth]

Ah, reproduced in Firefox, but not Chrome.

[youknowriad]

@aduth Here's a gif showing the issue (I'm using firefox)

[aduth]

Seems related to the Firefox-specific condition that was added in #5112. Removing the condition resolves this issue, but of course regresses on the intended issue addressed in #5112.

[aduth]

Personally I'd find this to be a more important fix than the one addressed in #5112, particularly since the toolbar is still problematic in Safari, another solution is likely needed. I'll continue debugging for a bit longer though...

[aduth]

I tried to demonstrate the issue with a simplistic CodePen: https://codepen.io/aduth/pen/MQxRME

In master, the block's edit wrapper is focusable, but since the toolbar is outside the edit wrapper and buttons do not emit focus events on click in Firefox and Safari, the focus instead occurs on the parent block's edit wrapper. If we move tabIndex to the top-level wrapper instead, the nested block will flag the event as block-handled in the top-level IgnoreNestedEvents (previously, nothing existed between the button and the parent block's edit wrapper which was focusable).

I'm still trying to understand the original reason it was moved in #2934. Given original attempts at a fix to what ultimately led to the move, I'm wondering if it's even still relevant, since part of the changes proposed here is to remove this target === this.node condition altogether.

[aduth]

Re-testing the original issues described at #2934 (comment), they still appear to work correctly here.

[aduth]

Discovered an issue: Arrowing to an image placeholder block skips it to the following block.

[aduth]

Not so surprisingly, this was a result of our fragile direct selectors being fragile. Fixed in 386ac09.

[aduth]

If these changes are solid, I suspect we might also be able to drop the onMouseDown selection which exists here:

gutenberg/editor/components/block-list/block.js

Lines 415 to 417 in f80c123

	if ( ! this.props.isSelected ) {
	this.props.onSelect();
	}

I'd explored this once in the past and I think it was needed at the time based on how focus of blocks works. Would be worth exploring, but not necessary here since it's more a simplification of removing unnecessary event handlers.

[aduth]

This was punted as part of the 2.3 release because of the complexity of the changes on a release day, but I'd like to get this in to allow remaining issues to be settled in time for the next release.

I will plan to merge this if there is no other remaining feedback.

[aduth]

What started as an exploration of fixing an issue where arrow navigating to an image placeholder would focus but not select a block (4a85e23) resulted in a cascade of commits. In the end, I think it's a more stable implementation overall, but I've come to the conclusion that this is all rather risky code to be touching, so would appreciate another look.

[aduth]

Noticing an issue, but also exists on master:

1. Add image block, leave as placeholder
2. Add paragraph block
3. Start from title, press arrow down
   • Should be focused on image block
4. Press tab
   • Should be focused on Upload button
5. Press down

Expected: In subsequent paragraph block
Actual: At end of post in default appender