[Select] Dispatch click event with same target of the mousedown event #25630

m4theushw · 2021-04-05T22:04:18Z

I have followed (at least) the PR section of the contributing guide.

Trade-offs:

Two click events in the capture phase
Test cases that call fireEvent.mouseDown must also call fireEvent.click to clear the added listener. Related to [Select] No longer listens to click events #18655

A codesandbox with the upstream issue and the fix in this PR: https://codesandbox.io/s/datagrid-bug-demo-forked-ntsb3?file=/package.json

mui-pr-bot · 2021-04-05T22:06:22Z

Details of bundle changes

@material-ui/core: parsed: +0.11% , gzip: +0.09%

Generated by 🚫 dangerJS against d3b4a0f

m4theushw · 2021-04-05T22:06:45Z

packages/material-ui/src/Select/Select.test.js

@@ -116,6 +116,7 @@ describe('<Select />', () => {
 const trigger = getByRole('button');

 fireEvent.mouseDown(trigger);
+ fireEvent.click(trigger);


I had to add this click to every fireEvent.mouseDown so that the previous added event listener is removed.

m4theushw · 2021-04-06T14:15:44Z

Added the "on hold" label as I want to use the e2e job to test this use case.

…event

oliviertassinari

I have updated the issue description to link #18655 that will likely come up again with the changes here. No objections to giving this approach a try. I would personally favor the change in the ClickAwayListener logic I had initially proposed. It would be more resilient, It's not too hard to trigger the same issue: https://codesandbox.io/s/simplepopper-material-demo-forked-1xw2n?file=/demo.tsx.

eps1lon

Fixing the behavior for the clickAwayListener is important. We can't also dispatch synthetic events. We would now compete with the browser and React. Adding new, unspecified behavior. But not only is the behavior unspecified, it actually violates existing specification (click events should be dispatched on the common ancestor of mousedown/mouseup).

I talked with the React folks who faced this very issue recently and will share their recommendations this week. Dispatching synthetic events was not among the considered approaches.

eps1lon · 2021-04-07T05:16:41Z

packages/material-ui/src/Select/Select.test.js

@@ -116,6 +116,7 @@ describe('<Select />', () => {
 const trigger = getByRole('button');

 fireEvent.mouseDown(trigger);
+ fireEvent.click(trigger);


This is one indicator that this change is problematic. What happens if there is no click registered on the trigger due to interactions with other elements?

There's no problem if the click occurs in other element, here it could even be in the document.body. I just need to issue it to clear the previous added listener or it will cause unexpected behavior in the next fireEvent.mouseDown below.

packages/material-ui/src/Select/SelectInput.js

eps1lon · 2021-04-07T05:24:36Z

packages/material-ui/src/Select/SelectInput.js

+ doc.removeEventListener('click', handleClick.current, true);
+ }
+
+ // Since the targets of the `mousedown` and `mouseup` events are different,


It's not clear at this point in the code that this is the case. Wouldn't you want to check that during mouseup?

To check during mouseup I would have to add another listener to the document and from this listener add another one to cancel and re-dispatch the click.

eps1lon · 2021-04-07T05:25:08Z

packages/material-ui/src/Select/SelectInput.js

+
+ // Since the targets of the `mousedown` and `mouseup` events are different,
+ // the browser will dispatch the `click` to a common ancestor.
+ // This stops the next `click` and re-dispatch it with the right target.


Why do you think the specified behavior is not the "right" target?

Because it's not clear that, because a transparent div is placed in the mousedown, the target of mouseup changes.

m4theushw · 2021-04-07T14:45:50Z

Fixing the behavior for the clickAwayListener is important. We can't also dispatch synthetic events. We would now compete with the browser and React. Adding new, unspecified behavior. But not only is the behavior unspecified, it actually violates existing specification (click events should be dispatched on the common ancestor of mousedown/mouseup).

I talked with the React folks who faced this very issue recently and will share their recommendations this week. Dispatching synthetic events was not among the considered approaches.

@eps1lon Even if we discard this PR to avoid dispatching new events, there's still the problem that the click doesn't propagate. It's not intuitive for users writing unit-tests to have to use the mousedown to click the select. #18655 #19250
I'll wait for your recommendations.

aditya2337 · 2021-04-08T07:42:48Z

Hello 👋 , I happened to stumble upon this PR after debugging what I think is a similar issue, in my case, I am trying to track the user's click event on the Select field, and I am doing this through heap analytics, although once a user clicks on select, heap recognizes the event on body.

Is this related to the current issue? If not could you point me how to fix this?

Thank you in advance 🤗

m4theushw · 2021-04-08T10:41:02Z

Hello 👋 , I happened to stumble upon this PR after debugging what I think is a similar issue, in my case, I am trying to track the user's click event on the Select field, and I am doing this through heap analytics, although once a user clicks on select, heap recognizes the event on body.

Is this related to the current issue? If not could you point me how to fix this?

Thank you in advance 🤗

@aditya2337 Yes, this PR is related to your issue. The click event is not dispatched in the Select field but in the body or other common ancestor. The mousedown event can be used to track user's click.

aditya2337 · 2021-04-08T11:24:45Z

Ah, awesome 👏 thank you!!

m4theushw · 2021-04-08T19:10:11Z

I changed the approach here. Instead of dispatching new events, I'm setting pointer-events to none between mousedown and click. This prevents the backdrop from being used as target for the mouseup, which also avoids the common ancestor logic in the click event.

oliviertassinari

The new approach makes me think of #23466. Looks better

eps1lon

Let's discuss this on the next planning meeting. There are some misunderstandings that we should sync on. Mostly onboarding issues.

oliviertassinari · 2021-04-24T15:38:57Z

I'm closing as the effort has been stale for a while.

m4theushw commented Apr 5, 2021

View reviewed changes

m4theushw added the on hold There is a blocker, we need to wait label Apr 6, 2021

m4theushw added 3 commits April 6, 2021 14:42

[SelectInput] Dispatch click event with same target of the mousedown …

e6ceb6b

…event

Fix tests

e7a5fbb

Add e2e test

36b249b

m4theushw force-pushed the selectInput-onClick branch from d6569a0 to 36b249b Compare April 6, 2021 20:48

github-actions bot mentioned this pull request Apr 6, 2021

Update snapshots for pr/25630 eps1lon/mui-scripts-incubator#2089

Closed

m4theushw added 2 commits April 6, 2021 18:33

Improve assertion

2426215

Remove page.pause

fa77bf5

m4theushw removed the on hold There is a blocker, we need to wait label Apr 6, 2021

m4theushw requested review from oliviertassinari and eps1lon April 6, 2021 22:03

oliviertassinari changed the title ~~[SelectInput] Dispatch click event with same target of the mousedown event~~ [Select] Dispatch click event with same target of the mousedown event Apr 6, 2021

oliviertassinari added the component: select This is the name of the generic UI component, not the React module! label Apr 6, 2021

improve error message if the test fail

902118f

oliviertassinari approved these changes Apr 6, 2021

View reviewed changes

oliviertassinari added the bug 🐛 Something doesn't work label Apr 6, 2021

eps1lon requested changes Apr 7, 2021

View reviewed changes

Suggestions from Sebastian

3f84217

Disable menu pointer events between mousedown and click

d3b4a0f

m4theushw requested review from eps1lon and oliviertassinari April 8, 2021 19:10

oliviertassinari reviewed Apr 8, 2021

View reviewed changes

eps1lon requested changes Apr 9, 2021

View reviewed changes

m4theushw mentioned this pull request Apr 12, 2021

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

Open

2 tasks

eps1lon mentioned this pull request Apr 12, 2021

[ClickAwayListener] Don't call onClickAway in the event cycle that mounted it #25741

Closed

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 15, 2021

oliviertassinari closed this Apr 24, 2021

m4theushw deleted the selectInput-onClick branch November 1, 2021 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Select] Dispatch click event with same target of the mousedown event #25630

[Select] Dispatch click event with same target of the mousedown event #25630

m4theushw commented Apr 5, 2021 •

edited by oliviertassinari

Loading

mui-pr-bot commented Apr 5, 2021 •

edited

Loading

m4theushw Apr 5, 2021 •

edited

Loading

m4theushw commented Apr 6, 2021

oliviertassinari left a comment •

edited

Loading

eps1lon left a comment

eps1lon Apr 7, 2021

m4theushw Apr 7, 2021

eps1lon Apr 7, 2021

m4theushw Apr 7, 2021

eps1lon Apr 7, 2021

m4theushw Apr 7, 2021

m4theushw commented Apr 7, 2021

aditya2337 commented Apr 8, 2021

m4theushw commented Apr 8, 2021

aditya2337 commented Apr 8, 2021

m4theushw commented Apr 8, 2021

oliviertassinari left a comment

eps1lon left a comment

oliviertassinari commented Apr 24, 2021

[Select] Dispatch click event with same target of the mousedown event #25630

[Select] Dispatch click event with same target of the mousedown event #25630

Conversation

m4theushw commented Apr 5, 2021 • edited by oliviertassinari Loading

mui-pr-bot commented Apr 5, 2021 • edited Loading

m4theushw Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

m4theushw commented Apr 6, 2021

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon Apr 7, 2021

Choose a reason for hiding this comment

m4theushw Apr 7, 2021

Choose a reason for hiding this comment

eps1lon Apr 7, 2021

Choose a reason for hiding this comment

m4theushw Apr 7, 2021

Choose a reason for hiding this comment

eps1lon Apr 7, 2021

Choose a reason for hiding this comment

m4theushw Apr 7, 2021

Choose a reason for hiding this comment

m4theushw commented Apr 7, 2021

aditya2337 commented Apr 8, 2021

m4theushw commented Apr 8, 2021

aditya2337 commented Apr 8, 2021

m4theushw commented Apr 8, 2021

oliviertassinari left a comment

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

oliviertassinari commented Apr 24, 2021

m4theushw commented Apr 5, 2021 •

edited by oliviertassinari

Loading

mui-pr-bot commented Apr 5, 2021 •

edited

Loading

m4theushw Apr 5, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading