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 ) {
return;
}
Comment on lines +109 to +111
Copy link
Member

Choose a reason for hiding this comment

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

I'm led here via #19462 (comment) , and trying to understand what it was that we were trying to accomplish with this condition. I'm guessing at the time it was when we were first introducing WritingFlow and using arrow keys to navigate between blocks. But in considering a link editor input: Why should we care whether there are suggestions present in deciding whether to prevent the default arrow key behavior? Based on what I describe in #19462 (comment), I sorta expect that arrow keys should always use the native behavior in the input, regardless of suggestions present. I don't really expect to be able to navigate between blocks in this context.

Trying to recall: Did we used to have the link fields inline within the blocks list, and this was how we tried to allow the arrow keys to let the user go to the next/previous focusable field? Most all of those are in popovers now, correct? Maybe we don't want to assume that, but I also don't think we need to optimize for WritingFlow here either.

In some ways it relates to #19488 (#18755), where I talk about what it is about these inputs which make us want them to be ignored by WritingFlow, ObserveTyping, etc. More and more, I think it's that being in a popover (modal) gives it its own context, where it should be treated as detached from the rest of the editor (and thus not inherit any of those typing behaviors).

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 sorta expect that arrow keys should always use the native behavior in the input, regardless of suggestions present.

If URLInput is not used in the popover (Button block at that time), it shouldn't be "an arrow navigation trap"

Copy link
Member

Choose a reason for hiding this comment

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

If URLInput is not used in the popover (Button block at that time), it shouldn't be "an arrow navigation trap"

Yeah, thinking about it more, it feels like "popover" is more the exception (like previously mentioned with related #19488, #18755). It has me wondering whether it would be reasonable to treat popovers as entirely separate context, and to (by default) stop all event bubbling propagation from escaping. Maybe that's an extreme approach, but I kinda see it making sense as a totally independent document, almost like an iframe.


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 [ ...this.container.querySelectorAll( tabbablesSelector ) ].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;