From 59067a72b8b254b4bc9512d091a062c1815dbb4c Mon Sep 17 00:00:00 2001
From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com>
Date: Thu, 19 Aug 2021 10:56:01 -0400
Subject: [PATCH] Template variables: Keyboard navigation improvements (#38001)
 (#38074)

* Fix variable labels

* Add proper labeling for input

* Add ids to PickerRenderer

* Fix tests

* Update PR feedback

* OptionsPicker: Change to id

* Inherit aria attributes

* Add checkbox role

* Fix typo

* Add proper label reference

* Update role and label

* Prevent spreadng non-DOM attributes

* Move form layout to other component

* Remove haspopup

* Add testid to selector

* Add HTMLProps extension

* Use list

* Move styles outside of class

* Add cx

(cherry picked from commit 1f091c448fc85d1248b34af818427b1350860bc9)

Co-authored-by: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com>
---
 .../src/selectors/pages.ts                    | 10 ++--
 .../components/SubMenu/DashboardLinks.tsx     |  6 +--
 .../dashboard/components/SubMenu/SubMenu.tsx  | 11 +++-
 .../components/SubMenu/SubMenuItems.tsx       |  3 +-
 .../OptionsPicker/OptionPicker.test.tsx       |  4 +-
 .../pickers/OptionsPicker/OptionsPicker.tsx   | 15 +++++-
 .../variables/pickers/PickerRenderer.tsx      |  6 ++-
 .../pickers/shared/VariableInput.tsx          |  7 ++-
 .../variables/pickers/shared/VariableLink.tsx | 22 +++++---
 .../pickers/shared/VariableOptions.tsx        | 53 ++++++++++++++-----
 .../textbox/TextBoxVariablePicker.tsx         | 12 ++++-
 11 files changed, 110 insertions(+), 39 deletions(-)

diff --git a/packages/grafana-e2e-selectors/src/selectors/pages.ts b/packages/grafana-e2e-selectors/src/selectors/pages.ts
index a99217651a6..0f1de6e0611 100644
--- a/packages/grafana-e2e-selectors/src/selectors/pages.ts
+++ b/packages/grafana-e2e-selectors/src/selectors/pages.ts
@@ -42,13 +42,13 @@ export const Pages = {
     },
     SubMenu: {
       submenu: 'Dashboard submenu',
-      submenuItem: 'Dashboard template variables submenu item',
-      submenuItemLabels: (item: string) => `Dashboard template variables submenu Label ${item}`,
+      submenuItem: 'data-testid template variable',
+      submenuItemLabels: (item: string) => `data-testid Dashboard template variables submenu Label ${item}`,
       submenuItemValueDropDownValueLinkTexts: (item: string) =>
-        `Dashboard template variables Variable Value DropDown value link text ${item}`,
-      submenuItemValueDropDownDropDown: 'Dashboard template variables Variable Value DropDown DropDown',
+        `data-testid Dashboard template variables Variable Value DropDown value link text ${item}`,
+      submenuItemValueDropDownDropDown: 'Variable options',
       submenuItemValueDropDownOptionTexts: (item: string) =>
-        `Dashboard template variables Variable Value DropDown option text ${item}`,
+        `data-testid Dashboard template variables Variable Value DropDown option text ${item}`,
     },
     Settings: {
       General: {
diff --git a/public/app/features/dashboard/components/SubMenu/DashboardLinks.tsx b/public/app/features/dashboard/components/SubMenu/DashboardLinks.tsx
index 96a46604631..196e47f71f7 100644
--- a/public/app/features/dashboard/components/SubMenu/DashboardLinks.tsx
+++ b/public/app/features/dashboard/components/SubMenu/DashboardLinks.tsx
@@ -44,15 +44,15 @@ export const DashboardLinks: FC<Props> = ({ dashboard, links }) => {
             href={sanitizeUrl(linkInfo.href)}
             target={link.targetBlank ? '_blank' : undefined}
             rel="noreferrer"
-            aria-label={selectors.components.DashboardLinks.link}
+            data-testid={selectors.components.DashboardLinks.link}
           >
-            <Icon name={linkIconMap[link.icon] as IconName} style={{ marginRight: '4px' }} />
+            <Icon aria-hidden name={linkIconMap[link.icon] as IconName} style={{ marginRight: '4px' }} />
             <span>{linkInfo.title}</span>
           </a>
         );
 
         return (
-          <div key={key} className="gf-form" aria-label={selectors.components.DashboardLinks.container}>
+          <div key={key} className="gf-form" data-testid={selectors.components.DashboardLinks.container}>
             {link.tooltip ? <Tooltip content={linkInfo.tooltip}>{linkElement}</Tooltip> : linkElement}
           </div>
         );
diff --git a/public/app/features/dashboard/components/SubMenu/SubMenu.tsx b/public/app/features/dashboard/components/SubMenu/SubMenu.tsx
index 2fe5da1470e..01aeca07e61 100644
--- a/public/app/features/dashboard/components/SubMenu/SubMenu.tsx
+++ b/public/app/features/dashboard/components/SubMenu/SubMenu.tsx
@@ -9,6 +9,7 @@ import { Annotations } from './Annotations';
 import { SubMenuItems } from './SubMenuItems';
 import { DashboardLink } from '../../state/DashboardModel';
 import { AnnotationQuery } from '@grafana/data';
+import { css } from '@emotion/css';
 
 interface OwnProps {
   dashboard: DashboardModel;
@@ -47,7 +48,9 @@ class SubMenuUnConnected extends PureComponent<Props> {
 
     return (
       <div className="submenu-controls">
-        <SubMenuItems variables={variables} />
+        <form aria-label="Template variables" className={styles}>
+          <SubMenuItems variables={variables} />
+        </form>
         <Annotations
           annotations={annotations}
           onAnnotationChanged={this.onAnnotationStateChanged}
@@ -67,6 +70,12 @@ const mapStateToProps: MapStateToProps<ConnectedProps, OwnProps, StoreState> = (
   };
 };
 
+const styles = css`
+  display: flex;
+  flex-wrap: wrap;
+  display: contents;
+`;
+
 export const SubMenu = connect(mapStateToProps)(SubMenuUnConnected);
 
 SubMenu.displayName = 'SubMenu';
diff --git a/public/app/features/dashboard/components/SubMenu/SubMenuItems.tsx b/public/app/features/dashboard/components/SubMenu/SubMenuItems.tsx
index ce20a183572..72f15ba4db8 100644
--- a/public/app/features/dashboard/components/SubMenu/SubMenuItems.tsx
+++ b/public/app/features/dashboard/components/SubMenu/SubMenuItems.tsx
@@ -9,6 +9,7 @@ interface Props {
 
 export const SubMenuItems: FunctionComponent<Props> = ({ variables }) => {
   const [visibleVariables, setVisibleVariables] = useState<VariableModel[]>([]);
+
   useEffect(() => {
     setVisibleVariables(variables.filter((state) => state.hide !== VariableHide.hideVariable));
   }, [variables]);
@@ -24,7 +25,7 @@ export const SubMenuItems: FunctionComponent<Props> = ({ variables }) => {
           <div
             key={variable.id}
             className="submenu-item gf-form-inline"
-            aria-label={selectors.pages.Dashboard.SubMenu.submenuItem}
+            data-testid={selectors.pages.Dashboard.SubMenu.submenuItem}
           >
             <PickerRenderer variable={variable} />
           </div>
diff --git a/public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx b/public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx
index f697ed09932..075908f023d 100644
--- a/public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx
+++ b/public/app/features/variables/pickers/OptionsPicker/OptionPicker.test.tsx
@@ -56,11 +56,11 @@ function setupTestContext({ pickerState = {}, variable = {} }: Args = {}) {
 }
 
 function getSubMenu(text: string) {
-  return screen.getByLabelText(selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts(text));
+  return screen.getByTestId(selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts(text));
 }
 
 function getOption(text: string) {
-  return screen.getByLabelText(selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownOptionTexts('A'));
+  return screen.getByTestId(selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownOptionTexts('A'));
 }
 
 describe('OptionPicker', () => {
diff --git a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx
index edf577eb68a..9c082a44658 100644
--- a/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx
+++ b/public/app/features/variables/pickers/OptionsPicker/OptionsPicker.tsx
@@ -75,7 +75,15 @@ export const optionPickerFactory = <Model extends VariableWithOptions | Variable
       const linkText = formatVariableLabel(variable);
       const loading = variable.state === LoadingState.Loading;
 
-      return <VariableLink text={linkText} onClick={this.onShowOptions} loading={loading} onCancel={this.onCancel} />;
+      return (
+        <VariableLink
+          id={variable.id}
+          text={linkText}
+          onClick={this.onShowOptions}
+          loading={loading}
+          onCancel={this.onCancel}
+        />
+      );
     }
 
     onCancel = () => {
@@ -83,12 +91,16 @@ export const optionPickerFactory = <Model extends VariableWithOptions | Variable
     };
 
     renderOptions(picker: OptionsPickerState) {
+      const { id } = this.props.variable;
       return (
         <ClickOutsideWrapper onClick={this.onHideOptions}>
           <VariableInput
+            id={id}
             value={picker.queryValue}
             onChange={this.props.filterOrSearchOptions}
             onNavigate={this.props.navigateOptions}
+            aria-expanded={true}
+            aria-controls={`options-${id}`}
           />
           <VariableOptions
             values={picker.options}
@@ -97,6 +109,7 @@ export const optionPickerFactory = <Model extends VariableWithOptions | Variable
             highlightIndex={picker.highlightIndex}
             multi={picker.multi}
             selectedValues={picker.selectedValues}
+            id={`options-${id}`}
           />
         </ClickOutsideWrapper>
       );
diff --git a/public/app/features/variables/pickers/PickerRenderer.tsx b/public/app/features/variables/pickers/PickerRenderer.tsx
index 2c67d3c4c50..7defe8ceb0b 100644
--- a/public/app/features/variables/pickers/PickerRenderer.tsx
+++ b/public/app/features/variables/pickers/PickerRenderer.tsx
@@ -37,7 +37,8 @@ function PickerLabel({ variable }: PropsWithChildren<Props>): ReactElement | nul
       <Tooltip content={variable.description} placement={'bottom'}>
         <label
           className="gf-form-label gf-form-label--variable"
-          aria-label={selectors.pages.Dashboard.SubMenu.submenuItemLabels(labelOrName)}
+          data-testid={selectors.pages.Dashboard.SubMenu.submenuItemLabels(labelOrName)}
+          htmlFor={variable.id}
         >
           {labelOrName}
         </label>
@@ -48,7 +49,8 @@ function PickerLabel({ variable }: PropsWithChildren<Props>): ReactElement | nul
   return (
     <label
       className="gf-form-label gf-form-label--variable"
-      aria-label={selectors.pages.Dashboard.SubMenu.submenuItemLabels(labelOrName)}
+      data-testid={selectors.pages.Dashboard.SubMenu.submenuItemLabels(labelOrName)}
+      htmlFor={variable.id}
     >
       {labelOrName}
     </label>
diff --git a/public/app/features/variables/pickers/shared/VariableInput.tsx b/public/app/features/variables/pickers/shared/VariableInput.tsx
index 51d1c040957..4ba293aac49 100644
--- a/public/app/features/variables/pickers/shared/VariableInput.tsx
+++ b/public/app/features/variables/pickers/shared/VariableInput.tsx
@@ -1,7 +1,7 @@
 import React, { PureComponent } from 'react';
 import { NavigationKey } from '../types';
 
-export interface Props {
+export interface Props extends Omit<React.HTMLProps<HTMLInputElement>, 'onChange' | 'value'> {
   onChange: (value: string) => void;
   onNavigate: (key: NavigationKey, clearOthers: boolean) => void;
   value: string | null;
@@ -21,8 +21,10 @@ export class VariableInput extends PureComponent<Props> {
   };
 
   render() {
+    const { value, id, onNavigate, ...restProps } = this.props;
     return (
       <input
+        {...restProps}
         ref={(instance) => {
           if (instance) {
             instance.focus();
@@ -31,9 +33,10 @@ export class VariableInput extends PureComponent<Props> {
         }}
         type="text"
         className="gf-form-input"
-        value={this.props.value ?? ''}
+        value={value ?? ''}
         onChange={this.onChange}
         onKeyDown={this.onKeyDown}
+        placeholder="Enter variable value"
       />
     );
   }
diff --git a/public/app/features/variables/pickers/shared/VariableLink.tsx b/public/app/features/variables/pickers/shared/VariableLink.tsx
index 70bb9bfb3de..e50fda6d2a5 100644
--- a/public/app/features/variables/pickers/shared/VariableLink.tsx
+++ b/public/app/features/variables/pickers/shared/VariableLink.tsx
@@ -9,12 +9,16 @@ interface Props {
   text: string;
   loading: boolean;
   onCancel: () => void;
+  /**
+   *  htmlFor, needed for the label
+   */
+  id: string;
 }
 
-export const VariableLink: FC<Props> = ({ loading, onClick: propsOnClick, text, onCancel }) => {
+export const VariableLink: FC<Props> = ({ loading, onClick: propsOnClick, text, onCancel, id }) => {
   const styles = useStyles(getStyles);
   const onClick = useCallback(
-    (event: MouseEvent<HTMLAnchorElement>) => {
+    (event: MouseEvent<HTMLButtonElement>) => {
       event.stopPropagation();
       event.preventDefault();
       propsOnClick();
@@ -26,8 +30,9 @@ export const VariableLink: FC<Props> = ({ loading, onClick: propsOnClick, text,
     return (
       <div
         className={styles.container}
-        aria-label={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts(`${text}`)}
+        data-testid={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts(`${text}`)}
         title={text}
+        id={id}
       >
         <VariableLinkText text={text} />
         <LoadingIndicator onCancel={onCancel} />
@@ -36,15 +41,18 @@ export const VariableLink: FC<Props> = ({ loading, onClick: propsOnClick, text,
   }
 
   return (
-    <a
+    <button
       onClick={onClick}
       className={styles.container}
-      aria-label={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts(`${text}`)}
+      data-testid={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownValueLinkTexts(`${text}`)}
+      aria-expanded={false}
+      aria-controls={`options-${id}`}
+      id={id}
       title={text}
     >
       <VariableLinkText text={text} />
-      <Icon name="angle-down" size="sm" />
-    </a>
+      <Icon aria-hidden name="angle-down" size="sm" />
+    </button>
   );
 };
 
diff --git a/public/app/features/variables/pickers/shared/VariableOptions.tsx b/public/app/features/variables/pickers/shared/VariableOptions.tsx
index 4af10003d00..2c6dc2dd19d 100644
--- a/public/app/features/variables/pickers/shared/VariableOptions.tsx
+++ b/public/app/features/variables/pickers/shared/VariableOptions.tsx
@@ -3,14 +3,19 @@ import { Tooltip } from '@grafana/ui';
 import { selectors } from '@grafana/e2e-selectors';
 
 import { VariableOption } from '../../types';
+import { css, cx } from '@emotion/css';
 
-export interface Props {
+export interface Props extends React.HTMLProps<HTMLUListElement> {
   multi: boolean;
   values: VariableOption[];
   selectedValues: VariableOption[];
   highlightIndex: number;
   onToggle: (option: VariableOption, clearOthers: boolean) => void;
   onToggleAll: () => void;
+  /**
+   * Used for aria-controls
+   */
+  id: string;
 }
 
 export class VariableOptions extends PureComponent<Props> {
@@ -31,18 +36,20 @@ export class VariableOptions extends PureComponent<Props> {
   }
 
   render() {
-    const { multi, values } = this.props;
+    // Don't want to pass faulty rest props to the div
+    const { multi, values, highlightIndex, selectedValues, onToggle, onToggleAll, ...restProps } = this.props;
 
     return (
-      <div
-        className={`${multi ? 'variable-value-dropdown multi' : 'variable-value-dropdown single'}`}
-        aria-label={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownDropDown}
-      >
+      <div className={`${multi ? 'variable-value-dropdown multi' : 'variable-value-dropdown single'}`}>
         <div className="variable-options-wrapper">
-          <div className="variable-options-column">
+          <ul
+            className={listStyles}
+            aria-label={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownDropDown}
+            {...restProps}
+          >
             {this.renderMultiToggle()}
             {values.map((option, index) => this.renderOption(option, index))}
-          </div>
+          </ul>
         </div>
       </div>
     );
@@ -54,12 +61,20 @@ export class VariableOptions extends PureComponent<Props> {
     const highlightClass = index === highlightIndex ? `${selectClass} highlighted` : selectClass;
 
     return (
-      <a key={`${option.value}`} className={highlightClass} onClick={this.onToggle(option)}>
-        <span className="variable-option-icon"></span>
-        <span aria-label={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownOptionTexts(`${option.text}`)}>
-          {option.text}
-        </span>
-      </a>
+      <li>
+        <a
+          key={`${option.value}`}
+          role="checkbox"
+          aria-checked={option.selected}
+          className={highlightClass}
+          onClick={this.onToggle(option)}
+        >
+          <span className="variable-option-icon"></span>
+          <span data-testid={selectors.pages.Dashboard.SubMenu.submenuItemValueDropDownOptionTexts(`${option.text}`)}>
+            {option.text}
+          </span>
+        </a>
+      </li>
     );
   }
 
@@ -78,7 +93,10 @@ export class VariableOptions extends PureComponent<Props> {
               ? 'variable-options-column-header many-selected'
               : 'variable-options-column-header'
           }`}
+          role="checkbox"
+          aria-checked={selectedValues.length > 1 ? 'mixed' : 'false'}
           onClick={this.onToggleAll}
+          aria-label="Toggle all values"
           data-placement="top"
         >
           <span className="variable-option-icon"></span>
@@ -88,3 +106,10 @@ export class VariableOptions extends PureComponent<Props> {
     );
   }
 }
+
+const listStyles = cx(
+  'variable-options-column',
+  css`
+    list-style-type: none;
+  `
+);
diff --git a/public/app/features/variables/textbox/TextBoxVariablePicker.tsx b/public/app/features/variables/textbox/TextBoxVariablePicker.tsx
index 33fc06f52d4..88ea0c7a501 100644
--- a/public/app/features/variables/textbox/TextBoxVariablePicker.tsx
+++ b/public/app/features/variables/textbox/TextBoxVariablePicker.tsx
@@ -50,5 +50,15 @@ export function TextBoxVariablePicker({ variable, onVariableChange }: Props): Re
     }
   };
 
-  return <Input type="text" value={updatedValue} onChange={onChange} onBlur={onBlur} onKeyDown={onKeyDown} />;
+  return (
+    <Input
+      type="text"
+      value={updatedValue}
+      onChange={onChange}
+      onBlur={onBlur}
+      onKeyDown={onKeyDown}
+      placeholder="Enter variable value"
+      id={variable.id}
+    />
+  );
 }