Copilot commented on code in PR #38791:
URL: https://github.com/apache/superset/pull/38791#discussion_r2989544416
##########
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx:
##########
@@ -49,148 +44,97 @@ const DetailsPanelPopover = ({
onHighlightFilterSource,
children,
popoverVisible,
- popoverContentRef,
- popoverTriggerRef,
setPopoverVisible,
}: DetailsPanelProps) => {
Review Comment:
`popoverContentRef`/`popoverTriggerRef` are still part of
`DetailsPanelProps` but are no longer used by this component (they aren’t
destructured or attached to any element). The parent `FiltersBadge` still
attempts to focus `popoverContentRef.current` when opening, so `current` will
remain `null` now and keyboard focus/navigation will regress. Either attach
`popoverContentRef` to a real focusable element in the dropdown (e.g., a
wrapper around `Menu` with `tabIndex={-1}` / Menu autoFocus) or remove these
refs from the API and also remove the parent focus effect.
##########
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}cc`}
- content={content}
+ <NoAnimationDropdown
+ popupRender={() => (
+ <Menu
+ selectable={false}
+ items={menuItems}
+ onClick={() => setPopoverVisible(false)}
+ />
+ )}
+ overlayStyle={{ zIndex: 1001, animationDuration: '0s' }}
+ trigger={['click', '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}
+ </span>
Review Comment:
The dropdown trigger is a non-focusable `<span role="button">` wrapping a
focusable child (`StyledFilterCount` has `tabIndex={0}` + `role="button"`).
This creates nested interactive roles (invalid a11y) and also means
`NoAnimationDropdown`’s `autoFocus` can’t reliably focus the trigger element.
Consider using the actual focusable child as the dropdown trigger (remove the
wrapper span), or make the wrapper the only interactive element (add
`tabIndex={0}`, remove role/tabIndex from the child, and forward the
ref/handlers).
##########
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:
The updated arrow-key navigation test doesn’t actually verify focus
movement: it never explicitly focuses the first item before typing, and after
`{arrowdown}` it asserts the *same* item still has focus. Also
`userEvent.type(...)` is async in most setups, so these calls should be awaited
to avoid race conditions. Update the test to (1) focus the first menu item, (2)
assert focus moves to the next/previous item on ArrowDown/ArrowUp, and (3)
await userEvent interactions.
--
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]
