From 6f3200d4f01d4208a2bfa57046c003ef6b1f9a47 Mon Sep 17 00:00:00 2001
From: Tom Ratcliffe <tom.ratcliffe@grafana.com>
Date: Wed, 7 May 2025 15:43:48 +0100
Subject: [PATCH] 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>
---
 eslint.config.js                              |   4 +-
 .../src/components/Carousel/Carousel.tsx      | 174 +++++++++---------
 .../Table/TableNG/Cells/HeaderCell.tsx        |   1 +
 .../Table/TableNG/Cells/RowExpander.tsx       |   2 +-
 .../ThemeSelector/ThemeSelectorDrawer.tsx     |   2 +
 .../edit-pane/DashboardEditPane.tsx           |  10 +-
 .../edit-pane/DashboardOutline.tsx            |   3 +-
 .../scene/layout-rows/RowItemRenderer.tsx     |   1 -
 .../scene/layout-tabs/TabItemRenderer.tsx     |   1 -
 .../new-toolbar/actions/ToolbarSwitch.tsx     |   7 +-
 .../variables/VariableSetEditableElement.tsx  |   2 +
 .../logs/components/LogRowMessage.tsx         |   5 +-
 .../logs/components/panel/LogLine.tsx         |   1 +
 13 files changed, 114 insertions(+), 99 deletions(-)

diff --git a/eslint.config.js b/eslint.config.js
index d33fae54961..4611b52e9ac 100644
--- 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',
         {
diff --git a/packages/grafana-ui/src/components/Carousel/Carousel.tsx b/packages/grafana-ui/src/components/Carousel/Carousel.tsx
index 17cf1efbf09..f6ce2b239a2 100644
--- 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,72 +98,88 @@ 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">
-          <IconButton
-            name="times"
-            aria-label={t('carousel.close', 'Close')}
-            size="xl"
-            onClick={closePreview}
-            className={cx(styles.closeButton)}
-          />
+        <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')}
+                size="xl"
+                onClick={closePreview}
+                className={cx(styles.closeButton)}
+              />
 
-          <IconButton
-            size="xl"
-            name="angle-left"
-            aria-label={t('carousel.previous', 'Previous')}
-            onClick={(e) => {
-              e.stopPropagation();
-              goToPrevious();
-            }}
-            className={cx(styles.navigationButton, styles.previousButton)}
-            data-testid="previous-button"
-          />
+              <IconButton
+                size="xl"
+                name="angle-left"
+                aria-label={t('carousel.previous', 'Previous')}
+                onClick={goToPrevious}
+                data-testid="previous-button"
+              />
 
-          <div
-            style={{ position: 'relative', maxWidth: '90%', maxHeight: '90%' }}
-            onClick={(e) => e.stopPropagation()}
-            data-testid="carousel-full-image"
-          >
-            <img
-              src={validImages[selectedIndex].path}
-              alt={validImages[selectedIndex].name}
-              onError={() => handleImageError(validImages[selectedIndex].path)}
-            />
-          </div>
+              <div data-testid="carousel-full-image">
+                <img
+                  className={styles.imagePreview}
+                  src={validImages[selectedIndex].path}
+                  alt={validImages[selectedIndex].name}
+                  onError={() => handleImageError(validImages[selectedIndex].path)}
+                />
+              </div>
 
-          <IconButton
-            size="xl"
-            name="angle-right"
-            aria-label={t('carousel.next', 'Next')}
-            onClick={(e) => {
-              e.stopPropagation();
-              goToNext();
-            }}
-            className={cx(styles.navigationButton, styles.nextButton)}
-            data-testid="next-button"
-          />
-        </div>
+              <IconButton
+                size="xl"
+                name="angle-right"
+                aria-label={t('carousel.next', 'Next')}
+                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),
   }),
 });
diff --git a/packages/grafana-ui/src/components/Table/TableNG/Cells/HeaderCell.tsx b/packages/grafana-ui/src/components/Table/TableNG/Cells/HeaderCell.tsx
index 5c53de65886..9537c502f09 100644
--- 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}
diff --git a/packages/grafana-ui/src/components/Table/TableNG/Cells/RowExpander.tsx b/packages/grafana-ui/src/components/Table/TableNG/Cells/RowExpander.tsx
index 37d6ba653b0..b02e660d77d 100644
--- 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
diff --git a/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx b/public/app/core/components/ThemeSelector/ThemeSelectorDrawer.tsx
index 32a2e869b90..3e9d3d3c5ad 100644
--- 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
diff --git a/public/app/features/dashboard-scene/edit-pane/DashboardEditPane.tsx b/public/app/features/dashboard-scene/edit-pane/DashboardEditPane.tsx
index 24847ad8a6e..7dee13bafa9 100644
--- 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}>
diff --git a/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx b/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx
index 725714df3d8..aa31caefaf2 100644
--- 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)}
diff --git a/public/app/features/dashboard-scene/scene/layout-rows/RowItemRenderer.tsx b/public/app/features/dashboard-scene/scene/layout-rows/RowItemRenderer.tsx
index a4de7c0a1ac..ccbc7bceda1 100644
--- 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
diff --git a/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx b/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx
index 178a6870967..68c8c33553f 100644
--- 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}
diff --git a/public/app/features/dashboard-scene/scene/new-toolbar/actions/ToolbarSwitch.tsx b/public/app/features/dashboard-scene/scene/new-toolbar/actions/ToolbarSwitch.tsx
index c057a50be2b..6d9bac314b3 100644
--- 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>
   );
 };
diff --git a/public/app/features/dashboard-scene/settings/variables/VariableSetEditableElement.tsx b/public/app/features/dashboard-scene/settings/variables/VariableSetEditableElement.tsx
index b52127ad95b..75dd1fce2d0 100644
--- 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>
diff --git a/public/app/features/logs/components/LogRowMessage.tsx b/public/app/features/logs/components/LogRowMessage.tsx
index d3788b3c7cf..f05fc96decd 100644
--- 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,
diff --git a/public/app/features/logs/components/panel/LogLine.tsx b/public/app/features/logs/components/panel/LogLine.tsx
index 53972f6ff7f..f39c97b2924 100644
--- 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