[ButtonBase] Enable pointer-events #23101
Conversation
|
@material-ui/core: parsed: +0.13% , gzip: +0.05% |
There was a problem hiding this comment.
Should the PR be in a draft mode?
The initial fix comes from #8416 (comment). Then it was improved with:
- [Tooltip] New warning message #8988 add a warning for failing test case
- disabled buttons have pointer-events: none which prevent overriding of cursor property #14455 document the point-event tradeoff for cursor
- [docs] Add a note in disabled tooltip #17421 cover when the tooltip wrap custom button component (not coming from us)
| @@ -20,7 +20,7 @@ export const styles = (theme) => ({ | |||
| duration: theme.transitions.duration.short, | |||
| }, | |||
| ), | |||
| '&:hover': { | |||
| '&:hover:not($disabled)': { | |||
There was a problem hiding this comment.
not() increases the specificity => harder customizations. We had iterations in the past to remove them, e.g. #14953. What's the use case for the change?
There was a problem hiding this comment.
To not be a breaking change.
There was a problem hiding this comment.
This might be overzealous i.e. not needed in every case. I just noticed it for Button.
There was a problem hiding this comment.
not() increases the specificity => harder customizations.
Not every increase in specificity is bad. You have to show me an existing customization (that is still valid with this change) that is no longer possible due to an increase in specificity.
There was a problem hiding this comment.
Sure, here is an example https://deploy-preview-23101--material-ui.netlify.app/guides/interoperability/#plain-css, a button that customizes the hover state. The hover state is wrong.
| expect(() => { | ||
| render( | ||
| <Tooltip title="Hello World"> | ||
| <button type="submit" disabled> |
There was a problem hiding this comment.
I don't understand. This still doesn't work. Why remove the warning?
https://codesandbox.io/s/simpletooltips-material-demo-forked-s1pqe?file=/demo.tsx
There was a problem hiding this comment.
I can't stress enough how useless these reviews are. Please describe the steps to reproduce, the expected behavior, the actual behavior and your environment. Same concern with your behavior as #23088 (comment)
The tooltip is working for me fine as far as I can tell with all the examples but I didn't do any browser testing yet.
There was a problem hiding this comment.
I'm not reporting a bug. I'm providing a demonstration that the removed test case is still relevant, that we need to keep it.
- environment: Chrome, Edge
- steps to reproduce: hover the button
- expect/actual behavior, don't apply. I'm not reporting a bug.
The codesandbox renders the test case. It demonstrates that the tooltip doesn't display. It also demonstrates that the instruction in the documentation should be kept as they allow the tooltip to open.
Does the PointerEvent API address this?
👍 Yes |
github-actions
bot
added
the
PR: out-of-date
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mui-org/material-ui/g9e8kxkys |
|
Prioritizing pickers at the moment. Changing this might be a bit too disruptive. Would be an easy choice when we would restart. Disabling pointer-events for disabled components has unintended consequences. |
Alternate to #22937. Need a deploy for browser testing.
https://deploy-preview-23101--material-ui.netlify.app/components/tooltips/#disabled-elements
Closes #23589