A11y: Introduce ClickableElement #7977
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7977 +/- ##
==========================================
+ Coverage 73.05% 73.06% +0.01%
==========================================
Files 1299 1300 +1
Lines 40372 40376 +4
Branches 7506 7505 -1
==========================================
+ Hits 29492 29500 +8
+ Misses 10880 10876 -4 ☔ View full report in Codecov by Sentry. |
fregante
changed the title
| > = ({ children, ...props }) => ( | ||
| <div | ||
| {...props} | ||
| tabIndex={0} |
There was a problem hiding this comment.
why specify the tabIndex specifically as 0? Won't this mess up the tab order on the page?
There was a problem hiding this comment.
I would set tabIndex={0} before spreading props, so we can choose to override it otherwise.
There was a problem hiding this comment.
Right, 0 value basically will let the dom auto-order these focusable elements.
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex
There was a problem hiding this comment.
Good idea in theory, but in practice we should never set tabIndex > 0 https://webaim.org/techniques/keyboard/tabindex#tabindex
| * - https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/docs/rules/mouse-events-have-key-events.md | ||
| * | ||
| */ | ||
| const ClickableElement: React.FC< |
There was a problem hiding this comment.
We should also probably include some default styling for cursor: pointer
There was a problem hiding this comment.
I think it might be best to address that globally, for all role=button, via CSS. I can open a followup PR that takes care of all existing definitions for that
src/components/ClickableElement.tsx
Outdated
| props.onClick(event); | ||
| } | ||
| }} | ||
| // TODO: also handle onMouseOver -> onFocus |
There was a problem hiding this comment.
What custom logic were you thinking for these events?
There was a problem hiding this comment.
It's part of one of the a11y rules linked above. However in reality we should just avoid such events and use CSS instead
There was a problem hiding this comment.
Gotcha. It wasn't clear from this TODO comment what it was referring to.
It seems like this could be addressed with some TS discriminated unions but I'm also not sure we even want to always enforce this rule. For example, a popular pattern for form inputs is to autosave onBlur, but not onMouseLeave.
| const handleClick = jest.fn(); | ||
| render(<ClickableElement onClick={handleClick} />); | ||
| const clickableElement = screen.getByRole("button"); | ||
| fireEvent.keyPress(clickableElement, { |
There was a problem hiding this comment.
NIT: why not use userEvent.keyboard?
There was a problem hiding this comment.
This was copiloted. I can change it if you prefer
What does this PR do?
This component and lint rule essentially replaces most instances of:
This also documents the usage of clickable
divelements and ensures consistent/safe usage.Checklist