From 70e06dceaa0e472c0a1f1b16a1d3fbc8be395fd5 Mon Sep 17 00:00:00 2001
From: Peter Makowski <peter@makowski.uk>
Date: Fri, 9 Sep 2022 10:21:55 +0200
Subject: [PATCH] add Field to MachineSelect - handle onKeyPress within
 MachineSelectTable

---
 .../DhcpFormFields/DhcpFormFields.test.tsx    |  2 +-
 .../MachineSelect/MachineSelect.test.tsx      |  4 +-
 .../MachineSelect/MachineSelect.tsx           | 63 +++++++++++--------
 .../MachineSelectBox.test.tsx                 |  2 +-
 .../MachineSelectBox/MachineSelectBox.tsx     | 38 ++++++-----
 .../MachineSelectTable.test.tsx               | 21 +++++++
 .../MachineSelectTable/MachineSelectTable.tsx |  6 ++
 7 files changed, 89 insertions(+), 47 deletions(-)

diff --git a/src/app/base/components/DhcpFormFields/DhcpFormFields.test.tsx b/src/app/base/components/DhcpFormFields/DhcpFormFields.test.tsx
index c093ca3ba3..648310f24e 100644
--- a/src/app/base/components/DhcpFormFields/DhcpFormFields.test.tsx
+++ b/src/app/base/components/DhcpFormFields/DhcpFormFields.test.tsx
@@ -183,7 +183,7 @@ describe("DhcpFormFields", () => {
     );
     within(screen.getByRole("grid")).getByText(machine.hostname).click();
     expect(
-      screen.getByRole("button", { name: machine.hostname })
+      screen.getByRole("button", { name: new RegExp(machine.hostname, "i") })
     ).toHaveAccessibleDescription(Labels.AppliesTo);
     // Change the type. The select value should be cleared.
     await userEvent.selectOptions(typeSelect, "subnet");
diff --git a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.test.tsx b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.test.tsx
index aaf09d0ecb..8b83e8990a 100644
--- a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.test.tsx
+++ b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.test.tsx
@@ -15,7 +15,7 @@ it("can open select box on click", async () => {
 
   expect(screen.queryByRole("listbox")).not.toBeInTheDocument();
   await userEvent.click(
-    screen.getByRole("button", { name: Labels.ChooseMachine })
+    screen.getByRole("button", { name: new RegExp(Labels.ChooseMachine, "i") })
   );
   expect(screen.getByRole("listbox")).toBeInTheDocument();
 });
@@ -28,7 +28,7 @@ it("sets focus on the input field on open", async () => {
   );
 
   await userEvent.click(
-    screen.getByRole("button", { name: Labels.ChooseMachine })
+    screen.getByRole("button", { name: new RegExp(Labels.ChooseMachine, "i") })
   );
   expect(
     screen.getByPlaceholderText("Search by hostname, system ID or tags")
diff --git a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.tsx b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.tsx
index 0514c0a82b..bda59cbcd5 100644
--- a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.tsx
+++ b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelect.tsx
@@ -1,7 +1,8 @@
 import type { HTMLProps } from "react";
 import { useEffect, useState } from "react";
 
-import { Label, useId, useOnEscapePressed } from "@canonical/react-components";
+import { useId, useOnEscapePressed } from "@canonical/react-components";
+import Field from "@canonical/react-components/dist/components/Field";
 import className from "classnames";
 import { useFormikContext } from "formik";
 import { useDispatch } from "react-redux";
@@ -37,10 +38,12 @@ export const MachineSelect = ({
   label = Labels.AppliesTo,
   defaultOption = Labels.ChooseMachine,
   value,
+  ...props
 }: Props): JSX.Element => {
   const { setFieldValue } = useFormikContext();
   const dispatch = useDispatch();
   const [isOpen, setIsOpen] = useState(false);
+  const labelId = useId();
   const selectId = useId();
   const handleSelect = (machine: Machine | null) => {
     setIsOpen(false);
@@ -55,33 +58,41 @@ export const MachineSelect = ({
     dispatch(tagActions.fetch());
   }, [dispatch]);
 
+  const buttonLabel = selectedMachine?.hostname || defaultOption;
+
   return (
     <div className="machine-select">
-      <Label id={selectId}>{label}</Label>
-      <OutsideClickHandler onClick={() => setIsOpen(false)}>
-        <SelectButton
-          aria-describedby={selectId}
-          aria-haspopup="listbox"
-          className="u-no-margin--bottom"
-          onClick={() => {
-            setIsOpen(!isOpen);
-            if (!isOpen) {
-              setFieldValue(name, "", false);
-            }
-          }}
-        >
-          {selectedMachine?.hostname || defaultOption}
-        </SelectButton>
-        <div
-          className={className("machine-select-box-wrapper", {
-            "machine-select-box-wrapper--is-open": isOpen,
-          })}
-        >
-          {isOpen ? (
-            <MachineSelectBox filters={filters} onSelect={handleSelect} />
-          ) : null}
-        </div>
-      </OutsideClickHandler>
+      {/* TODO: update once Field allows a custom label id
+      https://github.com/canonical/react-components/issues/820 */}
+      <Field label={<span id={labelId}>{label}</span>} {...props}>
+        <OutsideClickHandler onClick={() => setIsOpen(false)}>
+          <SelectButton
+            aria-describedby={labelId}
+            aria-expanded={isOpen ? "true" : "false"}
+            aria-haspopup="listbox"
+            aria-label={`${buttonLabel} - open list`}
+            className="u-no-margin--bottom"
+            id={selectId}
+            onClick={() => {
+              setIsOpen(!isOpen);
+              if (!isOpen) {
+                setFieldValue(name, "", false);
+              }
+            }}
+          >
+            {buttonLabel}
+          </SelectButton>
+          <div
+            className={className("machine-select-box-wrapper", {
+              "machine-select-box-wrapper--is-open": isOpen,
+            })}
+          >
+            {isOpen ? (
+              <MachineSelectBox filters={filters} onSelect={handleSelect} />
+            ) : null}
+          </div>
+        </OutsideClickHandler>
+      </Field>
     </div>
   );
 };
diff --git a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.test.tsx b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.test.tsx
index b6d9626477..a50cdb0863 100644
--- a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.test.tsx
+++ b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.test.tsx
@@ -62,7 +62,7 @@ it("requests machines filtered by the free text input value", async () => {
   });
 
   const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
-  await user.type(screen.getByRole("searchbox"), "test-machine");
+  await user.type(screen.getByRole("combobox"), "test-machine");
   const expectedInitialAction = machineActions.fetch("mocked-nanoid-1", {
     filter: { free_text: "" },
     group_collapsed: undefined,
diff --git a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.tsx b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.tsx
index d48c3c3987..6b2b98cac9 100644
--- a/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.tsx
+++ b/src/app/base/components/DhcpFormFields/MachineSelect/MachineSelectBox/MachineSelectBox.tsx
@@ -28,31 +28,35 @@ const MachineSelectBox = ({
     },
   });
   return (
-    <div className="machine-select-box" role="listbox">
+    <div className="machine-select-box">
       <DebounceSearchBox
+        aria-label="Search by hostname, system ID or tags"
         autoComplete="off"
         autoFocus
         onDebounced={setDebouncedText}
         placeholder="Search by hostname, system ID or tags"
+        role="combobox"
         searchText={searchText}
         setSearchText={setSearchText}
       />
-      <MachineSelectTable
-        machines={machines}
-        machinesLoading={loading}
-        onMachineClick={(machine) => {
-          onSelect(machine);
-        }}
-        searchText={searchText}
-        setSearchText={setSearchText}
-      />
-      <MachineListPagination
-        currentPage={currentPage}
-        itemsPerPage={pageSize}
-        machineCount={machineCount}
-        machinesLoading={loading}
-        paginate={setPage}
-      />
+      <div role="listbox">
+        <MachineSelectTable
+          machines={machines}
+          machinesLoading={loading}
+          onMachineClick={(machine) => {
+            onSelect(machine);
+          }}
+          searchText={searchText}
+          setSearchText={setSearchText}
+        />
+        <MachineListPagination
+          currentPage={currentPage}
+          itemsPerPage={pageSize}
+          machineCount={machineCount}
+          machinesLoading={loading}
+          paginate={setPage}
+        />
+      </div>
     </div>
   );
 };
diff --git a/src/app/base/components/MachineSelectTable/MachineSelectTable.test.tsx b/src/app/base/components/MachineSelectTable/MachineSelectTable.test.tsx
index 123ec969ef..170d5498c1 100644
--- a/src/app/base/components/MachineSelectTable/MachineSelectTable.test.tsx
+++ b/src/app/base/components/MachineSelectTable/MachineSelectTable.test.tsx
@@ -110,4 +110,25 @@ describe("MachineSelectTable", () => {
     });
     expect(within(ownerCols[0]).getByText("tagA")).toBeInTheDocument();
   });
+
+  it("can select machine by pressing Enter key", async () => {
+    const onMachineClick = jest.fn();
+    const machine = machines[0];
+    renderWithMockStore(
+      <MachineSelectTable
+        machines={machines}
+        onMachineClick={onMachineClick}
+        searchText=""
+        setSearchText={jest.fn()}
+      />,
+      { state }
+    );
+    screen
+      .getByRole("row", {
+        name: machine.hostname,
+      })
+      .focus();
+    await userEvent.keyboard("{enter}");
+    expect(onMachineClick).toHaveBeenCalledWith(machine);
+  });
 });
diff --git a/src/app/base/components/MachineSelectTable/MachineSelectTable.tsx b/src/app/base/components/MachineSelectTable/MachineSelectTable.tsx
index dd912ef53e..0f6969ce91 100644
--- a/src/app/base/components/MachineSelectTable/MachineSelectTable.tsx
+++ b/src/app/base/components/MachineSelectTable/MachineSelectTable.tsx
@@ -73,6 +73,12 @@ const generateRows = (
     ],
     "data-testid": "machine-select-row",
     onClick: () => onRowClick(machine),
+    onKeyPress: (e: KeyboardEvent) => {
+      if (e.key === "Enter") {
+        e.preventDefault();
+        onRowClick(machine);
+      }
+    },
     tabIndex: 0,
   }));
 };