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.

Sign up for GitHub

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: improved tabbing for Edit mode #19235

Merged
merged 1 commit into from
Dec 19, 2019
Merged

Writing flow: improved tabbing for Edit mode #19235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 77 additions & 1 deletion packages/block-editor/src/components/writing-flow/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
isBlockFocusStop,
isInSameBlock,
hasInnerBlocksContext,
getBlockFocusableWrapper,
} from '../../utils/dom';

/**
Expand Down Expand Up @@ -72,6 +73,33 @@ export function isNavigationCandidate( element, keyCode, hasModifier ) {
return tagName !== 'INPUT' && tagName !== 'TEXTAREA';
}

/**
* Renders focus capturing areas to redirect focus to the selected block if not
* in Navigation mode.
*/
function FocusCapture( { isReverse, clientId, isNavigationMode } ) {
function onFocus() {
const wrapper = getBlockFocusableWrapper( clientId );

if ( isReverse ) {
const tabbables = focus.tabbable.find( wrapper );
last( tabbables ).focus();
} else {
wrapper.focus();
}
}

return (
<div
tabIndex={ clientId && ! isNavigationMode ? '0' : undefined }
onFocus={ onFocus }
// Needs to be positioned within the viewport, so focus to this
// element does not scroll the page.
style={ { position: 'fixed' } }
/>
);
}

class WritingFlow extends Component {
constructor() {
super( ...arguments );
Expand Down Expand Up @@ -235,6 +263,7 @@ class WritingFlow extends Component {
selectionBeforeEndClientId,
selectionAfterEndClientId,
isNavigationMode,
selectionStartClientId,
} = this.props;

const { keyCode, target } = event;
Expand Down Expand Up @@ -273,6 +302,38 @@ class WritingFlow extends Component {
return;
}

const clientId = selectedBlockClientId || selectionStartClientId;

// In Edit mode, Tab should focus the first tabbable element after the
// content, which is normally the sidebar (with block controls) and
// Shift+Tab should focus the first tabbable element before the content,
// which is normally the block toolbar.
// Arrow keys can be used, and Tab and arrow keys can be used in
// Navigation mode (press Esc), to navigate through blocks.
if ( isTab && clientId ) {
const wrapper = getBlockFocusableWrapper( clientId );

if ( isShift ) {
if ( target === wrapper ) {
const focusableParent = this.container.closest( '[tabindex]' );
const beforeEditorElement = focus.tabbable.findPrevious( focusableParent );
beforeEditorElement.focus();
event.preventDefault();
return;
}
} else {
const tabbables = focus.tabbable.find( wrapper );

if ( target === last( tabbables ) ) {
const focusableParent = this.container.closest( '[tabindex]' );
const afterEditorElement = focus.tabbable.findNext( focusableParent );
afterEditorElement.focus();
event.preventDefault();
return;
}
}
}

// When presing any key other than up or down, the initial vertical
// position must ALWAYS be reset. The vertical position is saved so it
// can be restored as well as possible on sebsequent vertical arrow key
Expand Down Expand Up @@ -378,7 +439,13 @@ class WritingFlow extends Component {
}

render() {
const { children } = this.props;
const {
children,
isNavigationMode,
selectedBlockClientId,
selectionStartClientId,
} = this.props;
const clientId = selectedBlockClientId || selectionStartClientId;

// Disable reason: Wrapper itself is non-interactive, but must capture
// bubbling events from children to determine focus transition intents.
Expand All @@ -390,7 +457,16 @@ class WritingFlow extends Component {
onKeyDown={ this.onKeyDown }
onMouseDown={ this.onMouseDown }
>
<FocusCapture
clientId={ clientId }
isNavigationMode={ isNavigationMode }
/>
{ children }
<FocusCapture
clientId={ clientId }
isNavigationMode={ isNavigationMode }
isReverse
/>
</div>
<div
ref={ this.appender }
Expand Down
51 changes: 48 additions & 3 deletions packages/dom/src/tabbable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { without } from 'lodash';
import { without, first, last } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -125,11 +125,56 @@ function compareObjectTabbables( a, b ) {
return aTabIndex - bTabIndex;
}

export function find( context ) {
return findFocusable( context )
/**
* Givin focusable elements, filters out tabbable element.
*
* @param {Array} focusables Focusable elements to filter.
*
* @return {Array} Tabbable elements.
*/
function filterTabbable( focusables ) {
return focusables
.filter( isTabbableIndex )
.map( mapElementToObjectTabbable )
.sort( compareObjectTabbables )
.map( mapObjectTabbableToElement )
.reduce( createStatefulCollapseRadioGroup(), [] );
}

export function find( context ) {
return filterTabbable( findFocusable( context ) );
}

/**
* Given a focusable element, find the preceding tabbable element.
*
* @param {Element} element The focusable element before which to look. Defaults
* to the active element.
*/
export function findPrevious( element = document.activeElement ) {
const focusables = findFocusable( document.body );
const index = focusables.indexOf( element );

// Remove all focusables after and including `element`.
focusables.length = index;

return last( filterTabbable( focusables ) );
}

/**
* Given a focusable element, find the next tabbable element.
*
* @param {Element} element The focusable element after which to look. Defaults
* to the active element.
*/
export function findNext( element = document.activeElement ) {
const focusables = findFocusable( document.body );
const index = focusables.indexOf( element );

// Remove all focusables before and inside `element`.
const remaining = focusables
.slice( index + 1 )
.filter( ( node ) => ! element.contains( node ) );

return first( filterTabbable( remaining ) );
}
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/editor/blocks/preformatted.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe( 'Preformatted', () => {
await page.keyboard.press( 'Enter' );
await page.keyboard.type( '2' );
await page.keyboard.press( 'Enter' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'ArrowDown' );
await page.keyboard.type( '3' );
await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.press( 'Backspace' );
Expand Down
107 changes: 44 additions & 63 deletions packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,88 +7,70 @@ import {
pressKeyWithModifier,
} from '@wordpress/e2e-test-utils';

async function getActiveLabel() {
return await page.evaluate( () => {
return (
document.activeElement.getAttribute( 'aria-label' ) ||
document.activeElement.innerHTML
);
} );
}

const navigateToContentEditorTop = async () => {
// Use 'Ctrl+`' to return to the top of the editor
await pressKeyWithModifier( 'ctrl', '`' );
await pressKeyWithModifier( 'ctrl', '`' );

// Tab into the Title block
await page.keyboard.press( 'Tab' );
};

const tabThroughParagraphBlock = async ( paragraphText ) => {
// Tab to the next paragraph block
await page.keyboard.press( 'Tab' );
await expect( await getActiveLabel() ).toBe( 'Block: Paragraph' );

// The block external focusable wrapper has focus
const isFocusedParagraphBlock = await page.evaluate(
() => document.activeElement.dataset.type
);
await expect( isFocusedParagraphBlock ).toEqual( 'core/paragraph' );

// Tab causes 'add block' button to receive focus
await page.keyboard.press( 'Tab' );
const isFocusedParagraphInserterToggle = await page.evaluate( () =>
document.activeElement.classList.contains( 'block-editor-inserter__toggle' )
);
await expect( isFocusedParagraphInserterToggle ).toBe( true );
await expect( await getActiveLabel() ).toBe( 'Add block' );

await tabThroughBlockMoverControl();
await tabThroughBlockToolbar();

// Tab causes the paragraph content to receive focus
await page.keyboard.press( 'Tab' );
const isFocusedParagraphContent = await page.evaluate(
() => document.activeElement.contentEditable
);
// The value of 'contentEditable' should be the string 'true'
await expect( isFocusedParagraphContent ).toBe( 'true' );

const paragraphEditableContent = await page.evaluate(
() => document.activeElement.innerHTML
);
await expect( paragraphEditableContent ).toBe( paragraphText );
await expect( await getActiveLabel() ).toBe( 'Paragraph block' );
await expect( await page.evaluate( () =>
document.activeElement.innerHTML
) ).toBe( paragraphText );

await page.keyboard.press( 'Tab' );
await expect( await getActiveLabel() ).toBe( 'Open publish panel' );
};

const tabThroughBlockMoverControl = async () => {
// Tab to focus on the 'move up' control
await page.keyboard.press( 'Tab' );
const isFocusedMoveUpControl = await page.evaluate( () =>
document.activeElement.classList.contains( 'block-editor-block-mover__control' )
);
await expect( isFocusedMoveUpControl ).toBe( true );
await expect( await getActiveLabel() ).toBe( 'Move up' );

// Tab to focus on the 'move down' control
await page.keyboard.press( 'Tab' );
const isFocusedMoveDownControl = await page.evaluate( () =>
document.activeElement.classList.contains( 'block-editor-block-mover__control' )
);
await expect( isFocusedMoveDownControl ).toBe( true );
await expect( await getActiveLabel() ).toBe( 'Move down' );
};

const tabThroughBlockToolbar = async () => {
// Tab to focus on the 'block switcher' control
await page.keyboard.press( 'Tab' );
const isFocusedBlockSwitcherControl = await page.evaluate( () =>
document.activeElement.classList.contains(
'block-editor-block-switcher__toggle'
)
);
await expect( isFocusedBlockSwitcherControl ).toBe( true );

// Tab to focus on the 'Change text alignment' dropdown control
await expect( await getActiveLabel() ).toBe( 'Change block type or style' );

await page.keyboard.press( 'Tab' );
await expect( await getActiveLabel() ).toBe( 'Change text alignment' );

await page.keyboard.press( 'Tab' );
await expect( await getActiveLabel() ).toBe( 'Bold' );

await page.keyboard.press( 'Tab' );
const isFocusedChangeTextAlignmentControl = await page.evaluate( () =>
document.activeElement.classList.contains( 'components-dropdown-menu__toggle' )
);
await expect( isFocusedChangeTextAlignmentControl ).toBe( true );
await expect( await getActiveLabel() ).toBe( 'Italic' );

// Tab to focus on the 'More formatting' dropdown toggle
await page.keyboard.press( 'Tab' );
const isFocusedBlockSettingsDropdown = await page.evaluate( () =>
document.activeElement.classList.contains( 'components-dropdown-menu__toggle' )
);
await expect( isFocusedBlockSettingsDropdown ).toBe( true );
await expect( await getActiveLabel() ).toBe( 'Link' );

await page.keyboard.press( 'Tab' );
await expect( await getActiveLabel() ).toBe( 'More rich text controls' );

await page.keyboard.press( 'Tab' );
await expect( await getActiveLabel() ).toBe( 'More options' );
};

describe( 'Order of block keyboard navigation', () => {
Expand All @@ -103,21 +85,20 @@ describe( 'Order of block keyboard navigation', () => {
for ( const paragraphBlock of paragraphBlocks ) {
await insertBlock( 'Paragraph' );
await page.keyboard.type( paragraphBlock );
await page.keyboard.press( 'Enter' );
}

await navigateToContentEditorTop();
// Select the middle block.
await page.keyboard.press( 'ArrowUp' );
// Move the mouse to show the block toolbar
await page.mouse.move( 0, 0 );
await page.mouse.move( 10, 10 );
Comment on lines +92 to +94
Copy link
Member

Choose a reason for hiding this comment

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

It troubles me when I see interactions like these in the end-to-end tests, because the first question which comes to mind is: Is there not an easier way to express this as a keyboard interaction? Or, worse, is it even possible to achieve this behavior via keyboard alone?

I'm not sure if it matters much for this specific test, since it seems to be at least somewhat contrived example to verify which options are present, but in general I'm a bit concerned about navigation mode, or at least specifically options for "cancelling" the isTyping flag. Previously, it was problematic in how it relied on a mouse or clunky selection operations to control. Later, Escape was added as an option to unset the flag (#10906), but I guess now that's been overridden to control whether the block editor is in navigation mode. I worry this all has put us back in a position we were in prior to #10906 where certain interactions are only possible using the cursor.


for ( const paragraphBlock of paragraphBlocks ) {
await tabThroughParagraphBlock( paragraphBlock );
}
await navigateToContentEditorTop();
await tabThroughParagraphBlock( 'Paragraph 1' );

// Repeat the same steps to ensure that there is no change introduced in how the focus is handled.
// This prevents the previous regression explained in: https://github.com/WordPress/gutenberg/issues/11773.
await navigateToContentEditorTop();

for ( const paragraphBlock of paragraphBlocks ) {
await tabThroughParagraphBlock( paragraphBlock );
}
await tabThroughParagraphBlock( 'Paragraph 1' );
} );
} );