[Slider] Improve tracking of mouse events #22557
Conversation
|
This looks like another of this type: #22401. Did you get your whole team to upvote it? 😆
|
There was a problem hiding this comment.
Does it make https://github.com/mui-org/material-ui/blob/50c7a0e06ac20742df34bd13a1f0adeee61f2b9d/packages/material-ui/src/Slider/Slider.js#L612 unreachable?
I'm primarily using a mac laptop, the lack of a mouse is probably why I never notice this.
| @@ -714,6 +723,10 @@ const Slider = React.forwardRef(function Slider(props, ref) { | |||
| }, [disabled, handleTouchEnd, handleTouchMove]); | |||
|
|
|||
| const handleMouseDown = useEventCallback((event) => { | |||
| // don't listen to right clicks | |||
| if (event.buttons === 2) { | |||
There was a problem hiding this comment.
Why not?
| if (event.buttons === 2) { | |
| if (event.buttons !== 1) { |
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons
There was a problem hiding this comment.
Note that buttons is a bitmap meaning that primary and secondary button press would result in event.buttons === 3.
If you only want to ignore right clicks
| if (event.buttons === 2) { | |
| if (event.button === 1) { |
would match that behavior. Though we probably just want to consider clicks with the primary button and ignore anything else so
| if (event.buttons === 2) { | |
| if (event.button !== 0) { |
would be the way to go.
Reading buttons should only ever happen using bit-operators. For the rest event.button is more descriptive.
There was a problem hiding this comment.
Why not?
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons
because unless you prevent the default behavior it opens a right click menu there's no scenerio where you'd want that. perhaps we'd want to make an option to allow it and also prevent the context menu from opening so that people can do things things it. i can't personally think of a usecase where you'd want the slider to activate on right click.
There was a problem hiding this comment.
Note that
buttonsis a bitmap meaning that primary and secondary button press would result inevent.buttons === 3.If you only want to ignore right clicks
would match that behavior. Though we probably just want to consider clicks with the primary button and ignore anything else so
would be the way to go.
Reading
buttonsshould only ever happen using bit-operators. For the restevent.buttonis more descriptive.
Wouldn't you want left + right to activate it though? I only figured it'd make sense to explicitly hard block when it's explicitly only a right click. What's the reasoning to also block combinations of right? It seems reasonable overall to me, but a bit more heavy handed, and it's already sort of an opinionated change..
There was a problem hiding this comment.
perhaps on mousedown it should be .button and for mousemove it should be .buttons 🤔
There was a problem hiding this comment.
Wouldn't you want left + right to activate it though?
mouse 4 and 5 are usually for page navigation. It'd be odd if you prevent that because you just happened to hover over the slider. Native sliders also ignore mouse 4 or 5. Currently we start dragging but upon release button 4 navigation still starts.
But more importantly: comment and code don't match. This is problematic for maintaining this code.
There was a problem hiding this comment.
It'd be odd if you prevent that because you just happened to hover over the slider.
I have tested it, it prevents navigation if you click on the previous button of the mouse while hovering the slider 🙈.
Fixed in #22638
| handleTouchMove(event); | ||
| } else { | ||
| touchId.current = undefined; | ||
| stopListening(); |
There was a problem hiding this comment.
Shouldn't we call handleTouchEnd() instead to clear the active and open states?
There was a problem hiding this comment.
That was my first thought too. It doesn't have a proper finger in this scenario so it short circuits and doesn't run any of the cleanup code, that was why I finally decided to DRY the cleanup so it's easier to call from here.
There was a problem hiding this comment.
Why wouldn't it have a proper finger? We use the mouse position when not a touch event.
lol, i didn't get them to, they just did. we post all our PR's in slack and we just really appreciate each other :D |
|
I should be able to test the changes tomorrow. I need to find my old Razer Naga, I haven't used an actual mouse for 5 years |
|
@chrisinajar I have handled the feedback of the review in GetDutchie#1. Could you have a look and merge if happy with the changes (so this pull requests update and we get a chance to land the changes in the next branch). |
|
@chrisinajar Thanks for raising the issue to our attention. |
|
@oliviertassinari thanks for cleaning it up for me, I've been moving and it got away from me 👍🏡 |
Currently the slider responds to all mouse events, even right clicks, and also doesn't make sure that the mouse is still being held down when moving around.
This results in the following bugs:
This type of leaking events can, in combination with hyper specific race conditions with local navigation in iframes while dom events are bubbling, cause the component to permanently leak the mousemove handler resulting in the following exception:
Uncaught TypeError: Cannot read property 'getBoundingClientRect' of nullThis PR changes the behavior of the event tracking such that right clicks are ignored, and on each mouse move (mouse only) it makes sure you're still holding the left mouse button.