fix(a11y): make CopyToClipboard keyboard-operable for custom copyNode elements#41958
fix(a11y): make CopyToClipboard keyboard-operable for custom copyNode elements#41958eschutho wants to merge 1 commit into
Conversation
… elements WCAG 2.1 SC 2.1.1 — Level A.
dosubot
Bot
added
change:frontend
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #d350b4
Actionable Suggestions - 1
-
superset-frontend/src/components/CopyToClipboard/index.tsx - 1
- Duplicate keyboard handler logic · Line 89-96
Review Details
-
Files reviewed - 2 · Commit Range:
69876ec..69876ec- superset-frontend/src/components/CopyToClipboard/CopyToClipboard.test.tsx
- superset-frontend/src/components/CopyToClipboard/index.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| onKeyDown: disabled | ||
| ? undefined | ||
| : (event: KeyboardEvent<HTMLElement>) => { | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); | ||
| onClick(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The onKeyDown handler (lines 89-96) duplicates the exact same logic as handleKeyDown (lines 101-109). Both handle Enter/Space keys identically but diverge on future changes. Extract into a shared helper function to maintain consistency.
Code Review Run #d350b4
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| onKeyDown: disabled | ||
| ? undefined | ||
| : (event: KeyboardEvent<HTMLElement>) => { | ||
| if (event.key === 'Enter' || event.key === ' ') { | ||
| event.preventDefault(); | ||
| onClick(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Suggestion: The new keyboard handler triggers onClick() for every custom element, including native interactive controls like button that already fire click on Enter/Space. This causes duplicate copy executions (double toast / double callback) for existing call sites that pass button-based copyNodes. Only synthesize keyboard activation for non-native interactive elements (or when role-based button behavior is being polyfilled), and let native controls keep their default keyboard-to-click behavior. [incorrect condition logic]
Severity Level: Major ⚠️
❌ SQL Lab result copy fires twice on keyboard activation.
⚠️ Duplicate LOG_ACTIONS_SQLLAB_COPY_RESULT_TO_CLIPBOARD entries logged.
⚠️ Keyboard users see double success toasts for copy actions.Steps of Reproduction ✅
1. In SQL Lab, the `ResultSet` component at
`superset-frontend/src/SqlLab/components/ResultSet/index.tsx` lines ~448–476 renders a
`CopyToClipboard` instance whose `copyNode` is a `Button` from
`@superset-ui/core/components` (see `copyNode={<Button ... />}` and `onCopyEnd={() =>
logAction(LOG_ACTIONS_SQLLAB_COPY_RESULT_TO_CLIPBOARD, {})}` in that file).
2. The `CopyToClipboard` implementation at
`superset-frontend/src/components/CopyToClipboard/index.tsx` uses `cloneElement` in
`getDecoratedCopyNode()` (around lines 79–99) to decorate any element passed via
`copyNode`, adding `onClick: disabled ? undefined : onClick` (line 88) and the new
`onKeyDown` handler (lines 89–96) that calls `event.preventDefault(); onClick();` when
`event.key` is `'Enter'` or `' '`.
3. Run a query in SQL Lab so that `ResultSet` renders, then use keyboard Tab navigation in
the browser to focus the "Copy to Clipboard" button produced by the `Button` passed as
`copyNode` in `ResultSet/index.tsx` (this is the element being cloned and decorated by
`CopyToClipboard`).
4. Press Space or Enter while that button is focused: the key press first dispatches a
`keydown` event, invoking the injected `onKeyDown` and calling `onClick()` once; on keyup,
native button semantics synthesize a `click` event, which then triggers the same `onClick`
prop a second time. Because `onClick()` calls `copyToClipboard()` and `copyToClipboard()`
runs `onCopyEnd` in its `finally` block (`CopyToClipboard/index.tsx` lines 46–61), the
copy-to-clipboard success toast and
`logAction(LOG_ACTIONS_SQLLAB_COPY_RESULT_TO_CLIPBOARD, {})` both execute twice for a
single keyboard activation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/CopyToClipboard/index.tsx
**Line:** 89:96
**Comment:**
*Incorrect Condition Logic: The new keyboard handler triggers `onClick()` for every custom element, including native interactive controls like `button` that already fire click on Enter/Space. This causes duplicate copy executions (double toast / double callback) for existing call sites that pass button-based `copyNode`s. Only synthesize keyboard activation for non-native interactive elements (or when role-based button behavior is being polyfilled), and let native controls keep their default keyboard-to-click behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41958 +/- ##
=======================================
Coverage 64.80% 64.81%
=======================================
Files 2740 2740
Lines 153135 153140 +5
Branches 35129 35132 +3
=======================================
+ Hits 99246 99251 +5
Misses 51991 51991
Partials 1898 1898
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
WCAG rule
WCAG 2.1 SC 2.1.1 (Keyboard) — Level A.
What changed
CopyToClipboard'sgetDecoratedCopyNode()has two rendering branches. When a caller passes a customcopyNodeelement (thecloneElementbranch), the component wired uponClickbut never added anonKeyDownhandler, and lefttabIndexas whatever the caller happened to pass (oftenundefined). The result: any customcopyNode— even one that already hasrole="button"andaria-labelset by the caller, e.g.explore/components/EmbedCodeContent.tsx's copy-to-clipboard icon — was not keyboard-focusable or keyboard-operable, despite visually/semantically presenting as an accessible button.The sibling default-
<span>branch a few lines below already implements this correctly (tabIndex={0}+ an Enter/SpaceonKeyDownhandler). This PR extends the same behavior to thecloneElementbranch:onKeyDownhandler (Enter/Space triggers the sameonClick, withpreventDefault()).tabIndexto0when the caller didn't specify one (node.props.tabIndex ?? 0), while still respectingdisabled(-1) and any caller-specified value.This is a single shared-component fix, so it closes the gap for every call site that passes a custom
copyNode(EmbedCodeContent.tsx,URLShortLinkButton/index.tsx,TablePreview/index.tsx, etc.) without touching each call site individually.Also added a test (
CopyToClipboard.test.tsx) covering keyboard activation and defaulttabIndexfor thecloneElementbranch specifically — the existing keyboard test only covered the non-element/default-span branch.Behavior unchanged
No visual or click-behavior change. Mouse/touch click behavior is identical; this only adds keyboard support that was previously missing.
Test plan
CopyToClipboard.test.tsx— 13/13 tests pass, including the new keyboard test for thecloneElementbranch.copyNode(e.g. the "Copy to clipboard" icon in the Explore "Embed code" popover) using only the keyboard, and verify a screen reader announces its accessible name (e.g. "Copy to clipboard, button"), then press Enter or Space and verify the copy action fires.