ToolbarButton: Require tooltip or aria-label if no children are present (#114097)

* split into separate stories

* make tooltip/aria-label required if no children present

* kick CI

* fix unit tests

* cleaner

* clean up story

eslint-suppressions.json

@@ -742,11 +742,6 @@
       "count": 1
     }
   },
-  "packages/grafana-ui/src/components/PageLayout/PageToolbar.story.tsx": {
-    "no-restricted-syntax": {
-      "count": 1
-    }
-  },
   "packages/grafana-ui/src/components/PanelChrome/PanelContext.ts": {
     "@typescript-eslint/no-explicit-any": {
       "count": 2

packages/grafana-ui/src/components/PageLayout/PageToolbar.story.tsx

@@ -1,9 +1,7 @@
 import { action } from '@storybook/addon-actions';
 import { Meta } from '@storybook/react';
 
-import { StoryExample } from '../../utils/storybook/StoryExample';
 import { IconButton } from '../IconButton/IconButton';
-import { Stack } from '../Layout/Stack/Stack';
 import { ToolbarButton } from '../ToolbarButton/ToolbarButton';
 
 import { PageToolbar } from './PageToolbar';
@@ -11,48 +9,46 @@ import { PageToolbar } from './PageToolbar';
 const meta: Meta<typeof PageToolbar> = {
   title: 'Navigation/Deprecated/PageToolbar',
   component: PageToolbar,
-  parameters: {
-    // TODO fix a11y issue in story and remove this
-    a11y: { test: 'off' },
-  },
 };
 
-export const Examples = () => {
+export const WithNonClickableTitle = () => {
   return (
-    <Stack direction="column">
-      <StoryExample name="With non clickable title">
-        <PageToolbar pageIcon="bell" title="Dashboard">
-          <ToolbarButton icon="panel-add" />
-          <ToolbarButton icon="sync">Sync</ToolbarButton>
-        </PageToolbar>
-      </StoryExample>
-      <StoryExample name="With clickable title and parent">
-        <PageToolbar
-          pageIcon="apps"
-          title="A very long dashboard name"
-          parent="A long folder name"
-          titleHref=""
-          parentHref=""
-          leftItems={[
-            <IconButton name="share-alt" size="lg" key="share" tooltip="Share" />,
-            <IconButton name="favorite" iconType="mono" size="lg" key="favorite" tooltip="Add to favourites" />,
-          ]}
-        >
-          <ToolbarButton icon="panel-add" />
-          <ToolbarButton icon="share-alt" />
-          <ToolbarButton icon="sync">Sync</ToolbarButton>
-          <ToolbarButton icon="cog">Settings </ToolbarButton>
-        </PageToolbar>
-      </StoryExample>
-      <StoryExample name="Go back version">
-        <PageToolbar title="Service overview / Edit panel" onGoBack={() => action('Go back')}>
-          <ToolbarButton icon="cog" />
-          <ToolbarButton icon="save" />
-          <ToolbarButton>Discard</ToolbarButton>
-          <ToolbarButton>Apply</ToolbarButton>
-        </PageToolbar>
-      </StoryExample>
-    </Stack>
+    <PageToolbar pageIcon="bell" title="Dashboard">
+      <ToolbarButton icon="panel-add" tooltip="Add panel" />
+      <ToolbarButton icon="sync">Sync</ToolbarButton>
+    </PageToolbar>
+  );
+};
+
+export const WithClickableTitleAndParent = () => {
+  return (
+    <PageToolbar
+      pageIcon="apps"
+      title="A very long dashboard name"
+      parent="A long folder name"
+      titleHref=""
+      parentHref=""
+      leftItems={[
+        <IconButton name="share-alt" size="lg" key="share" tooltip="Share" />,
+        <IconButton name="favorite" iconType="mono" size="lg" key="favorite" tooltip="Add to favourites" />,
+      ]}
+    >
+      <ToolbarButton icon="panel-add" tooltip="Add panel" />
+      <ToolbarButton icon="share-alt" tooltip="Share" />
+      <ToolbarButton icon="sync">Sync</ToolbarButton>
+      <ToolbarButton icon="cog">Settings </ToolbarButton>
+    </PageToolbar>
+  );
+};
+
+export const GoBackVersion = () => {
+  return (
+    <PageToolbar title="Service overview / Edit panel" onGoBack={() => action('Go back')}>
+      <ToolbarButton icon="cog" tooltip="Settings" />
+      <ToolbarButton icon="save" aria-label="Save" />
+      <ToolbarButton>Discard</ToolbarButton>
+      <ToolbarButton>Apply</ToolbarButton>
+    </PageToolbar>
   );
 };
 

packages/grafana-ui/src/components/ToolbarButton/ToolbarButton.story.tsx

@@ -76,6 +76,7 @@ export const BasicWithIcon: StoryFn<typeof ToolbarButton> = (args) => {
       icon={args.icon}
       isOpen={args.isOpen}
       tooltip={args.tooltip}
+      aria-label="This is an aria-label"
       disabled={args.disabled}
       fullWidth={args.fullWidth}
       isHighlighted={args.isHighlighted}

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

@@ -1,5 +1,5 @@
 import { cx, css } from '@emotion/css';
-import { forwardRef, ButtonHTMLAttributes } from 'react';
+import { forwardRef, ButtonHTMLAttributes, ReactNode } from 'react';
 import * as React from 'react';
 
 import { GrafanaTheme2, IconName, isIconName } from '@grafana/data';
@@ -12,7 +12,7 @@ import { getActiveButtonStyles, getPropertiesForVariant } from '../Button/Button
 import { Icon } from '../Icon/Icon';
 import { Tooltip } from '../Tooltip/Tooltip';
 
-type CommonProps = {
+interface BaseProps extends ButtonHTMLAttributes<HTMLButtonElement> {
   /** Icon name */
   icon?: IconName | React.ReactNode;
   /** Icon size */
@@ -35,9 +35,21 @@ type CommonProps = {
   iconOnly?: boolean;
   /** Show highlight dot */
   isHighlighted?: boolean;
-};
+}
 
-export type ToolbarButtonProps = CommonProps & ButtonHTMLAttributes<HTMLButtonElement>;
+interface BasePropsWithChildren extends BaseProps {
+  children: ReactNode;
+}
+
+interface BasePropsWithTooltip extends BaseProps {
+  tooltip: string;
+}
+
+interface BasePropsWithAriaLabel extends BaseProps {
+  ['aria-label']: string;
+}
+
+export type ToolbarButtonProps = BasePropsWithChildren | BasePropsWithTooltip | BasePropsWithAriaLabel;
 
 export type ToolbarButtonVariant = 'default' | 'primary' | 'destructive' | 'active' | 'canvas';
 
@@ -46,72 +58,68 @@ export type ToolbarButtonVariant = 'default' | 'primary' | 'destructive' | 'acti
  *
  * https://developers.grafana.com/ui/latest/index.html?path=/docs/navigation-toolbarbutton--docs
  */
-export const ToolbarButton = forwardRef<HTMLButtonElement, ToolbarButtonProps>(
-  (
+export const ToolbarButton = forwardRef<HTMLButtonElement, ToolbarButtonProps>((props, ref) => {
+  const styles = useStyles2(getStyles);
+  const {
+    tooltip,
+    icon,
+    iconSize,
+    className,
+    children,
+    imgSrc,
+    imgAlt,
+    fullWidth,
+    isOpen,
+    narrow,
+    variant = 'default',
+    iconOnly,
+    'aria-label': ariaLabel,
+    isHighlighted,
+    ...rest
+  } = props;
+
+  const buttonStyles = cx(
     {
-      tooltip,
-      icon,
-      iconSize,
-      className,
-      children,
-      imgSrc,
-      imgAlt,
-      fullWidth,
-      isOpen,
-      narrow,
-      variant = 'default',
-      iconOnly,
-      'aria-label': ariaLabel,
-      isHighlighted,
-      ...rest
+      [styles.button]: true,
+      [styles.buttonFullWidth]: fullWidth,
+      [styles.narrow]: narrow,
     },
-    ref
-  ) => {
-    const styles = useStyles2(getStyles);
+    styles[variant],
+    className
+  );
 
-    const buttonStyles = cx(
-      {
-        [styles.button]: true,
-        [styles.buttonFullWidth]: fullWidth,
-        [styles.narrow]: narrow,
-      },
-      styles[variant],
-      className
-    );
+  const contentStyles = cx({
+    [styles.content]: true,
+    [styles.contentWithIcon]: !!icon,
+    [styles.contentWithRightIcon]: isOpen !== undefined,
+  });
 
-    const contentStyles = cx({
-      [styles.content]: true,
-      [styles.contentWithIcon]: !!icon,
-      [styles.contentWithRightIcon]: isOpen !== undefined,
-    });
+  const body = (
+    <button
+      ref={ref}
+      className={buttonStyles}
+      aria-label={getButtonAriaLabel(ariaLabel, tooltip)}
+      aria-expanded={isOpen}
+      type="button"
+      {...rest}
+    >
+      {renderIcon(icon, iconSize)}
+      {imgSrc && <img className={styles.img} src={imgSrc} alt={imgAlt ?? ''} />}
+      {children && !iconOnly && <div className={contentStyles}>{children}</div>}
+      {isOpen === false && <Icon name="angle-down" />}
+      {isOpen === true && <Icon name="angle-up" />}
+      {isHighlighted && <div className={styles.highlight} />}
+    </button>
+  );
 
-    const body = (
-      <button
-        ref={ref}
-        className={buttonStyles}
-        aria-label={getButtonAriaLabel(ariaLabel, tooltip)}
-        aria-expanded={isOpen}
-        type="button"
-        {...rest}
-      >
-        {renderIcon(icon, iconSize)}
-        {imgSrc && <img className={styles.img} src={imgSrc} alt={imgAlt ?? ''} />}
-        {children && !iconOnly && <div className={contentStyles}>{children}</div>}
-        {isOpen === false && <Icon name="angle-down" />}
-        {isOpen === true && <Icon name="angle-up" />}
-        {isHighlighted && <div className={styles.highlight} />}
-      </button>
-    );
-
-    return tooltip ? (
-      <Tooltip ref={ref} content={tooltip} placement="bottom">
-        {body}
-      </Tooltip>
-    ) : (
-      body
-    );
-  }
-);
+  return tooltip ? (
+    <Tooltip ref={ref} content={tooltip} placement="bottom">
+      {body}
+    </Tooltip>
+  ) : (
+    body
+  );
+});
 
 ToolbarButton.displayName = 'ToolbarButton';
 

public/app/features/explore/LiveTailButton.tsx

@@ -2,8 +2,8 @@ import { css } from '@emotion/css';
 import { useRef } from 'react';
 import { CSSTransition } from 'react-transition-group';
 
-import { Trans, t } from '@grafana/i18n';
-import { Tooltip, ButtonGroup, ToolbarButton } from '@grafana/ui';
+import { t } from '@grafana/i18n';
+import { ButtonGroup, ToolbarButton } from '@grafana/ui';
 
 type LiveTailButtonProps = {
   splitted: boolean;
@@ -24,31 +24,21 @@ export function LiveTailButton(props: LiveTailButtonProps) {
 
   return (
     <ButtonGroup>
-      <Tooltip
-        content={
-          isLive && !isPaused ? (
-            <>
-              <Trans i18nKey="explore.live-tail-button.pause-the-live-stream">Pause the live stream</Trans>
-            </>
-          ) : (
-            <>
-              <Trans i18nKey="explore.live-tail-button.start-live-stream-your-logs">Start live stream your logs</Trans>
-            </>
-          )
+      <ToolbarButton
+        iconOnly={splitted}
+        variant={buttonVariant}
+        icon={!isLive || isPaused ? 'play' : 'pause'}
+        onClick={onClickMain}
+        tooltip={
+          !isLive || isPaused
+            ? t('explore.live-tail-button.start-live-stream-your-logs', 'Start live stream your logs')
+            : t('explore.live-tail-button.pause-the-live-stream', 'Pause the live stream')
         }
-        placement="bottom"
       >
-        <ToolbarButton
-          iconOnly={splitted}
-          variant={buttonVariant}
-          icon={!isLive || isPaused ? 'play' : 'pause'}
-          onClick={onClickMain}
-        >
-          {isLive && isPaused
-            ? t('explore.live-tail-button.paused', 'Paused')
-            : t('explore.live-tail-button.live', 'Live')}
-        </ToolbarButton>
-      </Tooltip>
+        {isLive && isPaused
+          ? t('explore.live-tail-button.paused', 'Paused')
+          : t('explore.live-tail-button.live', 'Live')}
+      </ToolbarButton>
 
       <CSSTransition
         mountOnEnter={true}
@@ -63,18 +53,13 @@ export function LiveTailButton(props: LiveTailButtonProps) {
         }}
         nodeRef={transitionRef}
       >
-        <Tooltip
-          content={
-            <>
-              <Trans i18nKey="explore.live-tail-button.stop-and-exit-the-live-stream">
-                Stop and exit the live stream
-              </Trans>
-            </>
-          }
-          placement="bottom"
-        >
-          <ToolbarButton ref={transitionRef} variant={buttonVariant} onClick={stop} icon="square-shape" />
-        </Tooltip>
+        <ToolbarButton
+          tooltip={t('explore.live-tail-button.stop-and-exit-the-live-stream', 'Stop and exit the live stream')}
+          ref={transitionRef}
+          variant={buttonVariant}
+          onClick={stop}
+          icon="square-shape"
+        />
       </CSSTransition>
     </ButtonGroup>
   );

public/app/features/explore/TraceView/components/TraceTimelineViewer/VirtualizedTraceView.test.tsx

@@ -72,14 +72,14 @@ describe('<VirtualizedTraceViewImpl>', () => {
   it('renders without exploding', () => {
     render(<VirtualizedTraceView {...props} />);
     expect(screen.getByTestId('ListView')).toBeInTheDocument();
-    expect(screen.getByTitle('Scroll to top')).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Scroll to top' })).toBeInTheDocument();
   });
 
   it('renders when a trace is not set', () => {
     props = { ...props, trace: null as unknown as Trace };
     render(<VirtualizedTraceView {...props} />);
     expect(screen.getByTestId('ListView')).toBeInTheDocument();
-    expect(screen.getByTitle('Scroll to top')).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Scroll to top' })).toBeInTheDocument();
   });
 
   it('renders ListView', () => {

public/app/features/explore/TraceView/components/TraceTimelineViewer/VirtualizedTraceView.tsx

@@ -644,7 +644,7 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
           <ToolbarButton
             className={styles.scrollToTopButton}
             onClick={this.scrollToTop}
-            title={t('explore.unthemed-virtualized-trace-view.title-scroll-to-top', 'Scroll to top')}
+            tooltip={t('explore.unthemed-virtualized-trace-view.title-scroll-to-top', 'Scroll to top')}
             icon="arrow-up"
           ></ToolbarButton>
         )}

public/app/features/stars/StarToolbarButton.tsx

@@ -50,12 +50,9 @@ export function StarToolbarButton({ title, group, kind, id, onStarChange }: Prop
   })();
 
   const tooltipAndLabel = (() => {
-    if (isLoading) {
-      return {};
-    }
     return isStarred
-      ? { tooltip: tooltips.unstar, label: tooltips.unstarWithTitle }
-      : { tooltip: tooltips.star, label: tooltips.starWithTitle };
+      ? { tooltip: tooltips.unstar, label: isLoading ? undefined : tooltips.unstarWithTitle }
+      : { tooltip: tooltips.star, label: isLoading ? undefined : tooltips.starWithTitle };
   })();
 
   const icon = <Icon {...iconProps} size="lg" />;