korbit-ai[bot] commented on code in PR #33396:
URL: https://github.com/apache/superset/pull/33396#discussion_r2142968273
##########
superset-frontend/src/features/home/SubMenu.tsx:
##########
@@ -196,20 +198,22 @@ const SubMenuComponent: FunctionComponent<SubMenuProps> =
props => {
<StyledHeader>
<Row className="menu" role="navigation">
{props.name && <div className="header">{props.name}</div>}
- <Menu mode={showMenu} disabledOverflow>
+ <Menu mode={showMenu} disabledOverflow role="tablist">
{props.tabs?.map(tab => {
if ((props.usesRouter || hasHistory) && !!tab.usesRouter) {
return (
<Menu.Item key={tab.label}>
- <div
+ <Link
+ to={tab.url || ''}
role="tab"
+ id={tab.id || tab.name}
Review Comment:
### Non-unique IDs in tab elements <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using tab.name as a fallback for tab.id could lead to non-unique IDs if
multiple tabs have the same name but different URLs.
###### Why this matters
Non-unique IDs can cause screen readers to announce incorrect content and
break ARIA relationships, compromising accessibility functionality.
###### Suggested change ∙ *Feature Preview*
Generate unique IDs using a combination of tab name and index, or implement
a dedicated unique ID generator:
```typescript
id={tab.id || `tab-${tab.name}-${index}`}
```
This requires modifying the map function to include the index:
```typescript
{props.tabs?.map((tab, index) => {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd60c8bf-e347-42bb-886f-aec7588a9ca2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd60c8bf-e347-42bb-886f-aec7588a9ca2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd60c8bf-e347-42bb-886f-aec7588a9ca2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd60c8bf-e347-42bb-886f-aec7588a9ca2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cd60c8bf-e347-42bb-886f-aec7588a9ca2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5ac260c0-ac2b-4a13-be20-57315d2c18d7 -->
[](5ac260c0-ac2b-4a13-be20-57315d2c18d7)
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/RadioButtonControl.tsx:
##########
@@ -69,24 +69,45 @@ export default function RadioButtonControl({
boxShadow: 'none',
},
}}
+ role="tablist"
+ aria-label={typeof props.label === 'string' ? props.label : undefined}
>
<ControlHeader {...props} />
<div className="btn-group btn-group-sm">
{options.map(([val, label]) => (
<button
+ aria-label={typeof label === 'string' ? label : undefined}
+ id={`tab-${val}`}
key={JSON.stringify(val)}
type="button"
+ aria-selected={val === currentValue}
+ role="tab"
className={`btn btn-default ${
val === currentValue ? 'active' : ''
}`}
- onClick={() => {
+ onClick={e => {
+ e.currentTarget?.focus();
onChange(val);
}}
>
{label}
</button>
))}
</div>
+ {/* accessibility begin */}
+ <div
+ aria-live="polite"
+ style={{
+ position: 'absolute',
+ left: '-9999px',
+ height: '1px',
+ width: '1px',
+ overflow: 'hidden',
+ }}
+ >
+ {options.find(([val]) => val === currentValue)?.[1]} tab selected
+ </div>
+ {/* accessibility end */}
Review Comment:
### Insufficient Context for Accessibility Implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The inline comment markers 'accessibility begin/end' don't provide
meaningful context about why this accessibility div is necessary or how it
functions.
###### Why this matters
Without proper documentation, future developers might remove or modify this
critical accessibility feature, not understanding it's providing screen reader
announcements for tab selections.
###### Suggested change ∙ *Feature Preview*
/* Screen reader announcement div - Provides audio feedback when tabs are
selected.
Uses off-screen positioning to be invisible but still accessible to
screen readers. */
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5210310f-1295-40e7-8501-cca87fcfec8c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5210310f-1295-40e7-8501-cca87fcfec8c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5210310f-1295-40e7-8501-cca87fcfec8c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5210310f-1295-40e7-8501-cca87fcfec8c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5210310f-1295-40e7-8501-cca87fcfec8c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:80580cd3-e30d-436a-b77b-d01ff312fc8b -->
[](80580cd3-e30d-436a-b77b-d01ff312fc8b)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
