codeant-ai-for-open-source[bot] commented on code in PR #38791:
URL: https://github.com/apache/superset/pull/38791#discussion_r3488532789
##########
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx:
##########
@@ -243,28 +245,15 @@ test('Arrow key navigation switches focus between
indicators', () => {
);
// Query the indicators
- const firstIndicator = screen.getByRole('button', {
- name: 'search Clinical Stage',
- });
- const secondIndicator = screen.getByRole('button', {
- name: 'search Age Group',
- });
+ const firstMenuItem = screen.queryByText('Clinical Stage')?.closest('li')!;
+ const secondMenuItem = screen.queryByText('Age Group')?.closest('li')!;
- // Focus the first indicator
- firstIndicator.focus();
- expect(firstIndicator).toHaveFocus();
+ expect(firstMenuItem).toBeInTheDocument();
+ expect(secondMenuItem).toBeInTheDocument();
- // Simulate ArrowDown key press
- fireEvent.keyDown(document.activeElement as Element, {
- key: 'ArrowDown',
- code: 'ArrowDown',
- });
- expect(secondIndicator).toHaveFocus();
+ userEvent.type(firstMenuItem, '{arrowdown}');
+ expect(firstMenuItem).toHaveFocus();
- // Simulate ArrowUp key press
- fireEvent.keyDown(document.activeElement as Element, {
- key: 'ArrowUp',
- code: 'ArrowUp',
- });
- expect(firstIndicator).toHaveFocus();
+ userEvent.type(secondMenuItem, '{arrowdown}');
+ expect(secondMenuItem).toHaveFocus();
Review Comment:
**Suggestion:** This test claims to verify arrow-key focus movement, but it
asserts that focus stays on the same item after `ArrowDown`, which is the
opposite of the intended behavior and will not catch real navigation
regressions. Update the assertions to check that focus moves to the next menu
item. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Arrow-key navigation tests may give false confidence.
⚠️ Menu focus regressions could pass CI unnoticed.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Jest test suite so that `Arrow key navigation switches focus
between
indicators` in
`superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx:221-259`
is executed.
2. Observe that the test renders `DetailsPanel` with two
`appliedCrossFilterIndicators`
and queries the menu items via `screen.queryByText('Clinical
Stage')?.closest('li')` and
`screen.queryByText('Age Group')?.closest('li')` (lines 247-250).
3. The test then calls `userEvent.type(firstMenuItem, '{arrowdown}')` at
line 254 and
immediately asserts `expect(firstMenuItem).toHaveFocus()` at line 255,
followed by similar
logic for `secondMenuItem` at lines 257-258, meaning it expects focus to
remain on the
same item after ArrowDown.
4. Because the assertions verify that focus stays on the originating item
instead of
moving to the next item, any regression in AntD `Menu` arrow-key navigation
(e.g., focus
failing to move between indicators) would not cause this test to fail, so
the test does
not actually validate item-to-item arrow navigation despite its name.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=659c6a19f8e84c3cb0897fd21ee82463&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=659c6a19f8e84c3cb0897fd21ee82463&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx
**Line:** 254:258
**Comment:**
*Logic Error: This test claims to verify arrow-key focus movement, but
it asserts that focus stays on the same item after `ArrowDown`, which is the
opposite of the intended behavior and will not catch real navigation
regressions. Update the assertions to check that focus moves to the next menu
item.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38791&comment_hash=a342e9bdae1eb7a5a15b869aedf0c6f091a44dcad58368a20af99d90e738a663&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38791&comment_hash=a342e9bdae1eb7a5a15b869aedf0c6f091a44dcad58368a20af99d90e738a663&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx:
##########
@@ -49,148 +44,97 @@ const DetailsPanelPopover = ({
onHighlightFilterSource,
children,
popoverVisible,
- popoverContentRef,
- popoverTriggerRef,
setPopoverVisible,
}: DetailsPanelProps) => {
+ const theme = useTheme();
const activeTabs = useSelector<RootState>(
state => state.dashboardState?.activeTabs,
);
- // Combined ref array for all filter indicator elements
- const indicatorRefs = useRef<(HTMLButtonElement | null)[]>([]);
-
- const handleKeyDown = (event: KeyboardEvent<HTMLDivElement>) => {
- switch (event.key) {
- case 'Escape':
- case 'Enter':
- // timing out to allow for filter selection to happen first
- setTimeout(() => {
- // move back to the popover trigger element
- popoverTriggerRef?.current?.focus();
- // Close popover on ESC or ENTER
- setPopoverVisible(false);
- });
- break;
- case 'ArrowDown':
- case 'ArrowUp': {
- event.preventDefault(); // Prevent scrolling
- // Navigate through filters with arrows up/down
- const currentFocusIndex = indicatorRefs.current.findIndex(
- ref => ref === document.activeElement,
- );
- const maxIndex = indicatorRefs.current.length - 1;
- let nextFocusIndex = 0;
-
- if (event.key === 'ArrowDown') {
- nextFocusIndex =
- currentFocusIndex >= maxIndex ? 0 : currentFocusIndex + 1;
- } else if (event.key === 'ArrowUp') {
- nextFocusIndex =
- currentFocusIndex <= 0 ? maxIndex : currentFocusIndex - 1;
- }
- indicatorRefs.current[nextFocusIndex]?.focus();
- break;
- }
- case 'Tab':
- // forcing popover context until ESC or ENTER are pressed
- event.preventDefault();
- break;
- default:
- break;
- }
- };
- const handleVisibility = (isOpen: boolean) => {
- setPopoverVisible(isOpen);
- };
-
- // we don't need to clean up useEffect, setting { once: true } removes the
event listener after handle function is called
- useEffect(() => {
- if (popoverVisible) {
- window.addEventListener('resize', () => setPopoverVisible(false), {
- once: true,
- });
- }
- }, [popoverVisible]);
-
- // if tabs change, popover doesn't close automatically
useEffect(() => {
setPopoverVisible(false);
- }, [activeTabs]);
+ }, [activeTabs, setPopoverVisible]);
const indicatorKey = (indicator: Indicator): string =>
- `${indicator.column} - ${indicator.name}`;
- const theme = useTheme();
- const content = (
- <FiltersDetailsContainer
- ref={popoverContentRef}
- tabIndex={-1}
- onMouseLeave={() => setPopoverVisible(false)}
- onKeyDown={handleKeyDown}
- role="menu"
- >
- <div>
- {appliedCrossFilterIndicators.length ? (
- <div>
- <SectionName>
- {t(
- 'Applied cross-filters (%d)',
- appliedCrossFilterIndicators.length,
- )}
- </SectionName>
- <FiltersContainer>
- {appliedCrossFilterIndicators.map(indicator => (
- <FilterIndicator
- ref={el => indicatorRefs.current.push(el)}
- key={indicatorKey(indicator)}
- indicator={indicator}
- onClick={onHighlightFilterSource}
- />
- ))}
- </FiltersContainer>
- </div>
- ) : null}
- {appliedCrossFilterIndicators.length && appliedIndicators.length ? (
- <Separator />
- ) : null}
- {appliedIndicators.length ? (
- <div>
- <SectionName>
- {t('Applied filters (%d)', appliedIndicators.length)}
- </SectionName>
- <FiltersContainer>
- <List
- dataSource={appliedIndicators}
- renderItem={indicator => (
- <List.Item>
- <FilterIndicator
- ref={el => indicatorRefs.current.push(el)}
- key={indicatorKey(indicator)}
- indicator={indicator}
- onClick={onHighlightFilterSource}
- />
- </List.Item>
- )}
- />
- </FiltersContainer>
- </div>
- ) : null}
- </div>
- </FiltersDetailsContainer>
- );
+ `${indicator.column} - ${indicator.name} - ${indicator.path?.join('>') ??
''}`;
+
+ const menuItems: MenuProps['items'] = [];
+
+ if (appliedCrossFilterIndicators.length > 0) {
+ menuItems.push({
+ key: 'grp-cross',
+ type: 'group',
+ label: t(
+ 'Applied cross-filters (%d)',
+ appliedCrossFilterIndicators.length,
+ ),
+ children: appliedCrossFilterIndicators.map(indicator => ({
+ key: `cross-${indicatorKey(indicator)}`,
+ label: (
+ <FilterIndicator
+ indicator={indicator}
+ onClick={onHighlightFilterSource}
+ />
+ ),
+ })),
+ });
+ }
+
+ if (appliedIndicators.length > 0) {
+ if (appliedCrossFilterIndicators.length > 0) {
+ menuItems.push({ type: 'divider' });
+ }
+ menuItems.push({
+ key: 'grp-applied',
+ type: 'group',
+ label: t('Applied filters (%d)', appliedIndicators.length),
+ children: appliedIndicators.map(indicator => ({
+ key: `applied-${indicatorKey(indicator)}`,
+ label: (
+ <FilterIndicator
+ indicator={indicator}
+ onClick={onHighlightFilterSource}
+ />
+ ),
+ })),
+ });
+ }
return (
- <Popover
- color={theme.colorBgElevated}
- content={content}
+ <NoAnimationDropdown
+ popupRender={() => (
+ <Menu
+ selectable={false}
+ items={menuItems}
+ onClick={() => setPopoverVisible(false)}
+ />
+ )}
+ overlayStyle={{ zIndex: 1001, animationDuration: '0s' }}
+ trigger={['hover']}
open={popoverVisible}
- onOpenChange={handleVisibility}
+ onOpenChange={visible => setPopoverVisible(visible)}
placement="bottomRight"
- trigger={['hover']}
- data-test="filter-status-popover"
>
- {children}
- </Popover>
+ <span
+ role="button"
+ aria-label={t('View applied filters')}
+ aria-haspopup="menu"
+ aria-expanded={popoverVisible}
+ css={css`
+ display: inline-flex;
+ outline: none;
+ cursor: pointer;
+ border-radius: 4px;
+
+ &:focus-visible {
+ outline: 2px solid ${theme.colorPrimary};
+ outline-offset: 1px;
+ }
+ `}
+ >
+ {children}
Review Comment:
**Suggestion:** The new trigger wrapper is marked as a button but is not
keyboard-focusable, so keyboard users cannot land on this element and the new
`:focus-visible` outline/ARIA state on it will never activate. Use a natively
focusable control (or add `tabIndex={0}` with keyboard handling) on this
trigger-level element. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Trigger span exposes non-focusable button semantics.
⚠️ ARIA expanded state never aligns with keyboard focus.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/src/dashboard/components/FiltersBadge/index.tsx:39-71`,
render
`FiltersBadge`, which wraps the focusable `StyledFilterCount` badge in
`DetailsPanelPopover` for charts, and is used from `SliceHeader` at
`superset-frontend/src/dashboard/components/SliceHeader/index.tsx:333-15`.
2. Inspect `DetailsPanelPopover` in
`superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx:41-138`;
it wraps `children` in a trigger-level `<span role="button"
aria-label={t('View applied
filters')} aria-haspopup="menu" aria-expanded={popoverVisible} ...>` at
lines 118-123.
3. Note that this `span` element at lines 118-135 has no `tabIndex`, no
`ref`, and no
keyboard handlers, so it is not keyboard-focusable; when tabbing through the
dashboard,
focus lands instead on the inner `StyledFilterCount` div (which has
`tabIndex={0}` and
`role="button"`) defined in `FiltersBadge/index.tsx:49-59.
4. As a result, the element that exposes `role="button"` and `aria-expanded`
in
`DetailsPanelPopover` (the `span`) can never receive keyboard focus or show
its
`:focus-visible` outline, while assistive technology sees nested button
semantics split
between the non-focusable `span` and the inner `StyledFilterCount`, which is
an
incomplete, non-ideal implementation of the trigger’s ARIA and focus
behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=87e10d6b230345358c29d25ae3e71b44&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=87e10d6b230345358c29d25ae3e71b44&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx
**Line:** 118:135
**Comment:**
*Incomplete Implementation: The new trigger wrapper is marked as a
button but is not keyboard-focusable, so keyboard users cannot land on this
element and the new `:focus-visible` outline/ARIA state on it will never
activate. Use a natively focusable control (or add `tabIndex={0}` with keyboard
handling) on this trigger-level element.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38791&comment_hash=d97e53ae97c0e8cc29d9e5cf216c34ea388110dc0754fbf51bcd87314ac9247f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38791&comment_hash=d97e53ae97c0e8cc29d9e5cf216c34ea388110dc0754fbf51bcd87314ac9247f&reaction=dislike'>👎</a>
--
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]
