fix(sqllab): make MenuDotsDropdown trigger focusable for tab rename - #43015
Conversation
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 #e1ee0c
Actionable Suggestions - 1
-
superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx - 1
- Missing tabIndex=-1 on dropdown · Line 163-163
Additional Suggestions - 1
-
superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx - 1
-
Focus target changed to dropdown · Line 140-141Focus return now targets `dropdownTriggerRef` (the '...' dropdown) instead of `tabHeaderRef` (the tab title wrapper). The comment was updated to match, but verify this matches the intended UX — the user opened the modal by clicking the dropdown menu option, so returning focus to the dropdown trigger may feel appropriate.
-
Review Details
-
Files reviewed - 4 · Commit Range:
beb8643..beb8643- superset-frontend/packages/superset-ui-core/src/components/Dropdown/Dropdown.test.tsx
- superset-frontend/packages/superset-ui-core/src/components/Dropdown/index.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTabHeader/SqlEditorTabHeader.test.tsx
- superset-frontend/src/SqlLab/components/SqlEditorTabHeader/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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43015 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 2866 2866
Lines 162798 162799 +1
Branches 37507 37507
=======================================
+ Hits 108546 108547 +1
Misses 52130 52130
Partials 2122 2122
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:
|
There was a problem hiding this comment.
Pull request overview
Improves SQL Lab tab-rename focus management by making the MenuDotsDropdown trigger focusable and ref-addressable, so focus can be returned to the exact trigger that opened the rename modal (instead of the broader tab header wrapper).
Changes:
- Forward a ref through
MenuDotsDropdownand make its trigger focusable. - Update
SqlEditorTabHeaderto focus the menu-dots trigger after rename modal Save/Cancel. - Update/add unit tests to assert focus returns to the dropdown trigger.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset-frontend/src/SqlLab/components/SqlEditorTabHeader/SqlEditorTabHeader.test.tsx | Updates focus assertions to expect focus on the menu-dots trigger after modal close. |
| superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx | Replaces tab-header ref focusing with a MenuDotsDropdown trigger ref for more precise focus restoration. |
| superset-frontend/packages/superset-ui-core/src/components/Dropdown/index.tsx | Makes MenuDotsDropdown trigger focusable and forwards a ref to it. |
| superset-frontend/packages/superset-ui-core/src/components/Dropdown/Dropdown.test.tsx | Adds coverage for trigger focusability and ref forwarding behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #c7a439Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
LGTM. Switching the trigger to a native is the right focusability fix (Enter/Space activation, role=button, no nested-button since antd clones onto the single child). Minor: the PR description says the trigger is made focusable via tabIndex={0}, but the diff actually uses a native button (the better approach) so no tabIndex is needed — worth updating the wording.
sha174n
added
the
merge-if-green
|
@sha174n good catch, updated the description to match the diff. |
…ocus return Follow-up to #41329, which returned focus to the whole SQL Lab tab header wrapper after closing the rename-tab modal (the author explicitly deferred making the menu trigger itself focusable). The "..." trigger is now a focusable, ref-forwarding element, so focus returns to the specific "Rename tab" menu trigger instead of the wrapper around it. Co-Authored-By: Claude <noreply@anthropic.com>
… access Renders the trigger as a native <button> instead of a div with role="button", so it gets keyboard activation (Enter/Space) for free and satisfies the jsx-a11y prefer-tag-over-role rule; also adds an aria-label so screen readers announce it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…real button userEvent.keyboard() doesn't exist in the pinned @testing-library/user-event v12, so the new Dropdown keyboard test never ran; swap in fireEvent to simulate a native button's Enter-to-click activation. Also update TabbedSqlEditors' removeQueryEditor test, which picked the tab's first <button> by position and started grabbing the new "..." actions trigger instead of the actual remove button. Co-Authored-By: Claude <noreply@anthropic.com>
The real callers (SqlEditorTabHeader, GridTable HeaderMenu) always pass
trigger={['click']}; antd's Dropdown default trigger is hover, so the
test's fireEvent.click never opened the menu without it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
341906a to
0cc2b0c
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| <MenuDotsWrapper | ||
| ref={ref} | ||
| type="button" | ||
| aria-label={t('Actions')} | ||
| data-test="dropdown-trigger" | ||
| > | ||
| {RenderIcon(iconOrientation)} | ||
| </MenuDotsWrapper> |
There was a problem hiding this comment.
Suggestion: The disabled prop remains on AntdDropdown but is not forwarded to the new native trigger. Consequently, callers such as ActionCell that pass disabled still render a focusable and apparently clickable button, whereas the previous non-focusable div did not add a keyboard tab stop. Forward the disabled state to MenuDotsWrapper so disabled dropdown actions cannot receive focus or appear interactive. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Empty `ActionCell` menus leave inaccessible disabled controls focusable.
- ⚠️ Keyboard users encounter an action trigger with no available actions.
- ⚠️ Disabled state is visually and semantically incomplete.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/Dropdown/index.tsx
**Line:** 96:103
**Comment:**
*Api Mismatch: The `disabled` prop remains on `AntdDropdown` but is not forwarded to the new native trigger. Consequently, callers such as `ActionCell` that pass `disabled` still render a focusable and apparently clickable button, whereas the previous non-focusable `div` did not add a keyboard tab stop. Forward the disabled state to `MenuDotsWrapper` so disabled dropdown actions cannot receive focus or appear interactive.
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
|
Bito Automatic Review Skipped – PR Already Merged |
Follow-up to #41329.
SUMMARY
#41329 replaced SQL Lab's native
prompt()tab rename with aModal, and returns keyboard focus to the tab header after the modal closes. The author explicitly deferred making the "..." (MenuDotsDropdown) trigger itself focusable to a follow-up, so focus landed on the whole tab header wrapper instead of the specific "Rename tab" menu trigger that opened the modal.This makes
MenuDotsDropdown's trigger a native<button>(instead of adivwithrole="button") and forwards a ref to it, soSqlEditorTabHeadercan return focus to that trigger specifically, both after Save and after Cancel/close. A native button gets keyboard focusability and Enter/Space activation for free, and gets anaria-labelfor screen readers.TESTING INSTRUCTIONS
⋮menu and choose "Rename tab".⋮trigger for that tab, not the tab header as a whole.Covered by updated/added unit tests in
SqlEditorTabHeader.test.tsxandDropdown.test.tsx.ADDITIONAL INFORMATION
🤖 Generated with Claude Code