diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index f4014adbd752f7..84ec75d87f6e1b 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/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} 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; diff --git a/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap b/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap index 10f51e1be04b7c..2fddf0347d8890 100644 --- a/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap +++ b/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`] = ` " `; +exports[`splitting and merging blocks should forward delete from an empty paragraph 1`] = ` +" +

Second

+" +`; + exports[`splitting and merging blocks should gracefully handle if placing caret in empty container 1`] = ` "

Foo

@@ -32,6 +38,22 @@ exports[`splitting and merging blocks should not merge paragraphs if the selecti " `; +exports[`splitting and merging blocks should remove at most one paragraph in forward direction 1`] = ` +" +

First

+ + + +

+ + + +

Second

+" +`; + +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`] = ` "

First

diff --git a/test/e2e/specs/splitting-merging.test.js b/test/e2e/specs/splitting-merging.test.js index 3bc50c77875d07..d8bdc29de618e1 100644 --- a/test/e2e/specs/splitting-merging.test.js +++ b/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(); + } ); } ); diff --git a/test/e2e/support/utils.js b/test/e2e/support/utils.js index 61fdba52b7ceb0..988a4d6902ae0d 100644 --- a/test/e2e/support/utils.js +++ b/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(); } /**