Deleting an empty list with backspaces doesn't focus the previous block

[notnownikki]

Steps to Reproduce (for bugs)

1. Insert a new paragraph, type some stuff
2. Insert a new list after it. Press backspace to delete it.

Expected Behavior

Cursor should go to the paragraph block above the list.

Current Behavior

No block is selected, pressing enter does nothing, pressing backspace does nothing.

Browser

• Reproduced in Firefox 61 (Mac OS)
• Could not reproduce in Chrome

[designsimply]

Tested and confirmed that I cannot delete a list block and move into the previous block by pressing the backspace key.

Video: 31s

^{Seen at http://alittletestblog.com/wp-admin/post.php?post=13896&action=edit running WordPress 4.9.7 and Gutenberg 3.2.0 using Firefox 61.0.1 on macOS 10.13.5.}

[talldan]

@notnownikki - I think this one is now resolved. I couldn't reproduce, the previous block is selected when I tested. Happy for it to be closed?

[notnownikki]

This is almost working now. Works as expected in Chrome, but in Firefox when the previous paragraph block gets focus, you can't type anything.

I remember @aduth was looking at that, but I can't find the issue for it. If there's already one open, we can close this. If not, we should open one and close this :)

[talldan]

Well spotted, definitely still seems to be an issue in Firefox, though it now seems as though the preceding paragraph block has to be empty, or at least that's the only way I could reproduce.

[notnownikki]

What I'm seeing is on an empty paragraph you can't type at all. On a paragraph with content, pressing backspace again will fix it, but you cant just carry on typing

[aduth]

It's not quite fixed, even in Chrome, at least with the patterns transform. See also #8942 . From some debugging there's an issue specifically with the patterns transform and (a) how it includes an empty string as content and (b) how it will always create an li from said content, thus preventing the default behavior from removing the block. It's not quite the same though, since with those types of transforms I think "Backspace" should be more of an "Undo" than shifting focus to the previous block.

[aduth]

The Firefox issues with focus and inability to type is an issue with our implementation of placeCaretAtHorizontalEdge in @wordpress/dom choosing the lastChild of a container, where in some browsers that last node may be a br (linebreak, a TinyMCE "bogus" node), which prevents further typing.

gutenberg/packages/dom/src/dom.js

Lines 275 to 284 in 3767009

	// Select on extent child of the container, not the container itself. This
	// avoids the selection always being `endOffset` of 1 when placed at end,
	// where `startContainer`, `endContainer` would always be container itself.
	const rangeTarget = container[ isReverse ? 'lastChild' : 'firstChild' ];
	// If no range target, it implies that the container is empty. Focusing is
	// sufficient for caret to be placed correctly.
	if ( ! rangeTarget ) {
	return;
	}

I have a work-in-progress patch which should work to find a text node, but it surfaces yet other issues with forced caret re-placement by TinyMCE. I believe this is partially blocked by #8879, which may be related to tinymce/tinymce#4472 .

diff --git a/packages/dom/src/dom.js b/packages/dom/src/dom.js
index b62311310..0694f4f0b 100644
--- a/packages/dom/src/dom.js
+++ b/packages/dom/src/dom.js
@@ -15,6 +15,33 @@ const {
 	DOCUMENT_POSITION_FOLLOWING,
 } = window.Node;
 
+/**
+ * Returns the first occurrence of a node of the given type within the parent
+ * container, optionally in a reverse direction, or undefined if a node cannot
+ * be found.
+ *
+ * @param {Node}     parent    Container to search within.
+ * @param {number}   nodeType  Node type to find.
+ * @param {?boolean} isReverse Whether to search from last child backward.
+ *
+ * @return {?Node} Node of type, if found.
+ */
+export function findNodeOfType( parent, nodeType, isReverse = false ) {
+	const childProperty = isReverse ? 'lastChild' : 'firstChild';
+	const siblingProperty = isReverse ? 'previousSibling' : 'nextSibling';
+
+	for ( let node = parent[ childProperty ]; node; node = node[ siblingProperty ] ) {
+		if ( node.nodeType === nodeType ) {
+			return node;
+		}
+
+		const recursedNode = findNodeOfType( node, nodeType, isReverse );
+		if ( recursedNode ) {
+			return recursedNode;
+		}
+	}
+}
+
 /**
  * Returns true if the given selection object is in the forward direction, or
  * false otherwise.
@@ -275,7 +302,7 @@ export function placeCaretAtHorizontalEdge( container, isReverse ) {
 	// Select on extent child of the container, not the container itself. This
 	// avoids the selection always being `endOffset` of 1 when placed at end,
 	// where `startContainer`, `endContainer` would always be container itself.
-	const rangeTarget = container[ isReverse ? 'lastChild' : 'firstChild' ];
+	const rangeTarget = findNodeOfType( container, TEXT_NODE, isReverse );
 
 	// If no range target, it implies that the container is empty. Focusing is
 	// sufficient for caret to be placed correctly.
@@ -286,8 +313,10 @@ export function placeCaretAtHorizontalEdge( container, isReverse ) {
 	const selection = window.getSelection();
 	const range = document.createRange();
 
-	range.selectNodeContents( rangeTarget );
-	range.collapse( ! isReverse );
+	const offset = isReverse ? rangeTarget.nodeValue.length - 1 : 0;
+	range.selectNode( rangeTarget );
+	range.setStart( rangeTarget, offset );
+	range.setEnd( rangeTarget, offset );
 
 	selection.removeAllRanges();
 	selection.addRange( range );
diff --git a/packages/dom/src/test/dom.js b/packages/dom/src/test/dom.js
index 66d88c584..e33dd57b4 100644
--- a/packages/dom/src/test/dom.js
+++ b/packages/dom/src/test/dom.js
@@ -1,21 +1,101 @@
 /**
  * Internal dependencies
  */
-import { isHorizontalEdge, placeCaretAtHorizontalEdge, isTextField } from '../dom';
+import {
+	findNodeOfType,
+	isHorizontalEdge,
+	placeCaretAtHorizontalEdge,
+	isTextField,
+} from '../dom';
+
+const {
+	TEXT_NODE,
+} = window.Node;
 
 describe( 'DOM', () => {
-	let parent;
+	describe( 'findNodeOfType', () => {
+		it( 'returns undefined if there is no children', () => {
+			const parent = document.createElement( 'div' );
 
-	beforeEach( () => {
-		parent = document.createElement( 'div' );
-		document.body.appendChild( parent );
-	} );
+			const node = findNodeOfType( parent, TEXT_NODE );
+
+			expect( node ).toBeUndefined();
+		} );
+
+		it( 'returns undefined if there is no node of type', () => {
+			const parent = document.createElement( 'div' );
+			parent.appendChild( document.createElement( 'span' ) );
+
+			const node = findNodeOfType( parent, TEXT_NODE );
+
+			expect( node ).toBeUndefined();
+		} );
+
+		it( 'returns the first occurrence of node of type', () => {
+			const parent = document.createElement( 'div' );
+			parent.appendChild( document.createElement( 'span' ) );
+			const target = document.createTextNode( '' );
+			parent.appendChild( target );
+			parent.appendChild( document.createTextNode( '' ) );
+			parent.appendChild( document.createElement( 'span' ) );
+
+			const node = findNodeOfType( parent, TEXT_NODE );
+
+			expect( node ).toBe( target );
+		} );
+
+		it( 'returns the first occurrence of node of type (reverse)', () => {
+			const parent = document.createElement( 'div' );
+			parent.appendChild( document.createElement( 'span' ) );
+			parent.appendChild( document.createTextNode( '' ) );
+			const target = document.createTextNode( '' );
+			parent.appendChild( target );
+			parent.appendChild( document.createElement( 'span' ) );
+
+			const node = findNodeOfType( parent, TEXT_NODE, true );
 
-	afterEach( () => {
-		parent.remove();
+			expect( node ).toBe( target );
+		} );
+
+		it( 'recurses into children to find node of type', () => {
+			const parent = document.createElement( 'div' );
+			const child = document.createElement( 'span' );
+			parent.appendChild( child );
+			const target = document.createTextNode( '' );
+			child.appendChild( target );
+			child.appendChild( document.createTextNode( '' ) );
+
+			const node = findNodeOfType( parent, TEXT_NODE );
+
+			expect( node ).toBe( target );
+		} );
+
+		it( 'recurses into children to find node of type (reverse)', () => {
+			const parent = document.createElement( 'div' );
+			const child = document.createElement( 'span' );
+			parent.appendChild( child );
+			child.appendChild( document.createTextNode( '' ) );
+			const target = document.createTextNode( '' );
+			child.appendChild( target );
+
+			const node = findNodeOfType( parent, TEXT_NODE, true );
+
+			expect( node ).toBe( target );
+		} );
 	} );
 
 	describe( 'isHorizontalEdge', () => {
+		let parent;
+
+		beforeEach( () => {
+			parent = document.createElement( 'div' );
+			document.body.appendChild( parent );
+		} );
+
+		afterEach( () => {
+			parent.remove();
+		} );
+
 		it( 'should return true for empty input', () => {
 			const input = document.createElement( 'input' );
 			parent.appendChild( input );

[ellatrix]

Could this be re-tested with master or 4.0-rc?

[designsimply]

LGTM. Tested and confirmed that I can now delete a list block and move into the previous block by pressing the backspace key using Firefox 62.0.3 on macOS 10.13.6. I tested with WordPress 4.9.8 and Gutenberg 4.0.0-rc.1.

[ellatrix]

@notnownikki Could you confirm and close if it looks good now?

[aduth]

Let's err as fixed by testing confirmation #5056 (comment) and reopen if necessary.