RichText: Fix empty value delete behaviors

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 );
@@ -381,6 +382,64 @@ export class RichText extends Component {
 		this.context.onCreateUndoLevel();
 	}
 
+	/**
+	 * Handles a delete keyDown event to handle merge or removal for collapsed
+	 * selection where caret is at directional edge: forward for a delete key,
+	 * reverse for a backspace key.
+	 *
+	 * @link https://en.wikipedia.org/wiki/Caret_navigation
+	 *
+	 * @param {tinymce.EditorEvent<KeyboardEvent>} event Keydown event.
+	 */
+	onDeleteKeyDown( event ) {
+		const { onMerge, onRemove } = this.props;
+		if ( ! onMerge && ! onRemove ) {
+			return;
+		}
+
+		const { keyCode } = event;
+		const isReverse = keyCode === BACKSPACE;
+		const { isCollapsed } = getSelection();
+
+		// Only process delete if the key press occurs at uncollapsed edge.
+		if ( ! isCollapsed ) {
+			return;
+		}
+
+		// 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 ( ! isEdge ) {
+			return;
+		}
+
+		if ( onMerge ) {
+			onMerge( ! isReverse );
+		}
+
+		// Only handle remove on Backspace. This serves dual-purpose of being
+		// an intentional user interaction distinguishing between Backspace and
+		// Delete to remove the empty field, but also to avoid merge & remove
+		// causing destruction of two fields (merge, then removed merged).
+		if ( onRemove && isEmpty && isReverse ) {
+			onRemove( ! isReverse );
+		}
+
+		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 horizontal navigation key down event to handle the case where
 	 * TinyMCE attempts to preventDefault when on the outside edge of an inline
@@ -435,32 +494,9 @@ export class RichText extends Component {
 		const rootNode = this.editor.getBody();
 		const { keyCode } = event;
 
-		if (
-			getSelection().isCollapsed && (
-				( keyCode === BACKSPACE && isHorizontalEdge( rootNode, true ) ) ||
-				( keyCode === DELETE && isHorizontalEdge( rootNode, false ) )
-			)
-		) {
-			if ( ! this.props.onMerge && ! this.props.onRemove ) {
-				return;
-			}
-
-			const forward = keyCode === DELETE;
-
-			if ( this.props.onMerge ) {
-				this.props.onMerge( forward );
-			}
-
-			if ( this.props.onRemove && this.isEmpty() ) {
-				this.props.onRemove( forward );
-			}
-
-			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();
+		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>Second</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,22 @@ exports[`splitting and merging blocks should not merge paragraphs if the selecti
 <!-- /wp:paragraph -->"
 `;
 
+exports[`splitting and merging blocks should remove at most one paragraph in forward direction 1`] = `
+"<!-- wp:paragraph -->
+<p>First</p>
+<!-- /wp:paragraph -->
+
+<!-- wp:paragraph -->
+<p></p>
+<!-- /wp:paragraph -->
+
+<!-- wp:paragraph -->
+<p>Second</p>
+<!-- /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,47 @@ 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.type( 'Second' );
+		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();
+	} );
+
+	it( 'should remove at most one paragraph in forward direction', async () => {
+		// Regression Test: A forward delete on empty RichText previously would
+		// destroy two paragraphs on the dual-action of merge & remove.
+		//
+		// See: https://github.com/WordPress/gutenberg/pull/8735
+		await insertBlock( 'Paragraph' );
+		await page.keyboard.type( 'First' );
+		await page.keyboard.press( 'Enter' );
+		await page.keyboard.press( 'Enter' );
+		await page.keyboard.press( 'Enter' );
+		await page.keyboard.type( 'Second' );
+		await page.keyboard.press( 'ArrowUp' );
+		await page.keyboard.press( 'ArrowUp' );
+		await page.keyboard.press( 'Delete' );
+
+		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();
 }
 
 /**