refactor: add "button" role to clickable UI elements for improved accessibility (#26602)

2026-04-19 08:04:53 +00:00 · 2024-04-18 09:28:02 -07:00
parent 9ee56bbc5b
commit 7263c7cb47
23 changed files with 107 additions and 12 deletions
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/components/Legend.tsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/components/Legend.tsx
@@ -106,6 +106,7 @@ const Legend = ({
      <li key={k}>
        <a
          href="#"
+          role="button"
          onClick={() => toggleCategory(k)}
          onDoubleClick={() => showSingleCategory(k)}
        >
--- a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx
+++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx
@@ -435,6 +435,7 @@ export class TableRenderer extends React.Component {
            key={`colKey-${flatColKey}`}
            colSpan={colSpan}
            rowSpan={rowSpan}
+            role="columnheader button"
            onClick={this.clickHeaderHandler(
              pivotData,
              colKey,
@@ -463,6 +464,7 @@ export class TableRenderer extends React.Component {
            key={`colKeyBuffer-${flatKey(colKey)}`}
            colSpan={colSpan}
            rowSpan={rowSpan}
+            role="columnheader button"
            onClick={this.clickHeaderHandler(
              pivotData,
              colKey,
@@ -486,6 +488,7 @@ export class TableRenderer extends React.Component {
          key="total"
          className="pvtTotalLabel"
          rowSpan={colAttrs.length + Math.min(rowAttrs.length, 1)}
+          role="columnheader button"
          onClick={this.clickHeaderHandler(
            pivotData,
            [],
@@ -550,6 +553,7 @@ export class TableRenderer extends React.Component {
        <th
          className="pvtTotalLabel"
          key="padding"
+          role="columnheader button"
          onClick={this.clickHeaderHandler(
            pivotData,
            [],
@@ -637,6 +641,7 @@ export class TableRenderer extends React.Component {
            className={valueCellClassName}
            rowSpan={rowSpan}
            colSpan={colSpan}
+            role="columnheader button"
            onClick={this.clickHeaderHandler(
              pivotData,
              rowKey,
@@ -668,6 +673,7 @@ export class TableRenderer extends React.Component {
          key="rowKeyBuffer"
          colSpan={rowAttrs.length - rowKey.length + colIncrSpan}
          rowSpan={1}
+          role="columnheader button"
          onClick={this.clickHeaderHandler(
            pivotData,
            rowKey,
@@ -772,6 +778,7 @@ export class TableRenderer extends React.Component {
        key="label"
        className="pvtTotalLabel pvtRowTotalLabel"
        colSpan={rowAttrs.length + Math.min(colAttrs.length, 1)}
+        role="columnheader button"
        onClick={this.clickHeaderHandler(
          pivotData,
          [],
--- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
+++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
@@ -610,6 +610,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
                col.toggleSortBy();
              }
            }}
+            role="columnheader button"
            onClick={onClick}
            data-column-name={col.id}
            {...(allowRearrangeColumns && {
--- a/superset-frontend/src/components/Icons/AntdEnhanced.tsx
+++ b/superset-frontend/src/components/Icons/AntdEnhanced.tsx
@@ -25,9 +25,10 @@ import IconType from './IconType';
 const AntdEnhancedIcons = Object.keys(AntdIcons)
  .filter(k => !k.includes('TwoTone'))
  .map(k => ({
-    [k]: (props: IconType) => (
-      <StyledIcon component={AntdIcons[k]} {...props} />
-    ),
+    [k]: (props: IconType) => {
+      const whatRole = props?.onClick ? 'button' : 'img';
+      return <StyledIcon component={AntdIcons[k]} role={whatRole} {...props} />;
+    },
  }))
  .reduce((l, r) => ({ ...l, ...r }));

--- a/superset-frontend/src/components/Icons/Icon.tsx
+++ b/superset-frontend/src/components/Icons/Icon.tsx
@@ -68,10 +68,13 @@ export const Icon = (props: IconProps) => {
    };
  }, [fileName, ImportedSVG]);

+  const whatRole = props?.onClick ? 'button' : 'img';
+
  return (
    <StyledIcon
      component={ImportedSVG.current || TransparentIcon}
      aria-label={name}
+      role={whatRole}
      {...iconProps}
    />
  );
--- a/superset-frontend/src/components/Label/Label.test.tsx
+++ b/superset-frontend/src/components/Label/Label.test.tsx
@@ -31,6 +31,17 @@ describe('Label', () => {
    expect(React.isValidElement(<Label />)).toBe(true);
  });

+  it('renders with role=undefined when onClick is not present', () => {
+    wrapper = mount(<Label />);
+    expect(wrapper.find('span').prop('role')).toBeUndefined();
+  });
+
+  it('renders with role="button" when onClick is present', () => {
+    const mockAction = jest.fn();
+    wrapper = mount(<Label onClick={mockAction} />);
+    expect(wrapper.find('span').prop('role')).toBe('button');
+  });
+
  it('works with an onClick handler', () => {
    const mockAction = jest.fn();
    wrapper = mount(<Label onClick={mockAction} />);
--- a/superset-frontend/src/components/Label/index.tsx
+++ b/superset-frontend/src/components/Label/index.tsx
@@ -93,6 +93,7 @@ export default function Label(props: LabelProps) {
  return (
    <Tag
      onClick={onClick}
+      role={onClick ? 'button' : undefined}
      {...rest}
      css={{
        transition: `background-color ${transitionTiming}s`,
--- a/superset-frontend/src/components/MetadataBar/MetadataBar.test.tsx
+++ b/superset-frontend/src/components/MetadataBar/MetadataBar.test.tsx
@@ -265,3 +265,25 @@ test('correctly renders the tags tooltip', async () => {
    );
  });
 });
+
+test('renders StyledItem with role="button" when onClick is defined', () => {
+  const onClick = jest.fn();
+  const items = [
+    { ...ITEMS[0], onClick },
+    { ...ITEMS[1], onClick },
+  ];
+  render(<MetadataBar items={items} />);
+
+  const styledItems = screen.getAllByRole('button');
+
+  expect(styledItems.length).toBe(2);
+});
+
+test('renders StyledItem with role=undefined when onClick is not defined', () => {
+  const items = [ITEMS[0], ITEMS[1]];
+  render(<MetadataBar items={items} />);
+
+  const styledItems = screen.queryAllByRole('button');
+
+  expect(styledItems.length).toBe(0);
+});
--- a/superset-frontend/src/components/MetadataBar/MetadataBar.tsx
+++ b/superset-frontend/src/components/MetadataBar/MetadataBar.tsx
@@ -148,6 +148,7 @@ const Item = ({
      collapsed={collapsed}
      last={last}
      onClick={onClick ? () => onClick(type) : undefined}
+      role={onClick ? 'button' : undefined}
    >
      <Icon iconSize="l" className="metadata-icon" />
      {!collapsed && (
--- a/superset-frontend/src/components/Select/CustomTag.tsx
+++ b/superset-frontend/src/components/Select/CustomTag.tsx
@@ -20,6 +20,7 @@ import React from 'react';
 import { Tag as AntdTag } from 'antd';
 import { styled, useCSSTextTruncation } from '@superset-ui/core';
 import { Tooltip } from '../Tooltip';
+import { CustomCloseIcon } from '../Tags/Tag';
 import { CustomTagProps } from './types';
 import { SELECT_ALL_VALUE } from './utils';
 import { NoElement } from './styles';
@@ -42,7 +43,11 @@ const Tag = (props: any) => {
  const [tagRef, tagIsTruncated] = useCSSTextTruncation<HTMLSpanElement>();
  return (
    <Tooltip title={tagIsTruncated ? props.children : null}>
-      <StyledTag {...props} className="ant-select-selection-item">
+      <StyledTag
+        closeIcon={props?.closable ? CustomCloseIcon : undefined}
+        {...props}
+        className="ant-select-selection-item"
+      >
        <span className="tag-content" ref={tagRef}>
          {props.children}
        </span>
--- a/superset-frontend/src/components/Tags/Tag.tsx
+++ b/superset-frontend/src/components/Tags/Tag.tsx
@@ -22,6 +22,7 @@ import TagType from 'src/types/TagType';
 import AntdTag from 'antd/lib/tag';
 import React, { useMemo } from 'react';
 import { Tooltip } from 'src/components/Tooltip';
+import { CloseOutlined } from '@ant-design/icons';

 const StyledTag = styled(AntdTag)`
  ${({ theme }) => `
@@ -31,6 +32,8 @@ const StyledTag = styled(AntdTag)`
  `};
 `;

+export const CustomCloseIcon = <CloseOutlined role="button" />;
+
 const MAX_DISPLAY_CHAR = 20;

 const Tag = ({
@@ -47,6 +50,8 @@ const Tag = ({

  const handleClose = () => (index ? onDelete?.(index) : null);

+  const whatRole = onClick ? (!id ? 'button' : 'link') : undefined;
+
  const tagElem = (
    <>
      {editable ? (
@@ -56,13 +61,14 @@ const Tag = ({
            closable={editable}
            onClose={handleClose}
            color="blue"
+            closeIcon={editable ? CustomCloseIcon : undefined}
          >
            {tagDisplay}
          </StyledTag>
        </Tooltip>
      ) : (
        <Tooltip title={toolTipTitle} key={toolTipTitle}>
-          <StyledTag data-test="tag" role="link" key={id} onClick={onClick}>
+          <StyledTag data-test="tag" key={id} onClick={onClick} role={whatRole}>
            {id ? (
              <a
                href={`/superset/all_entities/?id=${id}`}
--- a/superset-frontend/src/dashboard/components/Header/index.jsx
+++ b/superset-frontend/src/dashboard/components/Header/index.jsx
@@ -553,6 +553,7 @@ class Header extends React.PureComponent {
                          <StyledUndoRedoButton
                            type="text"
                            disabled={undoLength < 1}
+                            onClick={undoLength && onUndo}
                          >
                            <Icons.Undo
                              css={[
@@ -560,7 +561,6 @@ class Header extends React.PureComponent {
                                this.state.emphasizeUndo && undoRedoEmphasized,
                                undoLength < 1 && undoRedoDisabled,
                              ]}
-                              onClick={undoLength && onUndo}
                              data-test="undo-action"
                              iconSize="xl"
                            />
@@ -573,6 +573,7 @@ class Header extends React.PureComponent {
                          <StyledUndoRedoButton
                            type="text"
                            disabled={redoLength < 1}
+                            onClick={redoLength && onRedo}
                          >
                            <Icons.Redo
                              css={[
@@ -580,7 +581,6 @@ class Header extends React.PureComponent {
                                this.state.emphasizeRedo && undoRedoEmphasized,
                                redoLength < 1 && undoRedoDisabled,
                              ]}
-                              onClick={redoLength && onRedo}
                              data-test="redo-action"
                              iconSize="xl"
                            />
--- a/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadAsImage.test.tsx
+++ b/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadAsImage.test.tsx
@@ -58,3 +58,9 @@ test('Should call download image on click', async () => {
    expect(props.addDangerToast).toBeCalledTimes(0);
  });
 });
+
+test('Component is rendered with role="button"', async () => {
+  renderComponent();
+  const button = screen.getByRole('button', { name: 'Download as Image' });
+  expect(button).toBeInTheDocument();
+});
--- a/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadAsPdf.test.tsx
+++ b/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadAsPdf.test.tsx
@@ -58,3 +58,9 @@ test('Should call download pdf on click', async () => {
    expect(props.addDangerToast).toBeCalledTimes(0);
  });
 });
+
+test('Component is rendered with role="button"', async () => {
+  renderComponent();
+  const button = screen.getByRole('button', { name: 'Export as PDF' });
+  expect(button).toBeInTheDocument();
+});
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/CrossFilter.test.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/CrossFilter.test.tsx
@@ -65,7 +65,7 @@ test('Column and value should be visible', () => {

 test('Tag should be closable', () => {
  setup(mockedProps);
-  expect(screen.getByRole('img', { name: 'close' })).toBeInTheDocument();
+  expect(screen.getByRole('button', { name: 'close' })).toBeInTheDocument();
 });

 test('Divider should not be visible', () => {
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/CrossFilterTag.test.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/CrossFilterTag.test.tsx
@@ -27,6 +27,7 @@ const mockedProps: {
  filter: CrossFilterIndicator;
  orientation: FilterBarOrientation;
  removeCrossFilter: (filterId: number) => void;
+  onClick?: () => void;
 } = {
  filter: {
    name: 'test',
@@ -77,8 +78,17 @@ test('Column and value should be visible', () => {

 test('Tag should be closable', () => {
  setup(mockedProps);
-  const close = screen.getByRole('img', { name: 'close' });
+  const close = screen.getByRole('button', { name: 'close' });
  expect(close).toBeInTheDocument();
  userEvent.click(close);
  expect(mockedProps.removeCrossFilter).toHaveBeenCalledWith(1);
 });
+
+test('Close icon should have role="button"', () => {
+  setup({
+    ...mockedProps,
+    onClick: jest.fn(),
+  });
+  const button = screen.getByRole('button');
+  expect(button).toBeInTheDocument();
+});
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/CrossFilterTag.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/CrossFilterTag.tsx
@@ -29,6 +29,7 @@ import { CrossFilterIndicator } from 'src/dashboard/components/nativeFilters/sel
 import { Tag } from 'src/components';
 import { Tooltip } from 'src/components/Tooltip';
 import { FilterBarOrientation } from 'src/dashboard/types';
+import { CustomCloseIcon } from 'src/components/Tags/Tag';
 import { ellipsisCss } from './styles';

 const StyledCrossFilterValue = styled.b`
@@ -68,6 +69,7 @@ const CrossFilterTag = (props: {
  const [valueRef, valueIsTruncated] = useCSSTextTruncation<HTMLSpanElement>();

  const columnLabel = getColumnLabel(filter.column ?? '');
+
  return (
    <StyledTag
      css={css`
@@ -81,6 +83,7 @@ const CrossFilterTag = (props: {
      `}
      closable
      onClose={() => removeCrossFilter(filter.emitterId)}
+      closeIcon={CustomCloseIcon}
    >
      <Tooltip title={columnIsTruncated ? columnLabel : null}>
        <StyledCrossFilterColumn ref={columnRef}>
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ChartsScopingListPanel.test.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ChartsScopingListPanel.test.tsx
@@ -173,3 +173,12 @@ it('Uses callbacks on click', () => {
  }
  expect(DEFAULT_PROPS.removeCustomScope).toHaveBeenCalledWith(4);
 });
+
+it('Renders charts scoping list panel with FilterTitle rendered with role="button"', () => {
+  setup();
+  expect(screen.getByText('All charts/global scoping')).toBeVisible();
+  expect(screen.getByText('All charts/global scoping')).toHaveAttribute(
+    'role',
+    'button',
+  );
+});
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ChartsScopingListPanel.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ChartsScopingListPanel.tsx
@@ -140,6 +140,7 @@ export const ChartsScopingListPanel = ({
        </Button>
      </AddButtonContainer>
      <FilterTitle
+        role="button"
        onClick={() => setCurrentChartId(undefined)}
        className={activeChartId === undefined ? 'active' : ''}
      >
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/VerticalCollapse.test.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/VerticalCollapse.test.tsx
@@ -98,7 +98,7 @@ test('Tags should be visible', () => {

 test('Tags should be closable', () => {
  setup(mockedProps);
-  expect(screen.getAllByRole('img', { name: 'close' })).toHaveLength(2);
+  expect(screen.getAllByRole('button', { name: 'close' })).toHaveLength(2);
 });

 test('Divider should be visible', () => {
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx
@@ -207,6 +207,7 @@ const VerticalFilterBar: React.FC<VerticalBarProps> = ({
          {...getFilterBarTestId('collapsable')}
          className={cx({ open: !filtersOpen })}
          onClick={openFiltersBar}
+          role="button"
          offset={offset}
        >
          <StyledCollapseIcon
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DependencyList.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DependencyList.tsx
@@ -171,7 +171,7 @@ const List = ({
        />
      ))}
      {availableFilters.length > rows.length && (
-        <AddFilter onClick={onAdd}>
+        <AddFilter role="button" onClick={onAdd}>
          <Icons.PlusSmall />
          {t('Add filter')}
        </AddFilter>
--- a/superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx
+++ b/superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx
@@ -127,7 +127,7 @@ export default class FixedOrMetricControl extends React.Component {
          <Collapse.Panel
            showArrow={false}
            header={
-              <Label onClick={() => undefined}>
+              <Label>
                {this.state.type === controlTypes.fixed && (
                  <span>{this.state.fixedValue}</span>
                )}