Block List: Move deselect behavior to block component

[aduth]

Closes #3712

This pull request seeks to improve block deselect behavior. The previous implementation had the visual editor test for click events on the canvas to infer a deselection. This was necessary because there are many cases where focus leaves a block, but the block should remain selected (e.g. clicking toolbar buttons, or inspector controls). However, the previous solution was not ideal because (a) it introduces an implicit dependency between the visual editor and block focus, (b) it prioritized click behavior and while tab focus changes worked, it was only because multiple singular blocks could not be selected at the same time, and (c) it conflicts with efforts to introduce multiple editor scopes (block nesting).

Implementation notes:

I see this as a dramatic simplification to VisualEditor, but still anticipate that improvements could be made. Specifically, the block component shouldn't need to have any awareness of editor headers or sidebars, as the proposed implementation includes. Two alternatives I see here include:

• Relying on React portal event bubbling behavior to detect changes in focus via events bubbled through virtual ancestry.
  • This requires refactoring toolbar and multi-selection behavior, see Revert React Portals for most uses #3083.
• Leveraging hooks filtering to make the block component initially unaware of any closest elements, but allow components (such as visual editor) to introduce tests to verify whether the block should be allowed to be deselected

Testing instructions:

Verify that there are no regressions in the behavior of selecting blocks, particularly:

• Selecting a block
• Transitioning selection from one block to another, via click and by keyboard tab
• Deselecting a block by clicking outside
• Interacting with a block via toolbar or inspector controls (including toggling between Document and Block settings in sidebar)

[aduth]

The e2e failures appear to be inconsistent, and I wasn't able to reproduce the failing behavior in manual testing. Will continue to investigate...

[youknowriad]

This component is reusable and these two selectors are not guaranteed to exist (example P2). Should we use a prop for these or a setting in the context?

[aduth]

This component is reusable and these two selectors are not guaranteed to exist (example P2). Should we use a prop for these or a setting in the context?

I agree, and noted in Implementation Notes that this was not ideal, but slightly improved from the previous implementation. I tried an alternate approach in 00fcfca, passing an onDeselectBlock callback from the visual editor to control deselection via event.preventDefault. What are your thoughts on this approach?

[codecov]

Codecov Report

Merging #3743 into master will decrease coverage by 0.08%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##           master    #3743      +/-   ##
==========================================
- Coverage   37.72%   37.63%   -0.09%     
==========================================
  Files         279      279              
  Lines        6738     6730       -8     
  Branches     1226     1225       -1     
==========================================
- Hits         2542     2533       -9     
- Misses       3535     3536       +1     
  Partials      661      661

Impacted Files	Coverage Δ
editor/components/block-list/block.js	2.14% <0%> (-0.1%)	⬇️
editor/edit-post/modes/visual-editor/index.js	0% <0%> (ø)	⬆️
...omponents/higher-order/with-focus-outside/index.js	100% <100%> (ø)	⬆️
editor/components/warning/index.js	0% <0%> (-100%)	⬇️
components/panel/row.js	0% <0%> (-100%)	⬇️
components/panel/color.js	0% <0%> (-100%)	⬇️
editor/utils/mobile/index.js	57.14% <0%> (-17.86%)	⬇️
editor/reducer.js	90.27% <0%> (-0.11%)	⬇️
editor/selectors.js	93.43% <0%> (-0.04%)	⬇️
editor/store-defaults.js	100% <0%> (ø)	⬆️
... and 7 more

---

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 dff261d...00fcfca. Read the comment docs.

[youknowriad]

clever

[youknowriad]

The extra prop is a bit annoying but it is still better than what we have.

Also, I'm seeing a difference between multi and mono selection. When I click the white space around the editor it deselects the block but if it's a multiselection, it doesn't work, should we aligh the behavior?

Also, I'm seeing a small issue:

• Select a block
• Click the inspector
• Try to unselect the block by clicking the white space around the editor
• The block remains selected

[aduth]

Also, I'm seeing a small issue:

Yeah, I see why this would be an issue, since focus has already left the block at this point, the focus outside handler would never be called again. Thinking click-outside could be a better handler here, except we'd probably want to limit it to be applied only on the selected block (otherwise we could have hundreds of handlers firing on every click for long posts).

[aduth]

I spent some time on this earlier in the week, and it's a very difficult problem to solve. Testing click outside of the selected block works mostly well, except:

• This doesn't help us for nested blocks, since clicking within a nested block is technically not outside, so the parent block wouldn't become unselected
• Tricky to get events coordinated with multi-selection, since technically multi-selection can incur a click outside (when dragging down across blocks) but we don't want to deselect in these cases

The most promising direction would be to migrate inspector and toolbar slots to behave with React event bubbling so focus stays within a block while interacting with these controls. But then this has a few issues as well:

• There may be a need to move away from fills for inspector controls vs. explicit properties (Try multi-select inspector for same blocks #3535 (comment))
• As described in original comment, there were issues with using React event bubbling for these controls (Revert React Portals for most uses #3083)
• We'd still need some manual intervention to exempt clicks, e.g. toggling between "Document" and "Block" in the sidebar header shouldn't deselect a block

[aduth]

I made another attempt to iterate on this, and made some decent progress with click detection. I've included my original summary below, but the e2e test failures led me to discover some blocking issues:

• The click deselect canceling doesn't work well with the default block appender (causing a flicker of block selection when clicking)
• Clicking is probably not the best event to be listening for, since we truly do want deselect behavior to be occurring on focus changing outside of the block

I'm going to take a break from this for now. My hunch is that maybe we need a document-wide focus listener which, similar to the bubbling behavior described below, could cancel deselect while focus transitions within a block. This would not be too much unlike how withFocusOutside already works, with the main difference being that we'd want to continue to watch for focus events even after focus has left the block to ensure that all focus transitions are excluded from deselection.

---

Original revisions summary:

The general idea is that while a block is selected, we detect mouse clicks on the document to deselect the block, with the following caveats:

• An onDeselect prop allows a rendering parent to cancel the deselect via event.preventDefault()
• Clicks occurring by the multi-select mouseup finishing event are accounted for by debouncing the deselect and canceling it if we've completed a multi-select
• Clicks occurring within the block, even those rendered elsewhere in the DOM by portalling, are accounted for by debouncing the deselect and canceling it on bubbled click events
  • Since technically the document-level click detection would occur after the click on the block ("bubbles up"), we must use capture phase for document event listener to reverse the order in which the callbacks are fired

This doesn't help us for nested blocks, since clicking within a nested block is technically not outside, so the parent block wouldn't become unselected

I think the most promising option here is: Stop propagation in the block's onClick handler to avoid bubbling up to the parent block's onClick which would be canceling the deselect, i.e. effectively allow the deselect on the parent block.

Since we don't need this behavior until nested blocks are implemented, I have omitted it from the changes here.

(An alternative option I had considered is testing whether the closest block wrapper where the click occurs is the same as the current block wrapper. The issue here is that Element#closest does not work with slotted content)

[aduth]

Another iteration today in ce2f722, binding focus events to try deselecting block when focus moves elsewhere in the page. This mostly works okay, but there's an issue with opening the block secondary menu causing the block to become deselected. I have a feeling that since we rely on event propagation to cancel the deselection, there may be some stopPropagation going on somewhere that's conflicting with this behavior.

[aduth]

Related: https://discuss.reactjs.org/t/ordering-of-native-and-react-events/829

The current implementation doesn't work well specifically here:

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

Lines 220 to 224 in ce2f722

	// Bind to the DOM reference of the rendered wrapper node, since React
	// synthetic event binding and focus normalization conflicts with our
	// own document event binding and therefore canceling occurs before the
	// deselect event is triggered (out of expected order).
	this.wrapperNode[ bindFn ]( 'focusin', this.cancelDeselect );

...because when opening the block settings menu, focus transitions within the popover content, which is not in the same DOM hierarchy as the rendered DOM node of the block itself, so it doesn't capture the focus event to cancel the deselect.

Normally we'd be able to handle this simply binding onFocus to the rendered element, which would capture the virtual DOM event bubbling:

diff --git a/editor/components/block-list/block.js b/editor/components/block-list/block.js
index 2bc51321..e46664ec 100644
--- a/editor/components/block-list/block.js
+++ b/editor/components/block-list/block.js
@@ -216,12 +216,6 @@ export class BlockListBlock extends Component {
 		const bindFn = isListening ? 'addEventListener' : 'removeEventListener';
 		document[ bindFn ]( 'focus', this.triggerDeselect, true );
 		document[ bindFn ]( 'deselect', this.debouncedDeselect );
-
-		// Bind to the DOM reference of the rendered wrapper node, since React
-		// synthetic event binding and focus normalization conflicts with our
-		// own document event binding and therefore canceling occurs before the
-		// deselect event is triggered (out of expected order).
-		this.wrapperNode[ bindFn ]( 'focusin', this.cancelDeselect );
 	}
 
 	setAttributes( attributes ) {
@@ -471,6 +465,7 @@ export class BlockListBlock extends Component {
 				data-type={ block.name }
 				onTouchStart={ this.onTouchStart }
 				onClick={ this.onClick }
+				onFocus={ this.cancelDeselect }
 				{ ...wrapperProps }
 			>
 				<BlockDropZone index={ order } />

But the problem appears to be that React's normalization of events is causing the cancel to be triggered before our own document focus events. We need it to occur the other way around, meaning we need a way to prioritize our own focus event handlers over those of the React synthetic events manager.

[aduth]

See also: facebook/react#285

The implementation here finally seems mostly solid. I was able to move back to relying on the synthetic events. Binding focus events to window with useCapture seems to get the order we need to perform cancels reliably from the synthetic block focus.

That all being said, the e2e tests are failing and I'm having a hard time debugging them, as repeating the steps from the failing test manually works well for me in my own browser. Will debug some more in the morning.

[aduth]

I no longer intend to pursue this approach for now, as it's proven to be quite problematic, and the current behavior that clearing is an effect of the action of clicking on the editor canvas is more agreeable than at initial consideration.

That said, there are still issues to resolve with deselection, namely #2322, which should be addressed separately.