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

Implement roving tabindex on the header toolbar #22354

Merged
merged 9 commits into from
Jun 8, 2020
Merged

Implement roving tabindex on the header toolbar #22354

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fdbba33
Implement roving tabindex on the top toolbar
diegohaz May 14, 2020
8ca8540
Update header toolbar styles
diegohaz May 26, 2020
2cd9f8c
Refactor block navigation dropdown code
diegohaz May 26, 2020
f4aabc9
Refactor header toolbar code
diegohaz May 26, 2020
ffa948b
Update inserter is-pressed styles
diegohaz May 26, 2020
9ae1761
Make tool selector unavailable on the header toolbar component
diegohaz May 26, 2020
d18eeaf
Merge branch 'master' into add/top-toolbar-roving-tabindex
diegohaz May 26, 2020
a759470
Merge branch 'master' into add/top-toolbar-roving-tabindex
diegohaz Jun 4, 2020
47a9bcb
Support as prop on ToolbarItem
diegohaz Jun 4, 2020
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
35 changes: 25 additions & 10 deletions packages/block-editor/src/components/block-navigation/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Button, Dropdown, SVG, Path } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import { useShortcut } from '@wordpress/keyboard-shortcuts';
import { useCallback } from '@wordpress/element';
import { useCallback, forwardRef } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -23,7 +23,13 @@ const MenuIcon = (
</SVG>
);

function BlockNavigationDropdownToggle( { isEnabled, onToggle, isOpen } ) {
function BlockNavigationDropdownToggle( {
isEnabled,
onToggle,
isOpen,
innerRef,
...props
} ) {
useShortcut(
'core/edit-post/toggle-block-navigation',
useCallback( onToggle, [ onToggle ] ),
Expand All @@ -42,6 +48,8 @@ function BlockNavigationDropdownToggle( { isEnabled, onToggle, isOpen } ) {

return (
<Button
{ ...props }
ref={ innerRef }
icon={ MenuIcon }
aria-expanded={ isOpen }
onClick={ isEnabled ? onToggle : undefined }
Expand All @@ -53,11 +61,15 @@ function BlockNavigationDropdownToggle( { isEnabled, onToggle, isOpen } ) {
);
}

function BlockNavigationDropdown( {
isDisabled,
__experimentalWithBlockNavigationSlots,
__experimentalWithEllipsisMenu,
} ) {
function BlockNavigationDropdown(
{
isDisabled,
__experimentalWithBlockNavigationSlots,
__experimentalWithEllipsisMenu,
...props
},
ref
) {
const hasBlocks = useSelect(
( select ) => !! select( 'core/block-editor' ).getBlockCount(),
[]
Expand All @@ -68,9 +80,12 @@ function BlockNavigationDropdown( {
<Dropdown
contentClassName="block-editor-block-navigation__popover"
position="bottom right"
renderToggle={ ( toggleProps ) => (
renderToggle={ ( { isOpen, onToggle } ) => (
<BlockNavigationDropdownToggle
{ ...toggleProps }
{ ...props }
innerRef={ ref }
isOpen={ isOpen }
onToggle={ onToggle }
isEnabled={ isEnabled }
/>
) }
Expand All @@ -89,4 +104,4 @@ function BlockNavigationDropdown( {
);
}

export default BlockNavigationDropdown;
export default forwardRef( BlockNavigationDropdown );
12 changes: 5 additions & 7 deletions packages/block-editor/src/components/tool-selector/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useSelect, useDispatch } from '@wordpress/data';
import { useViewportMatch } from '@wordpress/compose';
import { forwardRef } from '@wordpress/element';

const editIcon = (
<SVG
Expand All @@ -34,16 +34,12 @@ const selectIcon = (
</SVG>
);

function ToolSelector() {
function ToolSelector( props, ref ) {
const isNavigationTool = useSelect(
( select ) => select( 'core/block-editor' ).isNavigationMode(),
[]
);
const { setNavigationMode } = useDispatch( 'core/block-editor' );
const isMediumViewport = useViewportMatch( 'medium' );
if ( ! isMediumViewport ) {
return null;
}

const onSwitchMode = ( mode ) => {
setNavigationMode( mode === 'edit' ? false : true );
Expand All @@ -53,6 +49,8 @@ function ToolSelector() {
<Dropdown
renderToggle={ ( { isOpen, onToggle } ) => (
<Button
{ ...props }
ref={ ref }
icon={ isNavigationTool ? selectIcon : editIcon }
aria-expanded={ isOpen }
onClick={ onToggle }
Expand Down Expand Up @@ -99,4 +97,4 @@ function ToolSelector() {
);
}

export default ToolSelector;
export default forwardRef( ToolSelector );
64 changes: 47 additions & 17 deletions packages/edit-post/src/components/header/header-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ import {
EditorHistoryRedo,
EditorHistoryUndo,
} from '@wordpress/editor';
import { Button } from '@wordpress/components';
import {
Button,
__experimentalToolbarItem as ToolbarItem,
} from '@wordpress/components';
import { plus } from '@wordpress/icons';

function HeaderToolbar( { onToggleInserter, isInserterOpen } ) {
Expand Down Expand Up @@ -65,23 +68,50 @@ function HeaderToolbar( { onToggleInserter, isInserterOpen } ) {
className="edit-post-header-toolbar"
aria-label={ toolbarAriaLabel }
>
<Button
className="edit-post-header-toolbar__inserter-toggle"
isPrimary
isPressed={ isInserterOpen }
onClick={ onToggleInserter }
disabled={ ! isInserterEnabled }
icon={ plus }
label={ _x(
'Add block',
'Generic label for block inserter button'
<ToolbarItem>
{ ( itemProps ) => (
<Button
{ ...itemProps }
className="edit-post-header-toolbar__inserter-toggle"
isPrimary
isPressed={ isInserterOpen }
onClick={ onToggleInserter }
disabled={ ! isInserterEnabled }
icon={ plus }
label={ _x(
'Add block',
'Generic label for block inserter button'
) }
/>
) }
</ToolbarItem>
{ isLargeViewport && (
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 flagging this for myself to test what happens with focus when you change viewport of your browser and this button disappears when focused. I know it's an edge case and it might be an issue before but I'm simply curious :)

Copy link
Member

Choose a reason for hiding this comment

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

I tested it. When one of the blocks were selected before, then focus moves to the selected block 🙃

Copy link
Member

Choose a reason for hiding this comment

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

I was wrong, the blinking cursor tricked me, so the focus gets removed, and when you tab it gets restored to one of the items in the toolbar, not that bad I guess. Could Reakit move focus automatically to the next/previous item in such case or would be too far in what it should do? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

In the first iterations, Reakit's Composite (and therefore Toolbar) automatically moved focus to the next/previous item when the current focused item got unmounted. Later it was changed so the previously focused item would get focus, like when you open and close a new tab on Chrome. But I haven't found enough material to support any of those approaches, and this was making the code even more complex for little gain, so I reverted it.

For now, toolbar.currentId is still set to the previously focused item, but it doesn't automatically move focus. It's up to the user to decide what to do, but I do plan to revisit it in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I would expect it’s going to be tricky to handle 😅 A similar challenge is when you click a button and it should become disabled, like when triggering undo until there is nothing to undo.

In this particular case, I assume it’s fine to clean up internally Reakit’s state - that is the current value and leave the rest (focus) of handling to the implementer.

<ToolbarItem>
{ ( itemProps ) => <ToolSelector { ...itemProps } /> }
</ToolbarItem>
) }
<ToolbarItem>
{ ( itemProps ) => <EditorHistoryUndo { ...itemProps } /> }
</ToolbarItem>
diegohaz marked this conversation as resolved.
Show resolved Hide resolved
<ToolbarItem>
{ ( itemProps ) => <EditorHistoryRedo { ...itemProps } /> }
</ToolbarItem>
<ToolbarItem>
{ ( itemProps ) => (
<TableOfContents
hasOutlineItemsDisabled={ isTextModeEnabled }
{ ...itemProps }
/>
) }
</ToolbarItem>
<ToolbarItem>
{ ( itemProps ) => (
<BlockNavigationDropdown
isDisabled={ isTextModeEnabled }
diegohaz marked this conversation as resolved.
Show resolved Hide resolved
{ ...itemProps }
/>
) }
/>
<ToolSelector />
<EditorHistoryUndo />
<EditorHistoryRedo />
<TableOfContents hasOutlineItemsDisabled={ isTextModeEnabled } />
<BlockNavigationDropdown isDisabled={ isTextModeEnabled } />
</ToolbarItem>
{ displayBlockToolbar && (
<div className="edit-post-header-toolbar__block-toolbar">
<BlockToolbar hideDragHandle />
Expand Down
25 changes: 24 additions & 1 deletion packages/edit-post/src/components/header/header-toolbar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.edit-post-header-toolbar {
display: inline-flex;
align-items: center;
border: none;

// Hide all action buttons except the inserter on mobile.
> .components-button {
Expand All @@ -23,6 +24,28 @@
display: flex;
}
}

// The Toolbar component adds different styles to buttons, so we reset them
// here to the original button styles
> .components-button.has-icon,
> .components-dropdown > .components-button.has-icon {
height: $button-size;
min-width: $button-size;
padding: 6px;

&.is-pressed {
background: $dark-gray-primary;
}

&:focus:not(:disabled) {
box-shadow: 0 0 0 $border-width-focus $theme-color;
outline: 1px solid transparent;
}

&::before {
display: none;
}
}
}

// Block toolbar when fixed to the top of the screen.
Expand Down Expand Up @@ -82,7 +105,7 @@
}
}

.edit-post-header-toolbar .edit-post-header-toolbar__inserter-toggle {
.edit-post-header-toolbar .edit-post-header-toolbar__inserter-toggle.has-icon {
margin-right: $grid-unit-10;
// Special dimensions for this button.
min-width: 32px;
Expand Down
11 changes: 9 additions & 2 deletions packages/editor/src/components/editor-history/redo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ import { withSelect, withDispatch } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { displayShortcut } from '@wordpress/keycodes';
import { redo as redoIcon } from '@wordpress/icons';
import { forwardRef } from '@wordpress/element';

function EditorHistoryRedo( { hasRedo, redo } ) {
function EditorHistoryRedo( { hasRedo, redo, innerRef, ...props } ) {
return (
<Button
{ ...props }
ref={ innerRef }
icon={ redoIcon }
label={ __( 'Redo' ) }
shortcut={ displayShortcut.primaryShift( 'z' ) }
Expand All @@ -24,11 +27,15 @@ function EditorHistoryRedo( { hasRedo, redo } ) {
);
}

export default compose( [
const EnhancedEditorHistoryRedo = compose( [
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered refactoring this and other similar components (EditorHistoryUndo or TableOfContents) to use hooks? I think it would remove the need to use innerRef and you could apply ref directly on the component.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't played with @wordpress/data hooks yet, but that's a good idea! Will take a note on that for a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing👍

withSelect( ( select ) => ( {
hasRedo: select( 'core/editor' ).hasEditorRedo(),
} ) ),
withDispatch( ( dispatch ) => ( {
redo: dispatch( 'core/editor' ).redo,
} ) ),
] )( EditorHistoryRedo );

export default forwardRef( ( props, ref ) => (
<EnhancedEditorHistoryRedo { ...props } innerRef={ ref } />
) );
11 changes: 9 additions & 2 deletions packages/editor/src/components/editor-history/undo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ import { withSelect, withDispatch } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { displayShortcut } from '@wordpress/keycodes';
import { undo as undoIcon } from '@wordpress/icons';
import { forwardRef } from '@wordpress/element';

function EditorHistoryUndo( { hasUndo, undo } ) {
function EditorHistoryUndo( { hasUndo, undo, innerRef, ...props } ) {
return (
<Button
{ ...props }
ref={ innerRef }
icon={ undoIcon }
label={ __( 'Undo' ) }
shortcut={ displayShortcut.primary( 'z' ) }
Expand All @@ -24,11 +27,15 @@ function EditorHistoryUndo( { hasUndo, undo } ) {
);
}

export default compose( [
const EnhancedEditorHistoryUndo = compose( [
withSelect( ( select ) => ( {
hasUndo: select( 'core/editor' ).hasEditorUndo(),
} ) ),
withDispatch( ( dispatch ) => ( {
undo: dispatch( 'core/editor' ).undo,
} ) ),
] )( EditorHistoryUndo );

export default forwardRef( ( props, ref ) => (
<EnhancedEditorHistoryUndo { ...props } innerRef={ ref } />
) );
16 changes: 14 additions & 2 deletions packages/editor/src/components/table-of-contents/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,28 @@ import { __ } from '@wordpress/i18n';
import { Dropdown, Button } from '@wordpress/components';
import { withSelect } from '@wordpress/data';
import { info } from '@wordpress/icons';
import { forwardRef } from '@wordpress/element';

/**
* Internal dependencies
*/
import TableOfContentsPanel from './panel';

function TableOfContents( { hasBlocks, hasOutlineItemsDisabled } ) {
function TableOfContents( {
hasBlocks,
hasOutlineItemsDisabled,
innerRef,
...props
} ) {
return (
<Dropdown
position="bottom"
className="table-of-contents"
contentClassName="table-of-contents__popover"
renderToggle={ ( { isOpen, onToggle } ) => (
<Button
{ ...props }
ref={ innerRef }
onClick={ hasBlocks ? onToggle : undefined }
icon={ info }
aria-expanded={ isOpen }
Expand All @@ -37,8 +45,12 @@ function TableOfContents( { hasBlocks, hasOutlineItemsDisabled } ) {
);
}

export default withSelect( ( select ) => {
const TableOfContentsWithSelect = withSelect( ( select ) => {
return {
hasBlocks: !! select( 'core/block-editor' ).getBlockCount(),
};
} )( TableOfContents );

export default forwardRef( ( props, ref ) => (
<TableOfContentsWithSelect { ...props } innerRef={ ref } />
) );