This is an automated email from the ASF dual-hosted git repository. diegopucci pushed a commit to branch geido/fix/accessibility-applied-filters in repository https://gitbox.apache.org/repos/asf/superset.git
commit fcfaa88206ec3bf5e67eaed27b0f61c0a4295993 Author: Diego Pucci <[email protected]> AuthorDate: Fri Apr 12 17:12:30 2024 +0200 Enhance accessibility filters popover --- .../DetailsPanel/DetailsPanel.test.tsx | 92 +++++++++++++++++++++- .../components/FiltersBadge/DetailsPanel/index.tsx | 86 ++++++++++++++++---- .../FiltersBadge/FilterIndicator/index.tsx | 69 ++++++++-------- .../dashboard/components/FiltersBadge/Styles.tsx | 3 +- .../dashboard/components/FiltersBadge/index.tsx | 41 +++++++++- .../components/SliceHeaderControls/index.tsx | 1 + 6 files changed, 239 insertions(+), 53 deletions(-) diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx index ef4b49d218..82b8f3cb35 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx @@ -17,12 +17,28 @@ * under the License. */ import userEvent from '@testing-library/user-event'; -import React from 'react'; -import { render, screen } from 'spec/helpers/testing-library'; +import React, { RefObject } from 'react'; +import { render, screen, fireEvent } from 'spec/helpers/testing-library'; import { Indicator } from 'src/dashboard/components/nativeFilters/selectors'; import DetailsPanel from '.'; +const mockPopoverContentRef = { + current: { + focus: jest.fn(), + }, +} as unknown as RefObject<HTMLDivElement>; + +const mockPopoverTriggerRef = { + current: { + focus: jest.fn(), + }, +} as unknown as RefObject<HTMLDivElement>; + const createProps = () => ({ + popoverVisible: true, + popoverContentRef: mockPopoverContentRef, + popoverTriggerRef: mockPopoverTriggerRef, + setPopoverVisible: jest.fn(), appliedCrossFilterIndicators: [ { column: 'clinical_stage', @@ -168,3 +184,75 @@ test('Should render empty', () => { userEvent.click(screen.getByTestId('details-panel-content')); expect(screen.queryByRole('button')).not.toBeInTheDocument(); }); + +test('Close popover with ESC or ENTER', async () => { + const props = createProps(); + render( + <DetailsPanel {...props}> + <div>Content</div> + </DetailsPanel>, + { useRedux: true }, + ); + + const activeElement = document.activeElement as Element; + + // Close popover with Escape key + fireEvent.keyDown(activeElement, { key: 'Escape', code: 'Escape' }); + expect(props.setPopoverVisible).toHaveBeenCalledWith(false); + + // Open the popover for this test + props.popoverVisible = true; + + // Close with Enter + fireEvent.keyDown(activeElement, { key: 'Enter', code: 'Enter' }); + expect(props.setPopoverVisible).toHaveBeenCalledWith(false); +}); + +test('Arrow key navigation switches focus between indicators', () => { + // Prepare props with two indicators + const props = createProps(); + + props.appliedCrossFilterIndicators = [ + { + column: 'clinical_stage', + name: 'Clinical Stage', + value: [], + path: ['PATH_TO_CLINICAL_STAGE'], + }, + { + column: 'age_group', + name: 'Age Group', + value: [], + path: ['PATH_TO_AGE_GROUP'], + }, + ]; + + render( + <DetailsPanel {...props}> + <div>Content</div> + </DetailsPanel>, + { useRedux: true }, + ); + + // Query the indicators + const firstIndicator = screen.getByRole('button', { name: 'Clinical Stage' }); + const secondIndicator = screen.getByRole('button', { name: 'Age Group' }); + + // Focus the first indicator + firstIndicator.focus(); + expect(firstIndicator).toHaveFocus(); + + // Simulate ArrowDown key press + fireEvent.keyDown(document.activeElement as Element, { + key: 'ArrowDown', + code: 'ArrowDown', + }); + expect(secondIndicator).toHaveFocus(); + + // Simulate ArrowUp key press + fireEvent.keyDown(document.activeElement as Element, { + key: 'ArrowUp', + code: 'ArrowUp', + }); + expect(firstIndicator).toHaveFocus(); +}); diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx index 53d20cd1f8..482dc47e92 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useEffect, useState } from 'react'; +import React, { RefObject, useEffect, useRef } from 'react'; import { useSelector } from 'react-redux'; import { Global, css } from '@emotion/react'; import { t } from '@superset-ui/core'; @@ -36,6 +36,10 @@ export interface DetailsPanelProps { appliedIndicators: Indicator[]; onHighlightFilterSource: (path: string[]) => void; children: JSX.Element; + popoverVisible: boolean; + popoverContentRef: RefObject<HTMLDivElement>; + popoverTriggerRef: RefObject<HTMLDivElement>; + setPopoverVisible: (visible: boolean) => void; } const DetailsPanelPopover = ({ @@ -43,35 +47,87 @@ const DetailsPanelPopover = ({ appliedIndicators = [], onHighlightFilterSource, children, + popoverVisible, + popoverContentRef, + popoverTriggerRef, + setPopoverVisible, }: DetailsPanelProps) => { - const [visible, setVisible] = useState(false); const activeTabs = useSelector<RootState>( state => state.dashboardState?.activeTabs, ); + // Combined ref array for all filter indicator elements + const indicatorRefs = useRef<(HTMLButtonElement | null)[]>([]); + + const handleKeyDown = (event: React.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 (visible) { - window.addEventListener('resize', () => setVisible(false), { + if (popoverVisible) { + window.addEventListener('resize', () => setPopoverVisible(false), { once: true, }); } - }, [visible]); + }, [popoverVisible]); // if tabs change, popover doesn't close automatically useEffect(() => { - setVisible(false); + setPopoverVisible(false); }, [activeTabs]); - function handlePopoverStatus(isOpen: boolean) { - setVisible(isOpen); - } - const indicatorKey = (indicator: Indicator): string => `${indicator.column} - ${indicator.name}`; const content = ( - <FiltersDetailsContainer> + <FiltersDetailsContainer + ref={popoverContentRef} + tabIndex={-1} + onMouseLeave={() => setPopoverVisible(false)} + onKeyDown={handleKeyDown} + role="menu" + > <Global styles={theme => css` .filterStatusPopover { @@ -132,6 +188,7 @@ const DetailsPanelPopover = ({ <FiltersContainer> {appliedCrossFilterIndicators.map(indicator => ( <FilterIndicator + ref={el => indicatorRefs.current.push(el)} key={indicatorKey(indicator)} indicator={indicator} onClick={onHighlightFilterSource} @@ -151,6 +208,7 @@ const DetailsPanelPopover = ({ <FiltersContainer> {appliedIndicators.map(indicator => ( <FilterIndicator + ref={el => indicatorRefs.current.push(el)} key={indicatorKey(indicator)} indicator={indicator} onClick={onHighlightFilterSource} @@ -167,10 +225,10 @@ const DetailsPanelPopover = ({ <Popover overlayClassName="filterStatusPopover" content={content} - visible={visible} - onVisibleChange={handlePopoverStatus} + visible={popoverVisible} + onVisibleChange={handleVisibility} placement="bottomRight" - trigger="hover" + trigger={['hover']} > {children} </Popover> diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx index 2f8fd5af1d..2be5cb7431 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx @@ -17,7 +17,7 @@ * under the License. */ -import React, { FC } from 'react'; +import React from 'react'; import { css } from '@superset-ui/core'; import Icons from 'src/components/Icons'; import { getFilterValueForDisplay } from 'src/dashboard/components/nativeFilters/utils'; @@ -33,38 +33,39 @@ export interface IndicatorProps { onClick?: (path: string[]) => void; } -const FilterIndicator: FC<IndicatorProps> = ({ - indicator: { column, name, value, path = [] }, - onClick, -}) => { - const resultValue = getFilterValueForDisplay(value); - return ( - <FilterItem - onClick={ - onClick ? () => onClick([...path, `LABEL-${column}`]) : undefined - } - > - {onClick && ( - <i> - <Icons.SearchOutlined - iconSize="m" - css={css` - span { - vertical-align: 0; - } - `} - /> - </i> - )} - <div> - <FilterName> - {name} - {resultValue ? ': ' : ''} - </FilterName> - <FilterValue>{resultValue}</FilterValue> - </div> - </FilterItem> - ); -}; +const FilterIndicator = React.forwardRef<HTMLButtonElement, IndicatorProps>( + ({ indicator: { column, name, value, path = [] }, onClick }, ref) => { + const resultValue = getFilterValueForDisplay(value); + return ( + <FilterItem + ref={ref} + onClick={ + onClick ? () => onClick([...path, `LABEL-${column}`]) : undefined + } + tabIndex={-1} + > + {onClick && ( + <i> + <Icons.SearchOutlined + iconSize="m" + css={css` + span { + vertical-align: 0; + } + `} + /> + </i> + )} + <div> + <FilterName> + {name} + {resultValue ? ': ' : ''} + </FilterName> + <FilterValue>{resultValue}</FilterValue> + </div> + </FilterItem> + ); + }, +); export default FilterIndicator; diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx index 1b3be657af..9abd2c9ea1 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/Styles.tsx @@ -93,7 +93,8 @@ export const FilterItem = styled.button` transition: opacity ease-in-out ${theme.transitionTiming}; } - &:hover i svg { + &:hover i svg, + &:focus-visible i svg { opacity: 1; } `} diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx index 6dba29c661..3f771740a5 100644 --- a/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx +++ b/superset-frontend/src/dashboard/components/FiltersBadge/index.tsx @@ -16,7 +16,13 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import React, { + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { uniqWith } from 'lodash'; import cx from 'classnames'; @@ -25,6 +31,7 @@ import { Filters, JsonObject, styled, + t, usePrevious, } from '@superset-ui/core'; import Icons from 'src/components/Icons'; @@ -126,6 +133,9 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { const [dashboardIndicators, setDashboardIndicators] = useState<Indicator[]>( indicatorsInitialState, ); + const [popoverVisible, setPopoverVisible] = useState(false); + const popoverContentRef = useRef<HTMLDivElement>(null); + const popoverTriggerRef = useRef<HTMLDivElement>(null); const onHighlightFilterSource = useCallback( (path: string[]) => { @@ -134,6 +144,12 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { [dispatch], ); + const handleKeyDown = (event: React.KeyboardEvent<HTMLDivElement>) => { + if (event.key === 'Enter') { + setPopoverVisible(true); + } + }; + const prevChart = usePrevious(chart); const prevChartStatus = prevChart?.chartStatus; const prevDashboardFilters = usePrevious(dashboardFilters); @@ -141,6 +157,14 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { const showIndicators = chart?.chartStatus && ['rendered', 'success'].includes(chart.chartStatus); + useEffect(() => { + if (popoverVisible) { + setTimeout(() => { + popoverContentRef?.current?.focus(); + }); + } + }, [popoverVisible]); + useEffect(() => { if (!showIndicators && dashboardIndicators.length > 0) { setDashboardIndicators(indicatorsInitialState); @@ -180,6 +204,7 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { const prevDashboardLayout = usePrevious(present); const prevDataMask = usePrevious(dataMask); const prevChartConfig = usePrevious(chartConfiguration); + useEffect(() => { if (!showIndicators && nativeIndicators.length > 0) { setNativeIndicators(indicatorsInitialState); @@ -250,6 +275,8 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { ), [indicators], ); + const filterCount = + appliedIndicators.length + appliedCrossFilterIndicators.length; if (!appliedCrossFilterIndicators.length && !appliedIndicators.length) { return null; @@ -260,18 +287,28 @@ export const FiltersBadge = ({ chartId }: FiltersBadgeProps) => { appliedCrossFilterIndicators={appliedCrossFilterIndicators} appliedIndicators={appliedIndicators} onHighlightFilterSource={onHighlightFilterSource} + setPopoverVisible={setPopoverVisible} + popoverVisible={popoverVisible} + popoverContentRef={popoverContentRef} + popoverTriggerRef={popoverTriggerRef} > <StyledFilterCount + aria-label={t('Applied filters (%s)', filterCount)} + aria-haspopup="true" + role="button" + ref={popoverTriggerRef} className={cx( 'filter-counts', !!appliedCrossFilterIndicators.length && 'has-cross-filters', )} + tabIndex={0} + onKeyDown={handleKeyDown} > <Icons.Filter iconSize="m" /> <StyledBadge data-test="applied-filter-count" className="applied-count" - count={appliedIndicators.length + appliedCrossFilterIndicators.length} + count={filterCount} showZero /> </StyledFilterCount> diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx index 49a13362f3..3890c596e8 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -936,6 +936,7 @@ const SliceHeaderControls = (props: SliceHeaderControlsPropsWithRouter) => { id={`slice_${slice.slice_id}-controls`} role="button" aria-label="More Options" + aria-haspopup="true" tabIndex={0} > <VerticalDotsTrigger />
