From a34c471871cec9fb1f49341be498d60c57badd13 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 11 May 2018 11:22:52 -0400 Subject: [PATCH 1/6] Writing Flow: Consider horizontal handled by stopPropagation in RichText --- editor/components/rich-text/index.js | 71 +++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 6 deletions(-) diff --git a/editor/components/rich-text/index.js b/editor/components/rich-text/index.js index 3fcec36dcaba5..feda0ae90840f 100644 --- a/editor/components/rich-text/index.js +++ b/editor/components/rich-text/index.js @@ -25,7 +25,7 @@ import { getScrollContainer, } from '@wordpress/dom'; import { createBlobURL } from '@wordpress/blob'; -import { BACKSPACE, DELETE, ENTER, rawShortcut } from '@wordpress/keycodes'; +import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT, rawShortcut } from '@wordpress/keycodes'; import { withInstanceId, withSafeTimeout, Slot } from '@wordpress/components'; import { withSelect } from '@wordpress/data'; import { rawHandler } from '@wordpress/blocks'; @@ -44,6 +44,22 @@ import { withBlockEditContext } from '../block-edit/context'; import { domToFormat, valueToString } from './format'; import TokenUI from './tokens/ui'; +/** + * Browser dependencies + */ + +const { Node } = window; + +/** + * Zero-width space character used by TinyMCE as a caret landing point for + * inline boundary nodes. + * + * @see tinymce/src/core/main/ts/text/Zwsp.ts + * + * @type {string} + */ +const TINYMCE_ZWSP = '\uFEFF'; + /** * Returns true if the node is the inline node boundary. This is used in node * filtering prevent the inline boundary from being included in the split which @@ -58,7 +74,7 @@ import TokenUI from './tokens/ui'; */ export function isEmptyInlineBoundary( node ) { const text = node.nodeName === 'A' ? node.innerText : node.textContent; - return text === '\uFEFF'; + return text === TINYMCE_ZWSP; } /** @@ -112,6 +128,7 @@ export class RichText extends Component { this.onChange = this.onChange.bind( this ); this.onNewBlock = this.onNewBlock.bind( this ); this.onNodeChange = this.onNodeChange.bind( this ); + this.onHorizontalNavigationKeyDown = this.onHorizontalNavigationKeyDown.bind( this ); this.onKeyDown = this.onKeyDown.bind( this ); this.onKeyUp = this.onKeyUp.bind( this ); this.changeFormats = this.changeFormats.bind( this ); @@ -433,6 +450,42 @@ export class RichText extends Component { left: position.left - containerPosition.left + ( position.width / 2 ), }; } + /** + * Handles a horizontal navigation key down event to stop propagation if it + * can be inferred that it will be handled by TinyMCE (notably transitions + * out of an inline boundary node). + * + * @param {KeyboardEvent} event Keydown event. + */ + onHorizontalNavigationKeyDown( event ) { + const { focusNode, focusOffset } = window.getSelection(); + const { nodeType, nodeValue } = focusNode; + + if ( nodeType !== Node.TEXT_NODE ) { + return; + } + + const isReverse = event.keyCode === LEFT; + + let offset = focusOffset; + if ( isReverse ) { + offset--; + } + + // [WORKAROUND]: When in a new paragraph in a new inline boundary node, + // while typing the zero-width space occurs as the first child instead + // of at the end of the inline boundary where the caret is. This should + // only be exempt when focusNode is not _only_ the ZWSP, which occurs + // when caret is placed on the right outside edge of inline boundary. + if ( ! isReverse && focusOffset === nodeValue.length && + nodeValue.length > 1 && nodeValue[ 0 ] === TINYMCE_ZWSP ) { + offset = 0; + } + + if ( nodeValue[ offset ] === TINYMCE_ZWSP ) { + event.stopPropagation(); + } + } /** * Handles a keydown event from TinyMCE. @@ -442,10 +495,11 @@ export class RichText extends Component { onKeyDown( event ) { const dom = this.editor.dom; const rootNode = this.editor.getBody(); + const { keyCode } = event; if ( - ( event.keyCode === BACKSPACE && isHorizontalEdge( rootNode, true ) ) || - ( event.keyCode === DELETE && isHorizontalEdge( rootNode, false ) ) + ( keyCode === BACKSPACE && isHorizontalEdge( rootNode, true ) ) || + ( keyCode === DELETE && isHorizontalEdge( rootNode, false ) ) ) { if ( ! this.props.onMerge && ! this.props.onRemove ) { return; @@ -453,7 +507,7 @@ export class RichText extends Component { this.onCreateUndoLevel(); - const forward = event.keyCode === DELETE; + const forward = keyCode === DELETE; if ( this.props.onMerge ) { this.props.onMerge( forward ); @@ -471,9 +525,14 @@ export class RichText extends Component { event.stopImmediatePropagation(); } + const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT; + if ( isHorizontalNavigation ) { + this.onHorizontalNavigationKeyDown( event ); + } + // If we click shift+Enter on inline RichTexts, we avoid creating two contenteditables // We also split the content and call the onSplit prop if provided. - if ( event.keyCode === ENTER ) { + if ( keyCode === ENTER ) { if ( this.props.multiline ) { if ( ! this.props.onSplit ) { return; From 9eddb3614c11b29c1e0e4a4eeff7903ad0d76cf4 Mon Sep 17 00:00:00 2001 From: "Matthew Riley MacPherson (tofumatt)" Date: Thu, 28 Jun 2018 17:07:27 +0100 Subject: [PATCH 2/6] chore: Move TINYMCE_ZERO_WIDTH_SPACE to keycodes and expand name --- editor/components/rich-text/index.js | 26 ++++++++++++-------------- packages/keycodes/src/index.js | 10 ++++++++++ 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/editor/components/rich-text/index.js b/editor/components/rich-text/index.js index feda0ae90840f..b4740533bc237 100644 --- a/editor/components/rich-text/index.js +++ b/editor/components/rich-text/index.js @@ -25,7 +25,15 @@ import { getScrollContainer, } from '@wordpress/dom'; import { createBlobURL } from '@wordpress/blob'; -import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT, rawShortcut } from '@wordpress/keycodes'; +import { + BACKSPACE, + DELETE, + ENTER, + LEFT, + RIGHT, + TINYMCE_ZERO_WIDTH_SPACE, + rawShortcut, +} from '@wordpress/keycodes'; import { withInstanceId, withSafeTimeout, Slot } from '@wordpress/components'; import { withSelect } from '@wordpress/data'; import { rawHandler } from '@wordpress/blocks'; @@ -50,16 +58,6 @@ import TokenUI from './tokens/ui'; const { Node } = window; -/** - * Zero-width space character used by TinyMCE as a caret landing point for - * inline boundary nodes. - * - * @see tinymce/src/core/main/ts/text/Zwsp.ts - * - * @type {string} - */ -const TINYMCE_ZWSP = '\uFEFF'; - /** * Returns true if the node is the inline node boundary. This is used in node * filtering prevent the inline boundary from being included in the split which @@ -74,7 +72,7 @@ const TINYMCE_ZWSP = '\uFEFF'; */ export function isEmptyInlineBoundary( node ) { const text = node.nodeName === 'A' ? node.innerText : node.textContent; - return text === TINYMCE_ZWSP; + return text === TINYMCE_ZERO_WIDTH_SPACE; } /** @@ -478,11 +476,11 @@ export class RichText extends Component { // only be exempt when focusNode is not _only_ the ZWSP, which occurs // when caret is placed on the right outside edge of inline boundary. if ( ! isReverse && focusOffset === nodeValue.length && - nodeValue.length > 1 && nodeValue[ 0 ] === TINYMCE_ZWSP ) { + nodeValue.length > 1 && nodeValue[ 0 ] === TINYMCE_ZERO_WIDTH_SPACE ) { offset = 0; } - if ( nodeValue[ offset ] === TINYMCE_ZWSP ) { + if ( nodeValue[ offset ] === TINYMCE_ZERO_WIDTH_SPACE ) { event.stopPropagation(); } } diff --git a/packages/keycodes/src/index.js b/packages/keycodes/src/index.js index f5ab3ea87ce13..6e0d2d444e9f1 100644 --- a/packages/keycodes/src/index.js +++ b/packages/keycodes/src/index.js @@ -33,6 +33,16 @@ export const CTRL = 'ctrl'; export const COMMAND = 'meta'; export const SHIFT = 'shift'; +/** + * Zero-width space character used by TinyMCE as a caret landing point for + * inline boundary nodes. + * + * @see tinymce/src/core/main/ts/text/Zwsp.ts + * + * @type {string} + */ +export const TINYMCE_ZERO_WIDTH_SPACE = '\uFEFF'; + /** * Return true if platform is MacOS. * From a79442b4fee19d913ffdd8febe03055e6f61b554 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 28 Jun 2018 17:28:29 -0400 Subject: [PATCH 3/6] Revert "chore: Move TINYMCE_ZERO_WIDTH_SPACE to keycodes and expand name" This reverts commit 9eddb3614c11b29c1e0e4a4eeff7903ad0d76cf4. --- editor/components/rich-text/index.js | 26 ++++++++++++++------------ packages/keycodes/src/index.js | 10 ---------- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/editor/components/rich-text/index.js b/editor/components/rich-text/index.js index b4740533bc237..feda0ae90840f 100644 --- a/editor/components/rich-text/index.js +++ b/editor/components/rich-text/index.js @@ -25,15 +25,7 @@ import { getScrollContainer, } from '@wordpress/dom'; import { createBlobURL } from '@wordpress/blob'; -import { - BACKSPACE, - DELETE, - ENTER, - LEFT, - RIGHT, - TINYMCE_ZERO_WIDTH_SPACE, - rawShortcut, -} from '@wordpress/keycodes'; +import { BACKSPACE, DELETE, ENTER, LEFT, RIGHT, rawShortcut } from '@wordpress/keycodes'; import { withInstanceId, withSafeTimeout, Slot } from '@wordpress/components'; import { withSelect } from '@wordpress/data'; import { rawHandler } from '@wordpress/blocks'; @@ -58,6 +50,16 @@ import TokenUI from './tokens/ui'; const { Node } = window; +/** + * Zero-width space character used by TinyMCE as a caret landing point for + * inline boundary nodes. + * + * @see tinymce/src/core/main/ts/text/Zwsp.ts + * + * @type {string} + */ +const TINYMCE_ZWSP = '\uFEFF'; + /** * Returns true if the node is the inline node boundary. This is used in node * filtering prevent the inline boundary from being included in the split which @@ -72,7 +74,7 @@ const { Node } = window; */ export function isEmptyInlineBoundary( node ) { const text = node.nodeName === 'A' ? node.innerText : node.textContent; - return text === TINYMCE_ZERO_WIDTH_SPACE; + return text === TINYMCE_ZWSP; } /** @@ -476,11 +478,11 @@ export class RichText extends Component { // only be exempt when focusNode is not _only_ the ZWSP, which occurs // when caret is placed on the right outside edge of inline boundary. if ( ! isReverse && focusOffset === nodeValue.length && - nodeValue.length > 1 && nodeValue[ 0 ] === TINYMCE_ZERO_WIDTH_SPACE ) { + nodeValue.length > 1 && nodeValue[ 0 ] === TINYMCE_ZWSP ) { offset = 0; } - if ( nodeValue[ offset ] === TINYMCE_ZERO_WIDTH_SPACE ) { + if ( nodeValue[ offset ] === TINYMCE_ZWSP ) { event.stopPropagation(); } } diff --git a/packages/keycodes/src/index.js b/packages/keycodes/src/index.js index 6e0d2d444e9f1..f5ab3ea87ce13 100644 --- a/packages/keycodes/src/index.js +++ b/packages/keycodes/src/index.js @@ -33,16 +33,6 @@ export const CTRL = 'ctrl'; export const COMMAND = 'meta'; export const SHIFT = 'shift'; -/** - * Zero-width space character used by TinyMCE as a caret landing point for - * inline boundary nodes. - * - * @see tinymce/src/core/main/ts/text/Zwsp.ts - * - * @type {string} - */ -export const TINYMCE_ZERO_WIDTH_SPACE = '\uFEFF'; - /** * Return true if platform is MacOS. * From 48da235a5c1e99a366fdf9953f52928d14b6f78c Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 29 Jun 2018 09:02:21 -0400 Subject: [PATCH 4/6] Rich Text: Clarify documentation around horizontal handling --- editor/components/rich-text/index.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/editor/components/rich-text/index.js b/editor/components/rich-text/index.js index feda0ae90840f..3b681ccf8686f 100644 --- a/editor/components/rich-text/index.js +++ b/editor/components/rich-text/index.js @@ -467,16 +467,24 @@ export class RichText extends Component { const isReverse = event.keyCode === LEFT; + // Look to previous character for ZWSP if navigating in reverse. let offset = focusOffset; if ( isReverse ) { offset--; } - // [WORKAROUND]: When in a new paragraph in a new inline boundary node, - // while typing the zero-width space occurs as the first child instead - // of at the end of the inline boundary where the caret is. This should - // only be exempt when focusNode is not _only_ the ZWSP, which occurs - // when caret is placed on the right outside edge of inline boundary. + // Workaround: In a new inline boundary node, the zero-width space + // wrongly lingers at the beginning of the node, rather than following + // the caret. If we are at the extent of the inline boundary, but the + // ZWSP exists at the beginning, consider as though it were to be + // handled as a transition outside the inline boundary. + // + // Since this condition could also be satisfied in the case that we're + // on the right edge of an inline boundary -- where the ZWSP exists as + // as an otherwise empty focus node -- ensure that the focus node is + // non-empty. + // + // See: https://github.com/tinymce/tinymce/issues/4472 if ( ! isReverse && focusOffset === nodeValue.length && nodeValue.length > 1 && nodeValue[ 0 ] === TINYMCE_ZWSP ) { offset = 0; From 81a1b7d86c217afa63c913076517db54421a68fc Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 29 Jun 2018 09:02:43 -0400 Subject: [PATCH 5/6] Writing Flow: Add E2E test for inline boundary traversal --- .../__snapshots__/writing-flow.test.js.snap | 14 +++++ test/e2e/specs/writing-flow.test.js | 63 +++++++++++++++++-- test/e2e/support/utils.js | 32 ++++++++++ 3 files changed, 105 insertions(+), 4 deletions(-) diff --git a/test/e2e/specs/__snapshots__/writing-flow.test.js.snap b/test/e2e/specs/__snapshots__/writing-flow.test.js.snap index e9ad304b10799..d47e884d09a9e 100644 --- a/test/e2e/specs/__snapshots__/writing-flow.test.js.snap +++ b/test/e2e/specs/__snapshots__/writing-flow.test.js.snap @@ -21,3 +21,17 @@ exports[`adding blocks Should navigate inner blocks with arrow keys 1`] = `

Second paragraph

" `; + +exports[`adding blocks should navigate around inline boundaries 1`] = ` +" +

FirstAfter

+ + + +

BeforeInsideSecondInsideAfter

+ + + +

BeforeThird

+" +`; diff --git a/test/e2e/specs/writing-flow.test.js b/test/e2e/specs/writing-flow.test.js index 70665c8e2f192..ffeeed1271ebe 100644 --- a/test/e2e/specs/writing-flow.test.js +++ b/test/e2e/specs/writing-flow.test.js @@ -6,14 +6,13 @@ import { newPost, newDesktopBrowserPage, getHTMLFromCodeEditor, + pressWithModifier, + pressTimes, } from '../support/utils'; describe( 'adding blocks', () => { - beforeAll( async () => { - await newDesktopBrowserPage(); - } ); - beforeEach( async () => { + await newDesktopBrowserPage(); await newPost(); } ); @@ -64,4 +63,60 @@ describe( 'adding blocks', () => { expect( await getHTMLFromCodeEditor() ).toMatchSnapshot(); } ); + + it( 'should navigate around inline boundaries', async () => { + // Add demo content + await page.click( '.editor-default-block-appender__content' ); + await page.keyboard.type( 'First' ); + await page.keyboard.press( 'Enter' ); + await page.keyboard.type( 'Second' ); + await page.keyboard.press( 'Enter' ); + await page.keyboard.type( 'Third' ); + + // Navigate to second paragraph + await pressTimes( 'ArrowLeft', 6 ); + + // Bold second paragraph text + await page.keyboard.down( 'Shift' ); + await pressTimes( 'ArrowLeft', 6 ); + await page.keyboard.up( 'Shift' ); + await pressWithModifier( 'mod', 'b' ); + + // Arrow left from selected bold should traverse into first. + await page.keyboard.press( 'ArrowLeft' ); + await page.keyboard.type( 'After' ); + + // Arrow right from end of first should traverse to second, *BEFORE* + // the bolded text. Another press should move within inline boundary. + await pressTimes( 'ArrowRight', 2 ); + await page.keyboard.type( 'Inside' ); + + // Arrow left from end of beginning of inline boundary should move to + // the outside of the inline boundary. + await pressTimes( 'ArrowLeft', 6 ); + await page.keyboard.press( 'ArrowLeft' ); // Separate for emphasis. + await page.keyboard.type( 'Before' ); + + // Likewise, test at the end of the inline boundary for same effect. + await page.keyboard.press( 'ArrowRight' ); // Move inside + await pressTimes( 'ArrowRight', 12 ); + await page.keyboard.type( 'Inside' ); + await page.keyboard.press( 'ArrowRight' ); + + // Edge case: Verify that workaround to test for ZWSP at beginning of + // focus node does not take effect when on the right edge of inline + // boundary (thus preventing traversing to the next block by arrow). + await page.keyboard.press( 'ArrowRight' ); + await page.keyboard.press( 'ArrowLeft' ); + + // Should be after the inline boundary again. + await page.keyboard.type( 'After' ); + + // Finally, ensure that ArrowRight from end of unbolded text moves to + // the last paragraph + await page.keyboard.press( 'ArrowRight' ); + await page.keyboard.type( 'Before' ); + + expect( await getHTMLFromCodeEditor() ).toMatchSnapshot(); + } ); } ); diff --git a/test/e2e/support/utils.js b/test/e2e/support/utils.js index a9fb2d9984efd..eb347ef3d67e5 100644 --- a/test/e2e/support/utils.js +++ b/test/e2e/support/utils.js @@ -4,6 +4,11 @@ import { join } from 'path'; import { URL } from 'url'; +/** + * External dependencies + */ +import { times } from 'lodash'; + const { WP_BASE_URL = 'http://localhost:8888', WP_USERNAME = 'admin', @@ -26,6 +31,21 @@ const MOD_KEY = process.platform === 'darwin' ? 'Meta' : 'Control'; */ const REGEXP_ZWSP = /[\u200B\u200C\u200D\uFEFF]/; +/** + * Given an array of functions, each returning a promise, performs all + * promises in sequence (waterfall) order. + * + * @param {Function[]} sequence Array of promise creators. + * + * @return {Promise} Promise resolving once all in the sequence complete. + */ +async function promiseSequence( sequence ) { + return sequence.reduce( + ( current, next ) => current.then( next ), + Promise.resolve() + ); +} + export function getUrl( WPPath, query = '' ) { const url = new URL( WP_BASE_URL ); @@ -232,6 +252,18 @@ export async function openDocumentSettingsSidebar() { } } +/** + * Presses the given keyboard key a number of times in sequence. + * + * @param {string} key Key to press. + * @param {number} count Number of times to press. + * + * @return {Promise} Promise resolving when key presses complete. + */ +export async function pressTimes( key, count ) { + return promiseSequence( times( count, () => () => page.keyboard.press( key ) ) ); +} + export async function clearLocalStorage() { await page.evaluate( () => window.localStorage.clear() ); } From 8eda14ef3e1081a8a9b30f5b83767f416fb7f4a2 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 29 Jun 2018 12:00:32 -0400 Subject: [PATCH 6/6] Writing Flow: Prevent default writing flow by property setter --- editor/components/rich-text/index.js | 29 ++++++++++++++++++++----- editor/components/writing-flow/index.js | 10 +++++++++ 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/editor/components/rich-text/index.js b/editor/components/rich-text/index.js index 3b681ccf8686f..0de8197e6b064 100644 --- a/editor/components/rich-text/index.js +++ b/editor/components/rich-text/index.js @@ -212,6 +212,21 @@ export class RichText extends Component { this.editor.shortcuts.add( rawShortcut.primary( 'z' ), '', 'Undo' ); this.editor.shortcuts.add( rawShortcut.primaryShift( 'z' ), '', 'Redo' ); + // Bind directly to the document, since properties assigned to TinyMCE + // events are lost when accessed during bubbled handlers. This must be + // captured _before_ TinyMCE's internal handling of arrow keys, which + // would otherwise already have moved the caret position. This is why + // it is bound to the capture phase. + // + // Note: This is ideally a temporary measure, only needed so long as + // TinyMCE inaccurately prevents default around inline boundaries. + // Ideally we rely on the defaultPrevented property to stop WritingFlow + // from transitioning. + // + // See: https://github.com/tinymce/tinymce/issues/4476 + // See: WritingFlow#onKeyDown + this.editor.dom.doc.addEventListener( 'keydown', this.onHorizontalNavigationKeyDown, true ); + // Remove TinyMCE Core shortcut for consistency with global editor // shortcuts. Also clashes with Mac browsers. this.editor.shortcuts.remove( 'meta+y', '', 'Redo' ); @@ -450,6 +465,7 @@ export class RichText extends Component { left: position.left - containerPosition.left + ( position.width / 2 ), }; } + /** * Handles a horizontal navigation key down event to stop propagation if it * can be inferred that it will be handled by TinyMCE (notably transitions @@ -458,6 +474,12 @@ export class RichText extends Component { * @param {KeyboardEvent} event Keydown event. */ onHorizontalNavigationKeyDown( event ) { + const { keyCode } = event; + const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT; + if ( ! isHorizontalNavigation ) { + return; + } + const { focusNode, focusOffset } = window.getSelection(); const { nodeType, nodeValue } = focusNode; @@ -491,7 +513,7 @@ export class RichText extends Component { } if ( nodeValue[ offset ] === TINYMCE_ZWSP ) { - event.stopPropagation(); + event._navigationHandled = true; } } @@ -533,11 +555,6 @@ export class RichText extends Component { event.stopImmediatePropagation(); } - const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT; - if ( isHorizontalNavigation ) { - this.onHorizontalNavigationKeyDown( event ); - } - // If we click shift+Enter on inline RichTexts, we avoid creating two contenteditables // We also split the content and call the onSplit prop if provided. if ( keyCode === ENTER ) { diff --git a/editor/components/writing-flow/index.js b/editor/components/writing-flow/index.js index 1eb0a646a71f1..2e35cf0a195ab 100644 --- a/editor/components/writing-flow/index.js +++ b/editor/components/writing-flow/index.js @@ -188,6 +188,16 @@ class WritingFlow extends Component { onKeyDown( event ) { const { hasMultiSelection, onMultiSelect, blocks } = this.props; + // If navigation has already been handled (e.g. TinyMCE inline + // boundaries), abort. Ideally this uses Event#defaultPrevented. This + // is currently not possible because TinyMCE will misreport an event + // default as prevented on outside edges of inline boundaries. + // + // See: https://github.com/tinymce/tinymce/issues/4476 + if ( event.nativeEvent._navigationHandled ) { + return; + } + const { keyCode, target } = event; const isUp = keyCode === UP; const isDown = keyCode === DOWN;