fix(menu-full-screen): fix for new FocusTrap behavior

Sage · May 26, 2022 · 0c4b8b5 · 0c4b8b5
1 parent a2c795a
commit 0c4b8b5
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 85 deletions.
diff --git a/cypress/integration/common/menu.feature b/cypress/integration/common/menu.feature
@@ -119,31 +119,27 @@ Feature: Menu component
     When I click closeIcon
     Then Menu is in fullscreen mode is not visible
 
-  @ignore
-  # testing incorrect behaviour, ignored as that behaviour will be implemented as part of FE-5092
+  @positive
   Scenario: Check that close icon is focused in Fullscreen Menu
     Given I open "Menu Test" component page "menu-full-screen-story"
-    When I hit Tab key 1 times
+    When I continue to hit Tab key 1 times
     Then closeIcon has the border outline color "rgb(255, 181, 0)" and width "3px"
 
-  @ignore
-  # testing incorrect behaviour, ignored as that behaviour will be implemented as part of FE-5092
+  @positive
   Scenario: Check that inner Menu is available with tabbing in Fullscreen Menu
     Given I open "Menu Test" component page "menu-full-screen-story"
-    When I hit Tab key 5 times
+    When I continue to hit Tab key 5 times
     Then "fourth" inner menu element is focused
 
-  @ignore
-  # testing incorrect behaviour, ignored as that behaviour will be implemented as part of FE-5092
+  @positive
   Scenario: Check that previous inner Menu is available with shift tabbing in Fullscreen Menu
     Given I open "Menu Test" component page "menu-full-screen-story"
-      And I hit Tab key 6 times
+      And I continue to hit Tab key 6 times
     When I press Shift Tab on focused element
     Then "fourth" inner menu element is focused
 
-  @ignore
-  # testing incorrect behaviour, ignored as that behaviour will be implemented as part of separate task
+  @positive
   Scenario: Check that inner Menu without link is NOT available with tabbing in Fullscreen Menu
     Given I open "Menu Test" component page "menu-full-screen-story"
-    When I hit Tab key 8 times
+    When I continue to hit Tab key 8 times
     Then inner menu without active redirection is not focused
diff --git a/cypress/locators/menu/index.js b/cypress/locators/menu/index.js
@@ -26,5 +26,7 @@ export const submenuItem = (index) =>
 export const menuCanvas = () => cy.get(DLS_ROOT);
 export const fullscreenMenu = (index) =>
   cy.get(FULLSCREEN_MENU).find("div").eq(index);
+export const fullScreenMenuItem = (index) =>
+  cy.get(`${FULLSCREEN_MENU} ${MENU}`).find(`li:nth-child(${index})`);
 export const menu = () => cy.get(MENU);
 export const menuItem = () => cy.get(MENU_ITEM);
diff --git a/cypress/support/step-definitions/menu-steps.js b/cypress/support/step-definitions/menu-steps.js
@@ -12,6 +12,7 @@ import {
   fullscreenMenu,
   menu,
   menuItem,
+  fullScreenMenuItem,
 } from "../../locators/menu";
 import { searchInput, searchCrossIcon } from "../../locators/search/index";
 import { positionOfElement, keyCode } from "../helper";
@@ -181,21 +182,21 @@ Then("Menu is in fullscreen mode is not visible", () => {
 });
 
 Then("{string} inner menu element is focused", (position) => {
-  menuComponent(positionOfElement(position))
+  fullScreenMenuItem(positionOfElement(position))
     .find("ul > li")
     .eq(1)
     .children()
     .children()
     .should("have.css", "box-shadow")
     .and("contain", "rgb(255, 181, 0)");
-  menuComponent(positionOfElement(position))
+  fullScreenMenuItem(positionOfElement(position))
     .find("ul > li")
     .eq(1)
     .children()
     .children()
     .should("have.css", "background-color")
     .and("contain", "rgb(0, 126, 69)");
-  menuComponent(positionOfElement(position))
+  fullScreenMenuItem(positionOfElement(position))
     .find("ul > li")
     .eq(1)
     .children()

diff --git a/src/__internal__/focus-trap/focus-trap-utils.js b/src/__internal__/focus-trap/focus-trap-utils.js
@@ -1,12 +1,36 @@
 const defaultFocusableSelectors =
   'button:not([disabled]), [href], input:not([type="hidden"]):not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]';
 
+const waitForVisibleAndFocus = (element) => {
+  const INTERVAL = 10;
+  const MAX_TIME = 100;
+  let timeSoFar = 0;
+
+  const stylesMatch = () => {
+    const actualStyles = window.getComputedStyle(element);
+    return actualStyles.visibility === "visible";
+  };
+
+  const check = () => {
+    /* istanbul ignore else */
+    if (stylesMatch()) {
+      element.focus();
+    } else if (timeSoFar < MAX_TIME) {
+      setTimeout(check, INTERVAL);
+      timeSoFar += INTERVAL;
+    }
+    // just "fail" silently if maxTime exceeded - callback will never be called
+  };
+
+  check();
+};
+
 function setElementFocus(element) {
   if (typeof element === "function") {
     element();
   } else {
     const el = element.current || element;
-    el.focus();
+    waitForVisibleAndFocus(el);
   }
 }
 

diff --git a/src/components/menu/menu-full-screen/menu-full-screen.component.js b/src/components/menu/menu-full-screen/menu-full-screen.component.js
@@ -1,4 +1,4 @@
-import React, { useContext, useLayoutEffect, useRef } from "react";
+import React, { useContext, useRef } from "react";
 import PropTypes from "prop-types";
 
 import {
@@ -14,6 +14,7 @@ import IconButton from "../../icon-button";
 import Icon from "../../icon";
 import Portal from "../../portal";
 import MenuDivider from "../menu-divider/menu-divider.component";
+import useModalFocus from "../../../hooks/__internal__/useModalFocus";
 
 const MenuFullscreen = ({
   children,
@@ -26,27 +27,15 @@ const MenuFullscreen = ({
   const menuContentRef = useRef();
   const { menuType } = useContext(MenuContext);
 
+  const focusProps = useModalFocus(isOpen);
+
   const handleKeyDown = (ev) => {
     /* istanbul ignore else */
     if (Events.isEscKey(ev)) {
       onClose(ev);
     }
   };
 
-  useLayoutEffect(() => {
-    const checkTransitionEnd = () => {
-      menuContentRef.current.focus();
-    };
-
-    const wrapperRef = menuWrapperRef.current;
-
-    if (isOpen) {
-      wrapperRef.addEventListener("transitionend", checkTransitionEnd);
-    } else {
-      wrapperRef.removeEventListener("transitionend", checkTransitionEnd);
-    }
-  }, [isOpen]);
-
   const scrollVariants = {
     light: "light",
     dark: "dark",
@@ -64,14 +53,15 @@ const MenuFullscreen = ({
   return (
     <li aria-label="menu-fullscreen">
       <Portal>
-        <FocusTrap autoFocus={false} wrapperRef={menuWrapperRef}>
+        <FocusTrap wrapperRef={menuWrapperRef} isOpen={isOpen}>
           <StyledMenuFullscreen
             data-component="menu-fullscreen"
             ref={menuWrapperRef}
             isOpen={isOpen}
             menuType={menuType}
             startPosition={startPosition}
             onKeyDown={handleKeyDown}
+            {...focusProps}
             {...rest}
           >
             <StyledMenuFullscreenHeader
@@ -101,7 +91,6 @@ const MenuFullscreen = ({
                 flexDirection="column"
                 role="list"
                 inFullscreenView
-                tabIndex={-1}
               >
                 {React.Children.map(children, (child, index) => (
                   <MenuContext.Provider

diff --git a/src/components/menu/menu-full-screen/menu-full-screen.spec.js b/src/components/menu/menu-full-screen/menu-full-screen.spec.js
@@ -17,48 +17,53 @@ import {
   simulate,
 } from "../../../__spec_helper__/test-utils";
 import { baseTheme } from "../../../style/themes";
-import { StyledMenuItem, StyledMenuWrapper } from "../menu.style";
+import { StyledMenuItem } from "../menu.style";
 import menuConfigVariants from "../menu.config";
 
 const onClose = jest.fn();
 
-const render = ({ startPosition, isOpen, menuType = "light" }) => {
+// eslint-disable-next-line react/prop-types
+const TestMenu = ({ startPosition, isOpen }) => (
+  <MenuFullscreen
+    startPosition={startPosition}
+    isOpen={isOpen}
+    onClose={onClose}
+  >
+    <MenuItem maxWidth="200px" href="#">
+      Menu Item One
+    </MenuItem>
+    <MenuItem maxWidth="200px" onClick={() => {}} submenu="Menu Item Two">
+      <MenuItem maxWidth="200px" href="#">
+        Submenu Item One
+      </MenuItem>
+      <MenuItem maxWidth="200px" href="#">
+        Submenu Item Two
+      </MenuItem>
+    </MenuItem>
+    <MenuItem maxWidth="200px" href="#">
+      Menu Item Three
+    </MenuItem>
+    <MenuItem maxWidth="200px" href="#">
+      Menu Item Four
+    </MenuItem>
+    <MenuItem maxWidth="200px" submenu="Menu Item Five">
+      <MenuItem maxWidth="200px" href="#">
+        Submenu Item One
+      </MenuItem>
+      <MenuItem maxWidth="200px" href="#">
+        Submenu Item Two
+      </MenuItem>
+    </MenuItem>
+    <MenuItem maxWidth="200px" href="#">
+      Menu Item Six
+    </MenuItem>
+  </MenuFullscreen>
+);
+
+const render = ({ menuType = "light", ...props }) => {
   return mount(
     <MenuContext.Provider value={{ menuType }}>
-      <MenuFullscreen
-        startPosition={startPosition}
-        isOpen={isOpen}
-        onClose={onClose}
-      >
-        <MenuItem maxWidth="200px" href="#">
-          Menu Item One
-        </MenuItem>
-        <MenuItem maxWidth="200px" onClick={() => {}} submenu="Menu Item Two">
-          <MenuItem maxWidth="200px" href="#">
-            Submenu Item One
-          </MenuItem>
-          <MenuItem maxWidth="200px" href="#">
-            Submenu Item Two
-          </MenuItem>
-        </MenuItem>
-        <MenuItem maxWidth="200px" href="#">
-          Menu Item Three
-        </MenuItem>
-        <MenuItem maxWidth="200px" href="#">
-          Menu Item Four
-        </MenuItem>
-        <MenuItem maxWidth="200px" submenu="Menu Item Five">
-          <MenuItem maxWidth="200px" href="#">
-            Submenu Item One
-          </MenuItem>
-          <MenuItem maxWidth="200px" href="#">
-            Submenu Item Two
-          </MenuItem>
-        </MenuItem>
-        <MenuItem maxWidth="200px" href="#">
-          Menu Item Six
-        </MenuItem>
-      </MenuFullscreen>
+      <TestMenu {...props} />
     </MenuContext.Provider>
   );
 };
@@ -210,17 +215,23 @@ describe("MenuFullscreen", () => {
   });
 
   describe("focus behaviour", () => {
-    it("focuses the content container on open of menu", () => {
-      wrapper = render({ isOpen: true });
+    it("focuses the menu wrapper on open of menu", () => {
+      wrapper = mount(<TestMenu />);
+      wrapper.setProps({ isOpen: true });
 
       const element = wrapper.find(StyledMenuFullscreen).getDOMNode();
-      const event = new Event("transitionend", {
+      const startEvent = new Event("transitionstart", {
+        bubbles: true,
+        cancellable: true,
+      });
+      const endEvent = new Event("transitionend", {
         bubbles: true,
         cancellable: true,
       });
-      element.dispatchEvent(event);
+      element.dispatchEvent(startEvent);
+      element.dispatchEvent(endEvent);
 
-      expect(wrapper.find(StyledMenuWrapper)).toBeFocused();
+      expect(wrapper.find(StyledMenuFullscreen)).toBeFocused();
     });
   });
 });
diff --git a/src/components/menu/menu-full-screen/menu-full-screen.style.js b/src/components/menu/menu-full-screen/menu-full-screen.style.js
@@ -13,6 +13,7 @@ const StyledMenuFullscreen = styled.div`
   bottom: 0;
   height: 100vh;
   width: 100%;
+  outline: none;
 
   a,
   button,

diff --git a/src/hooks/__internal__/useModalFocus/useModalFocus.spec.tsx b/src/hooks/__internal__/useModalFocus/useModalFocus.spec.tsx
@@ -25,18 +25,13 @@ describe("useModalFocus custom hook", () => {
     wrapper.unmount();
   });
 
-  it("the container initially has no tabindex", () => {
-    expect(wrapper.find("div").props().tabIndex).toBeUndefined();
-  });
-
-  it("when the open argument is true, the wrapper has tabindex -1", () => {
-    wrapper.find("button").simulate("click");
-    expect(wrapper.find("div").props().tabIndex).toBe(-1);
+  it("the container initially has tabindex 0", () => {
+    expect(wrapper.find("div").props().tabIndex).toBe(0);
   });
 
   it("when open and the wrapper is blurred, the tabindex is removed", () => {
     wrapper.find("button").simulate("click");
-    expect(wrapper.find("div").props().tabIndex).toBe(-1);
+    expect(wrapper.find("div").props().tabIndex).toBe(0);
     wrapper.find("div").simulate("blur");
     expect(wrapper.find("div").props().tabIndex).toBeUndefined();
   });

diff --git a/src/hooks/__internal__/useModalFocus/useModalFocus.ts b/src/hooks/__internal__/useModalFocus/useModalFocus.ts
@@ -1,16 +1,19 @@
 import { useState, useEffect } from "react";
 
 export default function useModalFocus(open: boolean) {
-  const [tabIndex, setTabIndex] = useState<number | undefined>(undefined);
+  const [tabIndex, setTabIndex] = useState<number | undefined>(0);
 
   useEffect(() => {
+    // ideally would set to -1, but that fails Cypress tests due to Cypress bug.
+    // It's OK to set to 0 instead as the tabindex is removed on blur anyway.
     if (open) {
-      setTabIndex(-1);
+      setTabIndex(0);
     }
   }, [open]);
 
   // need to remove the container's tabindex after it has been used for initial focus,
-  // otherwise it becomes impossible to scroll any content using the keyboard
+  // otherwise (if tabindex is 0) the container would be reachable by keyboard tabbing which we don't want,
+  // and if the tabindex is -1 it makes it impossible to scroll any content using the keyboard
   const onBlur = () => {
     /* istanbul ignore else */
     if (open) {