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

Jump to bottom

Accessibility improvements for Block Inserter #37357

Merged
merged 17 commits into from
Jan 7, 2022
Merged

Accessibility improvements for Block Inserter #37357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
38868d5
Add Close button to Block Inserter. Adjust the useFocusOnMount hook t…
alexstine Dec 14, 2021
83fb9e0
Make tests stable.
alexstine Dec 14, 2021
ab9bd8d
Remove CSS and use the viewport hook to toggle between visual button …
alexstine Dec 14, 2021
5637346
Fix tests.
alexstine Dec 14, 2021
cc51c95
Toggle the tag type and save some code room.
alexstine Dec 14, 2021
1dbdad0
Focus block inserter search form.
alexstine Dec 14, 2021
7975ec1
Pass the open state to useEffect.
alexstine Dec 14, 2021
76962cb
Use ref forwarding.
alexstine Dec 16, 2021
701f930
Merge branch 'trunk' of https://github.com/wordpress/gutenberg into t…
alexstine Dec 16, 2021
1afc148
Try to fix tests.
alexstine Dec 16, 2021
9764c59
Revert package-lock.json changes.
alexstine Dec 16, 2021
0d9fdf2
Fix tests.
alexstine Dec 17, 2021
2a97df2
Fix tests.
alexstine Dec 17, 2021
64f0b8f
Fix inserter input focus
youknowriad Dec 17, 2021
b975b40
Apply changes to edit-site/edit-widgets.
alexstine Dec 20, 2021
c4715a1
Formatting fixes.
alexstine Dec 20, 2021
ea055f1
Fix: Unable to close block inserter in Widgets view.
alexstine Dec 20, 2021
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
23 changes: 19 additions & 4 deletions packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
* WordPress dependencies
*/
import { useDispatch, useSelect } from '@wordpress/data';
import { Button } from '@wordpress/components';
import { Button, VisuallyHidden } from '@wordpress/components';
import { __experimentalLibrary as Library } from '@wordpress/block-editor';
import { close } from '@wordpress/icons';
import {
useViewportMatch,
__experimentalUseDialog as useDialog,
} from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import { useEffect, useRef } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -28,23 +30,36 @@ export default function InserterSidebar() {
const { setIsInserterOpened } = useDispatch( editPostStore );

const isMobileViewport = useViewportMatch( 'medium', '<' );
const TagName = ! isMobileViewport ? VisuallyHidden : 'div';
const [ inserterDialogRef, inserterDialogProps ] = useDialog( {
onClose: () => setIsInserterOpened( false ),
focusOnMount: null,
Copy link
Contributor Author

@alexstine alexstine Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Had to set this to null to prevent the useFocusOnMount hook from firing. Now that useEffect is being used, it would cause duplicate focus events.

} );

const inserterContentRef = useRef();
useEffect( () => {
inserterContentRef.current
.querySelector( '.block-editor-inserter__search input' )
.focus();
}, [] );

return (
<div
ref={ inserterDialogRef }
{ ...inserterDialogProps }
className="edit-post-editor__inserter-panel"
>
<div className="edit-post-editor__inserter-panel-header">
<TagName className="edit-post-editor__inserter-panel-header">
<Button
icon={ close }
label={ __( 'Close block inserter' ) }
onClick={ () => setIsInserterOpened( false ) }
/>
</div>
<div className="edit-post-editor__inserter-panel-content">
</TagName>
<div
className="edit-post-editor__inserter-panel-content"
ref={ inserterContentRef }
>
<Library
showMostUsedBlocks={ showMostUsedBlocks }
showInserterHelpPanel
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-post/src/components/secondary-sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@
padding-right: $grid-unit-10;
display: flex;
justify-content: flex-end;

@include break-medium() {
display: none;
}
}

.edit-post-editor__inserter-panel-content,
Expand Down
23 changes: 19 additions & 4 deletions packages/edit-site/src/components/secondary-sidebar/inserter-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
import { Button } from '@wordpress/components';
import { Button, VisuallyHidden } from '@wordpress/components';
import { __experimentalLibrary as Library } from '@wordpress/block-editor';
import { close } from '@wordpress/icons';
import {
useViewportMatch,
__experimentalUseDialog as useDialog,
} from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import { useEffect, useRef } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -23,23 +25,36 @@ export default function InserterSidebar() {
);

const isMobile = useViewportMatch( 'medium', '<' );
const TagName = ! isMobile ? VisuallyHidden : 'div';
const [ inserterDialogRef, inserterDialogProps ] = useDialog( {
onClose: () => setIsInserterOpened( false ),
focusOnMount: null,
} );

const inserterContentRef = useRef();
useEffect( () => {
inserterContentRef.current
.querySelector( '.block-editor-inserter__search input' )
Copy link
Member

Choose a reason for hiding this comment

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

Could we use pass a searchInputRef to <Library> and all the way down to the actual <input> element here? So that we can omit the query selector, which feels like an implementation detail.

Or we can use useImperativeHandle to allow calling focusSearchInput() on the <Library> ref.

If all of the above can't be done, then I'd prefer using data-* attribute over class names for explicitness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevin940726 #37357 (comment)

I think @youknowriad wanted to avoid doing anything with the block-editor/inserter component because it could be used as a single element. E.g. keep all the implementation in the sidebar files and out of what could be a reusable stand-alone component.

I actually used forwardRef() all the way to the Search component a few commits back.

Copy link
Member

Choose a reason for hiding this comment

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

Yep I noticed that, but I think my approach is different because it's not using forwardRef to bind the ref to the inserter instance, but passing a different prop searchInputRef down to the search input element. useImperativeHandle can also work with forwardRef so that the ref is referencing the inserter instance but also providing focusSearchInput() functionality as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kevin940726 I think a ref with an added property is a sane approach indeed. I'm fine either way but we should be mindful of added complexity.

.focus();
}, [] );

return (
<div
ref={ inserterDialogRef }
{ ...inserterDialogProps }
className="edit-site-editor__inserter-panel"
>
<div className="edit-site-editor__inserter-panel-header">
<TagName className="edit-post-editor__inserter-panel-header">
<Button
icon={ close }
label={ __( 'Close block inserter' ) }
onClick={ () => setIsInserterOpened( false ) }
/>
</div>
<div className="edit-site-editor__inserter-panel-content">
</TagName>
<div
className="edit-post-editor__inserter-panel-content"
ref={ inserterContentRef }
>
<Library
showInserterHelpPanel
shouldFocusBlock={ isMobile }
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-site/src/components/secondary-sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@
padding-right: $grid-unit-10;
display: flex;
justify-content: flex-end;

@include break-medium() {
display: none;
}
}

.edit-site-editor__inserter-panel-content,
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-widgets/src/components/layout/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@
padding-right: $grid-unit-10;
display: flex;
justify-content: flex-end;

@include break-medium() {
display: none;
}
}

.edit-widgets-layout__inserter-panel-content {
Expand Down
31 changes: 24 additions & 7 deletions packages/edit-widgets/src/components/secondary-sidebar/inserter-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
/**
* WordPress dependencies
*/
import { Button } from '@wordpress/components';
import { Button, VisuallyHidden } from '@wordpress/components';
import { close } from '@wordpress/icons';
import { __experimentalLibrary as Library } from '@wordpress/block-editor';
import {
useViewportMatch,
__experimentalUseDialog as useDialog,
} from '@wordpress/compose';
import { useCallback } from '@wordpress/element';
import { useCallback, useEffect, useRef } from '@wordpress/element';
import { useDispatch } from '@wordpress/data';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
Expand All @@ -24,23 +25,39 @@ export default function InserterSidebar() {
const { setIsInserterOpened } = useDispatch( editWidgetsStore );

const closeInserter = useCallback( () => {
return () => setIsInserterOpened( false );
return setIsInserterOpened( false );
Copy link
Contributor

Choose a reason for hiding this comment

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

That is indeed a good fix.

}, [ setIsInserterOpened ] );

const TagName = ! isMobileViewport ? VisuallyHidden : 'div';
const [ inserterDialogRef, inserterDialogProps ] = useDialog( {
onClose: closeInserter,
focusOnMount: null,
} );

const inserterContentRef = useRef();
useEffect( () => {
inserterContentRef.current
.querySelector( '.block-editor-inserter__search input' )
.focus();
}, [] );

return (
<div
ref={ inserterDialogRef }
{ ...inserterDialogProps }
className="edit-widgets-layout__inserter-panel"
>
<div className="edit-widgets-layout__inserter-panel-header">
<Button icon={ close } onClick={ closeInserter } />
</div>
<div className="edit-widgets-layout__inserter-panel-content">
<TagName className="edit-widgets-layout__inserter-panel-header">
<Button
icon={ close }
onClick={ closeInserter }
label={ __( 'Close block inserter' ) }
/>
</TagName>
<div
className="edit-widgets-layout__inserter-panel-content"
ref={ inserterContentRef }
>
<Library
showInserterHelpPanel
shouldFocusBlock={ isMobileViewport }
Expand Down