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.

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

[TextField] Label shrink issue with disableGlobal: true #19224

Closed
valentinbourqui opened this issue Jan 14, 2020 · 6 comments · Fixed by #19497
Closed

[TextField] Label shrink issue with disableGlobal: true #19224

valentinbourqui opened this issue Jan 14, 2020 · 6 comments · Fixed by #19497
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@valentinbourqui
Copy link

valentinbourqui commented Jan 14, 2020

Current Behavior 😯

When we build an application in production and we add the variable at true for disableGlobal inside createGenerateClassName, TextField is shrinked automatically by default without value.

Expected Behavior 🤔

Should not be shrinked.

Steps to Reproduce 🕹

https://codesandbox.io/s/material-ui-error-input-4y9hv

Doesn't work on chrome and firefox but works on Safari. If you put disableGlobal at false it's work everywhere

@eps1lon eps1lon added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Jan 14, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 14, 2020

@valentinbourqui
Copy link
Author

^^ yes ! If you move your mouse on the page (codesandbox) then you refresh the page on the same time most of time it works ! If you stop moving and you refresh you got the error again. :S

@martinjlowm
Copy link
Contributor

The use of indexOf should reference the JSS generated name, rather than the static name as it currently does. It would be straightforward if withStyles and makeStyles exposed the generated CSS sheet and referenced the keyframe name as is described here: https://cssinjs.org/jss-syntax/?v=v10.0.3#keyframes-animation

One proposal is an extra option, similar to how the inclusion option of theme is done, e.g. withStyles({ ... }, { withTheme: true, withSheet: true }). The sheet is the staticSheet generated here: https://github.com/mui-org/material-ui/blob/bdebdfd4832c13a05463faeffef9687688be9715/packages/material-ui-styles/src/makeStyles/makeStyles.js#L87

staticSheet.keyframes['auto-fill-cancel'] reveals the generated name. Any thoughts on this solution?

@oliviertassinari
Copy link
Member

We will soon have to solve the same issue with styled-components and emotion (multi-engine support). They both give us access to the generated name of the keyframe but with our current integration with JSS, it's more challenging. In this context, what about we use global, prefixed, names?

diff --git a/packages/material-ui/src/InputBase/InputBase.js b/packages/material-ui/src/InputBase/InputBase.js
index 3ce557f0b..caded0597 100644
--- a/packages/material-ui/src/InputBase/InputBase.js
+++ b/packages/material-ui/src/InputBase/InputBase.js
@@ -28,6 +28,14 @@ export const styles = theme => {
   };

   return {
+    '@global': {
+      '@keyframes mui-auto-fill': {
+        from: {},
+      },
+      '@keyframes mui-auto-fill-cancel': {
+        from: {},
+      },
+    },
     /* Styles applied to the root element. */
     root: {
       // Mimics the default input display property used by browsers for an input.
@@ -87,7 +95,7 @@ export const styles = theme => {
       // Make the flex item shrink with Firefox
       minWidth: 0,
       width: '100%', // Fix IE 11 width issue
-      animationName: '$auto-fill-cancel',
+      animationName: 'mui-auto-fill-cancel',
       '&::-webkit-input-placeholder': placeholder,
       '&::-moz-placeholder': placeholder, // Firefox 19+
       '&:-ms-input-placeholder': placeholder, // IE 11
@@ -119,15 +127,9 @@ export const styles = theme => {
       },
       '&:-webkit-autofill': {
         animationDuration: '5000s',
-        animationName: '$auto-fill',
+        animationName: 'mui-auto-fill',
       },
     },
-    '@keyframes auto-fill': {
-      from: {},
-    },
-    '@keyframes auto-fill-cancel': {
-      from: {},
-    },
     /* Styles applied to the `input` element if `margin="dense"`. */
     inputMarginDense: {
       paddingTop: 4 - 1,
@@ -388,7 +390,7 @@ const InputBase = React.forwardRef(function InputBase(props, ref) {
   const handleAutoFill = event => {
     // Provide a fake value as Chrome might not let you access it for security reasons.
     checkDirty(
-      event.animationName.indexOf('auto-fill-cancel') !== -1 ? inputRef.current : { value: 'x' },
+      event.animationName === 'mui-auto-fill-cancel' ? inputRef.current : { value: 'x' },
     );
   };

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. labels Jan 30, 2020
@martinjlowm
Copy link
Contributor

martinjlowm commented Jan 31, 2020

We will soon have to solve the same issue with styled-components and emotion (multi-engine support). They both give us access to the generated name of the keyframe but with our current integration with JSS, it's more challenging. In this context, what about we use global, prefixed, names?

...

I agree, it seems like a cleaner approach.

It feels bloated having to supply the sheet as I proposed for one particular use-case.

@oliviertassinari
Copy link
Member

@martinjlowm Great, in this case, feel free to take the lead to get these changes in the master branch :).

@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! and removed package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Jan 31, 2020
@oliviertassinari oliviertassinari changed the title TextField Label shrinked on Chrome and Firefox [TextField] Label shrink issue with disableGlobal: true Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants