Make highlight Actions menu items (Edit and Delete) buttons

[DISCO-307] For clarity, changed `ol` to `menu` Also simplified return value of DropdownItem.
openstax · Aug 27, 2024 · d5b5e50 · d5b5e50
1 parent 4ef6866
commit d5b5e50
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 54 deletions.
diff --git a/src/app/components/Dropdown.spec.tsx b/src/app/components/Dropdown.spec.tsx
@@ -149,9 +149,10 @@ describe('Dropdown', () => {
     </TestContainer>);
 
     renderer.act(() => {
-      const [button1, button2] = component.root.findAllByType('a');
-      button1.props.onClick(mockEv);
-      button2.props.onClick(mockEv);
+      const items = component.root.findAll((i) => i.props.onClick && i.type === 'button');
+
+      items.forEach((i) => i.props.onClick(mockEv));
+      expect(items.length).toBe(2);
     });
 
     expect(mockEv.preventDefault).toHaveBeenCalledTimes(2);

diff --git a/src/app/components/Dropdown.tsx b/src/app/components/Dropdown.tsx
@@ -172,13 +172,14 @@ function TrappingDropdownList(props: object) {
   useTrapTabNavigation(ref);
 
   return (
-    <ol ref={ref} {...props} />
+    <menu ref={ref} {...props} />
   );
 }
 
 
 // tslint:disable-next-line:variable-name
 export const DropdownList = styled(TrappingDropdownList)`
+  list-style: none;
   margin: 0;
   padding: 0.6rem 0;
   background: ${theme.color.neutral.formBackground};
@@ -237,22 +238,23 @@ const DropdownItemContent = ({
   });
   return <FormattedMessage id={message}>
     {(msg) => href
-      ? <a href={href} tabIndex={0} onClick={onClick} target={target} {...analyticsDataProps}>{prefix}{msg}</a>
-      /*
-        this should be a button but Safari and firefox don't support focusing buttons
-        which breaks the tab transparent dropdown
-        https://bugs.webkit.org/show_bug.cgi?id=22261
-        https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
-      */
-      // eslint-disable-next-line jsx-a11y/anchor-is-valid
-      : <a
+      ? <a
+        role='button'
+        href={href}
+        tabIndex={0}
+        onClick={onClick}
+        target={target}
+        {...analyticsDataProps}
+      >{prefix}{msg}</a>
+      // Safari support tab-navigation of buttons; this operates with space or Enter
+      : <button
+        type='button'
         tabIndex={0}
-        href=''
         onClick={onClick ? flow(preventDefault, onClick) : preventDefault}
         {...analyticsDataProps}
       >
         {prefix}{msg}
-      </a>
+      </button>
     }
   </FormattedMessage>;
 };
@@ -261,9 +263,9 @@ const DropdownItemContent = ({
 export const DropdownItem = ({ariaMessage, ...contentProps}: DropdownItemProps) => {
   const intl = useIntl();
 
-  return ariaMessage
-    ? <li aria-label={intl.formatMessage({id: ariaMessage})}><DropdownItemContent {...contentProps}/></li>
-    : <li><DropdownItemContent {...contentProps} /></li>;
+  return <li aria-label={ariaMessage ? intl.formatMessage({id: ariaMessage}) : undefined}>
+    <DropdownItemContent {...contentProps}/>
+  </li>;
 };
 
 interface CommonDropdownProps {

diff --git a/src/app/components/__snapshots__/DotMenu.spec.tsx.snap b/src/app/components/__snapshots__/DotMenu.spec.tsx.snap
@@ -136,6 +136,7 @@ exports[`Dropdown matches snapshot 1`] = `
 }
 
 .c12 {
+  list-style: none;
   margin: 0;
   padding: 0.6rem 0;
   background: #f5f5f5;
@@ -224,28 +225,29 @@ exports[`Dropdown matches snapshot 1`] = `
         </svg>
       </div>
     </button>
-    <ol
+    <menu
       className="c12"
     >
       <li>
-        <a
-          href=""
+        <button
           onClick={[Function]}
           tabIndex={0}
+          type="button"
         >
           Delete
-        </a>
+        </button>
       </li>
       <li>
         <a
           href="/wooo"
           onClick={[Function]}
+          role="button"
           tabIndex={0}
         >
           Edit
         </a>
       </li>
-    </ol>
+    </menu>
   </div>
   <button
     aria-label="Actions"
@@ -409,6 +411,7 @@ exports[`Dropdown matches snapshot on right align 1`] = `
 }
 
 .c12 {
+  list-style: none;
   margin: 0;
   padding: 0.6rem 0;
   background: #f5f5f5;
@@ -497,29 +500,30 @@ exports[`Dropdown matches snapshot on right align 1`] = `
         </svg>
       </div>
     </button>
-    <ol
+    <menu
       className="c12"
       rightAlign={true}
     >
       <li>
-        <a
-          href=""
+        <button
           onClick={[Function]}
           tabIndex={0}
+          type="button"
         >
           Delete
-        </a>
+        </button>
       </li>
       <li>
         <a
           href="/wooo"
           onClick={[Function]}
+          role="button"
           tabIndex={0}
         >
           Edit
         </a>
       </li>
-    </ol>
+    </menu>
   </div>
   <button
     aria-label="Actions"

diff --git a/src/app/components/__snapshots__/Dropdown.spec.tsx.snap b/src/app/components/__snapshots__/Dropdown.spec.tsx.snap
@@ -92,6 +92,7 @@ exports[`Dropdown matches snapshot 1`] = `
 }
 
 .c6 {
+  list-style: none;
   margin: 0;
   padding: 0.6rem 0;
   background: #f5f5f5;
@@ -155,28 +156,29 @@ exports[`Dropdown matches snapshot 1`] = `
     >
       show more
     </button>
-    <ol
+    <menu
       className="c6"
     >
       <li>
-        <a
-          href=""
+        <button
           onClick={[Function]}
           tabIndex={0}
+          type="button"
         >
           Delete
-        </a>
+        </button>
       </li>
       <li>
         <a
           href="/wooo"
           onClick={[Function]}
+          role="button"
           tabIndex={0}
         >
           Edit
         </a>
       </li>
-    </ol>
+    </menu>
   </div>
   <button
     className="c4 c5"
@@ -400,6 +402,7 @@ exports[`Dropdown matches snapshot for tab hidden (open) 1`] = `
 }
 
 .c4 {
+  list-style: none;
   margin: 0;
   padding: 0.6rem 0;
   background: #f5f5f5;
@@ -461,27 +464,28 @@ exports[`Dropdown matches snapshot for tab hidden (open) 1`] = `
   >
     show more
   </button>
-  <ol
+  <menu
     className="c4"
   >
     <li>
-      <a
-        href=""
+      <button
         onClick={[Function]}
         tabIndex={0}
+        type="button"
       >
         Delete
-      </a>
+      </button>
     </li>
     <li>
       <a
         href="/wooo"
         onClick={[Function]}
+        role="button"
         tabIndex={0}
       >
         Edit
       </a>
     </li>
-  </ol>
+  </menu>
 </div>
 `;
diff --git a/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap b/src/app/content/highlights/components/SummaryPopup/__snapshots__/ContextMenu.spec.tsx.snap
@@ -513,6 +513,7 @@ exports[`ContextMenu match snapshot when open 1`] = `
 }
 
 .c13 {
+  list-style: none;
   margin: 0;
   padding: 0.6rem 0;
   background: #f5f5f5;
@@ -644,7 +645,7 @@ exports[`ContextMenu match snapshot when open 1`] = `
     <div
       className="c12"
     >
-      <ol
+      <menu
         className="c13"
       >
         <li>
@@ -967,10 +968,10 @@ exports[`ContextMenu match snapshot when open 1`] = `
         <li
           aria-label="Add note"
         >
-          <a
-            href=""
+          <button
             onClick={[Function]}
             tabIndex={0}
+            type="button"
           >
             <svg
               aria-hidden="true"
@@ -986,15 +987,15 @@ exports[`ContextMenu match snapshot when open 1`] = `
               />
             </svg>
             Add note
-          </a>
+          </button>
         </li>
         <li
           aria-label="Delete"
         >
-          <a
-            href=""
+          <button
             onClick={[Function]}
             tabIndex={0}
+            type="button"
           >
             <svg
               aria-hidden="true"
@@ -1010,12 +1011,13 @@ exports[`ContextMenu match snapshot when open 1`] = `
               />
             </svg>
             Delete
-          </a>
+          </button>
         </li>
         <li>
           <a
             data-analytics-region="MH gotohighlight"
             href="/link/to/highlight"
+            role="button"
             tabIndex={0}
             target="_blank"
           >
@@ -1035,7 +1037,7 @@ exports[`ContextMenu match snapshot when open 1`] = `
             Go to highlight
           </a>
         </li>
-      </ol>
+      </menu>
     </div>
   </div>
 </div>

diff --git a/src/app/content/highlights/components/__snapshots__/DisplayNote.spec.tsx.snap b/src/app/content/highlights/components/__snapshots__/DisplayNote.spec.tsx.snap
@@ -419,6 +419,7 @@ exports[`DisplayNote matches snapshot when focused with opened dropdown 1`] = `
 }
 
 .c12 {
+  list-style: none;
   margin: 0;
   padding: 0.6rem 0;
   background: #f5f5f5;
@@ -612,28 +613,28 @@ exports[`DisplayNote matches snapshot when focused with opened dropdown 1`] = `
         </svg>
       </div>
     </button>
-    <ol
+    <menu
       className="c11 c12"
     >
       <li>
-        <a
-          href=""
+        <button
           onClick={[Function]}
           tabIndex={0}
+          type="button"
         >
           Edit
-        </a>
+        </button>
       </li>
       <li>
-        <a
-          href=""
+        <button
           onClick={[Function]}
           tabIndex={0}
+          type="button"
         >
           Delete
-        </a>
+        </button>
       </li>
-    </ol>
+    </menu>
   </div>
   <svg
     aria-hidden="true"