[ClickAwayListener] Calling onClickAway straightaway when used with Select

[m4theushw]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When a Select is used in conjunction with the ClickAwayListener, the onClickAway prop is called straightaway after the select is open.

Expected Behavior 🤔

It should not call onClickAway while the Select is opening. onClickAway should only be called when the user clicks OUTSIDE the select while it's open.

Steps to Reproduce 🕹

Steps:

1. Open codesandbox
2. Click to open the select
3. Look at the console

Context 🔦

This issue was first raised in mui/mui-x#1314.

Proposed solution 💡

@oliviertassinari did some investigation in #18586 and I confirmed that the same behavior is happening here too. The click event is being called with the target body which forbids any attempt of the ClickAwayListener to check if it was inside or outside its children. I propose the following change based on Olivier's diff.

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx
index 6bb56f9c4e..984b7c22f0 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx
@@ -123,6 +123,12 @@ function ClickAwayListener(props: ClickAwayListenerProps): JSX.Element {
       return;
     }

+    // The click event couldn't find its target, ignore
+    const eventTarget = event.target as Node | null
+    if (eventTarget?.nodeName === 'BODY' && event.type === 'click') {
+      return;
+    }
+
     let insideDOM;

     // If not enough, can use https://github.com/DieterHolvoet/event-propagation-path/blob/master/propagationPath.js

Your Environment 🌎

`npx @material-ui/envinfo`

  System:
    OS: Linux 5.4 Ubuntu 20.04.2 LTS (Focal Fossa)
  Binaries:
    Node: 15.12.0 - ~/.nvm/versions/node/v15.12.0/bin/node
    Yarn: 1.22.10 - ~/.nvm/versions/node/v15.12.0/bin/yarn
    npm: 7.6.3 - ~/.nvm/versions/node/v15.12.0/bin/npm
  Browsers:
    Edge: 89.0.774.63
  npmPackages:
    @emotion/react: ^11.0.0 => 11.1.5
    @emotion/styled: ^11.0.0 => 11.1.5
    @material-ui/codemod:  5.0.0-alpha.27
    @material-ui/core:  5.0.0-alpha.28
    @material-ui/data-grid:  4.0.0-alpha.21
    @material-ui/docs:  5.0.0-alpha.28
    @material-ui/envinfo:  1.1.6
    @material-ui/icons:  5.0.0-alpha.28
    @material-ui/lab:  5.0.0-alpha.28
    @material-ui/styled-engine:  5.0.0-alpha.25
    @material-ui/styled-engine-sc:  5.0.0-alpha.25
    @material-ui/styles:  5.0.0-alpha.28
    @material-ui/system:  5.0.0-alpha.28
    @material-ui/types:  5.1.7
    @material-ui/unstyled:  5.0.0-alpha.28
    @material-ui/utils:  5.0.0-alpha.28
    @types/react: ^17.0.3 => 17.0.3
    react: ^17.0.1 => 17.0.1
    react-dom: ^17.0.1 => 17.0.1
    styled-components:  5.2.1
    typescript: ^4.1.2 => 4.2.3

[reapedjuggler]

Hello, Thank you for the issue Can I work on this issue?

[m4theushw]

Yes, feel free to send a PR.

[oliviertassinari]

I had a closer look at the issue. The problem seems to be related to how, on mouse down, another element overlays with the target, meaning, on mouse up, the target is different. I doubt that my proposed fix can work since eventTarget.nodeName === 'BODY' can easily happen when the body children don't expend enough.

[reapedjuggler]

Thank you for the information @oliviertassinari, Would you like to propose a better solution to the problem. I would be glad to implement it :)

[oliviertassinari]

It's a hard problem. I can't find any good solution.

[reapedjuggler]

I picked that one because of the good first issue label, Thank you for removing it 👍

[m4theushw]

I tried to understand why it's not intercepting the click event to make syntheticEventRef.current true. What I found is that the event is not being propagated down the tree. I didn't go any further but this could be investigated.

[oliviertassinari]

Actually, there might be a way. The issue is specific to the click event. We could resolve the outcome at mouseup, and wait for the click event (that might not trigger if we are on a scrollbar) to resolve it:

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx
index 6bb56f9c4e..1b4c4a7c00 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.tsx
@@ -74,6 +74,8 @@ function ClickAwayListener(props: ClickAwayListenerProps): JSX.Element {
   const nodeRef = React.useRef<Element>(null);
   const activatedRef = React.useRef(false);
   const syntheticEventRef = React.useRef(false);
+  const ignoreNonTouchEvents = React.useRef(false);
+  const triggerClickAwayRef = React.useRef(false);

   React.useEffect(() => {
     // Ensure that this component is not "activated" synchronously.
@@ -99,6 +101,15 @@ function ClickAwayListener(props: ClickAwayListenerProps): JSX.Element {
   // and hitting left arrow to move the cursor in a text input etc.
   // Only special HTML elements have these default behaviors.
   const handleClickAway = useEventCallback((event: MouseEvent | TouchEvent) => {
+    if (event.type.indexOf('touch') === 0) {
+      ignoreNonTouchEvents.current = true;
+    } else if (ignoreNonTouchEvents.current) {
+      setTimeout(() => {
+        ignoreNonTouchEvents.current = false;
+      });
+      return;
+    }
+
     // Given developers can stop the propagation of the synthetic event,
     // we can only be confident with a positive value.
     const insideReactTree = syntheticEventRef.current;
@@ -140,7 +151,20 @@ function ClickAwayListener(props: ClickAwayListenerProps): JSX.Element {
         );
     }

-    if (!insideDOM && (disableReactTree || !insideReactTree)) {
+    const triggerClickAway = !insideDOM && (disableReactTree || !insideReactTree);
+
+    if (event.type === 'click') {
+      triggerClickAwayRef.current = triggerClickAway;
+      return;
+    }
+
+    if (triggerClickAway) {
+      onClickAway(event);
+    }
+  });
+
+  const handleClick = useEventCallback((event: MouseEvent | TouchEvent) => {
+    if (triggerClickAwayRef.current) {
       onClickAway(event);
     }
   });
@@ -185,15 +209,17 @@ function ClickAwayListener(props: ClickAwayListenerProps): JSX.Element {
     return undefined;
   }, [handleClickAway, touchEvent]);

-  if (mouseEvent !== false) {
+  if (mouseEvent === 'onMouseDown' || mouseEvent === 'onMouseUp') {
     childrenProps[mouseEvent] = createHandleSynthetic(mouseEvent);
+  } else if (mouseEvent === 'onClick') {
+    childrenProps.onMouseUp = createHandleSynthetic(mouseEvent);
   }

   React.useEffect(() => {
-    if (mouseEvent !== false) {
-      const mappedMouseEvent = mapEventPropToEvent(mouseEvent);
-      const doc = ownerDocument(nodeRef.current);
+    const mappedMouseEvent = mapEventPropToEvent(mouseEvent);
+    const doc = ownerDocument(nodeRef.current);

+    if (mouseEvent === 'onMouseDown' || mouseEvent === 'onMouseUp') {
       doc.addEventListener(mappedMouseEvent, handleClickAway);

       return () => {
@@ -201,6 +227,16 @@ function ClickAwayListener(props: ClickAwayListenerProps): JSX.Element {
       };
     }

+    if (mouseEvent === 'onClick') {
+      doc.addEventListener('mouseup', handleClickAway);
+      doc.addEventListener('click', handleClick);
+
+      return () => {
+        doc.removeEventListener('mouseup', handleClickAway);
+        doc.removeEventListener('click', handleClick);
+      };
+    }
+
     return undefined;
   }, [handleClickAway, mouseEvent]);

[oliviertassinari]

@m4theushw What do you think about the above diff?

[m4theushw]

It's a viable solution, but I think we're adding complexity to solve a bug which is a side effect of another bug. Let me explain. The ClickAwayListener works nicely when it can catch the click event on the children and check later if the event happened inside the tree. However, in the scenario ClickAwayListener + Select, this click event is dispatched with body as target, which triggers the onClickAway . We should be looking into making the event to be dispatched with the right target.

About this last point, I dived into the inner components of the Select component and I found something interesting. It listens for a mousedown event and when this occurs it changes the open prop of the Popover to true which immediately places a div occupying the whole screen. When the click event is dispatched, the browser can't find the right target (because there's a div that wasn't there before) and fallbacks to body or other element. I created a codesandbox illustrating the problem with primitives. In the gif below the click should have the button as target too.

I managed to get the click event with the right target by delaying a couple of ms the insertion of this div. In Material-UI, it means delaying the update of the open prop in the SelectInput. As another consequence, this also fixes a potential bug if some user attaches a click event to the Select component, which would never be trigged because it doesn't propagate.

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 9923a9b2a5..596a9a5c15 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -74,6 +74,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
   const [menuMinWidthState, setMenuMinWidthState] = React.useState();
   const [openState, setOpenState] = React.useState(false);
   const handleRef = useForkRef(ref, inputRefProp);
+  const [delayedOpen, setDelayedOpen] = React.useState(false);

   const handleDisplayRef = React.useCallback((node) => {
     displayRef.current = node;
@@ -234,6 +235,18 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {

   const open = displayNode !== null && (isOpenControlled ? openProp : openState);

+  React.useEffect(() => {
+    let timeout = null;
+    if (open) {
+      timeout = setTimeout(() => {
+        setDelayedOpen(true);
+      }, 100);
+    } else {
+      setDelayedOpen(false);
+    }
+    return () => clearTimeout(timeout);
+  }, [open]);
+
   const handleBlur = (event) => {
     // if open event.stopImmediatePropagation
     if (!open && onBlur) {
@@ -428,7 +441,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       <Menu
         id={`menu-${name || ''}`}
         anchorEl={displayNode}
-        open={open}
+        open={delayedOpen}
         onClose={handleClose}
         {...MenuProps}
         MenuListProps={{