Writing Flow: Fix vertical arrow navigation skips

editor/components/writing-flow/index.js

@@ -31,6 +31,12 @@ import {
 	hasInnerBlocksContext,
 } from '../../utils/dom';
 
+/**
+ * Browser constants
+ */
+
+const { getSelection } = window;
+
 /**
  * Given an element, returns true if the element is a tabbable text field, or
  * false otherwise.
@@ -246,7 +252,7 @@ class WritingFlow extends Component {
 
 		if ( isShift && ( hasMultiSelection || (
 			this.isTabbableEdge( target, isReverse ) &&
-			isNavEdge( target, isReverse, true )
+			isNavEdge( target, isReverse )
 		) ) ) {
 			// Shift key is down, and there is multi selection or we're at the end of the current block.
 			this.expandSelection( isReverse );
@@ -255,14 +261,14 @@ class WritingFlow extends Component {
 			// Moving from block multi-selection to single block selection
 			this.moveSelection( isReverse );
 			event.preventDefault();
-		} else if ( isVertical && isVerticalEdge( target, isReverse, isShift ) ) {
+		} else if ( isVertical && isVerticalEdge( target, isReverse ) ) {
 			const closestTabbable = this.getClosestTabbable( target, isReverse );
 
 			if ( closestTabbable ) {
 				placeCaretAtVerticalEdge( closestTabbable, isReverse, this.verticalRect );
 				event.preventDefault();
 			}
-		} else if ( isHorizontal && isHorizontalEdge( target, isReverse, isShift ) ) {
+		} else if ( isHorizontal && getSelection().isCollapsed && isHorizontalEdge( target, isReverse ) ) {
 			const closestTabbable = this.getClosestTabbable( target, isReverse );
 			placeCaretAtHorizontalEdge( closestTabbable, isReverse );
 			event.preventDefault();

packages/dom/src/dom.js

@@ -1,25 +1,73 @@
 /**
  * External dependencies
  */
-import { includes, first } from 'lodash';
-import tinymce from 'tinymce';
+import { includes } from 'lodash';
 
 /**
  * Browser dependencies
  */
-const { getComputedStyle, DOMRect } = window;
-const { TEXT_NODE, ELEMENT_NODE } = window.Node;
+
+const { getComputedStyle } = window;
+const {
+	TEXT_NODE,
+	ELEMENT_NODE,
+	DOCUMENT_POSITION_PRECEDING,
+	DOCUMENT_POSITION_FOLLOWING,
+} = window.Node;
+
+/**
+ * Returns true if the given selection object is in the forward direction, or
+ * false otherwise.
+ *
+ * @see https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition
+ *
+ * @param {Selection} selection Selection object to check.
+ *
+ * @return {boolean} Whether the selection is forward.
+ */
+function isSelectionForward( selection ) {
+	const {
+		anchorNode,
+		focusNode,
+		anchorOffset,
+		focusOffset,
+	} = selection;
+
+	const position = anchorNode.compareDocumentPosition( focusNode );
+
+	// Disable reason: `Node#compareDocumentPosition` returns a bitmask value,
+	// so bitwise operators are intended.
+	/* eslint-disable no-bitwise */
+	// Compare whether anchor node precedes focus node. If focus node (where
+	// end of selection occurs) is after the anchor node, it is forward.
+	if ( position & DOCUMENT_POSITION_PRECEDING ) {
+		return false;
+	}
+
+	if ( position & DOCUMENT_POSITION_FOLLOWING ) {
+		return true;
+	}
+	/* eslint-enable no-bitwise */
+
+	// `compareDocumentPosition` returns 0 when passed the same node, in which
+	// case compare offsets.
+	if ( position === 0 ) {
+		return anchorOffset <= focusOffset;
+	}
+
+	// This should never be reached, but return true as default case.
+	return true;
+}
 
 /**
- * Check whether the caret is horizontally at the edge of the container.
+ * Check whether the selection is horizontally at the edge of the container.
  *
- * @param {Element} container      Focusable element.
- * @param {boolean} isReverse      Set to true to check left, false for right.
- * @param {boolean} collapseRanges Whether or not to collapse the selection range before the check.
+ * @param {Element} container Focusable element.
+ * @param {boolean} isReverse Set to true to check left, false for right.
  *
  * @return {boolean} True if at the horizontal edge, false if not.
  */
-export function isHorizontalEdge( container, isReverse, collapseRanges = false ) {
+export function isHorizontalEdge( container, isReverse ) {
 	if ( includes( [ 'INPUT', 'TEXTAREA' ], container.tagName ) ) {
 		if ( container.selectionStart !== container.selectionEnd ) {
 			return false;
@@ -36,61 +84,36 @@ export function isHorizontalEdge( container, isReverse, collapseRanges = false )
 		return true;
 	}
 
-	// If the container is empty, the caret is always at the edge.
-	if ( tinymce.DOM.isEmpty( container ) ) {
-		return true;
-	}
-
 	const selection = window.getSelection();
-	let range = selection.rangeCount ? selection.getRangeAt( 0 ) : null;
-	if ( collapseRanges ) {
-		range = range.cloneRange();
-		range.collapse( isReverse );
-	}
-
-	if ( ! range || ! range.collapsed ) {
-		return false;
-	}
-
-	const position = isReverse ? 'start' : 'end';
-	const order = isReverse ? 'first' : 'last';
-	const offset = range[ `${ position }Offset` ];
-
-	let node = range.startContainer;
-
-	if ( isReverse && offset !== 0 ) {
-		return false;
-	}
 
-	const maxOffset = node.nodeType === TEXT_NODE ? node.nodeValue.length : node.childNodes.length;
+	// Create copy of range for setting selection to find effective offset.
+	const range = selection.getRangeAt( 0 ).cloneRange();
 
-	if ( ! isReverse && offset !== maxOffset ) {
-		return false;
+	// Collapse in direction of selection.
+	if ( ! selection.isCollapsed ) {
+		range.collapse( ! isSelectionForward( selection ) );
 	}
 
-	while ( node !== container ) {
-		const parentNode = node.parentNode;
-
-		if ( parentNode[ `${ order }Child` ] !== node ) {
-			return false;
-		}
-
-		node = parentNode;
-	}
+	const { endContainer, endOffset } = range;
+	range.selectNodeContents( container );
+	range.setEnd( endContainer, endOffset );
 
-	return true;
+	// Check if the caret position is at the expected start/end position based
+	// on the value of `isReverse`. If so, consider the horizontal edge to be
+	// reached.
+	const caretOffset = range.toString().length;
+	return caretOffset === ( isReverse ? 0 : container.textContent.length );
 }
 
 /**
- * Check whether the caret is vertically at the edge of the container.
+ * Check whether the selection is vertically at the edge of the container.
  *
- * @param {Element} container      Focusable element.
- * @param {boolean} isReverse      Set to true to check top, false for bottom.
- * @param {boolean} collapseRanges Whether or not to collapse the selection range before the check.
+ * @param {Element} container Focusable element.
+ * @param {boolean} isReverse Set to true to check top, false for bottom.
  *
  * @return {boolean} True if at the edge, false if not.
  */
-export function isVerticalEdge( container, isReverse, collapseRanges = false ) {
+export function isVerticalEdge( container, isReverse ) {
 	if ( includes( [ 'INPUT', 'TEXTAREA' ], container.tagName ) ) {
 		return isHorizontalEdge( container, isReverse );
 	}
@@ -100,16 +123,8 @@ export function isVerticalEdge( container, isReverse, collapseRanges = false ) {
 	}
 
 	const selection = window.getSelection();
-	let range = selection.rangeCount ? selection.getRangeAt( 0 ) : null;
-	if ( collapseRanges && range && ! range.collapsed ) {
-		const newRange = document.createRange();
-		// Get the end point of the selection (see focusNode vs. anchorNode)
-		newRange.setStart( selection.focusNode, selection.focusOffset );
-		newRange.collapse( true );
-		range = newRange;
-	}
-
-	if ( ! range || ! range.collapsed ) {
+	const range = selection.rangeCount ? selection.getRangeAt( 0 ) : null;
+	if ( ! range ) {
 		return false;
 	}
 
@@ -150,21 +165,21 @@ export function getRectangleFromRange( range ) {
 		return range.getBoundingClientRect();
 	}
 
-	// If the collapsed range starts (and therefore ends) at an element node,
-	// `getClientRects` will return undefined. To fix this we can get the
-	// bounding rectangle of the element node to create a DOMRect based on that.
-	if ( range.startContainer.nodeType === ELEMENT_NODE ) {
-		const { x, y, height } = range.startContainer.getBoundingClientRect();
-
-		// Create a new DOMRect with zero width.
-		return new DOMRect( x, y, 0, height );
-	}
+	let rect = range.getClientRects()[ 0 ];
 
-	// For normal collapsed ranges (exception above), the bounding rectangle of
-	// the range may be inaccurate in some browsers. There will only be one
-	// rectangle since it is a collapsed range, so it is safe to pass this as
-	// the union of them. This works consistently in all browsers.
-	return first( range.getClientRects() );
+	// If the collapsed range starts (and therefore ends) at an element node,
+	// `getClientRects` can be empty in some browsers. This can be resolved
+	// by adding a temporary text node with zero-width space to the range.
+	//
+	// See: https://stackoverflow.com/a/6847328/995445
+	if ( ! rect ) {
+		const padNode = document.createTextNode( '\u200b' );
+		range.insertNode( padNode );
+		rect = range.getClientRects()[ 0 ];
+		padNode.parentNode.removeChild( padNode );
+	}
+
+	return rect;
 }
 
 /**
@@ -182,7 +197,7 @@ export function computeCaretRect( container ) {
 	const selection = window.getSelection();
 	const range = selection.rangeCount ? selection.getRangeAt( 0 ) : null;
 
-	if ( ! range || ! range.collapsed ) {
+	if ( ! range ) {
 		return;
 	}
 
@@ -314,7 +329,7 @@ export function placeCaretAtVerticalEdge( container, isReverse, rect, mayUseScro
 	// equivalent to a point at half the height of a line of text.
 	const buffer = rect.height / 2;
 	const editableRect = container.getBoundingClientRect();
-	const x = rect.left + ( rect.width / 2 );
+	const x = rect.left;
 	const y = isReverse ? ( editableRect.bottom - buffer ) : ( editableRect.top + buffer );
 	const selection = window.getSelection();
 

test/e2e/specs/__snapshots__/adding-blocks.test.js.snap

@@ -47,7 +47,7 @@ exports[`adding blocks Should insert content using the placeholder and the regul
 </blockquote>
 <!-- /wp:quote -->
 
-<!-- wp:code -->
-<pre class=\\"wp-block-code\\"><code>Code block</code></pre>
-<!-- /wp:code -->"
+<!-- wp:preformatted -->
+<pre class=\\"wp-block-preformatted\\">Pre text<br/><br/>Foo</pre>
+<!-- /wp:preformatted -->"
 `;

test/e2e/specs/adding-blocks.test.js

@@ -7,6 +7,7 @@ import {
 	newDesktopBrowserPage,
 	insertBlock,
 	getEditedPostContent,
+	pressTimes,
 } from '../support/utils';
 
 describe( 'adding blocks', () => {
@@ -56,8 +57,19 @@ describe( 'adding blocks', () => {
 		expect( await getEditedPostContent() ).toMatchSnapshot();
 
 		// Using the regular inserter
-		await insertBlock( 'Code' );
-		await page.keyboard.type( 'Code block' );
+		await insertBlock( 'Preformatted' );
+		await page.keyboard.type( 'Pre block' );
+		await page.keyboard.press( 'Enter' );
+		await page.keyboard.press( 'Enter' );
+
+		// Verify vertical traversal at offset. This has been buggy in the past
+		// where verticality on a blank newline would skip into previous block.
+		await page.keyboard.type( 'Foo' );
+		await page.keyboard.press( 'ArrowUp' );
+		await page.keyboard.press( 'ArrowUp' );
+		await pressTimes( 'ArrowRight', 3 );
+		await pressTimes( 'Delete', 6 );
+		await page.keyboard.type( ' text' );
 
 		// Unselect blocks to avoid conflicts with the inbetween inserter
 		await page.click( '.editor-post-title__input' );