rusackas commented on code in PR #42006:
URL: https://github.com/apache/superset/pull/42006#discussion_r3573030253
##########
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:
Fair, these aren't real menus, they only stop click propagation, so the role
and the tab stop were both wrong. Dropped `role="menu"` and the `tabIndex` so
there's no dead focus target.
##########
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:
Fair, these aren't real menus, they only stop click propagation, so the role
and the tab stop were both wrong. Dropped `role="menu"` and the `tabIndex` so
there's no dead focus target.
##########
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}
onClick={e => e.stopPropagation()}
Review Comment:
Fair, these aren't real menus, they only stop click propagation, so the role
and the tab stop were both wrong. Dropped `role="menu"` and the `tabIndex` so
there's no dead focus target.
##########
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:
Fair, these aren't real menus, they only stop click propagation, so the role
and the tab stop were both wrong. Dropped `role="menu"` and the `tabIndex` so
there's no dead focus target.
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx:
##########
@@ -115,6 +115,7 @@ const AdhocMetricEditPopoverTitle:
FC<AdhocMetricEditPopoverTitleProps> = ({
data-test="AdhocMetricEditTitle#trigger"
onMouseOver={handleMouseOver}
onMouseOut={handleMouseOut}
+ onFocus={handleMouseOver}
Review Comment:
Good catch, real bug. `handleBlur` now clears the hover flag too, so the
icon doesn't stay lit after focus leaves.
##########
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:
This is the `interactive-supports-focus` rule, which just makes the element
focusable. The Enter/Space activation is `click-events-have-key-events`, which
is the next PR in this series and adds handlers to these same elements.
tabIndex is the right fix for the rule this PR turns on.
##########
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:
This is the `interactive-supports-focus` rule, which just makes the element
focusable. The Enter/Space activation is `click-events-have-key-events`, which
is the next PR in this series and adds handlers to these same elements.
tabIndex is the right fix for the rule this PR turns on.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.tsx:
##########
@@ -139,6 +139,7 @@ const ItemTitleContainer = forwardRef<HTMLDivElement,
Props>(
<ItemTitle
Review Comment:
This is the `interactive-supports-focus` rule, which just makes the element
focusable. The Enter/Space activation is `click-events-have-key-events`, which
is the next PR in this series and adds handlers to these same elements.
tabIndex is the right fix for the rule this PR turns on.
##########
superset-frontend/src/explore/components/controls/OptionControls/index.tsx:
##########
@@ -350,6 +350,7 @@ export const OptionControlLabel = ({
role="button"
data-test="remove-control-button"
onClick={onRemove}
+ tabIndex={0}
Review Comment:
This is the `interactive-supports-focus` rule, which just makes the element
focusable. The Enter/Space activation is `click-events-have-key-events`, which
is the next PR in this series and adds handlers to these same elements.
tabIndex is the right fix for the rule this PR turns on.
##########
superset-frontend/src/features/queries/QueryPreviewModal.tsx:
##########
@@ -148,6 +148,7 @@ function QueryPreviewModal({
<QueryViewToggle>
<TabButton
role="button"
+ tabIndex={0}
Review Comment:
This is the `interactive-supports-focus` rule, which just makes the element
focusable. The Enter/Space activation is `click-events-have-key-events`, which
is the next PR in this series and adds handlers to these same elements.
tabIndex is the right fix for the rule this PR turns on.
##########
superset-frontend/packages/superset-ui-core/src/components/RefreshLabel/index.tsx:
##########
@@ -36,6 +36,7 @@ const RefreshLabel = ({
<Icons.SyncOutlined
iconSize="l"
role="button"
+ tabIndex={disabled ? -1 : 0}
Review Comment:
This is the `interactive-supports-focus` rule, which just makes the element
focusable. The Enter/Space activation is `click-events-have-key-events`, which
is the next PR in this series and adds handlers to these same elements.
tabIndex is the right fix for the rule this PR turns on.
--
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]
