From 52b8f97de1ddeb4255c9e2b737793cb8afec9492 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 17 Jul 2018 11:48:28 -0400 Subject: [PATCH] Writing Flow: Fix vertical arrow navigation skips --- editor/components/writing-flow/index.js | 12 +- packages/dom/src/dom.js | 169 ++++++++++-------- .../__snapshots__/adding-blocks.test.js.snap | 6 +- test/e2e/specs/adding-blocks.test.js | 16 +- 4 files changed, 118 insertions(+), 85 deletions(-) diff --git a/editor/components/writing-flow/index.js b/editor/components/writing-flow/index.js index e7beed323d3fac..64c68367ac9012 100644 --- a/editor/components/writing-flow/index.js +++ b/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(); diff --git a/packages/dom/src/dom.js b/packages/dom/src/dom.js index 1ea0ae74cb44bf..64dabe4fb087f6 100644 --- a/packages/dom/src/dom.js +++ b/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(); diff --git a/test/e2e/specs/__snapshots__/adding-blocks.test.js.snap b/test/e2e/specs/__snapshots__/adding-blocks.test.js.snap index 5509ccb1c980e7..edc0b163cff072 100644 --- a/test/e2e/specs/__snapshots__/adding-blocks.test.js.snap +++ b/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 - -
Code block
-" + +
Pre text

Foo
+" `; diff --git a/test/e2e/specs/adding-blocks.test.js b/test/e2e/specs/adding-blocks.test.js index 8ce4de2c999eb9..329fcccff3be5a 100644 --- a/test/e2e/specs/adding-blocks.test.js +++ b/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' );