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

Writing Flow: Avoid horizontal writing flow navigation in native inputs #9978

Merged
merged 1 commit into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions packages/editor/src/components/writing-flow/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,29 @@ const isTabbableTextField = overEvery( [
focus.tabbable.isTabbableIndex,
] );

/**
* Returns true if the element should consider edge navigation upon a keyboard
* event of the given directional key code, or false otherwise.
*
* @param {Element} element HTML element to test.
* @param {number} keyCode KeyboardEvent keyCode to test.
* @param {boolean} hasModifier Whether a modifier is pressed.
*
* @return {boolean} Whether element should consider edge navigation.
*/
export function isNavigationCandidate( element, keyCode, hasModifier ) {
const isVertical = ( keyCode === UP || keyCode === DOWN );

// Currently, all elements support unmodified vertical navigation.
if ( isVertical && ! hasModifier ) {
return true;
}

// Native inputs should not navigate horizontally.
const { tagName } = element;
return tagName !== 'INPUT' && tagName !== 'TEXTAREA';
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to apply this to all the editables?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate on what you mean? Are you thinking to reverse this condition so that we test whether the candidate is explicitly contentEditable ? Or do you mean we should include editables In the excluded set of candidates? That would cause issues for navigating between paragraphs, where we do want ArrowRight to navigate between blocks.

}

class WritingFlow extends Component {
constructor() {
super( ...arguments );
Expand Down Expand Up @@ -209,6 +232,7 @@ class WritingFlow extends Component {
const isVertical = isUp || isDown;
const isNav = isHorizontal || isVertical;
const isShift = event.shiftKey;
const hasModifier = isShift || event.ctrlKey || event.altKey || event.metaKey;
const isNavEdge = isVertical ? isVerticalEdge : isHorizontalEdge;

// This logic inside this condition needs to be checked before
Expand Down Expand Up @@ -243,6 +267,12 @@ class WritingFlow extends Component {
return;
}

// Abort if our current target is not a candidate for navigation (e.g.
// preserve native input behaviors).
if ( ! isNavigationCandidate( target, keyCode, hasModifier ) ) {
return;
}

if ( ! isVertical ) {
this.verticalRect = null;
} else if ( ! this.verticalRect ) {
Expand Down
51 changes: 51 additions & 0 deletions packages/editor/src/components/writing-flow/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* WordPress dependencies
*/
import { UP, DOWN, LEFT, RIGHT } from '@wordpress/keycodes';

/**
* Internal dependencies
*/
import { isNavigationCandidate } from '../';

describe( 'isNavigationCandidate', () => {
let elements;
beforeAll( () => {
elements = {};
elements.input = document.createElement( 'input' );
elements.contentEditable = document.createElement( 'p' );
elements.contentEditable.contentEditable = true;
} );

it( 'should return true if vertically navigating input without modifier', () => {
[ UP, DOWN ].forEach( ( keyCode ) => {
const result = isNavigationCandidate( elements.input, keyCode, false );

expect( result ).toBe( true );
} );
} );

it( 'should return false if vertically navigating input with modifier', () => {
[ UP, DOWN ].forEach( ( keyCode ) => {
const result = isNavigationCandidate( elements.input, keyCode, true );

expect( result ).toBe( false );
} );
} );

it( 'should return false if horizontally navigating input', () => {
[ LEFT, RIGHT ].forEach( ( keyCode ) => {
const result = isNavigationCandidate( elements.input, keyCode, false );

expect( result ).toBe( false );
} );
} );

it( 'should return true if horizontally navigating non-input', () => {
[ LEFT, RIGHT ].forEach( ( keyCode ) => {
const result = isNavigationCandidate( elements.contentEditable, keyCode, false );

expect( result ).toBe( true );
} );
} );
} );
28 changes: 28 additions & 0 deletions test/e2e/specs/writing-flow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,32 @@ describe( 'adding blocks', () => {
await pressWithModifier( 'Shift', 'Enter' );
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should navigate native inputs vertically, not horizontally', async () => {
// See: https://github.com/WordPress/gutenberg/issues/9626

// Title is within the editor's writing flow, and is a <textarea>
await page.click( '.editor-post-title' );

// Should remain in title upon ArrowRight:
await page.keyboard.press( 'ArrowRight' );
let isInTitle = await page.evaluate( () => (
!! document.activeElement.closest( '.editor-post-title' )
) );
expect( isInTitle ).toBe( true );

// Should remain in title upon modifier + ArrowDown:
await pressWithModifier( META_KEY, 'ArrowDown' );
isInTitle = await page.evaluate( () => (
!! document.activeElement.closest( '.editor-post-title' )
) );
expect( isInTitle ).toBe( true );

// Should navigate into blocks list upon ArrowDown:
await page.keyboard.press( 'ArrowDown' );
const isInBlock = await page.evaluate( () => (
!! document.activeElement.closest( '[data-type]' )
) );
expect( isInBlock ).toBe( true );
} );
} );