Re-enable `jsx-a11y` recommended rules (#104637)

* Re-enable `jsx-a11y` recommended rules * apply rule in correct place, couple of fixes * fix up some a11y issues * add ignore for keyboard a11y for now * readd testid * close carousel on backdrop click * use type="button" --------- Co-authored-by: Ashley Harrison <ashley.harrison@grafana.com>
2 months ago · 6f3200d4f0
parent 60ea65ca69
commit 6f3200d4f0
13 changed files with 117 additions and 102 deletions
--- a/eslint.config.js
+++ b/eslint.config.js
@ -173,9 +173,7 @@ module.exports = [
    files: ['**/*.tsx'],
    ignores: ['**/*.{spec,test}.tsx'],
    rules: {
-      // rules marked "off" are those left in the recommended preset we need to fix
-      // we should remove the corresponding line and fix them one by one
-      // any marked "error" contain specific overrides we'll need to keep
+      ...jsxA11yPlugin.configs.recommended.rules,
      'jsx-a11y/no-autofocus': [
        'error',
        {
--- a/packages/grafana-ui/src/components/Carousel/Carousel.tsx
+++ b/packages/grafana-ui/src/components/Carousel/Carousel.tsx
@ -1,11 +1,15 @@
 import { css, cx } from '@emotion/css';
-import { useState, useEffect } from 'react';
+import { useDialog } from '@react-aria/dialog';
+import { FocusScope } from '@react-aria/focus';
+import { OverlayContainer, useOverlay } from '@react-aria/overlays';
+import { useState, useEffect, useRef } from 'react';

 import { GrafanaTheme2 } from '@grafana/data';

 import { useStyles2 } from '../../themes';
 import { t } from '../../utils/i18n';
 import { Alert } from '../Alert/Alert';
+import { clearButtonStyles } from '../Button';
 import { IconButton } from '../IconButton/IconButton';

 // Define the image item interface
@ -23,7 +27,8 @@ export const Carousel: React.FC<CarouselProps> = ({ images }) => {
  const [imageErrors, setImageErrors] = useState<Record<string, boolean>>({});
  const [validImages, setValidImages] = useState<CarouselImage[]>(images);

-  const styles = useStyles2(getStyles());
+  const styles = useStyles2(getStyles);
+  const resetButtonStyles = useStyles2(clearButtonStyles);

  const handleImageError = (path: string) => {
    setImageErrors((prev) => ({
@ -77,6 +82,11 @@ export const Carousel: React.FC<CarouselProps> = ({ images }) => {
    }
  };

+  const ref = useRef<HTMLDivElement>(null);
+
+  const { overlayProps, underlayProps } = useOverlay({ isOpen: selectedIndex !== null, onClose: closePreview }, ref);
+  const { dialogProps } = useDialog({}, ref);
+
  if (validImages.length === 0) {
    return (
      <Alert
@ -88,18 +98,35 @@ export const Carousel: React.FC<CarouselProps> = ({ images }) => {
  }

  return (
-    <div onKeyDown={handleKeyDown} tabIndex={0}>
+    <>
      <div className={cx(styles.imageGrid)}>
        {validImages.map((image, index) => (
-          <div key={image.path} onClick={() => openPreview(index)} style={{ cursor: 'pointer' }}>
+          <button
+            type="button"
+            key={image.path}
+            onClick={() => openPreview(index)}
+            className={cx(resetButtonStyles, styles.imageButton)}
+          >
            <img src={image.path} alt={image.name} onError={() => handleImageError(image.path)} />
            <p>{image.name}</p>
-          </div>
+          </button>
        ))}
      </div>

      {selectedIndex !== null && (
-        <div className={cx(styles.fullScreenDiv)} onClick={closePreview} data-testid="carousel-full-screen">
+        <OverlayContainer>
+          <div role="presentation" className={styles.underlay} onClick={closePreview} {...underlayProps} />
+          <FocusScope contain autoFocus restoreFocus>
+            {/* convenience method for keyboard users */}
+            {/* eslint-disable-next-line jsx-a11y/no-static-element-interactions */}
+            <div
+              data-testid="carousel-full-screen"
+              ref={ref}
+              {...overlayProps}
+              {...dialogProps}
+              onKeyDown={handleKeyDown}
+              className={styles.overlay}
+            >
              <IconButton
                name="times"
                aria-label={t('carousel.close', 'Close')}
@ -112,20 +139,13 @@ export const Carousel: React.FC<CarouselProps> = ({ images }) => {
                size="xl"
                name="angle-left"
                aria-label={t('carousel.previous', 'Previous')}
-            onClick={(e) => {
-              e.stopPropagation();
-              goToPrevious();
-            }}
-            className={cx(styles.navigationButton, styles.previousButton)}
+                onClick={goToPrevious}
                data-testid="previous-button"
              />

-          <div
-            style={{ position: 'relative', maxWidth: '90%', maxHeight: '90%' }}
-            onClick={(e) => e.stopPropagation()}
-            data-testid="carousel-full-image"
-          >
+              <div data-testid="carousel-full-image">
                <img
+                  className={styles.imagePreview}
                  src={validImages[selectedIndex].path}
                  alt={validImages[selectedIndex].name}
                  onError={() => handleImageError(validImages[selectedIndex].path)}
@ -136,24 +156,30 @@ export const Carousel: React.FC<CarouselProps> = ({ images }) => {
                size="xl"
                name="angle-right"
                aria-label={t('carousel.next', 'Next')}
-            onClick={(e) => {
-              e.stopPropagation();
-              goToNext();
-            }}
-            className={cx(styles.navigationButton, styles.nextButton)}
+                onClick={goToNext}
                data-testid="next-button"
              />
            </div>
+          </FocusScope>
+        </OverlayContainer>
      )}
-    </div>
+    </>
  );
 };

-const getStyles = () => (theme: GrafanaTheme2) => ({
+const getStyles = (theme: GrafanaTheme2) => ({
+  imageButton: css({
+    textAlign: 'left',
+  }),
+  imagePreview: css({
+    maxWidth: '100%',
+    maxHeight: '80vh',
+    objectFit: 'contain',
+  }),
  imageGrid: css({
    display: 'grid',
    gridTemplateColumns: `repeat(auto-fill, minmax(200px, 1fr))`,
-    gap: '16px',
+    gap: theme.spacing(2),
    marginBottom: '20px',

    '& img': {
@ -165,49 +191,33 @@ const getStyles = () => (theme: GrafanaTheme2) => ({
      boxShadow: theme.shadows.z1,
    },
    '& p': {
-      margin: '4px 0',
+      margin: theme.spacing(0.5, 0),
      fontWeight: theme.typography.fontWeightMedium,
      color: theme.colors.text.primary,
    },
  }),
-  fullScreenDiv: css({
+  underlay: css({
    position: 'fixed',
    zIndex: theme.zIndex.modalBackdrop,
-    top: 0,
-    right: 0,
-    bottom: 0,
-    left: 0,
+    inset: 0,
    backgroundColor: theme.components.overlay.background,
+  }),
+  overlay: css({
    alignItems: 'center',
-    justifyContent: 'center',
    display: 'flex',
-
-    '& img': {
-      maxWidth: '100%',
-      maxHeight: '80vh',
-      objectFit: 'contain',
-    },
+    gap: theme.spacing(1),
+    height: 'fit-content',
+    marginBottom: 'auto',
+    marginTop: 'auto',
+    padding: theme.spacing(2),
+    position: 'fixed',
+    inset: 0,
+    zIndex: theme.zIndex.modal,
  }),
  closeButton: css({
-    position: 'absolute',
-    top: '20px',
-    right: '20px',
-    backgroundColor: 'transparent',
    color: theme.colors.text.primary,
-  }),
-  navigationButton: css({
-    position: 'absolute',
-    backgroundColor: 'transparent',
-    color: theme.colors.text.primary,
-    cursor: 'pointer',
-    display: 'flex',
-    alignItems: 'center',
-    justifyContent: 'center',
-  }),
-  nextButton: css({
-    right: '20px',
-  }),
-  previousButton: css({
-    left: '20px',
+    position: 'fixed',
+    top: theme.spacing(2),
+    right: theme.spacing(2),
  }),
 });
--- a/packages/grafana-ui/src/components/Table/TableNG/Cells/HeaderCell.tsx
+++ b/packages/grafana-ui/src/components/Table/TableNG/Cells/HeaderCell.tsx
@ -99,6 +99,7 @@ const HeaderCell: React.FC<HeaderCellProps> = ({
  }, [column]); // eslint-disable-line react-hooks/exhaustive-deps

  return (
+    // eslint-disable-next-line jsx-a11y/no-static-element-interactions
    <div
      ref={headerRef}
      className={styles.headerCell}
--- a/packages/grafana-ui/src/components/Table/TableNG/Cells/RowExpander.tsx
+++ b/packages/grafana-ui/src/components/Table/TableNG/Cells/RowExpander.tsx
@ -16,7 +16,7 @@ export function RowExpander({ height, onCellExpand, isExpanded }: RowExpanderNGP
    }
  }
  return (
-    <div className={styles.expanderCell} onClick={onCellExpand} onKeyDown={handleKeyDown}>
+    <div role="button" tabIndex={0} className={styles.expanderCell} onClick={onCellExpand} onKeyDown={handleKeyDown}>
      <Icon
        aria-label={
          isExpanded
--- a/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx
+++ b/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx
@ -75,6 +75,8 @@ function ThemeCard({ themeOption, isExperimental, isSelected, onSelect }: ThemeC
  const styles = useStyles2(getStyles);

  return (
+    // this is a convenience for mouse users. keyboard/screen reader users will use the radio button
+    // eslint-disable-next-line jsx-a11y/no-static-element-interactions,jsx-a11y/click-events-have-key-events
    <div className={styles.card} onClick={onSelect}>
      <div className={styles.header}>
        <RadioButtonDot
--- a/public/app/features/dashboard-scene/edit-pane/DashboardEditPane.tsx
+++ b/public/app/features/dashboard-scene/edit-pane/DashboardEditPane.tsx
@ -13,6 +13,7 @@ import {
  VizPanel,
 } from '@grafana/scenes';
 import {
+  clearButtonStyles,
  ElementSelectionContextItem,
  ElementSelectionContextState,
  ElementSelectionOnSelectOptions,
@ -199,6 +200,7 @@ export interface Props {
 export function DashboardEditPaneRenderer({ editPane, isCollapsed, onToggleCollapse, openOverlay }: Props) {
  const { selection } = useSceneObjectState(editPane, { shouldActivateOrKeepAlive: true });
  const styles = useStyles2(getStyles);
+  const clearButton = useStyles2(clearButtonStyles);
  const editableElement = useEditableElement(selection, editPane);
  const selectedObject = selection?.getFirstObject();
  const isNewElement = selection?.isNewElement() ?? false;
@ -280,17 +282,17 @@ export function DashboardEditPaneRenderer({ editPane, isCollapsed, onToggleColla
          data-edit-pane-splitter={true}
        />
        <div {...splitter.secondaryProps} className={cx(splitter.primaryProps.className, styles.paneContent)}>
-          <div
-            role="button"
+          <button
+            type="button"
            onClick={() => setOutlineCollapsed(!outlineCollapsed)}
-            className={styles.outlineCollapseButton}
+            className={cx(clearButton, styles.outlineCollapseButton)}
            data-testid={selectors.components.PanelEditor.Outline.section}
          >
            <Text weight="medium">
              <Trans i18nKey="dashboard-scene.dashboard-edit-pane-renderer.outline">Outline</Trans>
            </Text>
            <Icon name={outlineCollapsed ? 'angle-up' : 'angle-down'} />
-          </div>
+          </button>
          {!outlineCollapsed && (
            <div className={styles.outlineContainer}>
              <ScrollContainer showScrollIndicators={true}>
--- a/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx
+++ b/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx
@ -90,6 +90,8 @@ function DashboardOutlineNode({
      >
        {elementInfo.isContainer && (
          <button
+            // TODO fix keyboard a11y here
+            // eslint-disable-next-line jsx-a11y/role-has-required-aria-props
            role="treeitem"
            className={styles.angleButton}
            onPointerDown={onToggleCollapse}
@ -99,7 +101,6 @@ function DashboardOutlineNode({
          </button>
        )}
        <button
-          role="button"
          className={cx(styles.nodeName, isCloned && styles.nodeNameClone)}
          onDoubleClick={outlineRename.onNameDoubleClicked}
          data-testid={selectors.components.PanelEditor.Outline.item(instanceName)}
--- a/public/app/features/dashboard-scene/scene/layout-rows/RowItemRenderer.tsx
+++ b/public/app/features/dashboard-scene/scene/layout-rows/RowItemRenderer.tsx
@ -112,7 +112,6 @@ export function RowItemRenderer({ model }: SceneComponentProps<RowItem>) {
                    !isTopLevel && styles.rowTitleNested,
                    isCollapsed && styles.rowTitleCollapsed
                  )}
-                  role="heading"
                >
                  {!model.hasUniqueTitle() && (
                    <Tooltip
--- a/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx
+++ b/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx
@ -65,7 +65,6 @@ export function TabItemRenderer({ model }: SceneComponentProps<TabItem>) {
              isDropTarget && 'dashboard-drop-target'
            )}
            active={isActive}
-            role="presentation"
            title={titleInterpolated}
            href={href}
            aria-selected={isActive}
--- a/public/app/features/dashboard-scene/scene/new-toolbar/actions/ToolbarSwitch.tsx
+++ b/public/app/features/dashboard-scene/scene/new-toolbar/actions/ToolbarSwitch.tsx
@ -12,7 +12,7 @@ interface Props {
  checkedLabel?: string;
  disabled?: boolean;
  'data-testid'?: string;
-  onClick: (evt: MouseEvent<HTMLDivElement>) => void;
+  onClick: (evt: MouseEvent<HTMLButtonElement>) => void;
 }

 export const ToolbarSwitch = ({
@ -32,9 +32,8 @@ export const ToolbarSwitch = ({

  return (
    <Tooltip content={labelText}>
-      <div
+      <button
        aria-label={labelText}
-        role="button"
        className={cx({
          [styles.container]: true,
          [styles.containerChecked]: checked,
@ -46,7 +45,7 @@ export const ToolbarSwitch = ({
        <div className={cx(styles.box, checked && styles.boxChecked)}>
          <Icon name={iconName} size="xs" />
        </div>
-      </div>
+      </button>
    </Tooltip>
  );
 };
--- a/public/app/features/dashboard-scene/settings/variables/VariableSetEditableElement.tsx
+++ b/public/app/features/dashboard-scene/settings/variables/VariableSetEditableElement.tsx
@ -76,6 +76,8 @@ function VariableList({ set }: { set: SceneVariableSet }) {
  return (
    <Stack direction="column" gap={0}>
      {variables.map((variable) => (
+        // TODO fix keyboard a11y here
+        // eslint-disable-next-line jsx-a11y/no-static-element-interactions,jsx-a11y/click-events-have-key-events
        <div className={styles.variableItem} key={variable.state.name} onClick={() => onEditVariable(variable)}>
          {/* eslint-disable-next-line @grafana/no-untranslated-strings */}
          <Text>${variable.state.name}</Text>
--- a/public/app/features/logs/components/LogRowMessage.tsx
+++ b/public/app/features/logs/components/LogRowMessage.tsx
@ -88,15 +88,16 @@ const Ellipsis = ({ toggle, diff }: EllipsisProps) => {
  return (
    <>
      <Trans i18nKey="logs.log-row-message.ellipsis">… </Trans>
-      <span className={styles.showMore} onClick={handleClick}>
+      <button className={styles.showMore} onClick={handleClick}>
        {diff} <Trans i18nKey="logs.log-row-message.more">more</Trans>
-      </span>
+      </button>
    </>
  );
 };

 const getEllipsisStyles = (theme: GrafanaTheme2) => ({
  showMore: css({
+    backgroundColor: 'transparent',
    display: 'inline-flex',
    fontWeight: theme.typography.fontWeightMedium,
    fontSize: theme.typography.size.sm,
--- a/public/app/features/logs/components/panel/LogLine.tsx
+++ b/public/app/features/logs/components/panel/LogLine.tsx
@ -76,6 +76,7 @@ export const LogLine = ({
        className={`${styles.logLine} ${variant ?? ''} ${pinned ? styles.pinnedLogLine : ''}`}
        ref={onOverflow ? logLineRef : undefined}
        onMouseEnter={handleMouseOver}
+        onFocus={handleMouseOver}
      >
        <LogLineMenu styles={styles} log={log} />
        <div