From 5151bde6ce7515fd393b7cf06a838d7deb7b67e2 Mon Sep 17 00:00:00 2001
From: Yossi Saadi <yossisa@monday.com>
Date: Mon, 29 Jan 2024 13:41:28 +0200
Subject: [PATCH] fix(MenuItem): tooltip should on all item and not only on
 title (#1914)

Co-authored-by: Tal Koren <talkor@gmail.com>
Co-authored-by: Tal Koren <talko@monday.com>
---
 src/components/Menu/Menu/Menu.module.scss     |  1 +
 .../__tests__/__snapshots__/menu.jest.js.snap | 14 ++++++
 .../Menu/Menu/__tests__/menu-interactions.js  | 40 +++++++++++----
 .../Menu/MenuItem/MenuItem.module.scss        |  7 +++
 src/components/Menu/MenuItem/MenuItem.tsx     |  6 ++-
 .../Menu/MenuItem/__tests__/MenuItem.jest.js  |  4 +-
 .../menuItem-snapshots.jest.js.snap           | 49 +++++++++++++++++++
 ...litButtonMenu-snapshot-tests.jest.tsx.snap | 14 ++++++
 src/tests/interactions-utils.ts               | 14 ++++--
 9 files changed, 130 insertions(+), 19 deletions(-)

diff --git a/src/components/Menu/Menu/Menu.module.scss b/src/components/Menu/Menu/Menu.module.scss
index d6a7819cfe..311bdec285 100644
--- a/src/components/Menu/Menu/Menu.module.scss
+++ b/src/components/Menu/Menu/Menu.module.scss
@@ -3,6 +3,7 @@
 .menu {
   margin: unset;
   padding: unset;
+  position: relative;
 }
 
 .menu:focus {
diff --git a/src/components/Menu/Menu/__tests__/__snapshots__/menu.jest.js.snap b/src/components/Menu/Menu/__tests__/__snapshots__/menu.jest.js.snap
index 78e4c18d46..d81bb98f2f 100644
--- a/src/components/Menu/Menu/__tests__/__snapshots__/menu.jest.js.snap
+++ b/src/components/Menu/Menu/__tests__/__snapshots__/menu.jest.js.snap
@@ -38,6 +38,13 @@ exports[`Snapshots renders correctly with children 1`] = `
     >
       item 1
     </div>
+    <div
+      aria-hidden={true}
+      className="hiddenTitle"
+      tabIndex={-1}
+    >
+      item 1
+    </div>
     
     <div
       style={
@@ -64,6 +71,13 @@ exports[`Snapshots renders correctly with children 1`] = `
     >
       item 2
     </div>
+    <div
+      aria-hidden={true}
+      className="hiddenTitle"
+      tabIndex={-1}
+    >
+      item 2
+    </div>
     
     <div
       style={
diff --git a/src/components/Menu/Menu/__tests__/menu-interactions.js b/src/components/Menu/Menu/__tests__/menu-interactions.js
index 181cb345e1..ba15e367f6 100644
--- a/src/components/Menu/Menu/__tests__/menu-interactions.js
+++ b/src/components/Menu/Menu/__tests__/menu-interactions.js
@@ -19,33 +19,47 @@ const TWO_DEPTHS_MENU_TEXTS = {
   TOP_MENU_NON_SUB_MENU_ITEM: "Another item"
 };
 
+const HIDDEN_ELEMENT_SELECTOR = "[aria-hidden]";
+
 const showSubSubMenusOnHover = async canvas => {
   const menuElement = getMenuElement(canvas);
 
-  const topMenuItem = getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.TOP_MENU_SUB_MENU_ITEM);
+  const topMenuItem = getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.TOP_MENU_SUB_MENU_ITEM, {
+    ignore: HIDDEN_ELEMENT_SELECTOR
+  });
   await userEvent.hover(topMenuItem);
 
-  const innerMenuItem = getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM);
+  const innerMenuItem = getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM, {
+    ignore: HIDDEN_ELEMENT_SELECTOR
+  });
   await userEvent.hover(innerMenuItem);
 
   // validate showing sub sub item
   const optionToSelect = await waitForElementVisible(() =>
-    within(menuElement).findByText(TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM)
+    within(menuElement).findByText(TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM, { ignore: HIDDEN_ELEMENT_SELECTOR })
   );
   await clickElement(optionToSelect);
   expect(document.activeElement).toHaveTextContent(TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM);
 
   //close the sub-menus on hovering the top-level menu
-  await userEvent.hover(getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.TOP_MENU_NON_SUB_MENU_ITEM));
-  expect(canvas.queryByText(TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM)).not.toBeInTheDocument();
-  expect(canvas.queryByText(TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM)).not.toBeInTheDocument();
+  await userEvent.hover(
+    getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.TOP_MENU_NON_SUB_MENU_ITEM, { ignore: HIDDEN_ELEMENT_SELECTOR })
+  );
+  expect(
+    canvas.queryByText(TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM, { ignore: HIDDEN_ELEMENT_SELECTOR })
+  ).not.toBeInTheDocument();
+  expect(
+    canvas.queryByText(TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM, { ignore: HIDDEN_ELEMENT_SELECTOR })
+  ).not.toBeInTheDocument();
 };
 
 const showSubSubMenusWithKeyboard = async canvas => {
   const menuElement = getMenuElement(canvas);
 
   //set the initial focus, to make the keyboard events work
-  const topMenuItem = getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.TOP_MENU_SUB_MENU_ITEM);
+  const topMenuItem = getByText(menuElement, TWO_DEPTHS_MENU_TEXTS.TOP_MENU_SUB_MENU_ITEM, {
+    ignore: HIDDEN_ELEMENT_SELECTOR
+  });
   await userEvent.click(topMenuItem);
 
   //open sub menu
@@ -53,7 +67,9 @@ const showSubSubMenusWithKeyboard = async canvas => {
   await pressNavigationKey(NavigationCommand.DOWN_ARROW);
   await pressNavigationKey(NavigationCommand.RIGHT_ARROW);
   await waitForElementVisible(() =>
-    within(menuElement).findByText(new RegExp(`^${TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM}$`))
+    within(menuElement).findByText(new RegExp(`^${TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM}$`), {
+      ignore: HIDDEN_ELEMENT_SELECTOR
+    })
   );
   expectActiveElementToHavePartialText(TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM);
 
@@ -62,13 +78,17 @@ const showSubSubMenusWithKeyboard = async canvas => {
   await pressNavigationKey(NavigationCommand.DOWN_ARROW);
   await pressNavigationKey(NavigationCommand.RIGHT_ARROW);
   await waitForElementVisible(() =>
-    within(menuElement).findByText(new RegExp(`^${TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM}$`))
+    within(menuElement).findByText(new RegExp(`^${TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM}$`), {
+      ignore: HIDDEN_ELEMENT_SELECTOR
+    })
   );
   expectActiveElementToHavePartialText(TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM);
 
   //close sub-sub-menu - using left arrow
   await pressNavigationKey(NavigationCommand.LEFT_ARROW);
-  expect(canvas.queryByText(menuElement, TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM)).not.toBeInTheDocument();
+  expect(
+    canvas.queryByText(menuElement, TWO_DEPTHS_MENU_TEXTS.SUB_SUB_MENU_ITEM, { ignore: HIDDEN_ELEMENT_SELECTOR })
+  ).not.toBeInTheDocument();
   expectActiveElementToHavePartialText(TWO_DEPTHS_MENU_TEXTS.SUB_MENU_ITEM);
 
   //close sub-menu - using escape
diff --git a/src/components/Menu/MenuItem/MenuItem.module.scss b/src/components/Menu/MenuItem/MenuItem.module.scss
index 26ef76e94b..e6735007c1 100644
--- a/src/components/Menu/MenuItem/MenuItem.module.scss
+++ b/src/components/Menu/MenuItem/MenuItem.module.scss
@@ -103,3 +103,10 @@
   flex-grow: 1;
   padding-block: 1px;
 }
+
+.hiddenTitle {
+  width: 100%;
+  position: absolute;
+  left: 0;
+  opacity: 0;
+}
diff --git a/src/components/Menu/MenuItem/MenuItem.tsx b/src/components/Menu/MenuItem/MenuItem.tsx
index 9ed9f6c8ed..7a653cd22c 100644
--- a/src/components/Menu/MenuItem/MenuItem.tsx
+++ b/src/components/Menu/MenuItem/MenuItem.tsx
@@ -334,13 +334,15 @@ const MenuItem: VibeComponent<MenuItemProps> & {
           content={shouldShowTooltip ? finalTooltipContent : null}
           position={tooltipPosition}
           showDelay={tooltipShowDelay}
-          // Tooltip should be on a whole MenuItem, but it's a breaking change - should be fixed in the next major and then this can be removed
-          moveBy={icon && tooltipPosition === Tooltip.positions.LEFT ? { main: 30 } : undefined}
           {...tooltipProps}
         >
           <div ref={titleRef} className={styles.title}>
             {title}
           </div>
+          {/* Tooltip should be on a whole MenuItem, but it's a breaking change (tooltip adds span) - should be fixed in the next major and then this div be removed */}
+          <div className={styles.hiddenTitle} aria-hidden tabIndex={-1}>
+            {title}
+          </div>
         </Tooltip>
         {label && <Label kind={Label.kinds.LINE} text={label} />}
         {renderSubMenuIconIfNeeded()}
diff --git a/src/components/Menu/MenuItem/__tests__/MenuItem.jest.js b/src/components/Menu/MenuItem/__tests__/MenuItem.jest.js
index 45dbd1688a..285ebce879 100644
--- a/src/components/Menu/MenuItem/__tests__/MenuItem.jest.js
+++ b/src/components/Menu/MenuItem/__tests__/MenuItem.jest.js
@@ -16,8 +16,8 @@ describe("<MenuItem />", () => {
   });
 
   it("should be able to render menu item text", async () => {
-    const { getByText } = render(<MenuItem title={title} />);
-    const menuItemElement = getByText(title);
+    const { getAllByText } = render(<MenuItem title={title} />);
+    const menuItemElement = getAllByText(title);
     await waitFor(() => expect(menuItemElement).toBeTruthy());
   });
 
diff --git a/src/components/Menu/MenuItem/__tests__/__snapshots__/menuItem-snapshots.jest.js.snap b/src/components/Menu/MenuItem/__tests__/__snapshots__/menuItem-snapshots.jest.js.snap
index 93f3585808..b565e4c348 100644
--- a/src/components/Menu/MenuItem/__tests__/__snapshots__/menuItem-snapshots.jest.js.snap
+++ b/src/components/Menu/MenuItem/__tests__/__snapshots__/menuItem-snapshots.jest.js.snap
@@ -14,6 +14,13 @@ exports[`Snapshots renders correctly when disabled 1`] = `
   >
     my item
   </div>
+  <div
+    aria-hidden={true}
+    className="hiddenTitle"
+    tabIndex={-1}
+  >
+    my item
+  </div>
   
   <div
     style={
@@ -42,6 +49,13 @@ exports[`Snapshots renders correctly when selected 1`] = `
   >
     my item
   </div>
+  <div
+    aria-hidden={true}
+    className="hiddenTitle"
+    tabIndex={-1}
+  >
+    my item
+  </div>
   
   <div
     style={
@@ -70,6 +84,13 @@ exports[`Snapshots renders correctly with a label 1`] = `
   >
     my item
   </div>
+  <div
+    aria-hidden={true}
+    className="hiddenTitle"
+    tabIndex={-1}
+  >
+    my item
+  </div>
   <span
     className=""
     data-testid="label"
@@ -112,6 +133,13 @@ exports[`Snapshots renders correctly with custom class name 1`] = `
     className="title"
   >
     
+  </div>
+  <div
+    aria-hidden={true}
+    className="hiddenTitle"
+    tabIndex={-1}
+  >
+    
   </div>
   
   <div
@@ -140,6 +168,13 @@ exports[`Snapshots renders correctly with empty props 1`] = `
     className="title"
   >
     
+  </div>
+  <div
+    aria-hidden={true}
+    className="hiddenTitle"
+    tabIndex={-1}
+  >
+    
   </div>
   
   <div
@@ -191,6 +226,13 @@ exports[`Snapshots renders correctly with title and SVG icon 1`] = `
   >
     my item
   </div>
+  <div
+    aria-hidden={true}
+    className="hiddenTitle"
+    tabIndex={-1}
+  >
+    my item
+  </div>
   
   <div
     style={
@@ -231,6 +273,13 @@ exports[`Snapshots renders correctly with title and font icon 1`] = `
   >
     my item
   </div>
+  <div
+    aria-hidden={true}
+    className="hiddenTitle"
+    tabIndex={-1}
+  >
+    my item
+  </div>
   
   <div
     style={
diff --git a/src/components/SplitButton/SplitButtonMenu/__tests__/__snapshots__/splitButtonMenu-snapshot-tests.jest.tsx.snap b/src/components/SplitButton/SplitButtonMenu/__tests__/__snapshots__/splitButtonMenu-snapshot-tests.jest.tsx.snap
index f2d08698fd..5ff403f2c8 100644
--- a/src/components/SplitButton/SplitButtonMenu/__tests__/__snapshots__/splitButtonMenu-snapshot-tests.jest.tsx.snap
+++ b/src/components/SplitButton/SplitButtonMenu/__tests__/__snapshots__/splitButtonMenu-snapshot-tests.jest.tsx.snap
@@ -26,6 +26,13 @@ exports[`SplitButtonMenu renders correctly 1`] = `
     >
       Test 1
     </div>
+    <div
+      aria-hidden={true}
+      className="hiddenTitle"
+      tabIndex={-1}
+    >
+      Test 1
+    </div>
     
     <div
       style={
@@ -52,6 +59,13 @@ exports[`SplitButtonMenu renders correctly 1`] = `
     >
       Test 2
     </div>
+    <div
+      aria-hidden={true}
+      className="hiddenTitle"
+      tabIndex={-1}
+    >
+      Test 2
+    </div>
     
     <div
       style={
diff --git a/src/tests/interactions-utils.ts b/src/tests/interactions-utils.ts
index 5ca2477061..4cf41e3470 100644
--- a/src/tests/interactions-utils.ts
+++ b/src/tests/interactions-utils.ts
@@ -1,5 +1,5 @@
 import { fireEvent, queries, userEvent, within } from "@storybook/testing-library";
-import { BoundFunctions, Screen, waitFor } from "@testing-library/react";
+import { BoundFunctions, Screen, SelectorMatcherOptions, waitFor } from "@testing-library/react";
 import { NavigationCommand as NavigationCommandType } from "./constants";
 import { expect } from "@storybook/jest";
 
@@ -145,8 +145,8 @@ export const getFirstByClassName = (className: string) => {
   return document.getElementsByClassName(className)[0];
 };
 
-export const getByRole = (rootElement: HTMLElement | BoundFunctions<typeof queries>, role: string) => {
-  return getWithin(rootElement).getByRole(role);
+export const getByRole = (rootElement: HTMLElement | BoundFunctions<typeof queries>, role: string, options = {}) => {
+  return getWithin(rootElement).getByRole(role, options);
 };
 
 export const getAllByRole = (rootElement: HTMLElement | BoundFunctions<typeof queries>, role: string) => {
@@ -161,8 +161,12 @@ export const getAllByLabelText = (rootElement: HTMLElement, text: string) => {
   return getWithin(rootElement).getAllByLabelText(text);
 };
 
-export const getByText = (rootElement: HTMLElement | BoundFunctions<typeof queries>, text: string) => {
-  return getWithin(rootElement).getByText(text);
+export const getByText = (
+  rootElement: HTMLElement | BoundFunctions<typeof queries>,
+  text: string,
+  options: SelectorMatcherOptions = {}
+) => {
+  return getWithin(rootElement).getByText(text, options);
 };
 
 export const getAllByText = (rootElement: HTMLElement | BoundFunctions<typeof queries>, text: string) => {