From 25fb109c35066f2ac4a9a8e7f4503c5890171a3a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 8 Aug 2018 18:46:28 -0400 Subject: [PATCH 1/4] Testing: Wait for RichText initialization --- test/e2e/support/utils.js | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) 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(); } /** From 1c58754d481d1d53a5ea760b9a3e4ac083e18572 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 8 Aug 2018 18:47:09 -0400 Subject: [PATCH 2/4] RichText: Fix empty value delete behaviors --- .../editor/src/components/block-list/block.js | 31 +++--- .../editor/src/components/rich-text/README.md | 4 +- .../editor/src/components/rich-text/index.js | 96 ++++++++++++++----- .../splitting-merging.test.js.snap | 8 ++ test/e2e/specs/splitting-merging.test.js | 32 ++++++- 5 files changed, 131 insertions(+), 40 deletions(-) diff --git a/packages/editor/src/components/block-list/block.js b/packages/editor/src/components/block-list/block.js index efb8a0f33d2c39..eab1110f39d2ce 100644 --- a/packages/editor/src/components/block-list/block.js +++ b/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 ) { diff --git a/packages/editor/src/components/rich-text/README.md b/packages/editor/src/components/rich-text/README.md index b50833379d3785..a99c7ef402e416 100644 --- a/packages/editor/src/components/rich-text/README.md +++ b/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` diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index f4014adbd752f7..430f620a23f20f 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 ); @@ -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} 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; diff --git a/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap b/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap index 10f51e1be04b7c..822082cb41643d 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`] = ` +" +

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

Foo

@@ -32,6 +38,8 @@ exports[`splitting and merging blocks should not merge paragraphs if the selecti " `; +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..ed3176e94b83e4 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,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(); + } ); } ); From d29d5d131c7dd09d66de75d8b1e99ef69ca07bcb Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 9 Aug 2018 10:24:06 -0400 Subject: [PATCH 3/4] RichText: Handle remove only on backspace --- .../editor/src/components/block-list/block.js | 31 ++--- .../editor/src/components/rich-text/README.md | 4 +- .../editor/src/components/rich-text/index.js | 118 ++++++++---------- .../splitting-merging.test.js.snap | 16 ++- test/e2e/specs/splitting-merging.test.js | 19 +++ 5 files changed, 102 insertions(+), 86 deletions(-) diff --git a/packages/editor/src/components/block-list/block.js b/packages/editor/src/components/block-list/block.js index eab1110f39d2ce..efb8a0f33d2c39 100644 --- a/packages/editor/src/components/block-list/block.js +++ b/packages/editor/src/components/block-list/block.js @@ -218,31 +218,22 @@ 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; - const hasMergeableTarget = ( - ( forward && !! nextBlockClientId ) || - ( ! forward && !! previousBlockClientId ) - ); - - if ( hasMergeableTarget ) { - if ( forward ) { - onMerge( block.clientId, nextBlockClientId ); - } else { - onMerge( previousBlockClientId, block.clientId ); - } + // Do nothing when it's the first block. + if ( + ( ! forward && ! previousBlockClientId ) || + ( forward && ! nextBlockClientId ) + ) { + return; } - return hasMergeableTarget; + if ( forward ) { + onMerge( block.clientId, nextBlockClientId ); + } else { + onMerge( previousBlockClientId, block.clientId ); + } } insertBlocksAfter( blocks ) { diff --git a/packages/editor/src/components/rich-text/README.md b/packages/editor/src/components/rich-text/README.md index a99c7ef402e416..b50833379d3785 100644 --- a/packages/editor/src/components/rich-text/README.md +++ b/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. 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. +*Optional.* Called when blocks can be merged. `forward` is true when merging with the next block, false when merging with the previous block. ### `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. Called when pressing Backspace or Delete on an empty field and only when `onMerge` explicitly returns `false` to indicate non-merge. +*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. ### `formattingControls: Array` diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index 430f620a23f20f..4201354e5b4bce 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -383,53 +383,11 @@ export class RichText extends Component { } /** - * Handles a horizontal navigation key down event to handle the case where - * TinyMCE attempts to preventDefault when on the outside edge of an inline - * boundary when arrowing _away_ from the boundary, not within it. Replaces - * the TinyMCE event `preventDefault` behavior with a noop, such that those - * relying on `defaultPrevented` are not misinformed about the arrow event. - * - * If TinyMCE#4476 is resolved, this handling may be removed. - * - * @see https://github.com/tinymce/tinymce/issues/4476 + * 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. * - * @param {tinymce.EditorEvent} event Keydown event. - */ - onHorizontalNavigationKeyDown( event ) { - const { focusNode } = getSelection(); - const { nodeType, nodeValue } = focusNode; - - if ( nodeType !== Node.TEXT_NODE ) { - return; - } - - if ( nodeValue.length !== 1 || nodeValue[ 0 ] !== TINYMCE_ZWSP ) { - return; - } - - const { keyCode } = event; - - // Consider to be moving away from inline boundary based on: - // - // 1. Within a text fragment consisting only of ZWSP. - // 2. If in reverse, there is no previous sibling. If forward, there is - // no next sibling (i.e. end of node). - const isReverse = keyCode === LEFT; - const edgeSibling = isReverse ? 'previousSibling' : 'nextSibling'; - if ( ! focusNode[ edgeSibling ] ) { - // Note: This is not reassigning on the native event, rather the - // "fixed" TinyMCE copy, which proxies its preventDefault to the - // native event. By reassigning here, we're effectively preventing - // the proxied call on the native event, but not otherwise mutating - // the original event object. - event.preventDefault = noop; - } - } - - /** - * 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. + * @link https://en.wikipedia.org/wiki/Caret_navigation * * @param {tinymce.EditorEvent} event Keydown event. */ @@ -443,7 +401,7 @@ export class RichText extends Component { const isReverse = keyCode === BACKSPACE; const { isCollapsed } = getSelection(); - // Only process delete if occurs at uncollapsed edge. + // Only process delete if the key press occurs at uncollapsed edge. if ( ! isCollapsed ) { return; } @@ -462,26 +420,16 @@ export class RichText extends Component { return; } - let isHandled = false; - - let isMerged = false; if ( onMerge ) { - isMerged = onMerge( ! isReverse ); - isHandled = !! isMerged; + onMerge( ! isReverse ); } - // `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 ) { + // 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 ); - isHandled = true; - } - - if ( ! isHandled ) { - // Only prevent default behaviors if handled. - return; } event.preventDefault(); @@ -492,6 +440,50 @@ export class RichText extends Component { 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 + * boundary when arrowing _away_ from the boundary, not within it. Replaces + * the TinyMCE event `preventDefault` behavior with a noop, such that those + * relying on `defaultPrevented` are not misinformed about the arrow event. + * + * If TinyMCE#4476 is resolved, this handling may be removed. + * + * @see https://github.com/tinymce/tinymce/issues/4476 + * + * @param {tinymce.EditorEvent} event Keydown event. + */ + onHorizontalNavigationKeyDown( event ) { + const { focusNode } = getSelection(); + const { nodeType, nodeValue } = focusNode; + + if ( nodeType !== Node.TEXT_NODE ) { + return; + } + + if ( nodeValue.length !== 1 || nodeValue[ 0 ] !== TINYMCE_ZWSP ) { + return; + } + + const { keyCode } = event; + + // Consider to be moving away from inline boundary based on: + // + // 1. Within a text fragment consisting only of ZWSP. + // 2. If in reverse, there is no previous sibling. If forward, there is + // no next sibling (i.e. end of node). + const isReverse = keyCode === LEFT; + const edgeSibling = isReverse ? 'previousSibling' : 'nextSibling'; + if ( ! focusNode[ edgeSibling ] ) { + // Note: This is not reassigning on the native event, rather the + // "fixed" TinyMCE copy, which proxies its preventDefault to the + // native event. By reassigning here, we're effectively preventing + // the proxied call on the native event, but not otherwise mutating + // the original event object. + event.preventDefault = noop; + } + } + /** * Handles a keydown event from TinyMCE. * diff --git a/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap b/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap index 822082cb41643d..2fddf0347d8890 100644 --- a/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap +++ b/test/e2e/specs/__snapshots__/splitting-merging.test.js.snap @@ -8,7 +8,7 @@ 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

" `; @@ -38,6 +38,20 @@ 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`] = ` diff --git a/test/e2e/specs/splitting-merging.test.js b/test/e2e/specs/splitting-merging.test.js index ed3176e94b83e4..d8bdc29de618e1 100644 --- a/test/e2e/specs/splitting-merging.test.js +++ b/test/e2e/specs/splitting-merging.test.js @@ -135,6 +135,7 @@ describe( 'splitting and merging blocks', () => { // 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' ); @@ -151,4 +152,22 @@ describe( 'splitting and merging blocks', () => { 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(); + } ); } ); From eab0afa16c2995ed481070fff622250623c36239 Mon Sep 17 00:00:00 2001 From: "Matthew Riley MacPherson (tofumatt)" Date: Thu, 9 Aug 2018 15:47:41 +0100 Subject: [PATCH 4/4] docs: Very minor comment readability tweak --- packages/editor/src/components/rich-text/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/editor/src/components/rich-text/index.js b/packages/editor/src/components/rich-text/index.js index 4201354e5b4bce..84ec75d87f6e1b 100644 --- a/packages/editor/src/components/rich-text/index.js +++ b/packages/editor/src/components/rich-text/index.js @@ -383,7 +383,7 @@ export class RichText extends Component { } /** - * Handles a delete keydown event to handle merge or removal for collapsed + * 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. *