From 2ae8f2bbb1694d09be1d6c7d4e043730e7bd62c2 Mon Sep 17 00:00:00 2001
From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com>
Date: Tue, 8 Mar 2022 04:15:28 -0500
Subject: [PATCH] TagsInput: fix tags remove button accessibility issues
 (#46254) (#46315)

* TagsInput: fix remove button focusable state

* Add tests

* use IconButton

* reverted style changes & disable iconbutton hover animation

(cherry picked from commit 1ef247e0c6214d67051b400b0c2adf1195d003d5)

Co-authored-by: Giordano Ricci <me@giordanoricci.com>
---
 .../src/components/TagsInput/TagItem.tsx      | 23 +++++++++++++++----
 .../components/TagsInput/TagsInput.test.tsx   | 23 +++++++++++++++++++
 .../src/components/TagsInput/TagsInput.tsx    |  7 ++----
 3 files changed, 44 insertions(+), 9 deletions(-)
 create mode 100644 packages/grafana-ui/src/components/TagsInput/TagsInput.test.tsx

diff --git a/packages/grafana-ui/src/components/TagsInput/TagItem.tsx b/packages/grafana-ui/src/components/TagsInput/TagItem.tsx
index 766bd3b1733..7af715e5f00 100644
--- a/packages/grafana-ui/src/components/TagsInput/TagItem.tsx
+++ b/packages/grafana-ui/src/components/TagsInput/TagItem.tsx
@@ -2,12 +2,12 @@ import React, { FC } from 'react';
 import { css } from '@emotion/css';
 import { getTagColorsFromName } from '../../utils';
 import { stylesFactory, useTheme } from '../../themes';
-import { Icon } from '../Icon/Icon';
 import { GrafanaTheme } from '@grafana/data';
+import { IconButton } from '../IconButton/IconButton';
 
 interface Props {
   name: string;
-
+  disabled?: boolean;
   onRemove: (tag: string) => void;
 }
 
@@ -36,6 +36,13 @@ const getStyles = stylesFactory(({ theme, name }: { theme: GrafanaTheme; name: s
     nameStyle: css`
       margin-right: 3px;
     `,
+
+    buttonStyles: css`
+      margin: 0;
+      &:hover::before {
+        display: none;
+      }
+    `,
   };
 });
 
@@ -43,14 +50,22 @@ const getStyles = stylesFactory(({ theme, name }: { theme: GrafanaTheme; name: s
  * @internal
  * Only used internally by TagsInput
  * */
-export const TagItem: FC<Props> = ({ name, onRemove }) => {
+export const TagItem: FC<Props> = ({ name, disabled, onRemove }) => {
   const theme = useTheme();
   const styles = getStyles({ theme, name });
 
   return (
     <div className={styles.itemStyle}>
       <span className={styles.nameStyle}>{name}</span>
-      <Icon className="pointer" name="times" onClick={() => onRemove(name)} />
+      <IconButton
+        name="times"
+        size="lg"
+        disabled={disabled}
+        ariaLabel={`Remove ${name}`}
+        onClick={() => onRemove(name)}
+        type="button"
+        className={styles.buttonStyles}
+      />
     </div>
   );
 };
diff --git a/packages/grafana-ui/src/components/TagsInput/TagsInput.test.tsx b/packages/grafana-ui/src/components/TagsInput/TagsInput.test.tsx
new file mode 100644
index 00000000000..15e8697c1fd
--- /dev/null
+++ b/packages/grafana-ui/src/components/TagsInput/TagsInput.test.tsx
@@ -0,0 +1,23 @@
+import React from 'react';
+import { render, fireEvent, screen } from '@testing-library/react';
+import { TagsInput } from './TagsInput';
+
+describe('TagsInput', () => {
+  it('removes tag when clicking on remove button', async () => {
+    const onChange = jest.fn();
+    render(<TagsInput onChange={onChange} tags={['One', 'Two']} />);
+
+    fireEvent.click(await screen.findByRole('button', { name: /remove one/i }));
+
+    expect(onChange).toHaveBeenCalledWith(['Two']);
+  });
+
+  it('does NOT remove tag when clicking on remove button when disabled', async () => {
+    const onChange = jest.fn();
+    render(<TagsInput onChange={onChange} tags={['One', 'Two']} disabled />);
+
+    fireEvent.click(await screen.findByRole('button', { name: /remove one/i }));
+
+    expect(onChange).not.toHaveBeenCalled();
+  });
+});
diff --git a/packages/grafana-ui/src/components/TagsInput/TagsInput.tsx b/packages/grafana-ui/src/components/TagsInput/TagsInput.tsx
index a0f45e3b547..bcd41ca683e 100644
--- a/packages/grafana-ui/src/components/TagsInput/TagsInput.tsx
+++ b/packages/grafana-ui/src/components/TagsInput/TagsInput.tsx
@@ -42,10 +42,7 @@ export const TagsInput: FC<Props> = ({
   };
 
   const onRemove = (tagToRemove: string) => {
-    if (disabled) {
-      return;
-    }
-    onChange(tags?.filter((x) => x !== tagToRemove));
+    onChange(tags.filter((x) => x !== tagToRemove));
   };
 
   const onAdd = (event?: React.MouseEvent) => {
@@ -74,7 +71,7 @@ export const TagsInput: FC<Props> = ({
     <div className={cx(styles.wrapper, className, width ? css({ width: theme.spacing(width) }) : '')}>
       <div className={tags?.length ? styles.tags : undefined}>
         {tags?.map((tag: string, index: number) => {
-          return <TagItem key={`${tag}-${index}`} name={tag} onRemove={onRemove} />;
+          return <TagItem key={`${tag}-${index}`} name={tag} onRemove={onRemove} disabled={disabled} />;
         })}
       </div>
       <div>