[ClickAwayListener] Calling onClickAway straightaway when used with Select #25578
Comments
m4theushw
added
bug 🐛
|
Hello, Thank you for the issue Can I work on this issue? |
|
Yes, feel free to send a PR. |
|
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 |
|
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
removed
the
good first issue
|
It's a hard problem. I can't find any good solution. |
|
I picked that one because of the |
|
I tried to understand why it's not intercepting the |
|
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]); |
|
@m4theushw What do you think about the above diff? |
|
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 About this last point, I dived into the inner components of the Select component and I found something interesting. It listens for a
I managed to get the 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={{ |
|
@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. AFAIK, this option is not viable. |
Yeah, missed that. Clicking and holding invalidates my solution. I have another alternative. The idea is to cancel the next 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);
}; |
|
@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 |
|
I prefer to fix the SelectInput because it also avoids a future issue if an user tries to listen for I didn't understand how a synthetic listener can be used here. If, in place of What's missing here is that |
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 |
|
The issue boils down to mounting of portaled content during So this is not just an issue with
For For
|
|
|
Regarding the problems we are facing, the framing done so far, seems almost correct.
Regarding the solution, I personally think that
|
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. |
|
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 |
|
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. |
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 #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. |
@eps1lon Happy to hear more about the two solutions you have in mind 👍
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. |
Sure, will let you know when the next issue comes up that wasn't reported. |
|
Still not a bug. This requires significantly more research than just glancing at the issue. |
This comment has been minimized.
This comment has been minimized.
@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 :).
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
oliviertassinari
added
the
component: select
|
We just had a new bug report which is a duplicate of this one: mui/mui-x#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 |
|
Will add that while using |
|
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 |
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: |
|
The workaround @coot3 posted almost worked for me. I had to make changes to the It's worth noting that this will work regardless of whether I'm using 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
) |
|
Do we have any intentions in fixing this? |
Current Behavior 😯
When a
Selectis used in conjunction with theClickAwayListener, theonClickAwayprop is called straightaway after the select is open.Expected Behavior 🤔
It should not call
onClickAwaywhile theSelectis opening.onClickAwayshould only be called when the user clicks OUTSIDE the select while it's open.Steps to Reproduce 🕹
Steps:
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
bodywhich forbids any attempt of theClickAwayListenerto check if it was inside or outside its children. I propose the following change based on Olivier's diff.Your Environment 🌎
`npx @material-ui/envinfo`
The text was updated successfully, but these errors were encountered: