RichText: Fix empty value delete behaviors

packages/editor/src/components/block-list/block.js

@@ -218,22 +218,31 @@ export class BlockListBlock extends Component {
 		}
 	}
 
+	/**
+	 * Handles block merger, returning true if the merge is possible, or false
+	 * otherwise.
+	 *
+	 * @param {boolean} forward Whether merge should occur with the next block.
+	 *
+	 * @return {boolean} Whether merge is possible.
+	 */
 	mergeBlocks( forward = false ) {
 		const { block, previousBlockClientId, nextBlockClientId, onMerge } = this.props;
 
-		// Do nothing when it's the first block.
-		if (
-			( ! forward && ! previousBlockClientId ) ||
-			( forward && ! nextBlockClientId )
-		) {
-			return;
-		}
+		const hasMergeableTarget = (
+			( forward && !! nextBlockClientId ) ||
+			( ! forward && !! previousBlockClientId )
+		);
 
-		if ( forward ) {
-			onMerge( block.clientId, nextBlockClientId );
-		} else {
-			onMerge( previousBlockClientId, block.clientId );
+		if ( hasMergeableTarget ) {
+			if ( forward ) {
+				onMerge( block.clientId, nextBlockClientId );
+			} else {
+				onMerge( previousBlockClientId, block.clientId );
+			}
 		}
+
+		return hasMergeableTarget;
 	}
 
 	insertBlocksAfter( blocks ) {

packages/editor/src/components/rich-text/README.md

@@ -41,11 +41,11 @@ Render a rich [`contenteditable` input](https://developer.mozilla.org/en-US/docs
 
 ### `onMerge( forward: Boolean ): Function`
 
-*Optional.* Called when blocks can be merged. `forward` is true when merging with the next block, false when merging with the previous block.
+*Optional.* Called when blocks can be merged. `forward` is true when merging with the next block, false when merging with the previous block. The callback should return `true` if a merge is possible, or `false` otherwise. This is used in determining whether an empty field should be deleted instead of merged.
 
 ### `onRemove( forward: Boolean ): Function`
 
-*Optional.* Called when the block can be removed. `forward` is true when the selection is expected to move to the next block, false to the previous block.
+*Optional.* Called when the block can be removed. `forward` is true when the selection is expected to move to the next block, false to the previous block. Called when pressing Backspace or Delete on an empty field and only when `onMerge` explicitly returns `false` to indicate non-merge.
 
 ### `formattingControls: Array`
 

packages/editor/src/components/rich-text/index.js

@@ -81,6 +81,7 @@ export class RichText extends Component {
 		this.onFocus = this.onFocus.bind( this );
 		this.onChange = this.onChange.bind( this );
 		this.onNodeChange = this.onNodeChange.bind( this );
+		this.onDeleteKeyDown = this.onDeleteKeyDown.bind( this );
 		this.onHorizontalNavigationKeyDown = this.onHorizontalNavigationKeyDown.bind( this );
 		this.onKeyDown = this.onKeyDown.bind( this );
 		this.onKeyUp = this.onKeyUp.bind( this );
@@ -426,41 +427,84 @@ export class RichText extends Component {
 	}
 
 	/**
-	 * Handles a keydown event from TinyMCE.
+	 * Handles a delete key down event to handle merge or removal for a
+	 * collapsed selection where caret is at directional edge: forward for a
+	 * delete key down, reverse for a backspace key down.
 	 *
-	 * @param {KeydownEvent} event The keydown event as triggered by TinyMCE.
+	 * @param {tinymce.EditorEvent<KeyboardEvent>} event Keydown event.
 	 */
-	onKeyDown( event ) {
-		const dom = this.editor.dom;
-		const rootNode = this.editor.getBody();
+	onDeleteKeyDown( event ) {
+		const { onMerge, onRemove } = this.props;
+		if ( ! onMerge && ! onRemove ) {
+			return;
+		}
+
 		const { keyCode } = event;
+		const isReverse = keyCode === BACKSPACE;
+		const { isCollapsed } = getSelection();
 
-		if (
-			getSelection().isCollapsed && (
-				( keyCode === BACKSPACE && isHorizontalEdge( rootNode, true ) ) ||
-				( keyCode === DELETE && isHorizontalEdge( rootNode, false ) )
-			)
-		) {
-			if ( ! this.props.onMerge && ! this.props.onRemove ) {
-				return;
-			}
+		// Only process delete if occurs at uncollapsed edge.
+		if ( ! isCollapsed ) {
+			return;
+		}
 
-			const forward = keyCode === DELETE;
+		// It is important to consider emptiness because an empty container
+		// will include a bogus TinyMCE BR node _after_ the caret, so in a
+		// forward deletion the isHorizontalEdge function will incorrectly
+		// interpret the presence of the bogus node as not being at the edge.
+		const isEmpty = this.isEmpty();
+		const isEdge = (
+			isEmpty ||
+			isHorizontalEdge( this.editor.getBody(), isReverse )
+		);
 
-			if ( this.props.onMerge ) {
-				this.props.onMerge( forward );
-			}
+		if ( ! isEdge ) {
+			return;
+		}
 
-			if ( this.props.onRemove && this.isEmpty() ) {
-				this.props.onRemove( forward );
-			}
+		let isHandled = false;
 
-			event.preventDefault();
+		let isMerged = false;
+		if ( onMerge ) {
+			isMerged = onMerge( ! isReverse );
+			isHandled = !! isMerged;
+		}
+
+		// `onMerge`, if it is exists, is not required to return a value, thus
+		// the check on explicit false. This condition will cover cases where
+		// either (a) `onMerge` is not defined or (b) it returns false to
+		// indicate a merge was not possible.
+		if ( onRemove && false === isMerged && isEmpty ) {
+			onRemove( ! isReverse );
+			isHandled = true;
+		}
+
+		if ( ! isHandled ) {
+			// Only prevent default behaviors if handled.
+			return;
+		}
+
+		event.preventDefault();
+
+		// Calling onMerge() or onRemove() will destroy the editor, so it's
+		// important that we stop other handlers (e.g. ones registered by
+		// TinyMCE) from also handling this event.
+		event.stopImmediatePropagation();
+	}
+
+	/**
+	 * Handles a keydown event from TinyMCE.
+	 *
+	 * @param {KeydownEvent} event The keydown event as triggered by TinyMCE.
+	 */
+	onKeyDown( event ) {
+		const dom = this.editor.dom;
+		const rootNode = this.editor.getBody();
+		const { keyCode } = event;
 
-			// Calling onMerge() or onRemove() will destroy the editor, so it's important
-			// that we stop other handlers (e.g. ones registered by TinyMCE) from
-			// also handling this event.
-			event.stopImmediatePropagation();
+		const isDelete = keyCode === DELETE || keyCode === BACKSPACE;
+		if ( isDelete ) {
+			this.onDeleteKeyDown( event );
 		}
 
 		const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT;

test/e2e/specs/__snapshots__/splitting-merging.test.js.snap

@@ -6,6 +6,12 @@ exports[`splitting and merging blocks should delete an empty first line 1`] = `
 <!-- /wp:paragraph -->"
 `;
 
+exports[`splitting and merging blocks should forward delete from an empty paragraph 1`] = `
+"<!-- wp:paragraph -->
+<p></p>
+<!-- /wp:paragraph -->"
+`;
+
 exports[`splitting and merging blocks should gracefully handle if placing caret in empty container 1`] = `
 "<!-- wp:paragraph -->
 <p><strong>Foo</strong></p>
@@ -32,6 +38,8 @@ exports[`splitting and merging blocks should not merge paragraphs if the selecti
 <!-- /wp:paragraph -->"
 `;
 
+exports[`splitting and merging blocks should remove empty paragraph block on backspace 1`] = `""`;
+
 exports[`splitting and merging blocks should split and merge paragraph blocks using Enter and Backspace 1`] = `
 "<!-- wp:paragraph -->
 <p>First</p>

test/e2e/specs/splitting-merging.test.js

@@ -86,16 +86,22 @@ describe( 'splitting and merging blocks', () => {
 	} );
 
 	it( 'should not merge paragraphs if the selection is not collapsed', async () => {
+		// Regression Test: When all of a paragraph is selected, pressing
+		// backspace should delete the contents, not merge to previous.
+		//
+		// See: https://github.com/WordPress/gutenberg/issues/8268
 		await insertBlock( 'Paragraph' );
 		await page.keyboard.type( 'Foo' );
 		await insertBlock( 'Paragraph' );
 		await page.keyboard.type( 'Bar' );
 
+		// Select text.
 		await page.keyboard.down( 'Shift' );
 		await pressTimes( 'ArrowLeft', 3 );
 		await page.keyboard.up( 'Shift' );
-		await page.keyboard.press( 'Backspace' );
 
+		// Delete selection.
+		await page.keyboard.press( 'Backspace' );
 		expect( await getEditedPostContent() ).toMatchSnapshot();
 	} );
 
@@ -121,4 +127,28 @@ describe( 'splitting and merging blocks', () => {
 
 		expect( await getEditedPostContent() ).toMatchSnapshot();
 	} );
+
+	it( 'should forward delete from an empty paragraph', async () => {
+		// Regression test: Bogus nodes in a TinyMCE container can interfere
+		// with isHorizontalEdge detection, preventing forward deletion.
+		//
+		// See: https://github.com/WordPress/gutenberg/issues/8731
+		await insertBlock( 'Paragraph' );
+		await page.keyboard.press( 'Enter' );
+		await page.keyboard.press( 'ArrowUp' );
+		await page.keyboard.press( 'Delete' );
+
+		expect( await getEditedPostContent() ).toMatchSnapshot();
+	} );
+
+	it( 'should remove empty paragraph block on backspace', async () => {
+		// Regression Test: In a sole empty paragraph, pressing backspace
+		// should remove the block.
+		//
+		// See: https://github.com/WordPress/gutenberg/pull/8306
+		await insertBlock( 'Paragraph' );
+		await page.keyboard.press( 'Backspace' );
+
+		expect( await getEditedPostContent() ).toMatchSnapshot();
+	} );
 } );

test/e2e/support/utils.js

@@ -77,6 +77,34 @@ async function login() {
 	] );
 }
 
+/**
+ * Returns a promise which resolves once it's determined that the active DOM
+ * element is not within a RichText field, or the RichText field's TinyMCE has
+ * completed initialization. This is an unfortunate workaround to address an
+ * issue where TinyMCE takes its time to become ready for user input.
+ *
+ * TODO: This is a code smell, indicating that "too fast" resulting in breakage
+ * could be equally problematic for a fast human. It should be explored whether
+ * all event bindings we assign to TinyMCE to handle could be handled through
+ * the DOM directly instead.
+ *
+ * @return {Promise} Promise resolving once RichText is initialized, or is
+ *                   determined to not be a container of the active element.
+ */
+async function waitForRichTextInitialization() {
+	const isInRichText = await page.evaluate( () => {
+		return !! document.activeElement.closest( '.editor-rich-text__tinymce' );
+	} );
+
+	if ( ! isInRichText ) {
+		return;
+	}
+
+	return page.waitForFunction( () => {
+		return !! document.activeElement.closest( '.mce-content-body' );
+	} );
+}
+
 export async function visitAdmin( adminPath, query ) {
 	await goToWPPath( join( 'wp-admin', adminPath ), query );
 
@@ -170,6 +198,7 @@ export async function ensureSidebarOpened() {
  */
 export async function clickBlockAppender() {
 	await expect( page ).toClick( '.editor-default-block-appender__content' );
+	await waitForRichTextInitialization();
 }
 
 /**
@@ -199,6 +228,7 @@ export async function insertBlock( searchTerm, panelName = null ) {
 		await panelButton.click();
 	}
 	await page.click( `button[aria-label="${ searchTerm }"]` );
+	await waitForRichTextInitialization();
 }
 
 /**