From 758e5806ce430d56e0ab7812b6686c0965d2d2cf Mon Sep 17 00:00:00 2001
From: Sachin Kumar Prajapati
 <156404623+sprajapati-eightfold@users.noreply.github.com>
Date: Fri, 25 Oct 2024 10:43:24 +0530
Subject: [PATCH] feat: [Select] make dropdown role=group and pass correct id
 to options list for a11y (#897)

* feat: make dropdown role=group and fix id for a11y in Select

* feat: make dropdown role=group and fix id for a11y in Select-update snapshots

* feat: select a11y fixes - improve code coverage
---
 src/components/Dropdown/Dropdown.tsx          |   5 +-
 src/components/List/List.test.tsx             |  10 ++
 src/components/List/List.tsx                  |  15 ++-
 .../List/__snapshots__/List.test.tsx.snap     | 109 ++++++++++++++++++
 src/components/Select/Select.tsx              |   1 +
 .../Select/__snapshots__/Select.test.tsx.snap |  26 ++---
 6 files changed, 150 insertions(+), 16 deletions(-)

diff --git a/src/components/Dropdown/Dropdown.tsx b/src/components/Dropdown/Dropdown.tsx
index 7dacda85e..312df531c 100644
--- a/src/components/Dropdown/Dropdown.tsx
+++ b/src/components/Dropdown/Dropdown.tsx
@@ -321,10 +321,13 @@ export const Dropdown: FC<DropdownProps> = React.memo(
         }
         // If there's an ariaRef, apply the a11y attributes to it, rather than the immediate child.
         if (ariaRef?.current) {
-          ariaRef.current.setAttribute('aria-controls', dropdownId);
           ariaRef.current.setAttribute('aria-expanded', `${mergedVisible}`);
           ariaRef.current.setAttribute('aria-haspopup', 'true');
 
+          if (!ariaRef.current.hasAttribute('aria-controls')) {
+            ariaRef.current.setAttribute('aria-controls', dropdownId);
+          }
+
           if (!ariaRef.current.hasAttribute('role')) {
             ariaRef.current.setAttribute('role', 'button');
           }
diff --git a/src/components/List/List.test.tsx b/src/components/List/List.test.tsx
index cf236d6a8..2bc85497d 100644
--- a/src/components/List/List.test.tsx
+++ b/src/components/List/List.test.tsx
@@ -66,6 +66,16 @@ describe('List', () => {
     expect(container).toMatchSnapshot();
   });
 
+  test('Renders ol without crashing', () => {
+    const { container, getByTestId } = render(
+      <List {...listProps} listType="ol" />
+    );
+    const list = getByTestId('test-list');
+    expect(() => container).not.toThrowError();
+    expect(list).toBeTruthy();
+    expect(container).toMatchSnapshot();
+  });
+
   test('List is horizontal', () => {
     const { container } = render(<List {...listProps} layout="horizontal" />);
     expect(container.querySelector('.vertical')).toBeFalsy();
diff --git a/src/components/List/List.tsx b/src/components/List/List.tsx
index 6678b6b24..a4e7d0baa 100644
--- a/src/components/List/List.tsx
+++ b/src/components/List/List.tsx
@@ -34,6 +34,7 @@ export const List = <T extends any>({
   role,
   itemProps,
   getItem,
+  id,
   ...rest
 }: ListProps<T>) => {
   const htmlDir: string = useCanvasDirection();
@@ -252,13 +253,23 @@ export const List = <T extends any>({
     <div {...rest} className={classNames} style={style}>
       {getHeader()}
       {listType === 'ul' && (
-        <ul role={role} className={containerClasses} style={{ ...listStyle }}>
+        <ul
+          id={id}
+          role={role}
+          className={containerClasses}
+          style={{ ...listStyle }}
+        >
           {getItems()}
           {!!renderAdditionalItem && getAdditionalItem()}
         </ul>
       )}
       {listType === 'ol' && (
-        <ol role={role} className={containerClasses} style={{ ...listStyle }}>
+        <ol
+          id={id}
+          role={role}
+          className={containerClasses}
+          style={{ ...listStyle }}
+        >
           {getItems()}
           {!!renderAdditionalItem && getAdditionalItem()}
         </ol>
diff --git a/src/components/List/__snapshots__/List.test.tsx.snap b/src/components/List/__snapshots__/List.test.tsx.snap
index c31706a38..32f01f9a6 100644
--- a/src/components/List/__snapshots__/List.test.tsx.snap
+++ b/src/components/List/__snapshots__/List.test.tsx.snap
@@ -109,6 +109,115 @@ exports[`List List is horizontal 1`] = `
 </div>
 `;
 
+exports[`List Renders ol without crashing 1`] = `
+<div>
+  <div
+    class="my-ref-class"
+    data-testid="test-list"
+  >
+    <div
+      style="padding-left: 16px;"
+    >
+      <h2>
+        Header
+      </h2>
+    </div>
+    <ol
+      class="list-container my-list-class vertical"
+      role="list"
+    >
+      <li
+        class="list-item my-list-item-class"
+        style="padding: 8px 16px;"
+      >
+        <a
+          aria-disabled="false"
+          class="link-style full-width"
+          data-testid="User1"
+          href="#"
+          id="User1"
+          role="link"
+          target="_self"
+        >
+          User 1
+        </a>
+      </li>
+      <li
+        class="list-item my-list-item-class"
+        style="padding: 8px 16px;"
+      >
+        <a
+          aria-disabled="false"
+          class="link-style full-width"
+          data-testid="User2"
+          href="#"
+          id="User2"
+          role="link"
+          target="_self"
+        >
+          User 2
+        </a>
+      </li>
+      <li
+        class="list-item my-list-item-class"
+        style="padding: 8px 16px;"
+      >
+        <a
+          aria-disabled="false"
+          class="link-style full-width"
+          data-testid="User3"
+          href="#"
+          id="User3"
+          role="link"
+          target="_self"
+        >
+          User 3
+        </a>
+      </li>
+      <li
+        class="list-item my-list-item-class"
+        style="padding: 8px 16px;"
+      >
+        <a
+          aria-disabled="false"
+          class="link-style full-width"
+          data-testid="User4"
+          href="#"
+          id="User4"
+          role="link"
+          target="_self"
+        >
+          User 4
+        </a>
+      </li>
+      <li
+        class="list-item my-list-item-class"
+        style="padding: 8px 16px;"
+      >
+        <a
+          aria-disabled="false"
+          class="link-style full-width"
+          data-testid="User5"
+          href="#"
+          id="User5"
+          role="link"
+          target="_self"
+        >
+          User 5
+        </a>
+      </li>
+    </ol>
+    <div
+      style="padding-left: 16px;"
+    >
+      <h3>
+        Footer
+      </h3>
+    </div>
+  </div>
+</div>
+`;
+
 exports[`List Renders without crashing 1`] = `
 <div>
   <div
diff --git a/src/components/Select/Select.tsx b/src/components/Select/Select.tsx
index 75dae8e2b..cd5589800 100644
--- a/src/components/Select/Select.tsx
+++ b/src/components/Select/Select.tsx
@@ -873,6 +873,7 @@ export const Select: FC<SelectProps> = React.forwardRef(
                   options?.length > 0)
               }
               ref={dropdownRef}
+              role={'group'}
             >
               <div className={styles.selectInputWrapper}>
                 {/* When Dropdown is visible, place Pills in the reference element */}
diff --git a/src/components/Select/__snapshots__/Select.test.tsx.snap b/src/components/Select/__snapshots__/Select.test.tsx.snap
index 58e948c3d..d5ffc4b94 100644
--- a/src/components/Select/__snapshots__/Select.test.tsx.snap
+++ b/src/components/Select/__snapshots__/Select.test.tsx.snap
@@ -22,7 +22,7 @@ exports[`Select Renders empty options in multiple mode without crashing 1`] = `
               class="select-input-group input-group right-icon"
             >
               <input
-                aria-controls="dropdown-"
+                aria-controls="list-"
                 aria-disabled="false"
                 aria-expanded="false"
                 aria-haspopup="true"
@@ -99,7 +99,7 @@ exports[`Select Renders empty options in single mode without crashing 1`] = `
               class="select-input-group input-group right-icon"
             >
               <input
-                aria-controls="dropdown-"
+                aria-controls="list-"
                 aria-disabled="false"
                 aria-expanded="false"
                 aria-haspopup="true"
@@ -176,7 +176,7 @@ exports[`Select Renders null options in multiple mode without crashing 1`] = `
               class="select-input-group input-group right-icon"
             >
               <input
-                aria-controls="dropdown-"
+                aria-controls="list-"
                 aria-disabled="false"
                 aria-expanded="false"
                 aria-haspopup="true"
@@ -253,7 +253,7 @@ exports[`Select Renders null options in single mode without crashing 1`] = `
               class="select-input-group input-group right-icon"
             >
               <input
-                aria-controls="dropdown-"
+                aria-controls="list-"
                 aria-disabled="false"
                 aria-expanded="false"
                 aria-haspopup="true"
@@ -330,7 +330,7 @@ exports[`Select Renders with default value 1`] = `
               class="select-input-group input-group right-icon"
             >
               <input
-                aria-controls="dropdown-"
+                aria-controls="list-"
                 aria-disabled="false"
                 aria-expanded="false"
                 aria-haspopup="true"
@@ -477,7 +477,7 @@ exports[`Select Renders with default values when multiple 1`] = `
               class="select-input-group input-group right-icon"
             >
               <input
-                aria-controls="dropdown-"
+                aria-controls="list-"
                 aria-disabled="false"
                 aria-expanded="false"
                 aria-haspopup="true"
@@ -554,7 +554,7 @@ exports[`Select Renders without crashing 1`] = `
               class="select-input-group input-group right-icon"
             >
               <input
-                aria-controls="dropdown-"
+                aria-controls="list-"
                 aria-disabled="false"
                 aria-expanded="false"
                 aria-haspopup="true"
@@ -630,7 +630,7 @@ exports[`Select Select is large 1`] = `
             class="select-input-group input-group right-icon"
           >
             <input
-              aria-controls="dropdown-"
+              aria-controls="list-"
               aria-disabled="false"
               aria-expanded="false"
               aria-haspopup="true"
@@ -705,7 +705,7 @@ exports[`Select Select is medium 1`] = `
             class="select-input-group input-group right-icon"
           >
             <input
-              aria-controls="dropdown-"
+              aria-controls="list-"
               aria-disabled="false"
               aria-expanded="false"
               aria-haspopup="true"
@@ -780,7 +780,7 @@ exports[`Select Select is pill shaped 1`] = `
             class="select-input-group input-group right-icon"
           >
             <input
-              aria-controls="dropdown-"
+              aria-controls="list-"
               aria-disabled="false"
               aria-expanded="false"
               aria-haspopup="true"
@@ -855,7 +855,7 @@ exports[`Select Select is rectangle shaped 1`] = `
             class="select-input-group input-group right-icon"
           >
             <input
-              aria-controls="dropdown-"
+              aria-controls="list-"
               aria-disabled="false"
               aria-expanded="false"
               aria-haspopup="true"
@@ -930,7 +930,7 @@ exports[`Select Select is small 1`] = `
             class="select-input-group input-group right-icon"
           >
             <input
-              aria-controls="dropdown-"
+              aria-controls="list-"
               aria-disabled="false"
               aria-expanded="false"
               aria-haspopup="true"
@@ -1005,7 +1005,7 @@ exports[`Select Select is underline shaped 1`] = `
             class="select-input-group input-group right-icon"
           >
             <input
-              aria-controls="dropdown-"
+              aria-controls="list-"
               aria-disabled="false"
               aria-expanded="false"
               aria-haspopup="true"