Material-ui: [ClickAwayListener] Handle portaled element

[ccmartell]

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

Current Behavior 😯

When a select component is within a popper element and disablePortal is true, the dropdown items are positioned absolutely in relation to the popper element and not the page. This is also true for any absolutely positioned container that uses transform (which is what popper is using).

Expected Behavior 🤔

Expected behavior would be that the position of the dropdown would match the location of the select element and display correctly.

Steps to Reproduce 🕹

I have mocked the issue here

Steps:

1. Place absolutely positioned component and add a transform (or just use popper)
2. Add select component

Context 🔦

When using a clickaway listener with popper to display a popup, a portaled menu will cause the clickaway listener to fire, closing the popper and losing our work. We then were able to use disablePortal in the MenuProps, but that causes this issue. We have done a workaround using Popover as it doesn't have a transform and displays the Select menu correctly.

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.7.0 |
| React | v16.11.0 |
| Browser | Chrome |
| TypeScript | No |

[oliviertassinari]

Your problem is related to a "couple" of others 😆:

• #17636: A change of the Popover's positioning logic to use Popper would fix the placement issue.
• #11243: However, even if we fix the placement issue, the backdrop logic won't be correct. Either the backdrop needs to be portal (to block interactions with the rest of the page) or we need to use a click away listener.
  
  
  
  • If we want to support a portal backdrop, I would suggest the addition of a event.muiHandled logic to prevent the click away listener to trigger (cf. swipeable drawer) or a stop propagation or a prevent default.
  
  
  • If you nest two click away listeners, and if you click outside of both, both will trigger. I doubt it should be the case if they are nested. I would suggest the addition of a event.muiHandled logic to prevent it (cf. swipeable drawer).
  
  

[oliviertassinari]

We have a full reproduction example in #17990 that we can leverage. Based on it, I could come up with this potential solution.

Before

After

I would propose the following fix, let me know what you think about it:

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
index c689731b6..8e38b6635 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
@@ -16,10 +16,17 @@ function mapEventPropToEvent(eventProp) {
  * For instance, if you need to hide a menu when people click anywhere else on your page.
  */
 const ClickAwayListener = React.forwardRef(function ClickAwayListener(props, ref) {
-  const { children, mouseEvent = 'onClick', touchEvent = 'onTouchEnd', onClickAway } = props;
+  const {
+    children,
+    disableReactTree = false,
+    mouseEvent = 'onClick',
+    onClickAway,
+    touchEvent = 'onTouchEnd',
+  } = props;
   const movedRef = React.useRef(false);
   const nodeRef = React.useRef(null);
   const mountedRef = React.useRef(false);
+  const syntheticEvent = React.useRef(false);

   React.useEffect(() => {
     mountedRef.current = true;
@@ -40,6 +47,9 @@ const ClickAwayListener = React.forwardRef(function ClickAwayListener(props, ref
   const handleRef = useForkRef(children.ref, handleOwnRef);

   const handleClickAway = useEventCallback(event => {
+    const insideReactTree = syntheticEvent.current;
+    syntheticEvent.current = false;
+
     // Ignore events that have been `event.preventDefault()` marked.
     if (event.defaultPrevented) {
       return;
@@ -67,7 +77,8 @@ const ClickAwayListener = React.forwardRef(function ClickAwayListener(props, ref
     if (
       doc.documentElement &&
       doc.documentElement.contains(event.target) &&
-      !nodeRef.current.contains(event.target)
+      !nodeRef.current.contains(event.target) &&
+      (disableReactTree || !insideReactTree)
     ) {
       onClickAway(event);
     }
@@ -106,7 +117,39 @@ const ClickAwayListener = React.forwardRef(function ClickAwayListener(props, ref
     return undefined;
   }, [handleClickAway, mouseEvent]);

-  return <React.Fragment>{React.cloneElement(children, { ref: handleRef })}</React.Fragment>;
+  const handleSyntheticMouse = event => {
+    syntheticEvent.current = true;
+
+    const childrenProps = children.props;
+    if (childrenProps[mouseEvent]) {
+      childrenProps[mouseEvent](event);
+    }
+  };
+
+  const handleSyntheticTouch = event => {
+    syntheticEvent.current = true;
+
+    const childrenProps = children.props;
+    if (childrenProps[touchEvent]) {
+      childrenProps[touchEvent](event);
+    }
+  };
+
+  const childrenProps = {};
+
+  if (mouseEvent !== false) {
+    childrenProps[mouseEvent] = handleSyntheticMouse;
+  }
+
+  if (touchEvent !== false) {
+    childrenProps[touchEvent] = handleSyntheticTouch;
+  }
+
+  return (
+    <React.Fragment>
+      {React.cloneElement(children, { ref: handleRef, ...childrenProps })}
+    </React.Fragment>
+  );
 });

 ClickAwayListener.propTypes = {
@@ -114,6 +157,11 @@ ClickAwayListener.propTypes = {
    * The wrapped element.
    */
   children: elementAcceptingRef.isRequired,
+  /**
+   * If `true`, the React tree is ignored and only the DOM tree is considered.
+   * This prop changes how portaled elements are handled.
+   */
+  disableReactTree: PropTypes.bool,
   /**
    * The mouse event to listen to. You can disable the listener by providing `false`.
    */

cc @Izhaki as it might impact your hook API
cc @dmtrKovalenko for context

[eps1lon]

I don't think this is a good first issue.

This discussion should happen in the react core. Special casing where we want to ignore the component tree for event bubbling is (as with most special cases) not a good idea.

We should rather explore alternative APIs or contribute these use cases to the appropriate issues in the react repositories. Otherwise we introduce more suprises and bundle size into the API.

[oliviertassinari]

@eps1lon Do you mean that we shouldn't expose a disableReactTree prop?

Yeah, I think that we can wait to see a compelling use case for this prop 👍. Including the React tree should likely be the default behavior, and changed, independently from the DOM tree.

[Izhaki]

I need ClickAwayListener to intercept a mouseup outside and element with drag - nothing to do with MUI.

And the idea of having a dependency in mui/core for this seems to me a bit off.

The point I'm trying to make is that this functionality is possibly far more generic to be included in MUI. I'd consider just extracting it to its own package (I've seen at least 3 bespoke implementation of it already in other libraries, who would probably be delighted to see a package for it. currently one needs to have come across mui to know it exist).

[oliviertassinari]

And the idea of having a dependency in mui/core for this seems to me a bit off.

@Izhaki What makes you uncomfortable about it? Material-UI's primary mission is to bring material for developers to build UIs. Material Design is our default theme.
We may implement other design specs in the future (in 2-3 years, once we support enough rich components?).

[Izhaki]

What makes you uncomfortable about it?

I'm writing a library that has nothing to do with mui. I just need ClickAwayListener.

[oliviertassinari]

@Izhaki Would you feel better with a @material-ui/click-away-listener package?

[Izhaki]

Yes! (and No!)

I mean - all things are equal tree-shaking wise. But it's different having dependency on @material-ui/click-away-listener compared to @material-ui/core it communicates a completely different thing.

But it's a bit of rabbit hole really... why do this for clickAwayListener and not useForkRef (for instance)?

So you can make a case for @material-ui/utils described as "Non mui-specific React and Dom utils..." where ClickAwayListener goes, and useForkRef, and anything that others may need even if not using core.

[oliviertassinari]

@Izhaki I'm trying to navigate with your fears and the scope of our options. Exposing all our modules as packages could be interesting, especially with unstyled components.

[Izhaki]

I have updated my comment.

I don't really have a firm solution for this. But I do think there's a lot in this project that would be useful for people who don't use core, and it would be nice not to expose these via core.

[oliviertassinari]

@Izhaki I was recently thinking of another aspect of this problem: the documentation. I was considering adding a link to sources in the docs page (had the idea from Reach) and to automatically pull the bundle size info from (https://material-ui.com/size-snapshot), rather than hard coding it in the markdown. I think that it would reduce people's fears of bloatness and to better audit the quality of what they are "buying" (using).

[oliviertassinari]

Ok so after @eps1lon's recommendation:

• We should delay the introduction of the disableReactTree prop, it would be great to wait for the outcome of https://github.com/facebook/react/issues/11387 to settle on this prop. We are working on the edges of React, it's better to maximize for future flexibility. We want to reduce the chance of introducing a breaking change in the future.
• Clickaway shouldn't fire by default when coming from a descendant of a portaled element. At least if we based this on the fact that: 1. from the use cases shared, it makes a better default 2. React changed it moving from the unstable to the final portal API.
• In the future, we might be able to rely on the React's scopes .containsNode() API instead of a double event listener. At least, if React releases this API, we might in v5.
• It's also related to https://github.com/Pomax/react-onclickoutside/issues/273. They propose the usage of hardcoded selectors to ignore elements. This could be a solution but it doesn't seem to take full advantage of the options available: meaning, something that works outside of the box.
• I think that we can move forward with the above fix. The change of behavior is considered a bug fix. I hope it won't break people logic. If it does, we will have to revert and use a different strategy. At the minimum, we will learn more about the underlying problem, which is great.

[ccmartell]

Hi. I thought I would chime in on this. I think the scope of my issue has changed from popper positioning issues to clickaway listener issues? My main concern was the positioning of the popup menu when using a transform (similar to how popper works). Popover does not use a position transform but instead uses absolute positioning. The popover works correctly with a select dropdown field. See my original codesandbox for the example.

[oliviertassinari]

When using a clickaway listener with popper to display a popup, a portaled menu will cause the clickaway listener to fire, closing the popper and losing our work.

@ccmartell We are going after the root cause.
I don't think that we should spend the time required to solve the positioning issue you have reported.

[SofianeDjellouli]

Hi,
I'll give it a try by:

• using Popper in Popover,
• handling the backdrop logic.
  If that's OK with you.

[victorhurdugaci]

Hey folks! I was about to open a dupe of this issue. In case you're looking for more repros or test cases, here's my scenario in which onClickAway fires incorrectly.

import React from "react";
import { ClickAwayListener, Select, MenuItem } from "@material-ui/core";

export default function App() {
  return (
    <ClickAwayListener onClickAway={() => console.log("Clicked away")}>
      <Select value={"A"}>
        <MenuItem value={"A"}>A</MenuItem>
        <MenuItem value={"B"}>B</MenuItem>
        <MenuItem value={"C"}>C</MenuItem>
      </Select>
    </ClickAwayListener>
  );
}

If you click the Select, it fires the onClickAway event.

[oliviertassinari]

@victorhurdugaci Do you want to complete the proposed patch in https://github.com/mui-org/material-ui/issues/18586#issuecomment-562183410 with a pull request :)?

[NMinhNguyen]

I’ll try take a look this weekend unless someone else gets to it before me :) I know that you’ve provided a diff to apply but would like to get familiar with the underlying problem first @oliviertassinari

[pawlarius]

Hi, I am sorry to continue commenting on this closed PR, but I just tried the disableReactTree props, while it fixed to don't trigger onClickAway from ClickAwayListener, it still has some issues when using Select inside a ClickAwayListener wrapped by Popper.

I have made the demo here https://codesandbox.io/s/material-demo-89jyg?file=/demo.js.
In the demo, the Select are using props MenuProps={{disablePortal: true}}, if I dont use that, the Select will trigger ClickAwayListener onClickAway even though I am using disableReactTree={true}.

But if I use MenuProps={{disablePortal: true}}, it will cause wrong positioning and size of the Select Menu popup..

[oliviertassinari]

@pawlarius What problem to you try to solve?

[depiction]

@pawlarius I'm seeing the same issue in my project. I passed a classes prop to MenuProps then added custom styles to correct the position.

    <Select
        MenuProps={{
            classes: { paper: classes.selectPaper },
            disablePortal: true
        }}
    >
        <MenuItem value={10}>Ten</MenuItem>
    </Select>

    const styles = theme => ({
        selectPaper: {
            // hack to fix Material UI popover position when disablePortal is true
            left: `${theme.spacing(2)}px !important`,
            maxHeight: 'none !important',
            top: `${theme.spacing(2)}px !important`
        }
    })

[pawlarius]

@oliviertassinari the issue i am having is the same with what @depiction post above.. the issue is when we are using disablePortal: true, it kinda break the positioning and we need to add additional styling as a temporary hack to fix the position..

[pawlarius]

Sorry, I am not sure my comment is marked as outdated.. i tried update my demo here to 4.10.1 and still having the same issue.

[NMinhNguyen]

@pawlarius Would you like to create a new issue perhaps? Otherwise it's a bit hard to track discussions on a closed issue.