Accessibility improvements for Block Inserter #37357

alexstine · 2021-12-14T09:11:55Z

Description

This PR adds a Close button to the Block Inserter that displays all the time for screen readers only. This PR also makes an adjustment to useDialog() call passing focusOnMount: null to prevent duplicate focus on Inserter open.

Bug fix: This PR will fix a bug I noticed on the Widgets screen where Close button on Inserter wasn't working at all. Small tweak to useCallback() call fixed this up.

How has this been tested?

Login to WP Admin.
Go to Posts and open an existing post or create a new one.
Select the Toggle block inserter button.
Notice how there is a Close block inserter button but it is only visible to screen readers and shows up regardless of the viewport.
Also notice how focus is still placed in the search input.

Screenshots

Types of changes

Bug fix

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

…o be able to focus second element.

packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js

…and visually hidden button.

packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js

youknowriad · 2021-12-14T12:58:07Z

packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js

@@ -30,6 +31,7 @@ export default function InserterSidebar() {
 const isMobileViewport = useViewportMatch( 'medium', '<' );
 const [ inserterDialogRef, inserterDialogProps ] = useDialog( {
 onClose: () => setIsInserterOpened( false ),
+ focusOnMount: 'secondElement',


Is this still needed? It feels very specific to have a "secondElement" option in focusOnMount. The reasoning is that the developer might change what's inside a dialog without touching this call at all (two separate concerns), so basically "secondElement" is meaningless.

In other words, if we really want to target a specific element, we should probably use a dedicate useEffect(() => { ref.focus() } ) instead of trying to include this logic into a reusable hook.

I made this specifically for use in Core because I think it has other good uses. It also had a check to ensure the element existed before focus was attempted. Probably could have done a better job to work on a fallback.

However, I changed to the useEffect approach but this was tricky because I had to figure out a way to only fire useEffect on initial open. It seems useEffect fires every time the component mounts and focus kept happening when I tried to tab out of the search box.

Do you have any suggestions on if this is good enough? Should I try to access edit post store from menu.js directly or is passing a new property fine?

Thanks.

Do you have any suggestions on if this is good enough? Should I try to access edit post store from menu.js directly or is passing a new property fine?

Mmm interesting challenge, I guess neither of these are ideal to me. The Menu shouldn't be aware of an outside isOpen because it's not something that is always rendered in a toggable sidebar and block-editor shouldn't have any dependency on edit-post (cycle dependency).

We should try to find an alternative.

Can I ask why we want to focus the "second element" in the first place and not the "first element" like all dialogs?

Can I ask why we want to focus the "second element" in the first place and not the "first element" like all dialogs?

Because in the case of the Block Inserter, it is better to focus the search input first as most users will likely search for their block instead of looking through the list. I know that's how I personally use it. If we were to focus the first element, it would focus the Close button which would create extra key strokes to interact with the search field and the Block Inserter content.

What if I added my changes back to the useFocusOnMount hook with additional checks to make sure the element passed in is valid or default to the first? I understand most devs probably won't use this, but there's a need for it in the Core Gutenberg editor. As you said, passing the state probably isn't a good idea as the Inserter is not always used in a sidebar. Although, I did have one question about that. If the Inserter wasn't used in a sidebar, wouldn't that search field get focus by default since it uses useFocusOnMount hook or would it simply focus the first element on the page? I would think it would focus the first element in the Inserter since the useDialog hook. This would also be super bad UX if the Inserter was just on the page, not in a sidebar, and focus got placed there first. Although, maybe it wouldn't work this way because the Inserter doesn't use those hooks, the sidebar in the editor does.

Personally, I think passing that extra parameter is fine that way we don't need access to core/edit-post store and the variable is set to false, so it's not like it is going to effect other code. Users can still use the useFocusOnMount hook as I've only disabled it in the sidebar managed by Gutenberg. Nothing really changes for the Inserter component itself.

Sorry, this was long winded. Thanks.

The focus target for opening a dialog is variable depending on usage. For simple or informational dialogs (e.g., with only one or two focal points), placing focus at the beginning will be best (and ideally that should be the close button). For complex dialogs, however, the usage of the dialog really needs to be taken into consideration. Focusing on the task that needs to be performed using a dialog makes sense. It's still good to place focus as early in the modal as possible, but making it easier to complete the task is a good thing.

@youknowriad Sadly, your solution did not work. 😞 When I tried to have useEffect in InserterSidebar with a ref on the parent div, it never worked even when passing isOpen as an argument. Whenever I used console.log, useEffect was firing too early for the focus in the search field. I tried to delay useEffect with a timeout before the focus, but never could get that right.

I started reading about children functional components receiving refs and decided to use forwardRef all the way to the search component. This seems highly inefficient though. 😞

I could use the useEffect inside the search component but it would still require some type of state tracking or else it would never know when the inserter is open. I wish there was a way to unify the elements so it wasn't all separate.

What are your thoughts about the forwardRef approach? It seems bad practice, but it does work and to my knowledge, it could almost be seen as an improvement since now anyone could have control over the search part of the Inserter.

If not this way, can you think of any other ways? I was thinking maybe this could be refactored in to one component, but that kind of defeats the purpose of React. BTW, when I mentioned console.log above, what I meant was in useEffect, console.log fired before my console.log of the Inserter when it was fully loaded. I tried a timeout of 1000 and it wasn't enough.

Thanks for giving this a try. I appreciate the efforts.

The forwardRef approach is good practice when the forwarded ref represents the actual component, in the case of the inserter library, I'd expect the forwarded ref to be the inserter container, not just a specific element of that component (the input).

I'll take a look at the PR later today and see if I can find something that would work well.

I've pushed a fix to your branch that I think works. Let me know what you think. I also noticed that you had inverted the condition about when to show the close button (should be hidden on desktop).

Last thing, this InserterSidebar component is something that is also replicated in three other screens (site editor, widgets screen and navigation screen) so three other packages. I think we might want to make the same changes there as well.

@youknowriad Can you check it out now? I also fixed a bug where the Inserter would not close in Widgets view. I adjusted the code in useCallback just a bit, seems to work now.

BTW, this is the same approach I took except I didn't add the "input" selector. 😕 One tiny selector away and it would have worked for me. Thanks for the catch on this and ensuring I didn't create unnecessary ref forwarding to get this done.

alexstine · 2021-12-14T19:47:19Z

packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js

 const [ inserterDialogRef, inserterDialogProps ] = useDialog( {
 onClose: () => setIsInserterOpened( false ),
+ focusOnMount: null,


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.

…ry/accessibility-fixes-for-inserter

kevin940726 · 2021-12-20T05:02:58Z

packages/edit-site/src/components/secondary-sidebar/inserter-sidebar.js

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


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.

@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.

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.

@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.

youknowriad · 2021-12-20T08:25:10Z

packages/edit-widgets/src/components/secondary-sidebar/inserter-sidebar.js

@@ -25,7 +25,7 @@ export default function InserterSidebar() {
 const { setIsInserterOpened } = useDispatch( editWidgetsStore );

 const closeInserter = useCallback( () => {
- return () => setIsInserterOpened( false );
+ return setIsInserterOpened( false );


That is indeed a good fix.

youknowriad

This is looking good to me.

alexstine · 2021-12-20T16:34:48Z

@youknowriad @kevin940726 Do you think this is good as is or would you like me to work on a different approach with passing the ref to Search component? Also happy to add a:

data-inserter-focus="true"

If it would help with correct targeting for focus.

Thanks.

youknowriad

LGTM 👍

kevin940726 · 2022-01-07T06:15:54Z

@youknowriad @kevin940726 Do you think this is good as is or would you like me to work on a different approach with passing the ref to Search component? Also happy to add a:
data-inserter-focus="true"
If it would help with correct targeting for focus.

Thanks.

Preferably, I'd love to have a ref as the API so that it's more consistent to other React APIs. But we can do this in another PR :).

data-inserter-focus="true" seems like a good alternative in the meantime though 👍.

alexstine · 2022-01-07T12:56:33Z

@kevin940726 I will work on one over the weekend and see if I can pass the ref. I still am of the opinion that it's a lot of work for little reward, but maybe there's just something I don't see. Usually I'd be all for it but I think it's important to think of use context here. When I started this PR, I had no idea the library component could be used stand-alone so having a ref attached to it wouldn't actually help that much.

Let me give it a test over the weekend though and see what happens.

Add Close button to Block Inserter. Adjust the useFocusOnMount hook t…

38868d5

…o be able to focus second element.

alexstine requested a review from ajitbohra as a code owner December 14, 2021 09:11

alexstine requested review from youknowriad, talldan, noisysocks, tellthemachines and aristath December 14, 2021 09:13

alexstine self-assigned this Dec 14, 2021

youknowriad reviewed Dec 14, 2021

View reviewed changes

packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js Outdated Show resolved Hide resolved

alexstine added 3 commits December 14, 2021 05:28

Make tests stable.

83fb9e0

Remove CSS and use the viewport hook to toggle between visual button …

ab9bd8d

…and visually hidden button.

Fix tests.

5637346

youknowriad reviewed Dec 14, 2021

View reviewed changes

packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js Outdated Show resolved Hide resolved

youknowriad reviewed Dec 14, 2021

View reviewed changes

alexstine added 2 commits December 14, 2021 13:36

Toggle the tag type and save some code room.

cc51c95

Focus block inserter search form.

1dbdad0

alexstine requested a review from ellatrix as a code owner December 14, 2021 19:13

Pass the open state to useEffect.

7975ec1

alexstine commented Dec 14, 2021

View reviewed changes

alexstine added 5 commits December 16, 2021 17:31

Use ref forwarding.

76962cb

Merge branch 'trunk' of https://github.com/wordpress/gutenberg into t…

701f930

…ry/accessibility-fixes-for-inserter

Try to fix tests.

1afc148

Revert package-lock.json changes.

9764c59

Fix tests.

0d9fdf2

alexstine and others added 4 commits December 16, 2021 19:26

Fix tests.

2a97df2

Fix inserter input focus

64f0b8f

Apply changes to edit-site/edit-widgets.

b975b40

Formatting fixes.

c4715a1

alexstine requested review from adamziel, draganescu and kevin940726 as code owners December 20, 2021 02:39

Fix: Unable to close block inserter in Widgets view.

ea055f1

kevin940726 reviewed Dec 20, 2021

View reviewed changes

youknowriad reviewed Dec 20, 2021

View reviewed changes

youknowriad approved these changes Jan 6, 2022

View reviewed changes

youknowriad merged commit c9425ae into WordPress:trunk Jan 7, 2022

github-actions bot added this to the Gutenberg 12.4 milestone Jan 7, 2022

alexstine mentioned this pull request Jan 7, 2022

Try possibly better method for Block Inserter Search focus #37793

Merged

8 tasks

femkreations mentioned this pull request Apr 19, 2022

[HelpHub] Adding a new block WordPress/Documentation-Issue-Tracker#308

Closed

9 tasks

alexstine mentioned this pull request Oct 26, 2022

[Experiment - Inserter]: Try search input inside each tab #45203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility improvements for Block Inserter #37357

Accessibility improvements for Block Inserter #37357

alexstine commented Dec 14, 2021 •

edited

Loading

youknowriad Dec 14, 2021

alexstine Dec 14, 2021

youknowriad Dec 15, 2021

alexstine Dec 15, 2021

joedolson Dec 15, 2021

alexstine Dec 16, 2021

youknowriad Dec 17, 2021

youknowriad Dec 17, 2021

alexstine Dec 20, 2021

alexstine Dec 20, 2021

alexstine Dec 14, 2021 •

edited

Loading

kevin940726 Dec 20, 2021

alexstine Dec 20, 2021

kevin940726 Dec 20, 2021

youknowriad Dec 20, 2021

youknowriad Dec 20, 2021

youknowriad left a comment

alexstine commented Dec 20, 2021

youknowriad left a comment

kevin940726 commented Jan 7, 2022

alexstine commented Jan 7, 2022

Accessibility improvements for Block Inserter #37357

Accessibility improvements for Block Inserter #37357

Conversation

alexstine commented Dec 14, 2021 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexstine Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

alexstine commented Dec 20, 2021

youknowriad left a comment

Choose a reason for hiding this comment

kevin940726 commented Jan 7, 2022

alexstine commented Jan 7, 2022

alexstine commented Dec 14, 2021 •

edited

Loading

alexstine Dec 14, 2021 •

edited

Loading