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
91 changes: 91 additions & 0 deletions editor/utils/dom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* 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;
if ( isInputOrTextarea ) {
container.focus();
setTimeout( () => {
if ( start ) {
container.selectionStart = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Do we know that container still exists at this point? To the more pertinent point, do we need the setTimeout ? Can we comment as such to why it is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was necessary at the time of writing (not sure why?), but I made some tweaks later, I'll check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, the timeout is not necessary anymore :)

container.selectionEnd = 0;
} else {
container.selectionStart = container.value.length;
container.selectionEnd = container.value.length;
}
} );
return;
}

function placeCaretInContentEditable( element, atStart ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we create a function for this when we could inline it in place? Comments can serve as stand-in to self-documenting functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep probably not necessary

const range = document.createRange();
range.selectNodeContents( element );
range.collapse( atStart );
const sel = window.getSelection();
sel.removeAllRanges();
sel.addRange( range );
element.focus();
}

placeCaretInContentEditable( container, start );
}
98 changes: 98 additions & 0 deletions editor/utils/test/dom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/**
* Internal dependencies
*/
import { isEdge, placeCaretAtEdge } from '../dom';

jest.useFakeTimers();

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 );
jest.runAllTimers();
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 );
jest.runAllTimers();
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 tabblablesSelector = [
Copy link
Member

Choose a reason for hiding this comment

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

As in #2323 and #2392, we're increasingly drawn to needing general helpers with this sort of logic, and am tempted to bring in ally.js despite it not being terribly compact. Don't need to make changes here, but curious if you have thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a library or extracting to a general helper seems like a good thing to me. But I'm not sure it will work as expected, if I understand correctly ally.js returns different tabbables depending on the browser. We'll have to test I guess

Copy link
Member

Choose a reason for hiding this comment

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

Should this be 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( tabblablesSelector ) ).filter( isVisible );
}

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;