TimeRangePicker: Accessiblity improvements (#36912)

* Add low hanging aria markup * Add a list of radio buttons * Add ref to put focus * Add landmark markup * Add labels to icon buttons * Add radio button groupings * Add keyboard focus styles * Improve markup * Update test selector * Remove testid in favor of text * Fiddle around with test labels * Get correct testid selector * Add aria-expanded to button * Change order of quick select for keyboard nav * Create headings for easier heading navigation * Add another h3 * Add testid * Use selectors in e2e test * Add dataid selector for narrow absolute button * Update e2e test with proper selectors * Add more verbose aria-label * Change editor to picker
2021-08-04 09:53:52 +02:00
parent 0fc7dffee8
commit aba7013ce5
13 changed files with 146 additions and 66 deletions
@@ -6,11 +6,12 @@
 // prefix your selector string with 'data-test-id' so that when create the selectors we know to search for it on the right attribute
 export const Components = {
  TimePicker: {
-    openButton: 'TimePicker Open Button',
+    openButton: 'data-testid TimePicker Open Button',
    fromField: 'TimePicker from field',
    toField: 'TimePicker to field',
-    applyTimeRange: 'TimePicker submit button',
+    applyTimeRange: 'data-testid TimePicker submit button',
    calendar: 'TimePicker calendar',
+    absoluteTimeRangeTitle: 'data-testid-absolute-time-range-narrow',
  },
  DataSource: {
    TestData: {
@@ -8,7 +8,7 @@ export interface TimeRangeConfig {
 }

 export const setTimeRange = ({ from, to, zone }: TimeRangeConfig) => {
-  e2e().get('[aria-label="TimePicker Open Button"]').click();
+  e2e.components.TimePicker.openButton().click();

  if (zone) {
    e2e().contains('button', 'Change time zone').click();
@@ -21,9 +21,10 @@ export const setTimeRange = ({ from, to, zone }: TimeRangeConfig) => {
  }

  // For smaller screens
-  e2e().get('[aria-label="TimePicker absolute time range"]').click();
+  e2e.components.TimePicker.absoluteTimeRangeTitle().click();

-  e2e().get('[aria-label="TimePicker from field"]').clear().type(from);
-  e2e().get('[aria-label="TimePicker to field"]').clear().type(to);
-  e2e().get('[aria-label="TimePicker submit button"]').click();
+  e2e.components.TimePicker.fromField().clear().type(from);
+  e2e.components.TimePicker.toField().clear().type(to);
+
+  e2e.components.TimePicker.applyTimeRange().click();
 };
@@ -69,7 +69,13 @@ export const ToolbarButton = forwardRef<HTMLButtonElement, Props>(
    });

    const body = (
-      <button ref={ref} className={buttonStyles} aria-label={getButttonAriaLabel(ariaLabel, tooltip)} {...rest}>
+      <button
+        ref={ref}
+        className={buttonStyles}
+        aria-label={getButttonAriaLabel(ariaLabel, tooltip)}
+        aria-expanded={isOpen}
+        {...rest}
+      >
        {renderIcon(icon)}
        {imgSrc && <img className={styles.img} src={imgSrc} />}
        {children && !iconOnly && <div className={contentStyles}>{children}</div>}
@@ -26,6 +26,6 @@ describe('TimePicker', () => {
      />
    );

-    expect(container.queryByLabelText(/timepicker open button/i)).toBeInTheDocument();
+    expect(container.queryByLabelText(/Time range picker/i)).toBeInTheDocument();
  });
 });
@@ -25,6 +25,7 @@ import {
 import { Themeable } from '../../types';
 import { otherOptions, quickOptions } from './rangeOptions';
 import { ButtonGroup, ToolbarButton } from '../Button';
+import { selectors } from '@grafana/e2e-selectors';

 /** @public */
 export interface TimeRangePickerProps extends Themeable {
@@ -89,11 +90,21 @@ export class UnthemedTimeRangePicker extends PureComponent<TimeRangePickerProps,

    return (
      <ButtonGroup className={styles.container}>
-        {hasAbsolute && <ToolbarButton variant={variant} onClick={onMoveBackward} icon="angle-left" narrow />}
+        {hasAbsolute && (
+          <ToolbarButton
+            aria-label="Move time range backwards"
+            variant={variant}
+            onClick={onMoveBackward}
+            icon="angle-left"
+            narrow
+          />
+        )}

        <Tooltip content={<TimePickerTooltip timeRange={value} timeZone={timeZone} />} placement="bottom">
          <ToolbarButton
-            aria-label="TimePicker Open Button"
+            data-testid={selectors.components.TimePicker.openButton}
+            aria-label={`Time range picker with current time range ${formattedRange(value, timeZone)} selected`}
+            aria-controls="TimePickerContent"
            onClick={this.onOpen}
            icon="clock-nine"
            isOpen={isOpen}
@@ -120,10 +131,18 @@ export class UnthemedTimeRangePicker extends PureComponent<TimeRangePickerProps,

        {timeSyncButton}

-        {hasAbsolute && <ToolbarButton onClick={onMoveForward} icon="angle-right" narrow variant={variant} />}
+        {hasAbsolute && (
+          <ToolbarButton
+            aria-label="Move time range forwards"
+            onClick={onMoveForward}
+            icon="angle-right"
+            narrow
+            variant={variant}
+          />
+        )}

        <Tooltip content={ZoomOutTooltip} placement="bottom">
-          <ToolbarButton onClick={onZoom} icon="search-minus" variant={variant} />
+          <ToolbarButton aria-label="Zoom out time range" onClick={onZoom} icon="search-minus" variant={variant} />
        </Tooltip>
      </ButtonGroup>
    );
@@ -210,13 +210,13 @@ export const TimePickerCalendar = memo<Props>((props) => {
  if (isFullscreen) {
    return (
      <ClickOutsideWrapper onClick={props.onClose}>
-        <div
+        <section
          className={styles.container}
          onClick={stopPropagation}
          aria-label={selectors.components.TimePicker.calendar}
        >
          <Body {...props} />
-        </div>
+        </section>
      </ClickOutsideWrapper>
    );
  }
@@ -10,6 +10,8 @@ import { TimePickerTitle } from './TimePickerTitle';
 import { TimeRangeForm } from './TimeRangeForm';
 import { TimeRangeList } from './TimeRangeList';
 import { TimePickerFooter } from './TimePickerFooter';
+import { getFocusStyles } from '../../../themes/mixins';
+import { selectors } from '@grafana/e2e-selectors';

 const getStyles = stylesFactory((theme: GrafanaTheme2, isReversed, hideQuickRanges, isContainerTall) => {
  return {
@@ -30,6 +32,7 @@ const getStyles = stylesFactory((theme: GrafanaTheme2, isReversed, hideQuickRang
    `,
    body: css`
      display: flex;
+      flex-direction: row-reverse;
      height: ${isContainerTall ? '381px' : '217px'};
    `,
    leftSide: css`
@@ -64,6 +67,16 @@ const getNarrowScreenStyles = stylesFactory((theme: GrafanaTheme2) => {
      border-bottom: 1px solid ${theme.colors.border.weak};
      padding: 7px 9px 7px 9px;
    `,
+    expandButton: css`
+      background-color: transparent;
+      border: none;
+      display: flex;
+      width: 100%;
+
+      &:focus-visible {
+        ${getFocusStyles(theme)}
+      }
+    `,
    body: css`
      border-bottom: 1px solid ${theme.colors.border.weak};
    `,
@@ -163,13 +176,8 @@ export const TimePickerContentWithScreenSize: React.FC<PropsWithScreenSize> = (p
  };

  return (
-    <div className={cx(styles.container, className)}>
+    <div id="TimePickerContent" className={cx(styles.container, className)}>
      <div className={styles.body}>
-        {isFullscreen && (
-          <div className={styles.leftSide}>
-            <FullScreenForm {...props} historyOptions={historyOptions} />
-          </div>
-        )}
        {(!isFullscreen || !hideQuickRanges) && (
          <CustomScrollbar className={styles.rightSide}>
            {!isFullscreen && <NarrowScreenForm {...props} historyOptions={historyOptions} />}
@@ -192,6 +200,11 @@ export const TimePickerContentWithScreenSize: React.FC<PropsWithScreenSize> = (p
            )}
          </CustomScrollbar>
        )}
+        {isFullscreen && (
+          <div className={styles.leftSide}>
+            <FullScreenForm {...props} historyOptions={historyOptions} />
+          </div>
+        )}
      </div>
      {!hideTimeZone && isFullscreen && <TimePickerFooter timeZone={timeZone} onChangeTimeZone={onChangeTimeZone} />}
    </div>
@@ -218,21 +231,25 @@ const NarrowScreenForm: React.FC<FormProps> = (props) => {
  };

  return (
-    <>
-      <div
-        aria-label="TimePicker absolute time range"
-        className={styles.header}
-        onClick={() => {
-          if (!hideQuickRanges) {
-            setCollapsedFlag(!collapsed);
-          }
-        }}
-      >
-        <TimePickerTitle>Absolute time range</TimePickerTitle>
-        {!hideQuickRanges && <Icon name={!collapsed ? 'angle-up' : 'angle-down'} />}
-      </div>
+    <fieldset>
+      <h3 className={styles.header}>
+        <button
+          className={styles.expandButton}
+          onClick={() => {
+            if (!hideQuickRanges) {
+              setCollapsedFlag(!collapsed);
+            }
+          }}
+          data-testid={selectors.components.TimePicker.absoluteTimeRangeTitle}
+          aria-expanded={!collapsed}
+          aria-controls="expanded-timerange"
+        >
+          <TimePickerTitle>Absolute time range</TimePickerTitle>
+          {!hideQuickRanges && <Icon name={!collapsed ? 'angle-up' : 'angle-down'} />}
+        </button>
+      </h3>
      {!collapsed && (
-        <div className={styles.body}>
+        <div className={styles.body} id="expanded-timerange">
          <div className={styles.form}>
            <TimeRangeForm value={value} onApply={onChange} timeZone={timeZone} isFullscreen={false} />
          </div>
@@ -246,7 +263,7 @@ const NarrowScreenForm: React.FC<FormProps> = (props) => {
          )}
        </div>
      )}
-    </>
+    </fieldset>
  );
 };

@@ -261,9 +278,9 @@ const FullScreenForm: React.FC<FormProps> = (props) => {
  return (
    <>
      <div className={styles.container}>
-        <div aria-label="TimePicker absolute time range" className={styles.title}>
+        <h3 className={styles.title} data-testid={selectors.components.TimePicker.absoluteTimeRangeTitle}>
          <TimePickerTitle>Absolute time range</TimePickerTitle>
-        </div>
+        </h3>
        <TimeRangeForm
          value={props.value}
          timeZone={props.timeZone}
@@ -46,7 +46,7 @@ export const TimePickerFooter: FC<Props> = (props) => {
  if (isEditing) {
    return (
      <div className={cx(style.container, style.editContainer)}>
-        <div aria-label={selectors.components.TimeZonePicker.container} className={style.timeZoneContainer}>
+        <section aria-label={selectors.components.TimeZonePicker.container} className={style.timeZoneContainer}>
          <TimeZonePicker
            includeInternal={true}
            onChange={(timeZone) => {
@@ -59,13 +59,13 @@ export const TimePickerFooter: FC<Props> = (props) => {
            autoFocus={true}
            onBlur={onToggleChangeTz}
          />
-        </div>
+        </section>
      </div>
    );
  }

  return (
-    <div className={style.container}>
+    <section aria-label="Time zone selection" className={style.container}>
      <div className={style.timeZoneContainer}>
        <div className={style.timeZone}>
          <TimeZoneTitle title={info.name} />
@@ -78,7 +78,7 @@ export const TimePickerFooter: FC<Props> = (props) => {
      <Button variant="secondary" onClick={onToggleChangeTz} size="sm">
        Change time zone
      </Button>
-    </div>
+    </section>
  );
 };

@@ -9,6 +9,8 @@ const getStyle = stylesFactory((theme: GrafanaTheme) => {
      font-size: ${theme.typography.size.md};
      font-weight: ${theme.typography.weight.semibold};
      color: ${theme.colors.formLabel};
+      margin: 0;
+      display: flex;
    `,
  };
 });
@@ -17,7 +19,7 @@ export const TimePickerTitle = memo<PropsWithChildren<{}>>(({ children }) => {
  const theme = useTheme();
  const styles = getStyle(theme);

-  return <span className={styles.text}>{children}</span>;
+  return <h3 className={styles.text}>{children}</h3>;
 });

 TimePickerTitle.displayName = 'TimePickerTitle';
@@ -31,10 +31,10 @@ function setup(initial: TimeRange = defaultTimeRange, timeZone = 'utc'): TimeRan

 describe('TimeRangeForm', () => {
  it('should render form correcty', () => {
-    const { getByLabelText } = setup();
+    const { getByLabelText, getByText } = setup();
    const { TimePicker } = selectors.components;

-    expect(getByLabelText(TimePicker.applyTimeRange)).toBeInTheDocument();
+    expect(getByText('Apply time range')).toBeInTheDocument();
    expect(getByLabelText(TimePicker.fromField)).toBeInTheDocument();
    expect(getByLabelText(TimePicker.toField)).toBeInTheDocument();
  });
@@ -96,7 +96,7 @@ export const TimeRangeForm: React.FC<Props> = (props) => {
  const icon = isFullscreen ? null : <Button icon="calendar-alt" variant="secondary" onClick={onOpen} />;

  return (
-    <>
+    <form aria-label="Absolute time ranges">
      <Field label="From" invalid={from.invalid} error={from.errorMessage}>
        <Input
          onClick={(event) => event.stopPropagation()}
@@ -117,7 +117,7 @@ export const TimeRangeForm: React.FC<Props> = (props) => {
          value={to.value}
        />
      </Field>
-      <Button aria-label={selectors.components.TimePicker.applyTimeRange} onClick={onApply}>
+      <Button data-testid={selectors.components.TimePicker.applyTimeRange} onClick={onApply}>
        Apply time range
      </Button>

@@ -132,7 +132,7 @@ export const TimeRangeForm: React.FC<Props> = (props) => {
        timeZone={timeZone}
        isReversed={isReversed}
      />
-    </>
+    </form>
  );
 };

@@ -26,7 +26,7 @@ const getOptionsStyles = stylesFactory(() => {
 });

 interface Props {
-  title?: string;
+  title: string;
  options: TimeOption[];
  value?: TimeOption;
  onChange: (option: TimeOption) => void;
@@ -46,30 +46,35 @@ export const TimeRangeList: React.FC<Props> = (props) => {
  }

  return (
-    <>
-      <div className={styles.title}>
-        <TimePickerTitle>{title}</TimePickerTitle>
-      </div>
-      <Options {...props} />
-    </>
+    <section aria-label={title}>
+      <form>
+        <fieldset>
+          <div className={styles.title}>
+            <TimePickerTitle>{title}</TimePickerTitle>
+          </div>
+          <Options {...props} />
+        </fieldset>
+      </form>
+    </section>
  );
 };

-const Options: React.FC<Props> = ({ options, value, onChange }) => {
+const Options: React.FC<Props> = ({ options, value, onChange, title }) => {
  const styles = getOptionsStyles();

  return (
    <>
-      <div>
+      <ul aria-roledescription="Time range selection">
        {options.map((option, index) => (
          <TimeRangeOption
            key={keyForOption(option, index)}
            value={option}
            selected={isEqual(option, value)}
            onSelect={onChange}
+            name={title}
          />
        ))}
-      </div>
+      </ul>
      <div className={styles.grow} />
    </>
  );
@@ -2,12 +2,15 @@ import React, { memo } from 'react';
 import { css, cx } from '@emotion/css';
 import { GrafanaTheme2, TimeOption } from '@grafana/data';
 import { useStyles2 } from '../../../themes/ThemeContext';
+import { getFocusStyles } from '../../../themes/mixins';
+import { v4 as uuidv4 } from 'uuid';

 const getStyles = (theme: GrafanaTheme2) => {
  return {
    container: css`
      display: flex;
      align-items: center;
+      flex-direction: row-reverse;
      justify-content: space-between;
      padding: 7px 9px 7px 9px;

@@ -16,10 +19,20 @@ const getStyles = (theme: GrafanaTheme2) => {
        cursor: pointer;
      }
    `,
-    selected: css`    
-      background: ${theme.colors.action.selected};    
-    }
-  `,
+    selected: css`
+      background: ${theme.colors.action.selected};
+      font-weight: ${theme.typography.fontWeightMedium};
+    `,
+    radio: css`
+      opacity: 0;
+
+      &:focus-visible + label {
+        ${getFocusStyles(theme)};
+      }
+    `,
+    label: css`
+      cursor: pointer;
+    `,
  };
 };

@@ -27,15 +40,31 @@ interface Props {
  value: TimeOption;
  selected?: boolean;
  onSelect: (option: TimeOption) => void;
+  /**
+   *  Input identifier. This should be the same for all options in a group.
+   */
+  name: string;
 }

-export const TimeRangeOption = memo<Props>(({ value, onSelect, selected = false }) => {
+export const TimeRangeOption = memo<Props>(({ value, onSelect, selected = false, name }) => {
  const styles = useStyles2(getStyles);
+  // In case there are more of the same timerange in the list
+  const id = uuidv4();

  return (
-    <div className={cx(styles.container, selected && styles.selected)} onClick={() => onSelect(value)} tabIndex={-1}>
-      <span>{value.display}</span>
-    </div>
+    <li className={cx(styles.container, selected && styles.selected)}>
+      <input
+        className={styles.radio}
+        checked={selected}
+        name={name}
+        type="checkbox"
+        id={id}
+        onChange={() => onSelect(value)}
+      />
+      <label className={styles.label} htmlFor={id}>
+        {value.display}
+      </label>
+    </li>
  );
 });