-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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.
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
[Tooltip] Refactor touch handling #23088
Conversation
I have noticed a change of behavior after click the "Click" button on iOS Safari 14 and Android Chrome 86. The tooltip is no longer rendered with the "touch" styles. https://next.material-ui.com/components/tooltips/#triggers OK ✅ https://deploy-preview-23088--material-ui.netlify.app/components/tooltips/#triggers KO ❌ It sounds like we need to keep the logic but add a regression test case. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks for the report 👍 That's definitely indicative of an actual issue but can we talk about |
Which goes right into the discussion of
It's not actually opened by touch but click in the problematic demo so it's technically correct to not apply the styles. If you want to argue that this is concerned about "user-touch" then it doesn't make sense to apply these styles if |
We could improve the description of this prop and the behavior. I see two concerns. First, on a touch interaction, you want to compensate for the size of the finger that triggered it. Otherwise, you might not see the message, hidden behind your finger. It's the same concern with a mouse, only that it cover a smaller area (unless maginifed). So far, we have mixed the too and used the touch start event to trigger the style. I'm not aware of any significance issue with such approximation. |
I'm not sure what's the right thing to do here. At least, I hope that the previous comment explain the objective with the style. I can't remember why we have a disableTouchListener prop, I could only find #20245 |
I definitely understand why we have separate styles for touch devices. I think they should be applied at another point though. Why make the tooltip bigger when we register touch input but not the button that triggers it? Intuitively I rather add My point is more that if I want to disable the touch listener I would also expect any special handling of touch events to be ignored. |
To be careful, it might not work on Samsung Galaxy phones, to be tested. We discovered that in #1500. In mui/material-ui-pickers#1653, I have found that
Agree, for controlling the open state. I think that It would be surprising if the |
I want to do some more interaction research but I'm fairly certain we can just drop this prop. |
Will revisit later. |
detectTouch
was introduced in #20807. The added test still passes with this change so either the test is insufficient or the observed event order never actually happens (e.g. when emulating devices the events are sometimes wrong especially with Firefox).