mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
94.05k stars 32.32k forks source link

[ClickAwayListener] Calling onClickAway straightaway when used with Select #25578

Open m4theushw opened 3 years ago

m4theushw commented 3 years ago

Current Behavior 😯

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

clickawaylistener

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 https://github.com/mui-org/material-ui-x/issues/1314.

Proposed solution πŸ’‘

@oliviertassinari did some investigation in https://github.com/mui-org/material-ui/issues/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 commented 3 years ago

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

m4theushw commented 3 years ago

Yes, feel free to send a PR.

oliviertassinari commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

reapedjuggler commented 3 years ago

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

m4theushw commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@m4theushw What do you think about the above diff?

m4theushw commented 3 years ago

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.

mousedown

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={{
oliviertassinari commented 3 years ago

@m4theushw What if the mousedown -> mouseup interaction lasts for above 100ms, say 1s? It would still trigger a click event, and the clickaway callback would trigger with the wrong target. It would also force developers testing the component to sleep by this arbitrary amount in order to open the select.

AFAIK, this option is not viable.

m4theushw commented 3 years ago

What if the mousedown -> mouseup interaction lasts for above 100ms, say 1s? It would still trigger a click event, and the clickaway callback would trigger with the wrong target.

Yeah, missed that. Clicking and holding invalidates my solution.

I have another alternative. The idea is to cancel the next click event (with the wrong target) in the capture phase and redispatch it with the same target of the mousedown event.

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 9923a9b2a5..b066148b4d 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -141,6 +141,13 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     event.preventDefault();
     displayRef.current.focus();

+    const handleClick = (clickEvent) => {
+      clickEvent.stopPropagation();
+      document.removeEventListener('click', handleClick, true);
+      event.target.dispatchEvent(new Event('click', { bubbles: true }));
+    };
+    document.addEventListener('click', handleClick, true);
+
     update(true, event);
   };
oliviertassinari commented 3 years ago

@m4theushw Interesting. I didn't explore the SelectInput path as I assumed developers could use ClickAwayListener in some cases outside of our control. But on paper, this proposal seems to have potential. Not sure about the exact implementation (e.g. maybe using a synthetic listener + event.nativeEvent.stopPropagation could be better). No objection to pushing further. cc @eps1lon

m4theushw commented 3 years ago

I prefer to fix the SelectInput because it also avoids a future issue if an user tries to listen for click events. I'll wait for comments from @eps1lon and submit a PR.

I didn't understand how a synthetic listener can be used here. If, in place of addEventListener, I use onClickCapture, it will never be triggered, as the click event is dispatched to an unknown target.

What's missing here is that addEventListener should be called in the same document of the mousedown event.

eps1lon commented 3 years ago

I managed to get the click event with the right target by delaying a couple of ms the insertion of this div

Just whatever you decide on the implementation, don't solve it with arbitrary timeouts. This makes debugging incredibly hard. It might solve this particular combination but possibly breaks other combinations which are now impossible to debug. We already had to introduce one to fix a React 17 bug. Piling on more is not the solution.

Having worked with several timer related issues in the past months I'm now almost certain that any bug fix needs to happen with a deterministic, synchronous implementation.

As to synthetic dispatches: I don't like this solution as well because it introduces unexpected behavior for people familiar with the DOM. Clicks being dispatched on the nearest common ancestor if mousedown and mouseup target differ is well specified behavior.

The node.tagName === 'body' implementation is insufficient since the browser chooses the nearest common ancestor of the mousedown and mouseup target. That could be any node.

eps1lon commented 3 years ago

The issue boils down to mounting of portaled content during mousedown (naming these type of components MouseDownPortal here): https://codesandbox.io/s/mousedown-mouseup-target-reconciliation-in-click-x73gz

So this is not just an issue with Select + ClickAwayListener but:

  1. ClickAwayListener + any MouseDownPortal
  2. Select + any parent click listener (I think this was the issue you were talking about during meeting that isn't listed here yet)

For ClickAwayListener + MouseDownPortal I probably have a solution that's also a deterministic, sync solution to ClickAwayListener mounting too fast (i.e. the useEffect + portal react 17 bug). So I'll try that out this week.

For Select + any parent click listener we have two solutions:

  1. go back to click instead of mouseDown as a trigger
  2. wait for SelectField implementation that'll use a Popper instead of a Popover and make sure the listbox isn't mounted in front of the trigger.
m4theushw commented 3 years ago

Select + any parent click listener (I think this was the issue you were talking about during meeting that isn't listed here yet)

https://github.com/mui-org/material-ui/pull/25630#issuecomment-815532368

oliviertassinari commented 3 years ago

Regarding the problems we are facing, the framing done so far, seems almost correct.

  1. I would add one important precision. This is only true if the portal displays above the click target. See https://codesandbox.io/s/mousedown-mouseup-target-reconciliation-in-click-x73gz?file=/src/App.js.
  2. I would add one important precision. This is about the Popover component (not so much the Select), the fix should probably be at this level. See https://codesandbox.io/s/simplepopper-material-demo-forked-1xw2n?file=/demo.tsx

Regarding the solution, I personally think that

  1. Can be solved with https://github.com/mui-org/material-ui/issues/25578#issuecomment-812525716
  2. Can be solved with #25630 but by moving it at the Popover level
eps1lon commented 3 years ago

Can be solved with #25630 but by moving it at the Popover level

I'm still very much against that change. It fixes a very specific issue while ignoring an infinite amount of similar issues while also adding completely foreign behavior to the event order. Since it's unclear what actual problem it is fixing (the codesandbox in this issue is constructed i.e. missing real world use case), we should defer this change until after v5.

m4theushw commented 3 years ago

The real world use case is in the issue I added as context: https://codesandbox.io/s/datagrid-bug-demo-forked-u7dec.

I agree with @eps1lon that moving to the Popover may create negative side effects. The Popover can be opened through several ways. We would have to check if what changed the open prop to true was a mousedown event. BTW, the component that is interleaved when the mouseup fires is in the ModalUstyled:

https://github.com/mui-org/material-ui/blob/afc9a21ec377b82ec76820f58aade6e620917daf/packages/material-ui-unstyled/src/ModalUnstyled/ModalUnstyled.js#L256-L268

oliviertassinari commented 3 years ago

Ok, in this case, it sounds like we should ignore point 2. it's not worth it, and will eventually be fixed by removing the backdrop. As for point 1. Are we happy to move forward with it? It doesn't seem to have any major downsides.

eps1lon commented 3 years ago
  1. Are we happy to move forward with it?

No. I'll take a look at it over the week but there's a simple solution that requires digging up old context (might be impossible since PRs are oftentimes underdocumented) or a more DOM based solution like https://github.com/mui-org/material-ui/pull/25741.

Since it's a fairly exotic use case with a simple solution there's no need to rush this like you're currently doing. ClickAwayListener is a component that has been heavily experimented with in the past without due dilligence and it has to stop at some point. I understand that it's satisfying to come up with intricate technical solutions but these can happen in side projects with low risk of breakage. Not in a package of this scope.

oliviertassinari commented 3 years ago

I'll take a look at it over the week but there's a simple solution that requires digging up old context (might be impossible since PRs are oftentimes underdocumented) or a more DOM based solution like #25741.

@eps1lon Happy to hear more about the two solutions you have in mind πŸ‘

Since it's a fairly exotic use case with a simple solution there's no need to rush this like you're currently doing.

I personally think that mouseUp is not a solution. It fails with users that use the scrollbars, so it seems to be worse for data grid users if we changed the default trigger. I also think that the use case is standard. A Select inside a ClickAwayLister is all that is needed: https://codesandbox.io/s/clickaway-material-demo-forked-p9qs0?file=/demo.tsx. This was reported in #19681.

eps1lon commented 3 years ago

I personally think that mouseUp is not a solution. It fails with users that use the scrollbars, so it seems to be worse for data grid users if we changed the default trigger.

Let's not strawman each other. I did not include a full implementation so obviously there are cases where a naive implementation fails.

It's also concerning that you added mouseEvent="onMouseUp" if you know that this naive solution has bugs. Maybe this was clear when you added it but since you failed to provide context back then we now have to do all this work again. A perfect example why this work needs to happen now so that we don't waste this time again.

I also think that the use case is standard.

You are by your standards wrong though. We never had anybody come up with this use case. A maintainer constructing it, is irrelevant. But this is what I'm trying to say: Don't rush this since this is an exotic case and has unintended side-effects. You're willfully ignoring the cases caused by the untrusted event dispatch though. So all the more reason to slow this down since the thinking pushing this change is flawed.

oliviertassinari commented 3 years ago

I can't remember the history of this component. Digging into it, I could find these key events:

We never had anybody come up with this use case.

@eps1lon Unless I have missed something, here is one developer coming up with this use case: #19681. Another, damien also faced this use case in https://github.com/mui-org/material-ui-x/pull/411#discussion_r525014542. He wanted to use the Select but couldn't. His solution was the native select.

A maintainer constructing it, is irrelevant.

I think that it depends on the topic. We should still be able to take on important problems even or especially if developers don't complain about them. I think that we would stay stuck in a local maximum of value if we only handle the issues reported, it would yield the bed for the end project, no longer innovating. For instance, I believe we have dropped React 16 support in v5 because we had anticipated issues but never saw a developer come up with one (I personally don't remember one). Or take the utility/sx props, nobody asked for it on the repo, and yet, seems very popular (if we assume the growth of Tailwind and Chakra is not just the design).

eps1lon commented 3 years ago

I think that it depends on the topic.

Meaning it is irrelevant as an argument.

I think that we would stay stuck in a local maximum of value if we only handle the issues reported,

You do not believe this. I've had to argue with you in the past about this exact same issue. Now that it fits your narrative you're constructing this argument. Please let me do my work.

oliviertassinari commented 3 years ago

We never had anybody come up with this use case.

Actually, I forgot about this issue, another issue with the same use case: #12034 (before we use mousedown to open the select)

You do not believe this

I strongly believe that feedback we have are mainly related to problems that are "reachable" (small and close enough), which structurally exclude important leaps forward that can have a lot more value.

I think that it's only by: listening to the low signals, taking a step back, reasoning from fundamental principles, talking to users, hearing they recurring pain points, that we can reach significantly better solutions.

eps1lon commented 3 years ago

I strongly believe that feedback we have are mainly related to problems that are "reachable" (small and close enough), which structurally exclude important leaps forward that can have a lot more value.

Sure, will let you know when the next issue comes up that wasn't reported.

eps1lon commented 3 years ago

Still not a bug. This requires significantly more research than just glancing at the issue.

enheit commented 3 years ago

I have the same issue with TextField + ClickawayListener. It triggers the onClickAway callback right after focusing the input using the mouse.

P.S. Tab seems to work correctly

oliviertassinari commented 3 years ago

Sure, will let you know when the next issue comes up that wasn't reported.

@eps1lon I don't follow the link with the previous conversation. I also don't recall we talk about this in the past. It seems that we are not talking about the same subject, but maybe we are, in any case, it seems to be a topic worth discussing :).

not a bug.

Let's step a step back. There are two different notions, the product, and the tech.

Then, there is an ongoing discussion between product and tech. The vision the product has might not be possible to implement, tech come up with possible options, with different product tradeoffs.

So whether a behavior is a bug or not, is ultimately a product decision. I'm still responsible for the product. If you disagree with my previous judgment call, labeling this GitHub issue a bug, then feel free to make a case for why. Until you convinced me, please keep the current label. I currently have no information that would start to suggest it's not a bug. Labeling this a bug seems a straightforward judgment call.

@enheit Please open a new issue with minimal reproduction on the latest version. We are focusing on fixing the ClickAwayListener + Select issue here.

Bluefitdev commented 3 years ago

Sorry but there are plenty of issues affected by this that includes mui-org/material-ui-x#1439 and mui-org/material-ui-x#1314. How can you say that is not a bug? This even affects your paid product

oliviertassinari commented 3 years ago

@Bluefitdev mui-org/material-ui-x#1439 is unrelated to this issue.

oliviertassinari commented 3 years ago

We just had a new bug report which is a duplicate of this one: https://github.com/mui-org/material-ui-x/issues/1721.

It seems that the solution we are currently leaning to is to wait for a rewrite of the Select to not use a Modal on desktop. I'm personally advocating for resolving the "isClickAway" boolean state during the mouseup phase instead of the click phase (when using mouseEvent="onClick"), but waiting (x? months) for a rewrite of the Select seems OK too. The current workaround is to use Select with native={true}, it's reasonable.

joziahg commented 3 years ago

Will add that while using native={true} works, it does not work for places where you would want to use a multiselect. My use case is a multi select in the filter panel of the X-Grid

tapion commented 2 years ago

Hi guys, some workaround?. I'd like to use the Select component with all the features it has and is not par of the native view

coot3 commented 2 years ago

Will add that while using native={true} works, it does not work for places where you would want to use a multiselect. My use case is a multi select in the filter panel of the X-Grid

In case it helps anyone, here is a workaround I am using for an MUI Select component with the multiple attribute enabled as a custom input for a filter panel. I control the open state of the select using mouseup event listeners. I also keep the value of the select input in internal state and only run applyValue when the select menu is closed to prevent some weird menu flickering that was going on filter updates:

import React, {useEffect, useRef, useState} from "react";
import {InputLabel, MenuItem, Select} from "@mui/material";

export function IsOneOfInputValue(props, options) {
    const {item, applyValue} = props;
    const [openSelect, setOpenSelect] = useState(false)
    const [value, setValue] = useState(item.value || [])

    const selectRef = useRef(null);
    const menuRef = useRef(null);

    const handleFilterChange = (event, newValue) => {
        setValue(event.target.value);
    };

    const onSelectClose = () => {
        applyValue({...item, value})
    }

    useEffect(() => {
        const onMouseUp = (e) => {
            if (e.target.parentElement === selectRef.current) {
                setOpenSelect(true)
            } else {
                if (e.target.parentElement !== menuRef.current) {
                    setOpenSelect(false)
                }
            }
        }
        window.addEventListener('mouseup', onMouseU )
        return () => {
            window.removeEventListener('mouseup', onMouseUp)
        }
    }, [])

    return (
        <>
            <InputLabel id="select-label" shrink={true}>Value</InputLabel>
            <Select
                id={"tester"}
                ref={selectRef}
                labelId="select-label"
                name="custom-select-filter-operator"
                placeholder="Filter value"
                value={value}
                open={openSelect}
                onChange={handleFilterChange}
                onClose={onSelectClose}
                multiple
                size="small"
                variant="standard"
                MenuProps={{
                    MenuListProps: {
                        ref: menuRef,
                    },
                }}
                renderValue={(value) => value.reduce((prev, curr, i) => prev + (i === value.length - 1
                    ? ' or '
                    : ', ') + curr)}
                sx={{
                    height: '100%',
                }}
            >
                {
                    options.map(option => <MenuItem value={option}>{option}</MenuItem>)
                }
            </Select>
        </>
    );
}
bflemi3 commented 2 years ago

The workaround @coot3 posted almost worked for me. I had to make changes to the Select click handlers as well as the ClickAwayListener.onClickAway handler. I included only the relevant parts of my implementation below.

It's worth noting that this will work regardless of whether multiple={true} or not.

I'm using "@mui/material": "5.3.1"

Select Implementation

const MySelect = ({ multiple }) => {
  const selectRef = useRef<HTMLDivElement>()
  const menuRef = useRef<HTMLUListElement>()
  const [open, setOpen] = useState(false)

  useEffect(() => {
    const onMouseUp = (e) => {
      if (selectRef.current.contains(e.target)) {
        setOpen(true)
        return
      }
      if (
        (!multiple && menuRef.current?.contains(e.target)) ||
        !menuRef.current?.contains(e.target)
      ) {
        setOpen(false)
      }
    }
    window.addEventListener('mouseup', onMouseUp)
    return () => {
      window.removeEventListener('mouseup', onMouseUp)
    }
  }, [multiple])

  return (
    <FormControl>
      <Select
        multiple={multiple}
        MenuProps={{
          className: 'disable-click-away',
          MenuListProps: { ref: menuRef },
        }}
        open={open}
        ref={selectRef}
      >
        <MenuItem
          className="disable-click-away"
          value="1"
        >
          <ListItemText primary="Option 1" />
        </MenuItem>
      </Select>
    </FormControl>
  )
}

ClickAwayListener Implementation (In an ancestor component)

const onClickAway = (event: MouseEvent) => {
  if ((event.target as Element).closest('.disable-click-away')) {
    return
  }
  setOpen(false)
}

return (
  <Popper open={open} {...otherProps}>
    {({ TransitionProps, placement }) => (
      <Grow
        {...TransitionProps}
        style={{ transformOrigin: placement === 'bottom-end' ? 'right top' : 'right bottom' }}
      >
        <Paper>
          <ClickAwayListener onClickAway={_onClose}>{children}</ClickAwayListener>
        </Paper>
      </Grow>
    )}
  </Popper
)
gabrielyotoo commented 4 weeks ago

Do we have any intentions in fixing this?