From 972e045145711e39fd6fa167cb87fa05e062272c Mon Sep 17 00:00:00 2001
From: Charles <cc.lee@live.com.au>
Date: Mon, 28 Jun 2021 11:14:47 +1000
Subject: [PATCH] assorted filter pill fixes (#5987)

---
 .changeset/healthy-spoons-dance.md            |   5 +
 .changeset/loud-poems-crash.md                |   5 +
 .changeset/stale-hornets-remember.md          |   5 +
 .changeset/tough-walls-kneel.md               |   5 +
 design-system/packages/pill/src/index.tsx     | 204 +++++++++---------
 .../packages/popover/src/Popover.tsx          |   3 +-
 .../fields/src/types/text/views/index.tsx     |   2 +-
 .../admin-ui/pages/ListPage/FilterList.tsx    |  36 ++--
 8 files changed, 149 insertions(+), 116 deletions(-)
 create mode 100644 .changeset/healthy-spoons-dance.md
 create mode 100644 .changeset/loud-poems-crash.md
 create mode 100644 .changeset/stale-hornets-remember.md
 create mode 100644 .changeset/tough-walls-kneel.md

diff --git a/.changeset/healthy-spoons-dance.md b/.changeset/healthy-spoons-dance.md
new file mode 100644
index 00000000000..a20da6e97c0
--- /dev/null
+++ b/.changeset/healthy-spoons-dance.md
@@ -0,0 +1,5 @@
+---
+'@keystone-next/keystone': patch
+---
+
+Added assorted quality of life and screen reader improvements to Filter pills and dialogs in the admin-ui
diff --git a/.changeset/loud-poems-crash.md b/.changeset/loud-poems-crash.md
new file mode 100644
index 00000000000..698b58ec707
--- /dev/null
+++ b/.changeset/loud-poems-crash.md
@@ -0,0 +1,5 @@
+---
+'@keystone-ui/popover': patch
+---
+
+Added aria-hidden to dialog container so screen readers can't access when the dialog is not visible.
diff --git a/.changeset/stale-hornets-remember.md b/.changeset/stale-hornets-remember.md
new file mode 100644
index 00000000000..475cc730b4e
--- /dev/null
+++ b/.changeset/stale-hornets-remember.md
@@ -0,0 +1,5 @@
+---
+'@keystone-ui/pill': major
+---
+
+Added containerProps prop, these are spread onto the containing element. All other props are now spread onto the internal PillButton component (this change is inclusive of refs).
diff --git a/.changeset/tough-walls-kneel.md b/.changeset/tough-walls-kneel.md
new file mode 100644
index 00000000000..523d28ff557
--- /dev/null
+++ b/.changeset/tough-walls-kneel.md
@@ -0,0 +1,5 @@
+---
+'@keystone-next/fields': patch
+---
+
+Fixed autoFocus always being active in the text field Filter component.
diff --git a/design-system/packages/pill/src/index.tsx b/design-system/packages/pill/src/index.tsx
index 216d877843c..fc8b3a5559a 100644
--- a/design-system/packages/pill/src/index.tsx
+++ b/design-system/packages/pill/src/index.tsx
@@ -7,130 +7,132 @@ import { XIcon } from '@keystone-ui/icons/icons/XIcon';
 type Tone = 'active' | 'passive' | 'positive' | 'warning' | 'negative' | 'help';
 type Weight = 'bold' | 'light';
 
-const PillButton = ({
-  tone: toneKey,
-  weight,
-  onClick,
-  tabIndex,
-  ...props
-}: {
+type PillButtonProps = {
   tone: Tone;
   weight: Weight;
-} & ButtonHTMLAttributes<HTMLButtonElement>) => {
-  const { radii, spacing, tones, typography } = useTheme();
+} & ButtonHTMLAttributes<HTMLButtonElement>;
 
-  const isInteractive = !!onClick;
+const PillButton = forwardRef<HTMLButtonElement, PillButtonProps>(
+  ({ tone: toneKey, weight, onClick, tabIndex, ...props }, ref) => {
+    const { radii, spacing, tones, typography } = useTheme();
 
-  const tone = tones[toneKey];
-  const tokens = {
-    bold: {
-      background: tone.fill[0],
-      foreground: tone.fillForeground[0],
-      focus: {
-        shadow: `0 0 0 2px ${tone.focusRing}`,
-      },
-      hover: {
-        background: tone.fill[1],
+    const isInteractive = !!onClick;
+
+    const tone = tones[toneKey];
+    const tokens = {
+      bold: {
+        background: tone.fill[0],
+        foreground: tone.fillForeground[0],
+        focus: {
+          shadow: `0 0 0 2px ${tone.focusRing}`,
+        },
+        hover: {
+          background: tone.fill[1],
+        },
+        active: {
+          background: tone.fill[2],
+        },
       },
-      active: {
-        background: tone.fill[2],
+      light: {
+        background: tone.tint[0],
+        foreground: tone.foreground[0],
+        focus: {
+          shadow: `0 0 0 2px ${tone.focusRing}`,
+        },
+        hover: {
+          foreground: tone.foreground[1],
+          background: tone.tint[1],
+        },
+        active: {
+          foreground: tone.foreground[2],
+          background: tone.tint[2],
+        },
       },
-    },
-    light: {
-      background: tone.tint[0],
-      foreground: tone.foreground[0],
-      focus: {
-        shadow: `0 0 0 2px ${tone.focusRing}`,
+    }[weight];
+
+    const baseStyles = {
+      alignItems: 'center',
+      appearance: 'none',
+      background: 'none',
+      backgroundColor: tokens.background,
+      border: 0,
+      color: tokens.foreground,
+      display: 'flex',
+      fontSize: typography.fontSize.small,
+      fontWeight: typography.fontWeight.medium,
+      justifyContent: 'center',
+      maxWidth: '100%',
+      minWidth: 1,
+      outline: 0,
+      padding: `${spacing.small}px ${spacing.medium}px`,
+
+      ':first-of-type': {
+        paddingRight: spacing.small,
+        borderTopLeftRadius: radii.full,
+        borderBottomLeftRadius: radii.full,
+        marginRight: 1,
       },
-      hover: {
-        foreground: tone.foreground[1],
-        background: tone.tint[1],
+      ':last-of-type': {
+        paddingLeft: spacing.small,
+        borderTopRightRadius: radii.full,
+        borderBottomRightRadius: radii.full,
       },
-      active: {
-        foreground: tone.foreground[2],
-        background: tone.tint[2],
+      ':only-of-type': {
+        paddingLeft: spacing.medium,
+        paddingRight: spacing.medium,
       },
-    },
-  }[weight];
+    } as const;
 
-  const baseStyles = {
-    alignItems: 'center',
-    appearance: 'none',
-    background: 'none',
-    backgroundColor: tokens.background,
-    border: 0,
-    color: tokens.foreground,
-    display: 'flex',
-    fontSize: typography.fontSize.small,
-    fontWeight: typography.fontWeight.medium,
-    justifyContent: 'center',
-    maxWidth: '100%',
-    minWidth: 1,
-    outline: 0,
-    padding: `${spacing.small}px ${spacing.medium}px`,
+    const interactiveStyles = isInteractive
+      ? {
+          cursor: 'pointer',
+          ':focus': {
+            boxShadow: tokens.focus.shadow,
+          },
+          ':hover,:focus': {
+            backgroundColor: tokens.hover.background,
+            color: tokens.hover.foreground,
+          },
+          ':active': {
+            backgroundColor: tokens.active.background,
+            color: tokens.active.foreground,
+          },
+        }
+      : {};
 
-    ':first-of-type': {
-      paddingRight: spacing.small,
-      borderTopLeftRadius: radii.full,
-      borderBottomLeftRadius: radii.full,
-      marginRight: 1,
-    },
-    ':last-of-type': {
-      paddingLeft: spacing.small,
-      borderTopRightRadius: radii.full,
-      borderBottomRightRadius: radii.full,
-    },
-    ':only-of-type': {
-      paddingLeft: spacing.medium,
-      paddingRight: spacing.medium,
-    },
-  } as const;
-
-  const interactiveStyles = isInteractive
-    ? {
-        cursor: 'pointer',
-        ':focus': {
-          boxShadow: tokens.focus.shadow,
-        },
-        ':hover,:focus': {
-          backgroundColor: tokens.hover.background,
-          color: tokens.hover.foreground,
-        },
-        ':active': {
-          backgroundColor: tokens.active.background,
-          color: tokens.active.foreground,
-        },
-      }
-    : {};
-
-  return (
-    <button
-      css={{ ...baseStyles, ...interactiveStyles }}
-      onClick={onClick}
-      tabIndex={!isInteractive ? -1 : tabIndex}
-      {...props}
-    />
-  );
-};
+    return (
+      <button
+        ref={ref}
+        css={{ ...baseStyles, ...interactiveStyles }}
+        onClick={onClick}
+        tabIndex={!isInteractive ? -1 : tabIndex}
+        {...props}
+      />
+    );
+  }
+);
 
 type PillProps = {
   children: ReactNode;
   onClick?: () => void;
   onRemove?: () => void;
   tone?: Tone;
+  containerProps?: HTMLAttributes<HTMLDivElement>;
   weight?: Weight;
-} & HTMLAttributes<HTMLDivElement>;
+} & HTMLAttributes<HTMLButtonElement>;
 
-export const Pill = forwardRef<HTMLDivElement, PillProps>(
-  ({ weight = 'bold', tone = 'active', children, onClick, onRemove, ...props }, ref) => {
-    console.log(onClick);
+export const Pill = forwardRef<HTMLButtonElement, PillProps>(
+  (
+    { weight = 'bold', tone = 'active', containerProps, children, onClick, onRemove, ...props },
+    ref
+  ) => {
     return (
-      <div css={{ display: 'flex' }} {...props} ref={ref}>
-        <PillButton weight={weight} tone={tone} onClick={onClick}>
+      <div css={{ display: 'flex' }} {...containerProps}>
+        <PillButton ref={ref} weight={weight} tone={tone} onClick={onClick} {...props}>
           {children}
         </PillButton>
         {onRemove ? (
-          <PillButton weight={weight} tone={tone} onClick={onRemove}>
+          <PillButton aria-label="remove pill" weight={weight} tone={tone} onClick={onRemove}>
             <XIcon css={{ height: 14, width: 14 }} />
           </PillButton>
         ) : null}
diff --git a/design-system/packages/popover/src/Popover.tsx b/design-system/packages/popover/src/Popover.tsx
index 5dedb2a6e7d..35178bf6570 100644
--- a/design-system/packages/popover/src/Popover.tsx
+++ b/design-system/packages/popover/src/Popover.tsx
@@ -219,6 +219,7 @@ export const PopoverDialog = forwardRef<HTMLDivElement, DialogProps>(
       <Portal>
         <div
           role="dialog"
+          aria-hidden={isVisible ? 'false' : 'true'}
           aria-modal="true"
           ref={consumerRef}
           css={{
@@ -233,7 +234,7 @@ export const PopoverDialog = forwardRef<HTMLDivElement, DialogProps>(
           {...props}
         >
           <div data-popper-arrow ref={arrow.ref} className="tooltipArrow" {...arrow.props} />
-          <div ref={focusTrapRef}>{isVisible && children}</div>
+          <div ref={focusTrapRef}>{isVisible ? children : null}</div>
         </div>
       </Portal>
     );
diff --git a/packages-next/fields/src/types/text/views/index.tsx b/packages-next/fields/src/types/text/views/index.tsx
index a354182ec8a..c9fd5cc0245 100644
--- a/packages-next/fields/src/types/text/views/index.tsx
+++ b/packages-next/fields/src/types/text/views/index.tsx
@@ -74,7 +74,7 @@ export const controller = (
               props.onChange(event.target.value);
             }}
             value={props.value}
-            autoFocus
+            autoFocus={props.autoFocus}
           />
         );
       },
diff --git a/packages-next/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/FilterList.tsx b/packages-next/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/FilterList.tsx
index 3d8e218c9e2..a0677922e21 100644
--- a/packages-next/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/FilterList.tsx
+++ b/packages-next/keystone/src/___internal-do-not-use-will-break-in-patch/admin-ui/pages/ListPage/FilterList.tsx
@@ -42,17 +42,18 @@ function FilterPill({ filter, field }: { filter: Filter; field: FieldMeta }) {
   return (
     <Fragment>
       <Pill
-        onClick={() => {
-          setOpen(true);
+        containerProps={{
+          'aria-label': `Filter item ${filter.field}`,
         }}
+        aria-description={'Press to edit filter'}
         {...trigger.props}
         ref={trigger.ref}
+        onClick={() => setOpen(true)}
         weight="light"
         tone="passive"
         onRemove={() => {
           const { [`!${filter.field}_${filter.type}`]: _ignore, ...queryToKeep } = router.query;
-
-          router.push({ query: queryToKeep });
+          router.push({ pathname: router.pathname, query: queryToKeep });
         }}
       >
         {field.label}{' '}
@@ -62,14 +63,23 @@ function FilterPill({ filter, field }: { filter: Filter; field: FieldMeta }) {
           value={filter.value}
         />
       </Pill>
-      <PopoverDialog arrow={arrow} {...dialog.props} ref={dialog.ref} isVisible={isOpen}>
-        <EditDialog
-          onClose={() => {
-            setOpen(false);
-          }}
-          field={field}
-          filter={filter}
-        />
+      <PopoverDialog
+        aria-label="filter item config"
+        aria-description={`dialog for configuring ${filter.field} filter`}
+        arrow={arrow}
+        {...dialog.props}
+        isVisible={isOpen}
+        ref={dialog.ref}
+      >
+        {isOpen && (
+          <EditDialog
+            onClose={() => {
+              setOpen(false);
+            }}
+            field={field}
+            filter={filter}
+          />
+        )}
       </PopoverDialog>
     </Fragment>
   );
@@ -103,7 +113,7 @@ function EditDialog({
         onClose();
       }}
     >
-      <Filter autoFocus type={filter.type} value={value} onChange={setValue} />
+      <Filter type={filter.type} value={value} onChange={setValue} />
       <div css={{ display: 'flex', justifyContent: 'space-between' }}>
         <Button onClick={onClose}>Cancel</Button>
         <Button type="submit">Save</Button>