DashboardOutline: Improve markup semantics (#106008)

.betterer.results

@@ -1587,6 +1587,9 @@ exports[`better eslint`] = {
     "public/app/features/dashboard-scene/edit-pane/DashboardEditPaneRenderer.tsx:5381": [
       [0, 0, 0, "Do not use any type assertions.", "0"]
     ],
+    "public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx:5381": [
+      [0, 0, 0, "Do not use any type assertions.", "0"]
+    ],
     "public/app/features/dashboard-scene/inspect/HelpWizard/HelpWizard.tsx:5381": [
       [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "0"],
       [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "1"],

packages/grafana-ui/src/components/ElementSelectionContext/ElementSelectionContext.tsx

@@ -29,7 +29,7 @@ export const ElementSelectionContext = createContext<ElementSelectionContextStat
 export interface UseElementSelectionResult {
   isSelected?: boolean;
   isSelectable?: boolean;
-  onSelect?: (evt: React.PointerEvent, options?: ElementSelectionOnSelectOptions) => void;
+  onSelect?: (evt: React.MouseEvent, options?: ElementSelectionOnSelectOptions) => void;
   onClear?: () => void;
 }
 
@@ -45,7 +45,7 @@ export function useElementSelection(id: string | undefined): UseElementSelection
 
   const isSelected = context.selected.some((item) => item.id === id);
   const onSelect = useCallback(
-    (evt: React.PointerEvent, options: ElementSelectionOnSelectOptions = {}) => {
+    (evt: React.MouseEvent, options: ElementSelectionOnSelectOptions = {}) => {
       if (!context.enabled) {
         return;
       }

packages/grafana-ui/src/components/Text/Text.tsx

@@ -11,7 +11,7 @@ import { customWeight, customColor, customVariant } from './utils';
 
 export interface TextProps extends Omit<React.HTMLAttributes<HTMLElement>, 'className' | 'style'> {
   /** Defines what HTML element is defined underneath. "span" by default */
-  element?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'span' | 'p';
+  element?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'span' | 'p' | 'li';
   /** What typograpy variant should be used for the component. Only use if default variant for the defined element is not what is needed */
   variant?: keyof ThemeTypographyVariantTypes;
   /** Override the default weight for the used variant */

public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx

@@ -6,7 +6,7 @@ import { GrafanaTheme2 } from '@grafana/data';
 import { selectors } from '@grafana/e2e-selectors';
 import { Trans, useTranslate } from '@grafana/i18n';
 import { SceneObject } from '@grafana/scenes';
-import { Box, Icon, Text, useElementSelection, useStyles2, useTheme2 } from '@grafana/ui';
+import { Box, Icon, Text, useElementSelection, useStyles2 } from '@grafana/ui';
 
 import { DashboardGridItem } from '../scene/layout-default/DashboardGridItem';
 import { EditableDashboardElement } from '../scene/types/EditableDashboardElement';
@@ -25,41 +25,40 @@ export function DashboardOutline({ editPane }: Props) {
   const dashboard = getDashboardSceneFor(editPane);
 
   return (
-    <Box padding={1} gap={0} display="flex" direction="column">
+    <Box padding={1} gap={0} display="flex" direction="column" element="ul" role="tree" position="relative">
       <DashboardOutlineNode sceneObject={dashboard} editPane={editPane} depth={0} />
     </Box>
   );
 }
 
-function DashboardOutlineNode({
-  sceneObject,
-  editPane,
-  depth,
-}: {
+interface DashboardOutlineNodeProps {
   sceneObject: SceneObject;
   editPane: DashboardEditPane;
   depth: number;
-}) {
-  const [isCollapsed, setIsCollapsed] = useState(depth > 0);
-  const theme = useTheme2();
-  const { key } = sceneObject.useState();
+}
+
+function DashboardOutlineNode({ sceneObject, editPane, depth }: DashboardOutlineNodeProps) {
   const styles = useStyles2(getStyles);
+  const { key } = sceneObject.useState();
+  const [isCollapsed, setIsCollapsed] = useState(depth > 0);
   const { isSelected, onSelect } = useElementSelection(key);
   const isCloned = useMemo(() => isInCloneChain(key!), [key]);
   const editableElement = useMemo(() => getEditableElementFor(sceneObject)!, [sceneObject]);
   const { t } = useTranslate();
+  const noTitleText = t('dashboard.outline.tree-item.no-title', '<no title>');
 
   const children = sortBy(collectEditableElementChildren(sceneObject, [], 0), 'depth');
   const elementInfo = editableElement.getEditableElementInfo();
-  const noTitleText = t('dashboard.outline.tree-item.no-title', '<no title>');
   const instanceName = elementInfo.instanceName === '' ? noTitleText : elementInfo.instanceName;
   const elementCollapsed = editableElement.getCollapsedState?.();
   const outlineRename = useOutlineRename(editableElement);
 
-  const onNodeClicked = (evt: React.PointerEvent) => {
+  const onNodeClicked = (e: React.MouseEvent) => {
+    e.stopPropagation();
+
     // Only select via clicking outline never deselect
     if (!isSelected) {
-      onSelect?.(evt);
+      onSelect?.(e);
     }
 
     editableElement.scrollIntoView?.();
@@ -77,32 +76,34 @@ function DashboardOutlineNode({
 
   // Sync canvas element expanded state with outline element
   useEffect(() => {
-    if (elementCollapsed != null && elementCollapsed !== isCollapsed) {
+    if (elementCollapsed === !isCollapsed) {
       setIsCollapsed(elementCollapsed);
     }
   }, [isCollapsed, elementCollapsed]);
 
   return (
-    <>
-      <div
-        className={cx(styles.container, isSelected && styles.containerSelected)}
-        style={{ paddingLeft: theme.spacing(depth * 3) }}
-        onPointerDown={onNodeClicked}
-      >
+    // todo: add proper keyboard navigation
+    // eslint-disable-next-line jsx-a11y/click-events-have-key-events
+    <li
+      role="treeitem"
+      aria-selected={isSelected}
+      className={styles.container}
+      onClick={onNodeClicked}
+      style={{ '--depth': depth } as React.CSSProperties}
+    >
+      <div className={cx(styles.row, { [styles.rowSelected]: isSelected })}>
+        <div className={styles.indentation}></div>
         {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}
+            onClick={onToggleCollapse}
             data-testid={selectors.components.PanelEditor.Outline.node(instanceName)}
           >
-            <Icon name={!isCollapsed ? 'angle-down' : 'angle-right'} />
+            <Icon name={isCollapsed ? 'angle-right' : 'angle-down'} />
           </button>
         )}
         <button
-          className={cx(styles.nodeName, isCloned && styles.nodeNameClone)}
+          className={cx(styles.nodeName, { [styles.nodeNameClone]: isCloned })}
           onDoubleClick={outlineRename.onNameDoubleClicked}
           data-testid={selectors.components.PanelEditor.Outline.item(instanceName)}
         >
@@ -128,8 +129,7 @@ function DashboardOutlineNode({
       </div>
 
       {elementInfo.isContainer && !isCollapsed && (
-        <div className={styles.nodeChildren}>
-          <div className={styles.nodeChildrenLine} style={{ marginLeft: theme.spacing(depth * 3) }} />
+        <ul className={styles.nodeChildren} role="group">
           {children.length > 0 ? (
             children.map((child) => (
               <DashboardOutlineNode
@@ -140,13 +140,13 @@ function DashboardOutlineNode({
               />
             ))
           ) : (
-            <Text color="secondary">
+            <Text color="secondary" element="li">
               <Trans i18nKey="dashboard.outline.tree-item.empty">(empty)</Trans>
             </Text>
           )}
-        </div>
+        </ul>
       )}
-    </>
+    </li>
   );
 }
 
@@ -155,29 +155,35 @@ function getStyles(theme: GrafanaTheme2) {
     container: css({
       display: 'flex',
       gap: theme.spacing(0.5),
-      alignItems: 'center',
       flexGrow: 1,
+      flexDirection: 'column',
       borderRadius: theme.shape.radius.default,
-      position: 'relative',
-      marginBottom: theme.spacing(0.25),
       color: theme.colors.text.secondary,
-      '&:hover': {
-        color: theme.colors.text.primary,
-        outline: `1px dashed ${theme.colors.border.strong}`,
-        outlineOffset: '0px',
-        backgroundColor: theme.colors.emphasize(theme.colors.background.primary, 0.05),
-      },
     }),
     containerSelected: css({
       outline: `1px dashed ${theme.colors.primary.border} !important`,
       outlineOffset: '0px',
       color: theme.colors.text.primary,
+    }),
+    row: css({
+      display: 'flex',
+      gap: theme.spacing(0.5),
+      borderRadius: theme.shape.radius.default,
 
       '&:hover': {
-        outline: `1px dashed ${theme.colors.primary.border}`,
         color: theme.colors.text.primary,
+        outline: `1px dashed ${theme.colors.border.strong}`,
+        backgroundColor: theme.colors.emphasize(theme.colors.background.primary, 0.05),
       },
     }),
+    rowSelected: css({
+      color: theme.colors.text.primary,
+      outline: `1px dashed ${theme.colors.primary.border} !important`,
+      backgroundColor: theme.colors.emphasize(theme.colors.background.primary, 0.05),
+    }),
+    indentation: css({
+      marginLeft: `calc(var(--depth) * ${theme.spacing(3)})`,
+    }),
     angleButton: css({
       boxShadow: 'none',
       border: 'none',
@@ -191,7 +197,7 @@ function getStyles(theme: GrafanaTheme2) {
       boxShadow: 'none',
       border: 'none',
       background: 'transparent',
-      padding: theme.spacing(0.25, 1, 0.25, 0),
+      padding: 0,
       borderRadius: theme.shape.radius.default,
       color: 'inherit',
       display: 'flex',
@@ -227,14 +233,19 @@ function getStyles(theme: GrafanaTheme2) {
       display: 'flex',
       flexDirection: 'column',
       position: 'relative',
-    }),
-    nodeChildrenLine: css({
-      position: 'absolute',
-      width: '1px',
-      height: '100%',
-      left: '7px',
-      zIndex: 1,
-      backgroundColor: theme.colors.border.weak,
+      gap: theme.spacing(0.5),
+
+      // tree line
+      '&::before': {
+        content: '""',
+        position: 'absolute',
+        width: '1px',
+        height: '100%',
+        pointerEvents: 'none',
+        zIndex: 1,
+        background: theme.colors.border.weak,
+        marginLeft: `calc(11px + ${theme.spacing(3)} * var(--depth))`,
+      },
     }),
   };
 }