Writing Flow: Consider horizontal handled by stopPropagation in RichText

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 );
@@ -195,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' );
@@ -434,6 +466,57 @@ export class RichText extends Component {
 		};
 	}
 
+	/**
+	 * 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 { keyCode } = event;
+		const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT;
+		if ( ! isHorizontalNavigation ) {
+			return;
+		}
+
+		const { focusNode, focusOffset } = window.getSelection();
+		const { nodeType, nodeValue } = focusNode;
+
+		if ( nodeType !== Node.TEXT_NODE ) {
+			return;
+		}
+
+		const isReverse = event.keyCode === LEFT;
+
+		// Look to previous character for ZWSP if navigating in reverse.
+		let offset = focusOffset;
+		if ( isReverse ) {
+			offset--;
+		}
+
+		// 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;
+		}
+
+		if ( nodeValue[ offset ] === TINYMCE_ZWSP ) {
+			event._navigationHandled = true;
+		}
+	}
+
 	/**
 	 * Handles a keydown event from TinyMCE.
 	 *
@@ -442,18 +525,19 @@ 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;
 			}
 
 			this.onCreateUndoLevel();
 
-			const forward = event.keyCode === DELETE;
+			const forward = keyCode === DELETE;
 
 			if ( this.props.onMerge ) {
 				this.props.onMerge( forward );
@@ -473,7 +557,7 @@ export class RichText extends Component {
 
 		// 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;

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;

test/e2e/specs/__snapshots__/writing-flow.test.js.snap

@@ -21,3 +21,17 @@ exports[`adding blocks Should navigate inner blocks with arrow keys 1`] = `
 <p>Second paragraph</p>
 <!-- /wp:paragraph -->"
 `;
+
+exports[`adding blocks should navigate around inline boundaries 1`] = `
+"<!-- wp:paragraph -->
+<p>FirstAfter</p>
+<!-- /wp:paragraph -->
+
+<!-- wp:paragraph -->
+<p>Before<strong>InsideSecondInside</strong>After</p>
+<!-- /wp:paragraph -->
+
+<!-- wp:paragraph -->
+<p>BeforeThird</p>
+<!-- /wp:paragraph -->"
+`;

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();
+	} );
 } );

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() );
 }