Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framework: Extract arrow navigation to its own component #2424

Merged
merged 12 commits into from
Aug 29, 2017
Merged
6 changes: 6 additions & 0 deletions blocks/url-input/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ class UrlInput extends Component {

onKeyDown( event ) {
const { selectedSuggestion, posts } = this.state;
// If the suggestions are not shown, we shouldn't handle the arrow keys
// We shouldn't preventDefault to allow block arrow keys navigation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't return false; have the same effect as preventDefault?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not since it fixed the issue (maybe it was the stopPropagation) but any way safer to just return

if ( ! this.state.showSuggestions || ! this.state.posts.length ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a comment here to explain what this is for? I don't know what showSuggestions has to do with the key event.

return false;
}

switch ( event.keyCode ) {
case UP: {
event.stopPropagation();
Expand Down
3 changes: 1 addition & 2 deletions editor/modes/visual-editor/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class VisualEditorBlock extends Component {
}

// Focus node when focus state is programmatically transferred.
if ( this.props.focus && ! prevProps.focus ) {
if ( this.props.focus && ! prevProps.focus && ! this.node.contains( document.activeElement ) ) {
this.node.focus();
}

Expand Down Expand Up @@ -256,7 +256,6 @@ class VisualEditorBlock extends Component {

onKeyDown( event ) {
const { keyCode, target } = event;

if ( ENTER === keyCode && target === this.node ) {
event.preventDefault();

Expand Down
7 changes: 5 additions & 2 deletions editor/modes/visual-editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { KeyboardShortcuts } from '@wordpress/components';
import './style.scss';
import VisualEditorBlockList from './block-list';
import PostTitle from '../../post-title';
import WritingFlow from '../../writing-flow';
import { getBlockUids, getMultiSelectedBlockUids } from '../../selectors';
import { clearSelectedBlock, multiSelect, redo, undo, removeBlocks } from '../../actions';

Expand Down Expand Up @@ -98,8 +99,10 @@ class VisualEditor extends Component {
backspace: this.deleteSelectedBlocks,
del: this.deleteSelectedBlocks,
} } />
<PostTitle />
<VisualEditorBlockList ref={ this.bindBlocksContainer } />
<WritingFlow>
<PostTitle />
<VisualEditorBlockList ref={ this.bindBlocksContainer } />
</WritingFlow>
</div>
);
/* eslint-enable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */
Expand Down
88 changes: 88 additions & 0 deletions editor/utils/dom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/**
* Check whether the selection touches an edge of the container
*
* @param {Element} container DOM Element
* @param {Boolean} start Reverse means check if it touches the start of the container
* @return {Boolean} Is Edge or not
*/
export function isEdge( container, start = false ) {
if ( [ 'INPUT', 'TEXTAREA' ].indexOf( container.tagName ) !== -1 ) {
if ( container.selectionStart !== container.selectionEnd ) {
return false;
}

if ( start ) {
return container.selectionStart === 0;
}

return container.value.length === container.selectionStart;
}

if ( ! container.isContentEditable ) {
return true;
}

const selection = window.getSelection();
const range = selection.rangeCount ? selection.getRangeAt( 0 ) : null;
const position = start ? 'start' : 'end';
const order = start ? 'first' : 'last';
const offset = range[ `${ position }Offset` ];

let node = range.startContainer;

if ( ! range || ! range.collapsed ) {
return false;
}

if ( start && offset !== 0 ) {
return false;
}

if ( ! start && offset !== node.textContent.length ) {
return false;
}

while ( node !== container ) {
const parentNode = node.parentNode;

if ( parentNode[ `${ order }Child` ] !== node ) {
return false;
}

node = parentNode;
}

return true;
}

/**
* Places the caret at start or end of a given element
*
* @param {Element} container DOM Element
* @param {Boolean} start Position: Start or end of the element
*/
export function placeCaretAtEdge( container, start = false ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, it would be better to use a hash for the options so it's more readable. (Same for the other functions.)

const isInputOrTextarea = [ 'INPUT', 'TEXTAREA' ].indexOf( container.tagName ) !== -1;

// Inputs and Textareas
if ( isInputOrTextarea ) {
container.focus();
if ( start ) {
container.selectionStart = 0;
container.selectionEnd = 0;
} else {
container.selectionStart = container.value.length;
container.selectionEnd = container.value.length;
}
return;
}

// Content editables
const range = document.createRange();
range.selectNodeContents( container );
range.collapse( start );
const sel = window.getSelection();
sel.removeAllRanges();
sel.addRange( range );
container.focus();
}
94 changes: 94 additions & 0 deletions editor/utils/test/dom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/**
* Internal dependencies
*/
import { isEdge, placeCaretAtEdge } from '../dom';

describe( 'DOM', () => {
let parent;

beforeEach( () => {
parent = document.createElement( 'div' );
document.body.appendChild( parent );
} );

afterEach( () => {
parent.remove();
} );

describe( 'isEdge', () => {
it( 'Should return true for empty input', () => {
const input = document.createElement( 'input' );
parent.appendChild( input );
input.focus();
expect( isEdge( input, true ) ).toBe( true );
expect( isEdge( input, false ) ).toBe( true );
} );

it( 'Should return the right values if we focus the end of the input', () => {
const input = document.createElement( 'input' );
parent.appendChild( input );
input.value = 'value';
input.focus();
input.selectionStart = 5;
input.selectionEnd = 5;
expect( isEdge( input, true ) ).toBe( false );
expect( isEdge( input, false ) ).toBe( true );
} );

it( 'Should return the right values if we focus the start of the input', () => {
const input = document.createElement( 'input' );
parent.appendChild( input );
input.value = 'value';
input.focus();
input.selectionStart = 0;
input.selectionEnd = 0;
expect( isEdge( input, true ) ).toBe( true );
expect( isEdge( input, false ) ).toBe( false );
} );

it( 'Should return false if we\'re not at the edge', () => {
const input = document.createElement( 'input' );
parent.appendChild( input );
input.value = 'value';
input.focus();
input.selectionStart = 3;
input.selectionEnd = 3;
expect( isEdge( input, true ) ).toBe( false );
expect( isEdge( input, false ) ).toBe( false );
} );

it( 'Should return false if the selection is not collapseds', () => {
const input = document.createElement( 'input' );
parent.appendChild( input );
input.value = 'value';
input.focus();
input.selectionStart = 0;
input.selectionEnd = 5;
expect( isEdge( input, true ) ).toBe( false );
expect( isEdge( input, false ) ).toBe( false );
} );

it( 'Should always return true for non content editabless', () => {
const div = document.createElement( 'div' );
parent.appendChild( div );
expect( isEdge( div, true ) ).toBe( true );
expect( isEdge( div, false ) ).toBe( true );
} );
} );

describe( 'placeCaretAtEdge', () => {
it( 'should place caret at the start of the input', () => {
const input = document.createElement( 'input' );
input.value = 'value';
placeCaretAtEdge( input, true );
expect( isEdge( input, true ) ).toBe( true );
} );

it( 'should place caret at the end of the input', () => {
const input = document.createElement( 'input' );
input.value = 'value';
placeCaretAtEdge( input, false );
expect( isEdge( input, false ) ).toBe( true );
} );
} );
} );
97 changes: 97 additions & 0 deletions editor/writing-flow/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**
* WordPress dependencies
*/
import { Component } from 'element';
import { keycodes } from '@wordpress/utils';

/**
* Internal dependencies
*/
import { isEdge, placeCaretAtEdge } from '../utils/dom';

/**
* Module Constants
*/
const { UP, DOWN, LEFT, RIGHT } = keycodes;

class WritingFlow extends Component {
constructor() {
super( ...arguments );
this.zones = [];
this.onKeyDown = this.onKeyDown.bind( this );
this.onKeyUp = this.onKeyUp.bind( this );
this.bindContainer = this.bindContainer.bind( this );
this.state = {
shouldMove: false,
};
}

bindContainer( ref ) {
this.container = ref;
}

getVisibleTabbables() {
const tabbablesSelector = [
'*[contenteditable="true"]',
'*[tabindex]:not([tabindex="-1"])',
'textarea',
'input',
].join( ', ' );
const isVisible = ( elem ) => elem.offsetWidth > 0 || elem.offsetHeight > 0 || elem.getClientRects().length > 0;
return Array.from( this.container.querySelectorAll( tabbablesSelector ) ).filter( isVisible );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor (stylistic): Can be expressed with array spread too:

[ ...this.container.querySelectorAll( tabbablesSelector ) ]

}

moveFocusInContainer( target, direction = 'UP' ) {
const focusableNodes = this.getVisibleTabbables();
if ( direction === 'UP' ) {
focusableNodes.reverse();
}

const targetNode = focusableNodes
.slice( focusableNodes.indexOf( target ) )
.reduce( ( result, node ) => {
return result || ( node.contains( target ) ? null : node );
}, null );

if ( targetNode ) {
placeCaretAtEdge( targetNode, direction === 'DOWN' );
}
}

onKeyDown( event ) {
const { keyCode, target } = event;
const moveUp = ( keyCode === UP || keyCode === LEFT );
const moveDown = ( keyCode === DOWN || keyCode === RIGHT );

if ( ( moveUp || moveDown ) && isEdge( target, moveUp ) ) {
event.preventDefault();
this.setState( { shouldMove: true } );
Copy link
Member

@aduth aduth Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be using state for this, or could we assign an instance property? Thinking the latter would help to avoid two unnecessary rerenders.

this.shouldMove = true;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're probably right 👍

}
}

onKeyUp( event ) {
const { keyCode, target } = event;
const moveUp = ( keyCode === UP || keyCode === LEFT );
if ( this.state.shouldMove ) {
event.preventDefault();
this.moveFocusInContainer( target, moveUp ? 'UP' : 'DOWN' );
this.setState( { shouldMove: false } );
}
}

render() {
const { children } = this.props;

return (
<div
ref={ this.bindContainer }
onKeyDown={ this.onKeyDown }
onKeyUp={ this.onKeyUp }
>
{ children }
</div>
);
}
}

export default WritingFlow;