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

ToolbarButton: aria-disabled prop is also given the disabled attribute at the same time #57701

Closed
t-hamano opened this issue Jan 10, 2024 · 3 comments · Fixed by #63102
Closed
Assignees
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@t-hamano
Copy link
Contributor

Description

For Button component, the aria-disabled prop is used instead of the disabled prop to make the button inactive but still focusable.

For example, the Undo/Redo button in the Post Editor or Site Editor.

image

<Button
{ ...props }
ref={ ref }
icon={ ! isRTL() ? undoIcon : redoIcon }
/* translators: button label text should, if possible, be under 16 characters. */
label={ __( 'Undo' ) }
shortcut={ displayShortcut.primary( 'z' ) }
// If there are no undo levels we don't want to actually disable this
// button, because it will remove focus for keyboard users.
// See: https://github.com/WordPress/gutenberg/issues/3486
aria-disabled={ ! hasUndo }
onClick={ hasUndo ? undo : undefined }
className="editor-history__undo"
/>

On the other hand, ToolbarButton is used for the Undo/Redo button in the Widget Editor.

<ToolbarButton
icon={ ! isRTL() ? undoIcon : redoIcon }
label={ __( 'Undo' ) }
shortcut={ displayShortcut.primary( 'z' ) }
// If there are no undo levels we don't want to actually disable this
// button, because it will remove focus for keyboard users.
// See: https://github.com/WordPress/gutenberg/issues/3486
aria-disabled={ ! hasUndo }
onClick={ hasUndo ? undo : undefined }
/>

It's mentioned that the ToolbarButton component accepts the same API as the Button component, but it seems like the aria-disabled prop also opts into the disabled prop at the same time.

toolbar-button

As a result, as mentioned in #57647 and #57678, it is not possible to focus the Undo/Redo button in the Widget Editor when it is not enabled.

This can be solved by refactoring the Undo/Redo button into the Button component, as attempted in #57677, but if the current behavior of ToobarButton is not what we intended, I think it will need to be fixed.

Step-by-step reproduction instructions

  • Activate classic theme (Twenty Twenty-One).
  • Open the Wdiget Editor.
  • Use the tab key or arrow keys to move the focus within the header.
  • You should not be able to focus on the Undo/Redo button.

Screenshots, screen recording, code snippet

Note: In the screencast below, you can also see that the focus outline is cut off, but this issue should also be fixed separately.

46eeb77c2538939e5282514bff9b1edf.mp4

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Jan 10, 2024
@jeryj
Copy link
Contributor

jeryj commented Feb 24, 2024

Would the accessibleWhenDisabled prop on <ToolbarButton /> be useful here?

@t-hamano
Copy link
Contributor Author

@jeryj Thanks for the reply.

I didn't know about that prop, but maybe it's something that's being attempted in this discussion? If accessibleWhenDisabled can solve the problem, this issue may be closed.

@jeryj
Copy link
Contributor

jeryj commented Feb 27, 2024

I did end up using accessibleWhenDisabled in the comment you linked to and it seems to be working. I haven't looked into this issue, but hopefully it helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
3 participants