bito-code-review[bot] commented on code in PR #42006:
URL: https://github.com/apache/superset/pull/42006#discussion_r3573031415
##########
superset-frontend/src/SqlLab/components/TableExploreTree/TreeNodeRenderer.tsx:
##########
@@ -239,6 +239,7 @@ const TreeNodeRenderer: React.FC<TreeNodeRendererProps> = ({
<div
className="side-action-container"
role="menu"
+ tabIndex={0}
Review Comment:
<!-- Bito Reply -->
The decision to remove `role="menu"` and `tabIndex={0}` is appropriate.
Since the element was not intended to be an interactive menu, removing these
attributes correctly prevents the creation of an inaccessible focus target that
would otherwise confuse keyboard users.
##########
superset-frontend/src/SqlLab/components/TableExploreTree/TreeNodeRenderer.tsx:
##########
@@ -307,6 +308,7 @@ const TreeNodeRenderer: React.FC<TreeNodeRendererProps> = ({
<div
className="side-action-container"
role="menu"
+ tabIndex={0}
Review Comment:
<!-- Bito Reply -->
Removing the `role="menu"` and `tabIndex={0}` attributes is the correct
approach here. Since the element is not a functional menu and lacks keyboard
event handling, removing these attributes prevents the creation of an
inaccessible focus target, effectively addressing the concern raised by the
reviewer.
**superset-frontend/src/SqlLab/components/TableExploreTree/TreeNodeRenderer.tsx**
```
<div
className="side-action-container"
role="menu"
+ tabIndex={0}
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -139,6 +139,7 @@ const ItemTitleContainer = forwardRef<HTMLDivElement,
Props>(
<ItemTitle
role="tab"
key={`item-title-tab-${id}`}
+ tabIndex={0}
Review Comment:
<!-- Bito Reply -->
The suggestion to add keyboard activation handlers is correct and
appropriate for accessibility. While adding `tabIndex={0}` makes the element
focusable, it does not fulfill the requirement for keyboard interaction for
elements with `role="tab"`. Implementing the `onKeyDown` handler as suggested
ensures that users can activate the tab using Enter or Space, which is a
standard accessibility requirement for interactive components.
**superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx**
```
<ItemTitle
role="tab"
key={`item-title-tab-${id}`}
tabIndex={0}
onKeyDown={e => { if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault(); onChange(id); } }}
```
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailTableControls.tsx:
##########
@@ -163,6 +163,7 @@ export default function TableControls({
iconSize="l"
aria-label={t('Reload')}
role="button"
+ tabIndex={0}
Review Comment:
<!-- Bito Reply -->
The suggestion to add an `onKeyDown` handler is appropriate for
accessibility, as elements with `tabIndex={0}` and `role="button"` should be
keyboard-activatable. While `tabIndex={0}` satisfies the focus requirement,
adding the keyboard handler ensures the button functions correctly for users
navigating via keyboard.
**superset-frontend/src/components/Chart/DrillDetail/DrillDetailTableControls.tsx**
```
iconSize="l"
aria-label={t('Reload')}
role="button"
tabIndex={0}
onKeyDown={handleKeyDown}
```
--
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]
