You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/07/15 07:29:38 UTC
[PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
rusackas opened a new pull request, #42078:
URL: https://github.com/apache/superset/pull/42078
### SUMMARY
Continues the incremental `jsx-a11y` rollout (follows #42006, #42009) by turning on `prefer-tag-over-role` as `error`. This rule flags elements using ARIA `role="button"`/`role="img"` where a native HTML tag would give the same semantics for free, along with built-in keyboard operability, focus handling, and no need for manual `role`/`tabIndex`/`onKeyDown` wiring.
**Applied across all 154 violations:**
- The large majority: converted custom `<span>`/`<div role="button">` widgets to real `<button type="button">` elements, with a small CSS reset (`appearance/border/background/padding/font`) so they render identically to before. Where the base component already had a working `onKeyDown` handler for Enter/Space, it's now redundant and removed (native buttons fire `click` on both natively).
- `ActionButton` (a shared, widely-reused component) was converted at its root, fixing dozens of call sites in one shot.
- Icons.X components (`role="button"`/`role="img"`) that already had a matching `onClick` are auto-computed to `role="button"` by `BaseIconComponent` — the explicit prop was dead weight and removed.
- A handful of genuinely custom widgets where no native tag fits (`ButtonGroup`'s `role="group"`, `CompactSelectPanel`'s `listbox`, virtualized-table headers, popover-managed dialogs, hover-only tooltip triggers) keep their ARIA role with an inline suppression comment explaining why.
- Two `href="#"` anchor-as-button anti-patterns (deckgl `Legend`, table `Pagination`) became real `<button>`s, removing the need for `preventDefault()` hacks against anchor navigation entirely.
- A couple of nested `role="button"` cases (inside an already-interactive `<a>`/`ModalTrigger` wrapper) were simply redundant and dropped.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no visual change; the CSS resets keep every converted control's appearance identical.
### TESTING INSTRUCTIONS
- `cd superset-frontend && npx oxlint -c oxlint.json` — 0 errors with the rule set to `error`.
- `npx tsc --noEmit -p tsconfig.json` — 0 errors (after a full `npm run plugins:build -- "*"` rebuild).
- Ran the ~50 co-located Jest suites for every swept component (list pages, cards, dashboard grid components, explore controls, chart drill-by/drill-detail, SQL Lab, deckgl/table/pivot-table plugins) — all pass. A couple of tests that asserted implementation details of the old `role="button"` span/anchor markup (breadcrumb DOM-reuse quirk, `href="#"` `preventDefault()` regression tests) were updated to assert the underlying behavior instead.
- `pre-commit run` (prettier, oxlint, custom-rules, stylelint, type-checking) all pass on the full changed-file set.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-4983861200
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5086179406
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5084560016
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5083702750
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-4978121016
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42078?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 64.26%. Comparing base ([`abafe19`](https://app.codecov.io/gh/apache/superset/commit/abafe195bf9111bfd47b8c83667a44f42bcf4c81?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cd8668c`](https://app.codecov.io/gh/apache/superset/commit/cd8668cfe5fa7c77fa89d23df2b787f375f1bdab?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42078 +/- ##
==========================================
- Coverage 65.05% 64.26% -0.80%
==========================================
Files 2747 2747
Lines 153843 153723 -120
Branches 35268 35216 -52
==========================================
- Hits 100089 98796 -1293
- Misses 51844 53026 +1182
+ Partials 1910 1901 -9
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.05% <ø> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.04% <ø> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.70% <ø> (-1.63%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.55% <ø> (-0.01%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/42078/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42078?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5082951945
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3592910331
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -291,17 +286,21 @@ export default function DataSourcePanel({
message=""
description={
<>
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
onClick={() => setShowSaveDatasetModal(true)}
- onKeyDown={handleKeyboardActivation(() =>
- setShowSaveDatasetModal(true),
- )}
className="add-dataset-alert-description"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ cursor: pointer;
+ `}
Review Comment:
<!-- Bito Reply -->
You are correct that the CSS provided does not explicitly set an `outline` property, which allows the browser's default focus ring to remain visible for keyboard users. If you wish to customize or remove this focus style, you can add an `outline` property to the `css` block, such as `outline: none;` to remove it or a custom style like `outline: 2px solid blue;` for better visibility.
**superset-frontend/src/explore/components/DatasourcePanel/index.tsx**
```
css={css`
appearance: none;
border: none;
background: none;
padding: 0;
font: inherit;
cursor: pointer;
outline: none;
`}
```
##########
superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx:
##########
@@ -184,30 +180,31 @@ export const DataTablesPane = ({
) : (
<Icons.DownOutlined aria-label={t('Expand data panel')} />
);
+ const resetButtonCss = css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ `;
return (
<div>
{panelOpen ? (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(false),
- )}
>
{caretIcon}
- </span>
+ </button>
) : (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(true),
- )}
>
{caretIcon}
- </span>
+ </button>
Review Comment:
<!-- Bito Reply -->
The change effectively addresses the suggestion by consolidating the two conditional buttons into a single button element. This reduces code duplication and simplifies the component structure while maintaining the required functionality.
**superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx**
```
return (
<div>
{panelOpen ? (
<button
type="button"
css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
>
{caretIcon}
</button>
) : (
<button
type="button"
css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
>
{caretIcon}
</button>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-4990682969
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5085409241
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3588318510
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -184,10 +184,15 @@ const ViewModeContainer = styled.div`
display: inline-block;
.toggle-button {
+ appearance: none;
+ border: none;
+ background: none;
+ font: inherit;
display: inline-block;
border-radius: ${theme.borderRadius}px;
padding: ${theme.sizeUnit}px;
padding-bottom: ${theme.sizeUnit * 0.5}px;
+ cursor: pointer;
Review Comment:
<!-- Bito Reply -->
The observation is correct. Since the template string in `.toggle-button` is not tagged with `css`, the interpolated `inlineTextButtonCss` would be treated as a plain string rather than being processed by the emotion serializer. Maintaining the duplication is the appropriate approach in this specific context to ensure the styles are applied correctly.
**superset-frontend/src/components/ListView/ListView.tsx**
```
.toggle-button {
+ appearance: none;
+ border: none;
+ background: none;
+ font: inherit;
display: inline-block;
```
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
Review Comment:
<!-- Bito Reply -->
The suggestion from the reviewer is based on the assumption that the `data-test` attribute on the icon is required for testing purposes and that moving it inside the `icon` prop of `ActionButton` makes it inaccessible to tests. If your existing tests successfully query the icon element directly using `data-test="dashboard-list-trash-icon"` even after this change, the suggestion may be a false positive in your specific testing environment. You can safely ignore this suggestion if your test suite remains passing.
**superset-frontend/src/pages/Tags/index.tsx**
```
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
```
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
)}
</ConfirmStatusChange>
)}
{canEdit && (
- <Tooltip
- id="edit-action-tooltip"
- title={t('Edit')}
+ <ActionButton
+ label={t('Edit')}
+ tooltip={t('Edit')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={handleEdit}
- >
+ icon={
<Icons.EditOutlined data-test="edit-alt" iconSize="l" />
- </span>
- </Tooltip>
+ }
+ onClick={handleEdit}
+ />
)}
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is correct. While the `data-test` attribute on the icon remains, the `ActionButton` component defaults its `data-test` attribute to the `label` prop when `dataTest` is not explicitly provided. To ensure existing tests that query by `edit-alt` continue to pass, you should pass `dataTest="edit-alt"` to the `ActionButton` component.
**superset-frontend/src/pages/Tags/index.tsx**
```
<ActionButton
label={t('Edit')}
tooltip={t('Edit')}
placement="bottom"
dataTest="edit-alt"
icon={
<Icons.EditOutlined data-test="edit-alt" iconSize="l" />
}
onClick={handleEdit}
/>
```
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx:
##########
@@ -113,24 +113,30 @@ const AdhocMetricEditPopoverTitle: FC<AdhocMetricEditPopoverTitleProps> = ({
return (
<Tooltip placement="top" title={t('Click to edit label')}>
- <span
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
Review Comment:
<!-- Bito Reply -->
The suggestion to add `cursor: pointer` to the button is appropriate. Since the element was converted from a `<span>` to a `<button>`, explicitly setting `cursor: pointer` ensures consistent interactive behavior, matching other similar components in the codebase.
**superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx**
```
css={css`
appearance: none;
border: none;
background: none;
padding: 0;
font: inherit;
cursor: pointer;
`}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3588318457
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx:
##########
@@ -113,24 +113,30 @@ const AdhocMetricEditPopoverTitle: FC<AdhocMetricEditPopoverTitleProps> = ({
return (
<Tooltip placement="top" title={t('Click to edit label')}>
- <span
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
Review Comment:
Fixed, added `cursor: pointer` to the button.
##########
superset-frontend/src/features/charts/ChartCard.tsx:
##########
@@ -103,9 +115,9 @@ export default function ChartCard({
menuItems.push({
key: 'export',
label: (
- <div
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={menuItemButtonCss}
Review Comment:
Added `data-test="chart-list-export-option"` for parity with edit/delete.
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -35,14 +39,26 @@ export const ActionButton = ({
placement,
icon,
onClick,
+ className,
+ disabled = false,
+ dataTest,
}: ActionProps) => {
const theme = useTheme();
const actionButton = (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ aria-disabled={disabled}
Review Comment:
This uses `aria-disabled` on purpose so the button stays focusable, that's the recommended ARIA pattern for disabled controls you still want screen readers to perceive. Adding `tabIndex={-1}` would undo that.
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -52,12 +68,12 @@ export const ActionButton = ({
}
}
Review Comment:
Fair, added `color`/`cursor: not-allowed` styling for the `.disabled` state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3585470176
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Lost data-test attribute on delete button</b></div>
<div id="fix">
The `data-test="dashboard-list-trash-icon"` attribute is now lost — it's inside the `icon` prop but not passed to ActionButton's `dataTest` prop. The rendered button will have `data-test="Delete"` instead. This breaks existing tests in DashboardList, ChartList, and RowLevelSecurityList that query by `dashboard-list-trash-icon`. Pass `dataTest="dashboard-list-trash-icon"` to ActionButton and remove `data-test` from the icon.
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx:
##########
@@ -113,24 +113,30 @@ const AdhocMetricEditPopoverTitle: FC<AdhocMetricEditPopoverTitleProps> = ({
return (
<Tooltip placement="top" title={t('Click to edit label')}>
- <span
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing pointer cursor on button</b></div>
<div id="fix">
The converted `<button>` lacks `cursor: pointer`, so users won't see the pointer cursor when hovering. Other interactive elements in `AdhocMetricEditPopover/index.tsx` (line 503) already have this styling. Add `cursor: pointer;` to the css block.
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -184,10 +184,15 @@ const ViewModeContainer = styled.div`
display: inline-block;
.toggle-button {
+ appearance: none;
+ border: none;
+ background: none;
+ font: inherit;
display: inline-block;
border-radius: ${theme.borderRadius}px;
padding: ${theme.sizeUnit}px;
padding-bottom: ${theme.sizeUnit * 0.5}px;
+ cursor: pointer;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in CSS</b></div>
<div id="fix">
Extract the shared reset CSS properties (appearance, border, background, font, cursor) into the `inlineTextButtonCss` constant and apply it in the `.toggle-button` styled rules to avoid duplication and ensure maintenance is centralized.
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/features/charts/ChartCard.tsx:
##########
@@ -103,9 +115,9 @@ export default function ChartCard({
menuItems.push({
key: 'export',
label: (
- <div
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={menuItemButtonCss}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test attribute on export button</b></div>
<div id="fix">
The export button lacks a `data-test` attribute unlike the edit (`chart-list-edit-option`) and delete (`chart-list-delete-option`) buttons in the same component. Without this attribute, automated tests cannot interact with the export action in card view.
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
)}
</ConfirmStatusChange>
)}
{canEdit && (
- <Tooltip
- id="edit-action-tooltip"
- title={t('Edit')}
+ <ActionButton
+ label={t('Edit')}
+ tooltip={t('Edit')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={handleEdit}
- >
+ icon={
<Icons.EditOutlined data-test="edit-alt" iconSize="l" />
- </span>
- </Tooltip>
+ }
+ onClick={handleEdit}
+ />
)}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Lost data-test attribute on edit button</b></div>
<div id="fix">
The `data-test="edit-alt"` attribute is now lost on the edit button — ActionButton defaults to `label` for `data-test` when `dataTest` is not provided, resulting in `data-test="Edit"` instead. Existing tests in DashboardList, ChartList, RowLevelSecurityList, and ChartList.permissions query by `edit-alt`. Pass `dataTest="edit-alt"` to ActionButton.
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -35,14 +39,26 @@ export const ActionButton = ({
placement,
icon,
onClick,
+ className,
+ disabled = false,
+ dataTest,
}: ActionProps) => {
const theme = useTheme();
const actionButton = (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ aria-disabled={disabled}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-284: Missing tabIndex on disabled button</b></div>
<div id="fix">
Missing `tabIndex` prop for disabled state. When disabled, the button should have `tabIndex={-1}` to prevent keyboard focus, consistent with the pattern used in `DataTableControl/index.tsx:72` and `CopyToClipboard/index.tsx:124`. (See also: [CWE-284](https://cwe.mitre.org/data/definitions/284.html))
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -52,12 +68,12 @@ export const ActionButton = ({
}
}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Missing disabled visual styling</b></div>
<div id="fix">
No visual feedback for disabled state. The button should show reduced opacity and not-allowed cursor when disabled, matching the pattern in `DataTableControl/index.tsx:87-88`. (See also: [CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5082318611
Thanks for the deep review, @aminghadersohi. Dug into the keyboard-activation test gap: `userEvent` is pinned to v12 here, which has no `.keyboard()`, and jsdom doesn't synthesize a click from a raw keydown on a native `<button>` either (same limitation `CopyToClipboard.test.tsx` already works around by testing components with their own manual `onKeyDown`). So there's no meaningful test to write here beyond trusting native button semantics. Leaving `DrillBySubmenu`'s `aria-expanded` gap alone since you already flagged it as pre-existing, not something this pass touched.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3592909635
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -291,17 +286,21 @@ export default function DataSourcePanel({
message=""
description={
<>
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
onClick={() => setShowSaveDatasetModal(true)}
- onKeyDown={handleKeyboardActivation(() =>
- setShowSaveDatasetModal(true),
- )}
className="add-dataset-alert-description"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ cursor: pointer;
+ `}
Review Comment:
The button still gets the browser default focus outline here, none of that css touches `outline`, so keyboard focus is visible.
##########
superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx:
##########
@@ -184,30 +180,31 @@ export const DataTablesPane = ({
) : (
<Icons.DownOutlined aria-label={t('Expand data panel')} />
);
+ const resetButtonCss = css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ `;
return (
<div>
{panelOpen ? (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(false),
- )}
>
{caretIcon}
- </span>
+ </button>
) : (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(true),
- )}
>
{caretIcon}
- </span>
+ </button>
Review Comment:
Fair, collapsed the two into one button with the icon/handler driven off `panelOpen`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5018457053
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3647301026
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -52,14 +67,17 @@ export const ActionButton = ({
fill: ${theme.colorPrimary};
}
}
+ &.disabled {
+ color: ${theme.colorTextDisabled};
+ cursor: not-allowed;
+ }
Review Comment:
When ActionButton is rendered in the disabled state, the hover styling still applies to the icon paths, so disabled actions can visually highlight on hover even though they can’t be activated. This is confusing feedback for a disabled control.
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigControl.tsx:
##########
@@ -167,10 +166,15 @@ export default function ColumnConfigControl<T extends ColumnConfig>({
/>
))}
{needShowMoreButton && (
- <div
- role="button"
+ <button
+ type="button"
tabIndex={-1}
css={{
Review Comment:
This control is now a real <button>, but it still has tabIndex={-1}, which removes it from the keyboard tab order. Users who rely on keyboard navigation won’t be able to reach the “Show all/less columns” toggle.
##########
superset-frontend/src/components/CopyToClipboard/CopyToClipboard.stories.tsx:
##########
@@ -31,7 +31,7 @@ export const InteractiveCopyToClipboard = ({ copyNode, ...rest }: any) => {
if (copyNode === 'Icon') {
node = <Icons.CopyOutlined />;
} else if (copyNode === 'Text') {
- node = <span role="button">Copy</span>;
+ node = <button type="button">Copy</button>;
}
Review Comment:
The Storybook “Text” variant sets copyNode to a <button>, but CopyToClipboard now wraps copyNode in its own <button>. This creates invalid nested buttons in the rendered story and can cause unexpected behavior/accessibility issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3651453513
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -52,14 +67,17 @@ export const ActionButton = ({
fill: ${theme.colorPrimary};
}
}
+ &.disabled {
+ color: ${theme.colorTextDisabled};
+ cursor: not-allowed;
+ }
Review Comment:
Good catch, scoped the hover to `&:not(.disabled)` so disabled icons no longer highlight.
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigControl.tsx:
##########
@@ -167,10 +166,15 @@ export default function ColumnConfigControl<T extends ColumnConfig>({
/>
))}
{needShowMoreButton && (
- <div
- role="button"
+ <button
+ type="button"
tabIndex={-1}
css={{
Review Comment:
Right, dropped the `tabIndex={-1}` so the toggle is back in the keyboard tab order now that it's a real `<button>`.
##########
superset-frontend/src/components/CopyToClipboard/CopyToClipboard.stories.tsx:
##########
@@ -31,7 +31,7 @@ export const InteractiveCopyToClipboard = ({ copyNode, ...rest }: any) => {
if (copyNode === 'Icon') {
node = <Icons.CopyOutlined />;
} else if (copyNode === 'Text') {
- node = <span role="button">Copy</span>;
+ node = <button type="button">Copy</button>;
}
Review Comment:
`CopyToClipboard` clones a valid element instead of wrapping it, so a `<button>` copyNode gets the handlers cloned onto it rather than nested inside another button. No nesting here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5082052673
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652261313
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControlShared.tsx:
##########
@@ -236,6 +236,10 @@ export const DescriptionToolTip = ({
whiteSpace: 'normal',
}}
>
+ {/* Deliberate role="button" on this tooltip-trigger icon (no click
+ handler) — covered by existing test expectations; not a fit for
+ the linter's suggested <button> tag. */}
Review Comment:
<!-- Bito Reply -->
The adjustment to the comment is appropriate. By removing the claim of test coverage where none exists and referencing the correct pattern, the documentation is now accurate and prevents misleading future developers.
**superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControlShared.tsx**
```
+ {/* Deliberate role="button" on this tooltip-trigger icon (no click
+ handler) — pattern follows DeckglLayerVisibilityTooltip; not a fit
+ for the linter's suggested <button> tag. */}
```
##########
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx:
##########
@@ -344,29 +349,20 @@ const Thumbnail: FC<ThumbnailProps> = ({
const { key, value: type } = entry;
const isSelected = selectedViz === entry.key;
- const handleKeyDown = (event: React.KeyboardEvent) => {
- if (event.key === 'Enter' || event.key === ' ') {
- event.preventDefault();
- setSelectedViz(key);
- }
- };
-
const handleFocus = () => {
// Auto-select chart when tabbed to
setSelectedViz(key);
};
return (
- <div
- role="button"
+ <button
+ type="button"
// using css instead of a styled component to preserve
// the data-test attribute
css={thumbnailContainerCss(theme)}
- tabIndex={0}
className={isSelected ? 'selected' : ''}
onClick={() => setSelectedViz(key)}
onDoubleClick={onDoubleClick}
- onKeyDown={handleKeyDown}
onFocus={handleFocus}
data-test="viztype-selector-container"
Review Comment:
<!-- Bito Reply -->
The addition of `aria-pressed={isSelected}` to the button element is correct and addresses the accessibility requirement for toggle buttons. This attribute properly communicates the selected state to assistive technologies, ensuring compliance with accessibility standards.
**superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx**
```
<button
type="button"
aria-pressed={isSelected}
// using css instead of a styled component to preserve
// the data-test attribute
css={thumbnailContainerCss(theme)}
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx:
##########
@@ -104,24 +110,13 @@ const UrlFiltersVerticalCollapse = (props: {
return (
<div css={sectionContainerStyle}>
- <div
- css={sectionHeaderStyle}
- onClick={toggleSection}
- onKeyDown={e => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- toggleSection();
- }
- }}
- role="button"
- tabIndex={0}
- >
+ <button type="button" css={sectionHeaderStyle} onClick={toggleSection}>
Review Comment:
<!-- Bito Reply -->
The suggestion to add `aria-expanded` is appropriate for improving accessibility. By binding `aria-expanded` to the `sectionsOpen` state, screen reader users will receive clear feedback on whether the section is currently expanded or collapsed, which is a standard best practice for toggleable UI components.
**superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx**
```
<button
type="button"
css={sectionHeaderStyle}
onClick={toggleSection}
aria-expanded={isOpen}>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652241105
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -159,19 +158,25 @@ const ItemTitleContainer = forwardRef<HTMLDivElement, Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
data-test="undo-button"
- tabIndex={0}
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
- onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Keyboard repeat handler missing</b></div>
<div id="fix">
The new `<button>` is missing the `onKeyDown` handler that prevented duplicate callbacks on key hold. The original `<span role="button">` had `handleKeyboardActivation(() => restoreItem(id))` which checked `event.repeat` to ignore auto-repeat keydown events. Without this, holding Enter/Space fires multiple undo calls in rapid succession. Re-add the import and handler to match the original behavior.
</div>
</div>
<div><details><summary><b>Review Rule</b></summary><div>Bito created the rule <strong><code>Prevent duplicate keyboard activation on buttons</code></strong> for <strong>repo:</strong> <code>apache/superset</code>, <strong>language:</strong> <code>TSX</code>. Bito will avoid suggestions that match this rule. You can manage review rules <a href=https://alpha.bito.ai/home/ai-agents/review-rules>here</a>.</div></details></div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #42078:
URL: https://github.com/apache/superset/pull/42078
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652241087
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx:
##########
@@ -191,19 +190,25 @@ const FilterTitleContainer = forwardRef<HTMLDivElement, Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Keyboard accessibility gap</b></div>
<div id="fix">
The removed code at lines 195-203 provided explicit Enter/Space key handling via `handleKeyboardActivation`. While the native `<button>` element should handle keyboard activation, the existing test at FiltersConfigModal.test.tsx:1036 only verifies mouse clicks, not keyboard events. Add keyboard interaction test to ensure the undo button remains operable via keyboard.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx:
##########
@@ -184,30 +180,31 @@ export const DataTablesPane = ({
) : (
<Icons.DownOutlined aria-label={t('Expand data panel')} />
);
+ const resetButtonCss = css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ `;
return (
<div>
{panelOpen ? (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(false),
- )}
>
{caretIcon}
- </span>
+ </button>
) : (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(true),
- )}
>
{caretIcon}
- </span>
+ </button>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in conditional buttons</b></div>
<div id="fix">
Both buttons share identical `type`, `css`, and `onClick` props—only the `handleCollapseChange` argument differs. Future style changes require updating two locations, increasing maintenance burden and divergence risk. Extract the shared markup so the conditional only controls the icon/aria-label.
</div>
</div>
<small><i>Code Review Run #68a8a8</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>CWE-1004: Missing aria-label on buttons</b></div>
<div id="fix">
Screen readers will announce these as generic 'button' instead of 'Collapse data panel' or 'Expand data panel'. The icon inside carries the proper aria-label but the button wrapping it does not, degrading accessibility for keyboard and screen reader users. (See also: [CWE-1004](https://cwe.mitre.org/data/definitions/1004.html))
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx:
##########
@@ -344,29 +349,20 @@ const Thumbnail: FC<ThumbnailProps> = ({
const { key, value: type } = entry;
const isSelected = selectedViz === entry.key;
- const handleKeyDown = (event: React.KeyboardEvent) => {
- if (event.key === 'Enter' || event.key === ' ') {
- event.preventDefault();
- setSelectedViz(key);
- }
- };
-
const handleFocus = () => {
// Auto-select chart when tabbed to
setSelectedViz(key);
};
return (
- <div
- role="button"
+ <button
+ type="button"
// using css instead of a styled component to preserve
// the data-test attribute
css={thumbnailContainerCss(theme)}
- tabIndex={0}
className={isSelected ? 'selected' : ''}
onClick={() => setSelectedViz(key)}
onDoubleClick={onDoubleClick}
- onKeyDown={handleKeyDown}
onFocus={handleFocus}
data-test="viztype-selector-container"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing aria-pressed on toggle button</b></div>
<div id="fix">
The converted button element lacks `aria-pressed={isSelected}` to indicate its selected/toggle state to assistive technologies. This is a critical accessibility attribute for toggle buttons per WCAG 4.1.2 (Name, Role, Value). Without it, screen reader users cannot determine which viz type is currently selected.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -159,19 +158,25 @@ const ItemTitleContainer = forwardRef<HTMLDivElement, Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
data-test="undo-button"
- tabIndex={0}
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
- onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Keyboard repeat handler missing</b></div>
<div id="fix">
The new `<button>` is missing the `onKeyDown` handler that prevented duplicate callbacks on key hold. The original `<span role="button">` had `handleKeyboardActivation(() => restoreItem(id))` which checked `event.repeat` to ignore auto-repeat keydown events. Without this, holding Enter/Space fires multiple undo calls in rapid succession. Re-add the import and handler to match the original behavior.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControlShared.tsx:
##########
@@ -236,6 +236,10 @@ export const DescriptionToolTip = ({
whiteSpace: 'normal',
}}
>
+ {/* Deliberate role="button" on this tooltip-trigger icon (no click
+ handler) — covered by existing test expectations; not a fit for
+ the linter's suggested <button> tag. */}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>False claim about test coverage</b></div>
<div id="fix">
The comment at lines 239-241 claims this pattern is 'covered by existing test expectations,' but `DescriptionToolTip` has no tests verifying `role="button"` — the test fixture uses `description: ''`, so the tooltip never renders. Only `DeckglLayerVisibilityTooltip` has tests at lines 22-34 of `FilterControlShared.test.tsx` that check for a button role. Remove the misleading claim to prevent future developers from assuming test coverage.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx:
##########
@@ -104,24 +110,13 @@ const UrlFiltersVerticalCollapse = (props: {
return (
<div css={sectionContainerStyle}>
- <div
- css={sectionHeaderStyle}
- onClick={toggleSection}
- onKeyDown={e => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- toggleSection();
- }
- }}
- role="button"
- tabIndex={0}
- >
+ <button type="button" css={sectionHeaderStyle} onClick={toggleSection}>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing aria-expanded on toggle</b></div>
<div id="fix">
The toggle button now lacks `aria-expanded` to indicate its current state to screen readers. Since this button controls visibility of content below it, adding `aria-expanded={isOpen}` will enable users with assistive technologies to understand whether the section is currently expanded or collapsed.
</div>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.test.tsx:
##########
@@ -26,6 +26,7 @@ jest.mock(
'@superset-ui/core/components/Icons/AsyncIcon',
() =>
({ fileName }: { fileName: string }) => (
+ // eslint-disable-next-line jsx-a11y/prefer-tag-over-role -- mirrors AsyncIcon's real span+role="img" shape
<span role="img" aria-label={fileName.replace('_', '-')} />
),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Remove non-existent ESLint disable comment</b></div>
<div id="fix">
The eslint-disable-next-line comment references a rule 'jsx-a11y/prefer-tag-over-role' that is not defined in the project's ESLint configuration, causing the linter to fail. Remove this comment.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
() =>
({ fileName }: { fileName: string }) => (
<span role="img" aria-label={fileName.replace('_', '-')} />
),
````
</div>
</details>
</div>
<small><i>Code Review Run #7d4b9d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652260900
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx:
##########
@@ -191,19 +190,25 @@ const FilterTitleContainer = forwardRef<HTMLDivElement, Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
Review Comment:
<!-- Bito Reply -->
The suggestion to add a keyboard interaction test is intended to ensure that the new button remains accessible, as native button behavior can sometimes be obscured or overridden by custom styling or component wrappers. While native elements handle keyboard events by default, explicitly testing this interaction confirms that the component's implementation does not interfere with standard browser behavior. If the project's testing strategy consistently avoids redundant tests for native elements, it is reasonable to omit this test to maintain consistency.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -159,19 +158,25 @@ const ItemTitleContainer = forwardRef<HTMLDivElement, Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
data-test="undo-button"
- tabIndex={0}
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
- onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
>
Review Comment:
<!-- Bito Reply -->
The suggestion is to re-add a keyboard repeat handler to the new `<button>` element to prevent duplicate `restoreItem` calls when the Enter or Space key is held down. Applying this suggestion is appropriate because it maintains the original behavior of ignoring auto-repeat keydown events, which is necessary for consistent interaction handling.
**superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx**
```
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
```
##########
superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.test.tsx:
##########
@@ -26,6 +26,7 @@ jest.mock(
'@superset-ui/core/components/Icons/AsyncIcon',
() =>
({ fileName }: { fileName: string }) => (
+ // eslint-disable-next-line jsx-a11y/prefer-tag-over-role -- mirrors AsyncIcon's real span+role="img" shape
<span role="img" aria-label={fileName.replace('_', '-')} />
),
Review Comment:
<!-- Bito Reply -->
If the rule is not part of the project's ESLint configuration and is instead managed by oxlint, the suggestion to remove the disable comment is not applicable. You should ignore this suggestion, as the comment is necessary for the project's actual linting setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652261304
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx:
##########
@@ -104,24 +110,13 @@ const UrlFiltersVerticalCollapse = (props: {
return (
<div css={sectionContainerStyle}>
- <div
- css={sectionHeaderStyle}
- onClick={toggleSection}
- onKeyDown={e => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- toggleSection();
- }
- }}
- role="button"
- tabIndex={0}
- >
+ <button type="button" css={sectionHeaderStyle} onClick={toggleSection}>
Review Comment:
Added aria-expanded={isOpen} here. Also did the same for the two SectionHeaders in FilterControls.tsx while I was at it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3588317133
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -184,10 +184,15 @@ const ViewModeContainer = styled.div`
display: inline-block;
.toggle-button {
+ appearance: none;
+ border: none;
+ background: none;
+ font: inherit;
display: inline-block;
border-radius: ${theme.borderRadius}px;
padding: ${theme.sizeUnit}px;
padding-bottom: ${theme.sizeUnit * 0.5}px;
+ cursor: pointer;
Review Comment:
`inlineTextButtonCss` gets interpolated inside a plain (non `css`-tagged) template string in `.toggle-button`, not through emotion's serializer, so pulling it in there wouldn't actually resolve to CSS text. Leaving the duplication as is.
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
Review Comment:
The icon itself still carries `data-test="dashboard-list-trash-icon"`, same as before this PR, it was never on the outer element. Tests query the icon, not the button, so nothing's lost here.
##########
superset-frontend/src/pages/Tags/index.tsx:
##########
@@ -209,41 +209,31 @@ function TagList(props: TagListProps) {
onConfirm={() => handleTagsDelete([original])}
>
{confirmDelete => (
- <Tooltip
- id="delete-action-tooltip"
- title={t('Delete')}
+ <ActionButton
+ label={t('Delete')}
+ tooltip={t('Delete')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
+ icon={
<Icons.DeleteOutlined
data-test="dashboard-list-trash-icon"
iconSize="l"
/>
- </span>
- </Tooltip>
+ }
+ onClick={confirmDelete}
+ />
)}
</ConfirmStatusChange>
)}
{canEdit && (
- <Tooltip
- id="edit-action-tooltip"
- title={t('Edit')}
+ <ActionButton
+ label={t('Edit')}
+ tooltip={t('Edit')}
placement="bottom"
- >
- <span
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={handleEdit}
- >
+ icon={
<Icons.EditOutlined data-test="edit-alt" iconSize="l" />
- </span>
- </Tooltip>
+ }
+ onClick={handleEdit}
+ />
)}
Review Comment:
Same as the delete button, `data-test="edit-alt"` is on the icon and untouched by this change, it was never on the outer element before either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3588320494
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -35,14 +39,26 @@ export const ActionButton = ({
placement,
icon,
onClick,
+ className,
+ disabled = false,
+ dataTest,
}: ActionProps) => {
const theme = useTheme();
const actionButton = (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ aria-disabled={disabled}
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to add `tabIndex={-1}` to a disabled button is a common accessibility pattern, but your approach of using `aria-disabled={disabled}` on a native `<button>` element is also a valid and often preferred pattern. When using a native `<button>` with the `disabled` attribute, the browser automatically handles focus and accessibility. If you are using `aria-disabled` to keep the button focusable while indicating its state to screen readers, adding `tabIndex={-1}` would indeed prevent keyboard users from focusing the element, which contradicts your goal of keeping it focusable.
**superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx**
```
+ <button
+ type="button"
+ aria-disabled={disabled}
```
##########
superset-frontend/src/features/charts/ChartCard.tsx:
##########
@@ -103,9 +115,9 @@ export default function ChartCard({
menuItems.push({
key: 'export',
label: (
- <div
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={menuItemButtonCss}
Review Comment:
<!-- Bito Reply -->
The suggestion to add `data-test="chart-list-export-option"` is appropriate for maintaining consistency with other action buttons in the component, such as edit and delete. Applying this attribute ensures that automated tests can reliably target the export action in the card view, mirroring the pattern used elsewhere in the codebase.
**superset-frontend/src/features/charts/ChartCard.tsx**
```
+ <button
+ type="button"
+ css={menuItemButtonCss}
+ data-test="chart-list-export-option"
```
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -52,12 +68,12 @@ export const ActionButton = ({
}
}
Review Comment:
<!-- Bito Reply -->
The suggestion to add visual feedback for the disabled state is appropriate and improves the user experience by providing clear cues. Applying this change is recommended to ensure consistency with the existing design patterns in the codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-4989158130
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 6341a6ae4170baefc5c47e6ed32b91a862e05c97 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a588338bd28be000876110b |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-42078--superset-docs-preview.netlify.app](https://deploy-preview-42078--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctRCoMwDADQu-R7wVbRBW_gMRIbJ6MzJYpMxu5u_97P-0Fx-5QDRoAHiPM2r9Xzaq6NbixZ8b1_kWO8sLgu6njwC-2scMtaV9KS7ZpSfQP3RF1HkloSDSHQc4gxCPxv) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-42078--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-4988093526
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3585470202
##########
superset-frontend/packages/superset-ui-core/src/components/ActionButton/index.tsx:
##########
@@ -35,14 +39,26 @@ export const ActionButton = ({
placement,
icon,
onClick,
+ className,
+ disabled = false,
+ dataTest,
}: ActionProps) => {
const theme = useTheme();
const actionButton = (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ aria-disabled={disabled}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-284: Missing tabIndex on disabled button</b></div>
<div id="fix">
Missing `tabIndex` prop for disabled state. When disabled, the button should have `tabIndex={-1}` to prevent keyboard focus, consistent with the pattern used in `DataTableControl/index.tsx:72` and `CopyToClipboard/index.tsx:124`. (See also: [CWE-284](https://cwe.mitre.org/data/definitions/284.html))
</div>
</div>
<small><i>Code Review Run #356164</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Missing native disabled attribute</b></div>
<div id="fix">
The button uses `aria-disabled={disabled}` but lacks the native HTML `disabled` attribute. While `aria-disabled` conveys state to assistive tech, the native `disabled` attribute is required to prevent keyboard activation (Enter/Space) and show browser-native disabled styling. Without it, disabled buttons remain keyboard-operable and clickable.
</div>
</div>
<small><i>Code Review Run #68a8a8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx:
##########
@@ -184,30 +180,31 @@ export const DataTablesPane = ({
) : (
<Icons.DownOutlined aria-label={t('Expand data panel')} />
);
+ const resetButtonCss = css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ `;
return (
<div>
{panelOpen ? (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(false)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(false),
- )}
>
{caretIcon}
- </span>
+ </button>
) : (
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
+ css={resetButtonCss}
onClick={() => handleCollapseChange(true)}
- onKeyDown={handleKeyboardActivation(() =>
- handleCollapseChange(true),
- )}
>
{caretIcon}
- </span>
+ </button>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in conditional buttons</b></div>
<div id="fix">
Both buttons share identical `type`, `css`, and `onClick` props—only the `handleCollapseChange` argument differs. Future style changes require updating two locations, increasing maintenance burden and divergence risk. Extract the shared markup so the conditional only controls the icon/aria-label.
</div>
</div>
<small><i>Code Review Run #68a8a8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/DatasourcePanel/index.tsx:
##########
@@ -291,17 +286,21 @@ export default function DataSourcePanel({
message=""
description={
<>
- <span
- role="button"
- tabIndex={0}
+ <button
+ type="button"
onClick={() => setShowSaveDatasetModal(true)}
- onKeyDown={handleKeyboardActivation(() =>
- setShowSaveDatasetModal(true),
- )}
className="add-dataset-alert-description"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ cursor: pointer;
+ `}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1004: Missing focus styles</b></div>
<div id="fix">
The removed `handleKeyboardActivation` handler provided explicit Enter/Space key handling with `event.preventDefault()` and `event.repeat` protection. Native `<button>` handles Enter/Space automatically, but no focus styles are provided for keyboard users who tab to this element. ([CWE-1004](https://cwe.mitre.org/data/definitions/1004.html))
</div>
</div>
<small><i>Code Review Run #68a8a8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-5019485066
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652260708
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx:
##########
@@ -191,19 +190,25 @@ const FilterTitleContainer = forwardRef<HTMLDivElement, Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
Review Comment:
Native `<button>` already handles Enter/Space without a custom handler, same as every other role-to-tag swap in this PR. Adding a keyboard test just for this one spot would be inconsistent with the rest, so leaving it be.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -159,19 +158,25 @@ const ItemTitleContainer = forwardRef<HTMLDivElement, Props>(
<StyledWarning className="warning" iconSize="s" />
)}
{isRemoved && (
- <span
- css={{ alignSelf: 'flex-end', marginLeft: 'auto' }}
- role="button"
+ <button
+ type="button"
+ css={css`
+ appearance: none;
+ border: none;
+ background: none;
+ padding: 0;
+ font: inherit;
+ align-self: flex-end;
+ margin-left: auto;
+ `}
data-test="undo-button"
- tabIndex={0}
onClick={e => {
e.preventDefault();
restoreItem(id);
}}
- onKeyDown={handleKeyboardActivation(() => restoreItem(id))}
>
Review Comment:
Same story as the rest of these conversions, native activation instead of re-adding the repeat guard. Would be inconsistent to special-case just this one.
##########
superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.test.tsx:
##########
@@ -26,6 +26,7 @@ jest.mock(
'@superset-ui/core/components/Icons/AsyncIcon',
() =>
({ fileName }: { fileName: string }) => (
+ // eslint-disable-next-line jsx-a11y/prefer-tag-over-role -- mirrors AsyncIcon's real span+role="img" shape
<span role="img" aria-label={fileName.replace('_', '-')} />
),
Review Comment:
This rule's real, it's oxlint's jsx-a11y(prefer-tag-over-role), defined in oxlint.json which is what actually lints this repo (eslint proper isn't run here at all). Ran oxlint over the file and the disable comment suppresses it clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #42078:
URL: https://github.com/apache/superset/pull/42078#discussion_r3652261013
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControlShared.tsx:
##########
@@ -236,6 +236,10 @@ export const DescriptionToolTip = ({
whiteSpace: 'normal',
}}
>
+ {/* Deliberate role="button" on this tooltip-trigger icon (no click
+ handler) — covered by existing test expectations; not a fit for
+ the linter's suggested <button> tag. */}
Review Comment:
Fair, DescriptionToolTip doesn't actually have its own test for this. Reworded the comment to point at the sibling DeckglLayerVisibilityTooltip pattern instead of claiming coverage it doesn't have.
##########
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx:
##########
@@ -344,29 +349,20 @@ const Thumbnail: FC<ThumbnailProps> = ({
const { key, value: type } = entry;
const isSelected = selectedViz === entry.key;
- const handleKeyDown = (event: React.KeyboardEvent) => {
- if (event.key === 'Enter' || event.key === ' ') {
- event.preventDefault();
- setSelectedViz(key);
- }
- };
-
const handleFocus = () => {
// Auto-select chart when tabbed to
setSelectedViz(key);
};
return (
- <div
- role="button"
+ <button
+ type="button"
// using css instead of a styled component to preserve
// the data-test attribute
css={thumbnailContainerCss(theme)}
- tabIndex={0}
className={isSelected ? 'selected' : ''}
onClick={() => setSelectedViz(key)}
onDoubleClick={onDoubleClick}
- onKeyDown={handleKeyDown}
onFocus={handleFocus}
data-test="viztype-selector-container"
Review Comment:
Good catch, added aria-pressed={isSelected} to the thumbnail button.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx:
##########
@@ -104,24 +110,13 @@ const UrlFiltersVerticalCollapse = (props: {
return (
<div css={sectionContainerStyle}>
- <div
- css={sectionHeaderStyle}
- onClick={toggleSection}
- onKeyDown={e => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- toggleSection();
- }
- }}
- role="button"
- tabIndex={0}
- >
+ <button type="button" css={sectionHeaderStyle} onClick={toggleSection}>
Review Comment:
Added aria-expanded on both section headers, tied to sectionsOpen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(a11y): enable jsx-a11y/prefer-tag-over-role as error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42078:
URL: https://github.com/apache/superset/pull/42078#issuecomment-4977993233
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org