Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Slider] Fix override of event.target when preparing change events #24782

Merged
merged 9 commits into from Feb 12, 2021
Merged

[Slider] Fix override of event.target when preparing change events #24782

merged 9 commits into from Feb 12, 2021

Conversation

praveenkumar-kalidass
Copy link
Contributor

What does it do?

Clones the event if it is a native event.

Why is it needed?

Not cloning the native event leads to event leaks to its parent.

Related issue(s)/PR(s)

Closes #24740

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 5, 2021 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against 8050e5d

@eps1lon eps1lon added component: slider This is the name of the generic UI component, not the React module! PR: needs test The pull request can't be merged bug 🐛 Something doesn't work labels Feb 5, 2021
eps1lon
eps1lon requested changes Feb 5, 2021
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work!

Would you be so kind and add a test for this behavior. Otherwise we'll likely remove this code in the future since it's quite exotic.

@oliviertassinari oliviertassinari changed the title [Slider] Send cloned event.target on touch event [Slider] Fix leaky override of event.target Feb 7, 2021
@oliviertassinari oliviertassinari removed the PR: needs test The pull request can't be merged label Feb 7, 2021
oliviertassinari
oliviertassinari approved these changes Feb 7, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have refined the solution, polished it a bit, and added a test case. It turns out that it only happens with the touch event, e.g. not the mouse down. I have added the target as the third property in case a developer has a specific use case that requires it, and done some other minor improvements.

@github-actions github-actions bot mentioned this pull request Feb 8, 2021
Closed
@oliviertassinari oliviertassinari force-pushed the slider-touch-event branch 3 times, most recently from e0d155c to a65cdd1 Compare February 8, 2021 23:18
@oliviertassinari
Copy link
Member

@eps1lon I have fixed your new test case, as well as on the Select component (both were indeed having a global side effect). I believe that the current tradeoff delivers.

@eps1lon eps1lon changed the title [Slider] Fix leaky override of event.target [Slider] Fix override of event.target Feb 9, 2021
@eps1lon eps1lon changed the title [Slider] Fix override of event.target [Slider] Fix override of event.target when preparing change events Feb 9, 2021
eps1lon
eps1lon previously requested changes Feb 9, 2021
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test in Select

docs/src/pages/components/slider/InputSlider.tsx Outdated Show resolved Hide resolved
packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
eps1lon
eps1lon previously requested changes Feb 11, 2021
View reviewed changes
packages/material-ui/src/Select/Select.test.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari force-pushed the slider-touch-event branch 2 times, most recently from 72432cb to 0b4cf39 Compare February 11, 2021 12:42
eps1lon
eps1lon approved these changes Feb 12, 2021
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@eps1lon eps1lon merged commit 7c2a773 into mui:next Feb 12, 2021
@eps1lon
Copy link
Member

eps1lon commented Feb 12, 2021

@praveenkumar-kalidass Thanks for the work!

@oliviertassinari oliviertassinari mentioned this pull request Feb 12, 2021
Merged
@eps1lon eps1lon added breaking change component: select This is the name of the generic UI component, not the React module! labels Jul 19, 2021
This was referenced Jul 20, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@eps1lon eps1lon eps1lon approved these changes

Assignees
No one assigned
Labels
breaking change bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Slider] Sends incorrect event.target on touch event
4 participants
@praveenkumar-kalidass @mui-pr-bot @oliviertassinari @eps1lon