From 48808bc32b4437729a2d3aa74f1f1013ea354c8e Mon Sep 17 00:00:00 2001
From: Robin Zigmond <robin.zigmond@sage.com>
Date: Fri, 5 Aug 2022 14:11:14 +0100
Subject: [PATCH] fix(popover-container): ensure clicks inside popover do not
 close it

Fix bug where portals inside a component did not automatically work with useClickAwayListener.
Refactor useClickAwayListener to no longer take an array of refs and return a handler which can be
attached to the parent component and stops the clickAway handler firing on the document - this
automatically handlers any React children no matter whether they are rendered as DOM children or
not. Remove any refs in parent components which are now unneeded.

fix 5322
---
 cypress/e2e/common/popoverContainer.feature   | 10 ++++-
 src/components/date/date.component.js         | 33 +++++++-------
 src/components/date/date.spec.js              |  6 ++-
 .../__internal__/submenu/submenu.component.js |  7 ++-
 .../multi-action-button.spec.tsx.snap         |  1 +
 .../multi-action-button.component.tsx         |  3 +-
 .../popover-container-test.stories.tsx        | 23 ++++++++++
 .../popover-container.component.tsx           |  5 +--
 .../split-button/split-button.component.tsx   |  3 +-
 .../useClickAwayListener.spec.tsx             | 16 ++++---
 .../useClickAwayListener.ts                   | 43 +++++++++++--------
 11 files changed, 101 insertions(+), 49 deletions(-)

diff --git a/cypress/e2e/common/popoverContainer.feature b/cypress/e2e/common/popoverContainer.feature
index b4fc311be7..75d7ec31c1 100644
--- a/cypress/e2e/common/popoverContainer.feature
+++ b/cypress/e2e/common/popoverContainer.feature
@@ -43,4 +43,12 @@ Feature: Popover container component
     Examples:
       | key   |
       | Enter |
-      | Space |
\ No newline at end of file
+      | Space |
+
+  @positive
+  Scenario Outline: Select component inside Popover container does not close the Popover container when an option is selected
+    Given I open "Popover container Test" component page "with select"
+      And I open popover container
+      And I click on Select text
+    When I select the "green" Option
+    Then Popover container is visible
diff --git a/src/components/date/date.component.js b/src/components/date/date.component.js
index b1b71ea0c1..9cc2982adb 100644
--- a/src/components/date/date.component.js
+++ b/src/components/date/date.component.js
@@ -57,7 +57,6 @@ const DateInput = ({
   const wrapperRef = useRef();
   const parentRef = useRef();
   const inputRef = useRef();
-  const pickerRef = useRef();
   const alreadyFocused = useRef(false);
   const isBlurBlocked = useRef(false);
   const focusedViaPicker = useRef(false);
@@ -107,6 +106,19 @@ const DateInput = ({
     return ev;
   };
 
+  const handleClickAway = () => {
+    if (open) {
+      alreadyFocused.current = true;
+      inputRef.current.focus();
+      isBlurBlocked.current = false;
+      inputRef.current.blur();
+      setOpen(false);
+      alreadyFocused.current = false;
+    }
+  };
+
+  const handleClickInside = useClickAwayListener(handleClickAway, "mousedown");
+
   const handleChange = (ev) => {
     isInitialValue.current = false;
     onChange(buildCustomEvent(ev));
@@ -214,6 +226,8 @@ const DateInput = ({
   };
 
   const handleMouseDown = (ev) => {
+    handleClickInside(ev);
+
     if (disabled || readOnly) {
       return;
     }
@@ -235,8 +249,9 @@ const DateInput = ({
     handleMouseDown(e);
   };
 
-  const handlePickerMouseDown = () => {
+  const handlePickerMouseDown = (ev) => {
     isBlurBlocked.current = true;
+    handleClickInside(ev);
   };
 
   const assignInput = (input) => {
@@ -304,19 +319,6 @@ const DateInput = ({
     return value;
   };
 
-  const handleClickAway = () => {
-    if (open) {
-      alreadyFocused.current = true;
-      inputRef.current.focus();
-      isBlurBlocked.current = false;
-      inputRef.current.blur();
-      setOpen(false);
-      alreadyFocused.current = false;
-    }
-  };
-
-  useClickAwayListener([parentRef, pickerRef], handleClickAway, "mousedown");
-
   return (
     <StyledDateInput
       ref={wrapperRef}
@@ -359,7 +361,6 @@ const DateInput = ({
         onDayClick={handleDayClick}
         minDate={minDate}
         maxDate={maxDate}
-        ref={pickerRef}
         pickerMouseDown={handlePickerMouseDown}
         open={open}
       />
diff --git a/src/components/date/date.spec.js b/src/components/date/date.spec.js
index bb20bf22ea..ead0c22cec 100644
--- a/src/components/date/date.spec.js
+++ b/src/components/date/date.spec.js
@@ -957,7 +957,11 @@ function simulateMouseDownOnPicker(wrapper) {
   };
 
   act(() => {
-    input.simulate("mousedown", mockEvent);
+    input
+      .getDOMNode()
+      .dispatchEvent(
+        new MouseEvent("mousedown", { ...mockEvent, bubbles: true })
+      );
   });
 }
 
diff --git a/src/components/menu/__internal__/submenu/submenu.component.js b/src/components/menu/__internal__/submenu/submenu.component.js
index 543fd719a0..c0e2485900 100644
--- a/src/components/menu/__internal__/submenu/submenu.component.js
+++ b/src/components/menu/__internal__/submenu/submenu.component.js
@@ -48,7 +48,6 @@ const Submenu = React.forwardRef(
     const [submenuOpen, setSubmenuOpen] = useState(false);
     const [submenuFocusIndex, setSubmenuFocusIndex] = useState(undefined);
     const [characterString, setCharacterString] = useState("");
-    const submenuRef = useRef();
     const formattedChildren = React.Children.map(children, (child) => {
       if (child.type === ScrollableBlock) {
         return [...child.props.children];
@@ -253,16 +252,16 @@ const Submenu = React.forwardRef(
       // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [characterString]);
 
-    useClickAwayListener([submenuRef], handleClickAway);
+    const handleClickInside = useClickAwayListener(handleClickAway);
 
     if (inFullscreenView) {
       return (
         <StyledSubmenuWrapper
           data-component="submenu-wrapper"
-          ref={submenuRef}
           inFullscreenView={inFullscreenView}
           menuType={menuContext.menuType}
           asPassiveItem={asPassiveItem}
+          onClick={handleClickInside}
         >
           <StyledMenuItemWrapper
             {...rest}
@@ -309,8 +308,8 @@ const Submenu = React.forwardRef(
         data-component="submenu-wrapper"
         onMouseOver={!clickToOpen ? () => openSubmenu() : undefined}
         onMouseLeave={() => closeSubmenu()}
-        ref={submenuRef}
         isSubmenuOpen={submenuOpen}
+        onClick={handleClickInside}
       >
         <StyledMenuItemWrapper
           {...rest}
diff --git a/src/components/multi-action-button/__snapshots__/multi-action-button.spec.tsx.snap b/src/components/multi-action-button/__snapshots__/multi-action-button.spec.tsx.snap
index fd4f10acda..4d9268d3ba 100644
--- a/src/components/multi-action-button/__snapshots__/multi-action-button.spec.tsx.snap
+++ b/src/components/multi-action-button/__snapshots__/multi-action-button.spec.tsx.snap
@@ -361,6 +361,7 @@ exports[`MultiActionButton when rendered should match the snapshot 1`] = `
   aria-haspopup="true"
   className="c0"
   data-component="multi-action-button"
+  onClick={[Function]}
   onMouseLeave={[Function]}
 >
   <button
diff --git a/src/components/multi-action-button/multi-action-button.component.tsx b/src/components/multi-action-button/multi-action-button.component.tsx
index 3fc8a2e853..3805b57c90 100644
--- a/src/components/multi-action-button/multi-action-button.component.tsx
+++ b/src/components/multi-action-button/multi-action-button.component.tsx
@@ -143,12 +143,13 @@ export const MultiActionButton = ({
     </Popover>
   );
 
-  useClickAwayListener([ref], hideButtons);
+  const handleClick = useClickAwayListener(hideButtons);
 
   return (
     <StyledMultiActionButton
       aria-haspopup="true"
       onMouseLeave={hideButtons}
+      onClick={handleClick}
       ref={ref}
       data-component="multi-action-button"
       data-element={dataElement}
diff --git a/src/components/popover-container/popover-container-test.stories.tsx b/src/components/popover-container/popover-container-test.stories.tsx
index 4c561012e0..cd0d94f69e 100644
--- a/src/components/popover-container/popover-container-test.stories.tsx
+++ b/src/components/popover-container/popover-container-test.stories.tsx
@@ -3,6 +3,7 @@ import React from "react";
 
 import specialCharacters from "../../__internal__/utils/argTypes/specialCharacters";
 import PopoverContainer from "./popover-container.component";
+import { Select, Option } from "../select";
 
 export default {
   title: "Popover Container/Test",
@@ -27,6 +28,28 @@ export const Default = ({
   open: boolean;
 }) => <PopoverContainer title={title || titleSpecialCharacters} open={open} />;
 
+export const WithSelect = () => {
+  return (
+    <div style={{ height: 100 }}>
+      <PopoverContainer
+        containerAriaLabel="popover-container"
+        openButtonAriaLabel="open"
+        title="select example"
+      >
+        <Select label="my select">
+          <Option value="red" text="red" />
+          <Option value="green" text="green" />
+          <Option value="blue" text="blue" />
+        </Select>
+      </PopoverContainer>
+    </div>
+  );
+};
+
+WithSelect.story = {
+  name: "with select",
+};
+
 Default.story = {
   name: "default",
   args: {
diff --git a/src/components/popover-container/popover-container.component.tsx b/src/components/popover-container/popover-container.component.tsx
index 0abf80eb06..d8e98ee8c4 100644
--- a/src/components/popover-container/popover-container.component.tsx
+++ b/src/components/popover-container/popover-container.component.tsx
@@ -151,7 +151,6 @@ export const PopoverContainer = ({
   const isControlled = open !== undefined;
   const [isOpenInternal, setIsOpenInternal] = useState(false);
 
-  const ref = useRef<HTMLDivElement>(null);
   const closeButtonRef = useRef<HTMLButtonElement>(null);
   const openButtonRef = useRef<HTMLButtonElement>(null);
   const guid = useRef(createGuid());
@@ -209,14 +208,14 @@ export const PopoverContainer = ({
     if (onClose) onClose(e);
   };
 
-  useClickAwayListener([ref], handleClickAway, "mousedown");
+  const handleClick = useClickAwayListener(handleClickAway, "mousedown");
 
   return (
     <PopoverContainerWrapperStyle
       data-component="popover-container"
       role="region"
       aria-labelledby={popoverContainerId}
-      ref={ref}
+      onMouseDown={handleClick}
     >
       {renderOpenComponent(renderOpenComponentProps)}
       <Transition
diff --git a/src/components/split-button/split-button.component.tsx b/src/components/split-button/split-button.component.tsx
index f9d4f0fa38..9984306e83 100644
--- a/src/components/split-button/split-button.component.tsx
+++ b/src/components/split-button/split-button.component.tsx
@@ -242,12 +242,13 @@ export const SplitButton = ({
     );
   }
 
-  useClickAwayListener([splitButtonNode], hideButtons);
+  const handleClick = useClickAwayListener(hideButtons);
 
   return (
     <StyledSplitButton
       aria-haspopup="true"
       onMouseLeave={hideButtons}
+      onClick={handleClick}
       ref={splitButtonNode}
       {...componentTags()}
       {...filterStyledSystemMarginProps(rest)}
diff --git a/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.spec.tsx b/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.spec.tsx
index 2e8818d107..8111ed6cc4 100644
--- a/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.spec.tsx
+++ b/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.spec.tsx
@@ -1,4 +1,4 @@
-import React, { useRef } from "react";
+import React from "react";
 import { render, screen, fireEvent } from "@testing-library/react";
 import "@testing-library/jest-dom";
 import useClickAwayListener from "./useClickAwayListener";
@@ -8,13 +8,19 @@ interface ClickAwayProps {
   eventTypeId?: "mousedown" | "click";
 }
 
-const MockComponent = ({ handleClickAway, eventTypeId }: ClickAwayProps) => {
-  const ref = useRef<HTMLDivElement>(null);
+const eventHandlerPropNames = { click: "onClick", mousedown: "onMouseDown" };
 
-  useClickAwayListener([ref], handleClickAway, eventTypeId);
+const MockComponent = ({
+  handleClickAway,
+  eventTypeId = "click",
+}: ClickAwayProps) => {
+  const stopPropagation = useClickAwayListener(handleClickAway, eventTypeId);
+  const stopPropagationProp = {
+    [eventHandlerPropNames[eventTypeId]]: stopPropagation,
+  };
 
   return (
-    <div data-testid="target-element" ref={ref}>
+    <div data-testid="target-element" {...stopPropagationProp}>
       Child
     </div>
   );
diff --git a/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.ts b/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.ts
index 4d8e9c0140..7de0392977 100644
--- a/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.ts
+++ b/src/hooks/__internal__/useClickAwayListener/useClickAwayListener.ts
@@ -1,29 +1,38 @@
-import React, { useEffect, useRef } from "react";
+import { useEffect, useRef, useCallback } from "react";
+
+// Needs to also take Portals into account (so can't just check DOM containment), but ideally without using
+// event.stopPropagation() which could have unexpected and frustrating consequences for consumers.
+// Simple approach taken from https://github.com/facebook/react/issues/10962#issuecomment-444622208
 
 export default (
-  targets: React.RefObject<HTMLElement>[],
   handleClickAway: (ev: Event) => void,
   eventTypeId: "mousedown" | "click" = "click"
 ) => {
-  const targetsRef = useRef(targets);
-  targetsRef.current = targets;
+  const clickIsInside = useRef(false);
 
-  useEffect(() => {
-    const fnClickAway = (ev: Event) => {
-      const clickedElements = targetsRef.current.filter(
-        (targetRef: React.RefObject<HTMLElement>) =>
-          targetRef.current?.contains(ev?.target as Node)
-      );
-
-      if (!clickedElements?.length) {
-        handleClickAway(ev);
+  const onDocumentClick = useCallback(
+    (ev: Event) => {
+      if (clickIsInside.current) {
+        clickIsInside.current = false;
+        return;
       }
-    };
 
-    document.addEventListener(eventTypeId, fnClickAway as EventListener);
+      handleClickAway(ev);
+    },
+    [handleClickAway]
+  );
+
+  const onInsideClick = useCallback(() => {
+    clickIsInside.current = true;
+  }, []);
+
+  useEffect(() => {
+    document.addEventListener(eventTypeId, onDocumentClick);
 
     return function cleanup() {
-      document.removeEventListener(eventTypeId, fnClickAway as EventListener);
+      document.removeEventListener(eventTypeId, onDocumentClick);
     };
-  }, [handleClickAway, eventTypeId]);
+  }, [onDocumentClick, eventTypeId]);
+
+  return onInsideClick;
 };