[ClickAwayListener] Is being fired without an onClick event after upgrading react and react-dom to v17

[mforman1]

On my app I have a menu that worked fine until I upgraded react and react-dom.
After upgrading react and react-dom from version 16.14.0 to version 17.0.0- on menu click, the menu flashes-it opens and closes right away.

• 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 😯

On menu click- the menu opens and closes right away, for some reason the ClickAwayListener is being fired.

Expected Behavior 🤔

On menu click- the menu should open, on click away it should close.

Steps to Reproduce 🕹

Steps:

1. Upgrade react and react-dom to v17.0.0
2. Click the menu

Codesandbox:
https://codesandbox.io/s/material-ui-issue-forked-byfgm?file=/src/index.js Invalid. New repro: https://codesandbox.io/s/material-ui-issue-forked-13053?file=/src/Demo.js

Your Environment 🌎

Tech	Version
Material-UI/core	v4.11.0
React	v17.0.0
React-dom	v17.0.0
Chrome	v86.0.4240.75

[oliviertassinari]

Oh wow, here's what's going on:

1. Click on the icon button, a DOM event (a.) is triggered.
2. The event bubbles up to the React host.
3. React bubbles the event on each React component.
4. React calls the onClick handler on the icon button.
5. React renders the ClickAwayListener.
6. ClickAwayListener sets a listener on the document.
7. The drawer opens.
8. The same event (a.) keeps bubbling. It reaches the new listener set by the ClickAwayListener.
9. The drawer closes.

In React 16 the host in 2. is the document, in React 17 the host is an element in the body, hence the difference.

[oliviertassinari]

Do we really need to fix this? What's the use case for the ClickAwayListener here? The current workaround is to call the setState in a setTimeout().

This change internally produces the expected behavior of the codesandbox. It's unclear if we want to move forward with it.

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
index a25f43baca..e31c371924 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
@@ -49,6 +49,7 @@ function ClickAwayListener(props) {
     // Given developers can stop the propagation of the synthetic event,
     // we can only be confident with a positive value.
     const insideReactTree = syntheticEventRef.current;
@@ -130,9 +131,12 @@ function ClickAwayListener(props) {
       const mappedMouseEvent = mapEventPropToEvent(mouseEvent);
       const doc = ownerDocument(nodeRef.current);

-      doc.addEventListener(mappedMouseEvent, handleClickAway);
+      // Use a setTimeout to avoid capturing the click that has tricker the mount of the ClickAwayListener component.
+      const timeout = setTimeout(() => {
+        doc.addEventListener(mappedMouseEvent, handleClickAway);
+      });

       return () => {
+        clearTimeout(timeout);
         doc.removeEventListener(mappedMouseEvent, handleClickAway);
       };
     }

@eps1lon What do you think?

It's also probably time to do? :)

diff --git a/packages/material-ui/package.json b/packages/material-ui/package.json
index be6c895d41..c0ab3251f4 100644
--- a/packages/material-ui/package.json
+++ b/packages/material-ui/package.json
@@ -44,7 +44,7 @@
     "@emotion/core": "^10.0.27",
     "@emotion/styled": "^10.0.27",
     "@types/react": "^16.8.6",
-    "react": "^16.8.0",
+    "react": "^16.8.0 || ^17.0.0",
     "react-dom": "^16.8.0"
   },
   "peerDependenciesMeta": {

[dargue3]

This is a problem in our UI library built on top of Material as well. Curiously, changing the mouseEvent prop on ClickAwayListener from onClick → onMouseDown fixes the issue. I'm not aware enough of low-level JS events to determine whether this is a safe one-for-one swap or not.

We'd love to see it fixed though 🙏

[oliviertassinari]

onClick → onMouseDown

@dargue3 The only downside I'm aware of is that click away will fire false-positive when moving the scrollbar on nested containers (not the body).

[mbrevda]

What's the use case for the ClickAwayListener here?

The use case is being able to close the slide-out menu while clicking anywhere besides for the menu

[mforman1]

The use case is being able to close the slide-out menu while clicking anywhere besides for the menu

That's the use case 👆

[oliviertassinari]

I'm closing the use case isn't valid, use the onClose callback instead.

[mforman1]

Thank you for your immediate assistance,
as you suggested, switching to the onClose callback works fine:

<Drawer open={open} onClick={() => setTimeout(onClose, 1)}>

Thank you!

[stevewillard]

What about the use case where you have a <Popper /> and put a <ClickAwayListener /> on the children?

[oliviertassinari]

@stevewillard Do you have a reproduction?

[wkerswell-gresham]

Sandbox for the issue @stevewillard stated.
Using a popper with a clickaway listener in it.

[oliviertassinari]

@wkerswell-gresham Thanks. I would propose we move forward with #23215 (comment). Do we have another alternative?

[artisonian]

Is it safe to stop propagation instead?

[oliviertassinari]

@artisonian Do you have a working diff to propose? Stopping propagation is generally considered a bad practice. If the event should absolutely not be consumed by a parent, then yes, but we have to be certain.

[artisonian]

I spent some time in the source code...didn't realize the event listeners are being attached inside of an effect block. Never mind stopPropagation then.

[nickbouton]

I am seeing this issue as well with a <Popper /> containing a <ClickAwayListener /> since upgrading to React 17. Removing the ClickAwayListener resolves the issue but that doesn't solve our problem as we need it to dismiss our popover menus when clicking outside the menu.

[oliviertassinari]

@wkerswell-gresham A workaround: https://codesandbox.io/s/material-ui-issue-forked-zb3i5?file=/src/Demo.js

  console.log("settingsMenuOpen", settingsMenuOpen);

  const handleClick = (event) => {
    const target = event.currentTarget;
+   setTimeout(() => {
      setAnchorEl(anchorEl ? null : target);
+   });
  };

[valtism]

Stopping propagation on onClick handlers that trigger the popper being opened is another workaround.

[mforman1]

I'm just noticing we have the same issue with a <Snackbar />,
<ClickAwayListener /> is being fired and closes the <Snackbar /> right away.

Sandbox for the issue.

[oliviertassinari]

@mforman1 Thanks for the reproduction. Do you want to work on the proposed fix? We would probably only miss a test case to ensure we don't have regression in the future. It would also involve the react-next.diff file that we use to run all the tests with React 16 and 17.

[eps1lon]

I understand that everybody wants to switch right now but note that we don't officially support React 17 yet (hence the peer dependency mismatch warning you should see). We fixed most compat issue but some still slip through since we ourselves haven't fully switched yet.

Material-UI + React 17 is something you should consider as unstable so do your end-users a favor and don't switch to it in production yet. React 17 hasn't any big features for end-users anyway.

When it's safer to switch to React 17 we'll definitely announce that in the changelog. But it's only been out for a few days so the ecosystem needs some time to adjust. Thanks for understanding!

Current investigation:
useEffect timing changed in React 17 when portaled. So we're affected by facebook/react#20074 as well. It's interesting because I thought that event handlers should be attached synchronously. Though it's still weird to me how this used to work in class components where it was also attached synchronously. Though maybe it never did. Unfortunately the React is has been deprioritized so we have to come up with a fix in userland.

[eps1lon]

Tracking investigation in #23215 (comment).

Another workaround for now is

<Popper
  id="menuButton"
  open={settingsMenuOpen}
  anchorEl={anchorEl}
+  disablePortal
>

if you don't rely on portaling.

[valtism]

Will this fix be going into v4, or will it be v5 only?

[eps1lon]

Will this fix be going into v4, or will it be v5 only?

This fix will be backported to the next v4 feature release but that one will also include deprecation warnings. The recommendation in #23215 (comment) still stands: Material-UI v4 + React 17 is unstable and not fit for production yet.

[kushan1992]

<ClickAwayListener onClickAway={handleClickAway}>
      <Box sx={{ position: 'relative' }}>
        <button type="button" onClick={handleClick}>
          Open menu dropdown
        </button>
        {open ? (
          <Box sx={styles}>
            Click me, I will stay visible until you click outside.
          </Box>
        ) : null}
      </Box>
    </ClickAwayListener>

Always check this open condition when rendering middle of the popper.