[ButtonBase] Remove dead style #15503
Conversation
Details of bundle changes.Comparing: 5950587...b572e0c
|
Essentially we use MUI to create a widget that is embedded directly on other websites (not in an iframe). To avoid style conflicts, we prefix all of our styles with a base class, and mark all properties as important. We then have a reset sheet on our namespace. It is a bit ugly, but it does work, with the exception of a couple of items like the ripple effect, and the resizing of multiline text areas. |
|
@koshea Thanks. inline style has more specificity than style sheet, injected in the HEAD. What would be possibly creating a conflict in the ripple case? |
|
@oliviertassinari because our JSS plugin makes all of the styles !important, and the styles in the style attribute are not processed in this manner, they are overridden and the effect does not happen. Another option that would solve our use case would be to just make the inline styles important, but in my mind that is an uglier solution than running this through the style system. |
|
Have you considered this change? --- a/packages/material-ui/src/ButtonBase/TouchRipple.js
+++ b/packages/material-ui/src/ButtonBase/TouchRipple.js
@@ -24,10 +24,6 @@ export const styles = theme => ({
},
/* Styles applied to the internal `Ripple` components `ripple` class. */
ripple: {
- width: 50,
- height: 50,
- left: 0,
- top: 0,
opacity: 0,
position: 'absolute',
}, |
| ); | ||
|
|
||
| const rippleStyles = { |
There was a problem hiding this comment.
Could you revert this change? I believe that removing the !important style that overrides the inline style should be enough.
There was a problem hiding this comment.
Are you suggesting we keep the inline styles in addition to the new class?
There was a problem hiding this comment.
Do you confirm that we only need #15503 (comment) to solve the problem?
There was a problem hiding this comment.
I would rather use inline style than the style function (in our case).
There was a problem hiding this comment.
Oh, I see what you are saying. No, that resolves the need to use the index option on makeStyles, but it would not resolve my issue without the rest of the changes. I have to run my code on websites with crazy garbage like span { width: 200px !Important; } set in their stylesheets. So I have to clean that up in my namespace with a reset including things like .myNamespace * { width: auto !important; }.
What is the aversion to moving to a style function, performance? Just seems like it would be nice to solve the CSP issue and allow any sort of preprocessing to run on these styles that users would like.
There was a problem hiding this comment.
Thanks for the extra detail, I better understand the problem now. There is close to no performance implication in this case, but it will once it's made a systematic solution. We might be able to use CSS variables in v5 (#14420). I have reverted the change of this file.
Did you consider using the shadow DOM?
There was a problem hiding this comment.
I would love to be using shadow DOM, it is perfect for my use case. I may implement it for most users again soon (past attempted this in MUI 0.x and ran into too many issues).
That said, unfortunately we have a ton of IE11 and Edge users. The polyfill basically does what I am doing with my jss plugins, namespaces all of the CSS. So in those browsers I will still have this issue.
I understand not wanting to pull in this change, I will probably just patch it locally for now. Would you mind giving some insight into where you think I will hit a performance issue when changing the other cases of inline style usage?
There was a problem hiding this comment.
I can imagine that supporting IE 11 is important for you, shadow DOM is not an option. Dropping IE 11 would also enable CSS variables and probably solve the issue here at the same time.
I'm curious why is dynamically creating an iframe is not an option for you? We do it for a few of our demos in the documentation, e.g. the Drawers' demos. If you are familiar with Stripe, it's how they increase the security of their web input widget.
Using style functions is slower. If it's only 10% slower (end-to-end), it worth it, 💯 ! It would unlock a lot of interesting features for Material-UI. I'm happy to push this further but in a different pull request. Removing the dead CSS rules is already an improvement :).
There was a problem hiding this comment.
This discussion echos with the one of #15499 as it requires the usage of style functions in the core components.
oliviertassinari
changed the title
Our application uses some JSS plugins to modify the CSS. A few components in MUI use inline styles, particularly for animation, and this sometimes causes breakage for us. If you are open to this change for ripple, we could make similar adjustments to the remaining components using the style attribute.