Skip to content

Commit

Permalink
Writing Flow: Fix vertical arrow navigation skips
Browse files Browse the repository at this point in the history
  • Loading branch information
@aduth
aduth committed Jul 17, 2018
1 parent 73c24d8 commit 52b8f97
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 85 deletions.
12 changes: 9 additions & 3 deletions editor/components/writing-flow/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 );
Expand All @@ -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();
Expand Down
169 changes: 92 additions & 77 deletions packages/dom/src/dom.js
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 );
}
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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;
}

Expand Down Expand Up @@ -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();

Expand Down
6 changes: 3 additions & 3 deletions test/e2e/specs/__snapshots__/adding-blocks.test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 -->"
`;
16 changes: 14 additions & 2 deletions test/e2e/specs/adding-blocks.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
newDesktopBrowserPage,
insertBlock,
getEditedPostContent,
pressTimes,
} from '../support/utils';

describe( 'adding blocks', () => {
Expand Down Expand Up @@ -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' );
Expand Down

0 comments on commit 52b8f97

Please sign in to comment.