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

FlexItem should not allow onClick prop when it renders a non-interactive element #42461

Closed
afercia opened this issue Jul 15, 2022 · 4 comments · Fixed by #42668
Closed

FlexItem should not allow onClick prop when it renders a non-interactive element #42461

afercia opened this issue Jul 15, 2022 · 4 comments · Fixed by #42668
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 15, 2022

Description

The FlexItem component renders a <div> element by default. It can render other elements via the as prop.

When it renders an element that is not operable with a keyboard, it should not allow an onClick prop. Maybe, it should never allow an onClick prop.

An example of incorrect implementation is in AddCustomTemplateModal:

Screenshot 2022-07-15 at 12 12 39

The two choices at the bottom of the modal are just clickable <div> elements.

The can't be operated with a keyboard.

We should never, ever, use clickable divs or other non-interactive clickable elements.

This specific implementation in AddCustomTemplateModal can be probably fixed by using the as prop to render the flex item as a Button component and then adjust the style. Also, the buttons don't need to contain a heading (the H5 is a wrong heading level anyways).

However, a more future-proof measure would be to not allow an onClick prop.

Step-by-step reproduction instructions

  • go to the site editor and open the Templates browser
  • click Add New at the top right
  • select 'Category' or 'Tag' to open the modal
  • inspect the source and observe the two choices at the bottom of the modal are clickable divs
  • observe you can't move focus to them by using the keyboard

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 [a11y] Keyboard & Focus labels Jul 15, 2022
@t-hamano
Copy link
Contributor

I looked into FlexItem and could not find any other component that has the onClick property and renders as a div element.
Therefore, the accessibility issues occurring in FlexItem should only need to be corrected in the AddCustomTemplateModal component.
(However, this is not a future measure, but a fix only for problems that are currently occurring...)

And one suggestion, how about applying the UI of "Create template part" to AddCustomTemplateModal as well?
The UI for template parts has clickable elements.

"Create a template part" UI

template-part

"Add template" UI (proposal)

template

@talldan
Copy link
Contributor

talldan commented Jul 18, 2022

It might also be an option to add a linting rule for as="div" onClick={ ... }, similar to the one that's shown for <div onClick={ ... }, although they can be quite complicated to write:
Screen Shot 2022-07-18 at 3 28 24 pm

@afercia
Copy link
Contributor Author

afercia commented Jul 18, 2022

That would be a good start, as a div element is probably the most common case 🙂 However, not sure how to cover all the other cases.

@mirka
Copy link
Member

mirka commented Jul 19, 2022

Thanks for raising this issue, it's not good that we basically have zero accessibility linting coverage on these types of components. I do generally think this type of problem should be addressed with linters, rather than blocking event handlers at the code level (mostly for scalability reasons, since there are a lot of as prop components and many conditions for things to cause accessibility issues). I split out the linting part of the problem to #42540.

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). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants