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

Try/2031 manual focus #2392

Closed
wants to merge 36 commits into from
Closed
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
aa66504
Fix dropdown menu so it can't be open and have nothing selected
tiny-james Aug 8, 2017
14c9446
Make an open dropdown menu select forward/back with tab/shift+tab
tiny-james Aug 8, 2017
8bd904a
Make tabIndex of dropdown menu changeable
tiny-james Aug 8, 2017
fdc699b
Make block switcher use dropdown menu
tiny-james Aug 8, 2017
7be9165
Remove toolbars and other things from the default tab cycle
tiny-james Aug 8, 2017
2f86df0
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 8, 2017
13d9cc0
Use tab to jump between toolbars
tiny-james Aug 9, 2017
7664de5
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 9, 2017
763e186
Use arrow keys to cycle through toolbar items; Alt+F10 to select toolbar
tiny-james Aug 10, 2017
a99096e
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 11, 2017
8cc32a2
Select toolbar with F10, deselect with Escape
tiny-james Aug 14, 2017
c05dc9c
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 14, 2017
394371c
Added mover to the toolbar cycle
tiny-james Aug 14, 2017
829af3f
Add F10 to keycodes
tiny-james Aug 15, 2017
c210a69
Take disabled buttons into account when creating toolbar navigation c…
tiny-james Aug 15, 2017
2770127
Calculate if a block needs a focusable border
tiny-james Aug 15, 2017
fdc1120
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 15, 2017
5ae9a14
Remove unused code
tiny-james Aug 18, 2017
6574e4b
Remove unused imports to fix lint
tiny-james Aug 18, 2017
9dd13cf
Updated test to match new behaviour of tab key in drop-down menu
tiny-james Aug 18, 2017
c70d4f8
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 18, 2017
20218b0
Fix reference to removed code.
tiny-james Aug 18, 2017
36bb0d1
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 21, 2017
7924dd7
Fix incorrect merge
tiny-james Aug 21, 2017
3432da5
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 22, 2017
b1f5896
Fix the code block so it will take focus on tab
tiny-james Aug 22, 2017
b31ffee
Allow F10 to work to select standard buttons in blocks without toolbars
tiny-james Aug 22, 2017
16021fa
try/2031: Use Element.closest instead of hand rolled version
tiny-james Aug 22, 2017
aa77c25
try/2031: Allow hiding UI on ESCAPE before deselecting
tiny-james Aug 22, 2017
033d0f3
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 23, 2017
69f5011
try/2031: Move toolbar selection code into its own function
tiny-james Aug 24, 2017
84b183c
try/2031: Move let into more specific scope
tiny-james Aug 24, 2017
1facd3c
try/2031: Removed unneeded Array.from; Added a missing filter predicate
tiny-james Aug 24, 2017
c8f7363
Removed unneeded uses of findDOMNode
tiny-james Aug 24, 2017
f3af11c
Merge branch 'master' into try/2031-manual-focus
tiny-james Aug 29, 2017
0e533c2
Fix missed dependency
tiny-james Aug 29, 2017
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
2 changes: 1 addition & 1 deletion blocks/api/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export function registerBlockType( name, settings ) {
);
return;
}
const block = Object.assign( { name }, settings );
const block = { name, ...settings };
blocks[ name ] = block;
return block;
}
Expand Down
3 changes: 2 additions & 1 deletion blocks/library/code/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ registerBlockType( 'core/code', {
],
},

edit( { attributes, setAttributes, className } ) {
edit( { attributes, setAttributes, className, setFocus } ) {
return (
<TextareaAutosize
className={ className }
value={ attributes.content }
onChange={ ( event ) => setAttributes( { content: event.target.value } ) }
onFocus={ ( ) => setFocus() }
placeholder={ __( 'Write code…' ) }
/>
);
Expand Down
1 change: 1 addition & 0 deletions blocks/library/gallery/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ registerBlockType( 'core/gallery', {
buttonProps={ {
className: 'components-icon-button components-toolbar__control',
'aria-label': __( 'Edit Gallery' ),
tabIndex: -1,
Copy link
Member

@aduth aduth Aug 28, 2017

Choose a reason for hiding this comment

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

With these changes, what are the implications on block authors for managing tab index? How do we document this? Can we avoid it?

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to hijack the tab keypress to avoid the default tabIndex behavior when in the tabbable cycle, so that we don't need buttons to be assigned with tabIndex="-1"?

} }
onSelect={ onSelectImages }
type="image"
Expand Down
3 changes: 2 additions & 1 deletion blocks/library/image/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ class ImageBlock extends Component {
buttonProps={ {
className: 'components-icon-button components-toolbar__control',
'aria-label': __( 'Edit image' ),
tabIndex: '-1',
} }
onSelect={ this.onSelectImage }
type="image"
Expand All @@ -124,7 +125,7 @@ class ImageBlock extends Component {
<Dashicon icon="edit" />
</MediaUploadButton>
</li>
<UrlInputButton onChange={ this.onSetHref } url={ href } />
<UrlInputButton onChange={ this.onSetHref } url={ href } tabIndex="-1" />
</Toolbar>
</BlockControls>
)
Expand Down
1 change: 1 addition & 0 deletions blocks/library/table/table-block.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export default class TableBlock extends Component {
...control,
onClick: () => control.onClick( this.state.editor ),
} ) ) }
tabIndex="-1"
/>
</li>
</Toolbar>
Expand Down
3 changes: 2 additions & 1 deletion blocks/url-input/button.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class UrlInputButton extends Component {
}

render() {
const { url, onChange } = this.props;
const { url, onChange, tabIndex } = this.props;
const { expanded } = this.state;

return (
Expand All @@ -48,6 +48,7 @@ class UrlInputButton extends Component {
className={ classnames( 'components-toolbar__control', {
'is-active': url,
} ) }
tabIndex={ tabIndex }
/>
{ expanded &&
<form
Expand Down
18 changes: 11 additions & 7 deletions components/dropdown-menu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ export class DropdownMenu extends Component {
closeMenu() {
this.setState( {
open: false,
activeIndex: null,
} );
}

toggleMenu() {
this.setState( {
open: ! this.state.open,
activeIndex: this.state.open ? null : 0,
} );
}

Expand Down Expand Up @@ -114,8 +116,13 @@ export class DropdownMenu extends Component {
if ( this.state.open ) {
switch ( keydown.keyCode ) {
case TAB:
keydown.preventDefault();
keydown.stopPropagation();
this.closeMenu();
if ( keydown.shiftKey ) {
this.focusPrevious();
} else {
this.focusNext();
}
break;

case LEFT:
Expand Down Expand Up @@ -150,12 +157,7 @@ export class DropdownMenu extends Component {
}

componentDidUpdate( prevProps, prevState ) {
const { open, activeIndex } = this.state;

// Focus the first item when the menu opens.
if ( ! prevState.open && open ) {
this.focusIndex( 0 );
}
const { activeIndex } = this.state;

// Change focus to active index
const { menu } = this.nodes;
Expand All @@ -172,6 +174,7 @@ export class DropdownMenu extends Component {
label,
menuLabel,
controls,
tabIndex,
} = this.props;

if ( ! controls || ! controls.length ) {
Expand All @@ -196,6 +199,7 @@ export class DropdownMenu extends Component {
aria-haspopup="true"
aria-expanded={ this.state.open }
label={ label }
tabIndex={ tabIndex }
ref={ this.bindReferenceNode( 'toggle' ) }
>
<Dashicon icon="arrow-down" />
Expand Down
19 changes: 2 additions & 17 deletions components/dropdown-menu/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,12 @@ describe( 'DropdownMenu', () => {

assertKeyDown( RIGHT, 1 );
assertKeyDown( DOWN, 2 );
assertKeyDown( DOWN, 3 );
assertKeyDown( TAB, 3 );
assertKeyDown( DOWN, 0 ); // Reset to beginning
assertKeyDown( DOWN, 1 );
assertKeyDown( LEFT, 0 );
assertKeyDown( UP, 3 ); // Reset to end
assertKeyDown( TAB, 0 );
} );

it( 'should close menu on escape', () => {
Expand Down Expand Up @@ -166,21 +167,5 @@ describe( 'DropdownMenu', () => {

expect( wrapper.state( 'open' ) ).toBe( false );
} );

it( 'should close menu on tab', () => {
const wrapper = shallow( <DropdownMenu controls={ controls } /> );

// Open menu
wrapper.find( '> IconButton' ).simulate( 'click' );

// Close menu by tab
wrapper.simulate( 'keydown', {
stopPropagation: () => {},
preventDefault: () => {},
keyCode: TAB,
} );

expect( wrapper.state( 'open' ) ).toBe( false );
} );
} );
} );
1 change: 1 addition & 0 deletions components/toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ function Toolbar( { controls = [], children, className } ) {
className={ setIndex > 0 && controlIndex === 0 ? 'has-left-divider' : null }
>
<IconButton
tabIndex="-1"
icon={ control.icon }
label={ control.title }
data-subscript={ control.subscript }
Expand Down
2 changes: 2 additions & 0 deletions editor/block-mover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, blockType, f
-1,
) }
aria-disabled={ isFirst }
tabIndex="-1"
/>
<IconButton
className="editor-block-mover__control"
Expand All @@ -54,6 +55,7 @@ function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, blockType, f
1,
) }
aria-disabled={ isLast }
tabIndex="-1"
/>
</div>
);
Expand Down
2 changes: 2 additions & 0 deletions editor/block-settings-menu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ function BlockSettingsMenu( { onDelete, onSelect, isSidebarOpened, toggleSidebar
onClick={ toggleInspector }
icon="admin-generic"
label={ __( 'Show inspector' ) }
tabIndex="-1"
/>
<IconButton
className="editor-block-settings-menu__control"
onClick={ onDelete }
icon="trash"
label={ __( 'Delete the block' ) }
tabIndex="-1"
/>
</div>
);
Expand Down
76 changes: 17 additions & 59 deletions editor/block-switcher/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@
*/
import { connect } from 'react-redux';
import { uniq, get, reduce, find } from 'lodash';
import clickOutside from 'react-click-outside';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { Dashicon, IconButton } from '@wordpress/components';
import { Toolbar, DropdownMenu } from '@wordpress/components';
import { getBlockType, getBlockTypes, switchToBlockType } from '@wordpress/blocks';

/**
Expand All @@ -21,33 +20,9 @@ import { replaceBlocks } from '../actions';
import { getBlock } from '../selectors';

class BlockSwitcher extends Component {
constructor() {
super( ...arguments );
this.toggleMenu = this.toggleMenu.bind( this );
this.state = {
open: false,
};
}

handleClickOutside() {
if ( ! this.state.open ) {
return;
}

this.toggleMenu();
}

toggleMenu() {
this.setState( ( state ) => ( {
open: ! state.open,
} ) );
}

switchBlockType( name ) {
return () => {
this.setState( {
open: false,
} );
this.props.onTransform( this.props.block, name );
};
}
Expand All @@ -72,38 +47,21 @@ class BlockSwitcher extends Component {
}

return (
<div className="editor-block-switcher">
<IconButton
className="editor-block-switcher__toggle"
icon={ blockType.icon }
onClick={ this.toggleMenu }
aria-haspopup="true"
aria-expanded={ this.state.open }
label={ __( 'Change block type' ) }
>
<Dashicon icon="arrow-down" />
</IconButton>
{ this.state.open &&
<div
className="editor-block-switcher__menu"
role="menu"
tabIndex="0"
aria-label={ __( 'Block types' ) }
>
{ allowedBlocks.map( ( { name, title, icon } ) => (
<IconButton
key={ name }
onClick={ this.switchBlockType( name ) }
className="editor-block-switcher__menu-item"
icon={ icon }
role="menuitem"
>
{ title }
</IconButton>
) ) }
</div>
}
</div>
<Toolbar>
<li>
Copy link
Member

Choose a reason for hiding this comment

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

Aside: Ideally a consumer of the Toolbar component should not care that it renders itself as a ul, and could pass children without the li. In the implementation of the Toolbar component, children can be achieved with React.Children.map:

<ul>
	{ React.Children.map( ( child, i ) => (
		<li key={ i }>{ child }</li>
	) ) }
</ul>

Mentioning as aside since it's not really relevant to the scope of your pull request.

Copy link
Author

Choose a reason for hiding this comment

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

It would make using the Toolbar with children slightly easier (and I have considered it), however it would also remove some control from the caller (ie to add classes to some li elements).

Copy link
Member

Choose a reason for hiding this comment

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

however it would also remove some control from the caller (ie to add classes to some li elements).

To me, this is not control we want to allow, or at least not through exposing li directly. There are a few alternatives, such as a prop on Toolbar, a wrapper for each children like ToolbarItem, a wrapper of the item itself (apply class to a div parent of DropdownMenu), or more advanced examples like function as child components.

Ultimately, I don't think we know enough about the actual use cases to need to concern ourselves with this control just yet though.

<DropdownMenu
icon={ blockType.icon }
label={ __( 'Change block type' ) }
controls={
allowedBlocks.map( ( { name, title, icon } ) => ( {
icon,
title,
onClick: this.switchBlockType( name ),
} ) ) }
tabIndex="-1"
/>
</li>
</Toolbar>
);
}
}
Expand All @@ -120,4 +78,4 @@ export default connect(
) );
},
} )
)( clickOutside( BlockSwitcher ) );
)( BlockSwitcher );
Loading