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

Remove shadow buttons are unlabeled and don't show a tooltip #63196

Closed
afercia opened this issue Jul 5, 2024 · 1 comment · Fixed by #63197
Closed

Remove shadow buttons are unlabeled and don't show a tooltip #63196

afercia opened this issue Jul 5, 2024 · 1 comment · Fixed by #63197
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Components /packages/components [Package] Edit Site /packages/edit-site [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 5, 2024

Description

The 'Remove shadow' icon buttons in the Global Styles are unlabeled.

More accurately, propr to the button are passed via an object, where an ariaLabel and a tooltip keys are set.
However, the Button component either accepts a label prop or an aria-label attribute.
Not sure why the tooltip prop passed this way doesn't work btut, regardless, it's unnecessary if the label prop is set correctly.

The rendered markup is invalid as the key names are rendered as HTML attributes:

  • the button element has an arialabel attribute, it should be aria-label
  • the button element has an tooltip attribute

Screenshot of the button markup in the dev tools inspector and the accessibility tab selected to show the button doesn't have an accessible name:

Screenshot 2024-07-05 at 16 07 26

I'm not sure why those props are passed via an object in the first place, instead of being passed individually to the Button component. As far as I can tell there's no need to set a variable for that object in the first place.

I have to note this is one more occurrence of an unlabeled control in the editor. There have been dozens of cases because contributors to this project keep 'doing it wrong' and the base component are open to misuesd.

As pointed out in several other issues and conversations, unlabeled controls are not acceptable. The only way to make sure this doesn't happen again is by refactoring the base components and make the label prop required. See #51740

Cc @WordPress/gutenberg-components

Step-by-step reproduction instructions

  • Go to the Site Editor > Styles > Shadows > click one of the default shadows > click the plus icon button 'Add shadow'.
  • A new shadow item will appear in the list below, with a minus icon button. Screenshot:

Screenshot 2024-07-05 at 16 05 19

  • Hover or focus the minus icon button.
  • Observe no tooltip appears.
  • Inspect the source and observe the button has an arialabel attribute instead of aria-label.
  • Observe there's also a tooltip attribute.

Screenshots, screen recording, code snippet

No response

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

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Edit Site /packages/edit-site labels Jul 5, 2024
@afercia afercia self-assigned this Jul 5, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jul 5, 2024
@mirka
Copy link
Member

mirka commented Jul 5, 2024

This kind of error is unfortunate because an attempt was clearly made to be accessible, and it's something that could've easily been caught if TypeScript were enabled in this section of the codebase. It seems unlikely that the Gutenberg app is going to transition to TypeScript anytime soon, so type checking is never going to be a useful strategy for upholding accessibility within Gutenberg app code 😓

I'm starting to seriously consider experimenting with runtime checks. It could be feasible if it's well-targeted, and limited to dev mode within the Gutenberg app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Components /packages/components [Package] Edit Site /packages/edit-site [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants