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 rerendering causes global styles to recalculate on each keystroke with SC #28051

Closed
2 tasks done
0xF013 opened this issue Aug 30, 2021 · 4 comments · Fixed by #28070
Closed
2 tasks done

TextField rerendering causes global styles to recalculate on each keystroke with SC #28051

0xF013 opened this issue Aug 30, 2021 · 4 comments · Fixed by #28070
Assignees
Labels
component: text field This is the name of the generic UI component, not the React module! package: styled-engine-sc Specific to styled-components performance
Milestone

Comments

@0xF013
Copy link

0xF013 commented Aug 30, 2021

When using TextField and @material-ui/styled-engine-sc as a controlled input (in a render prop?), this part https://github.com/mui-org/material-ui/blob/7bdf8a35299fa4a7e9ae97eff5f3b074170c38ef/packages/material-ui/src/InputBase/InputBase.js#L486 causes the styled component blob <style> tag to redraw on each keystroke. I would assume that the styled-components' implementation of global styles is less efficient than emotion's (their source code uses a useLayoutEffect that flickers on each prop update).

image

Video: https://vimeo.com/manage/videos/594582087

While it's not really noticeable in a demo, it scales at least linearly with the number of styles in a project.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Expected Behavior 🤔

Steps to Reproduce 🕹

Steps:

  1. https://codesandbox.io/s/styled-components-forked-crstj?file=/src/demo.jsx
  2. Inspect the input and follow a style into the inspect mode
  3. Start typing in the input
  4. Notice the style tag flickering.
  5. Notice, at the bottom of the tag, the styles for the autofill flickering

Context 🔦

Your Environment 🌎

`npx @material-ui/envinfo`
  Don't forget to mention which browser you used.
  Output from `npx @material-ui/envinfo` goes here.
@0xF013 0xF013 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 30, 2021
@eps1lon eps1lon added this to the v5 milestone Aug 30, 2021
@eps1lon eps1lon added package: styled-engine-sc Specific to styled-components performance and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 30, 2021
@mnajdova
Copy link
Member

mnajdova commented Aug 30, 2021

Looks like the global keyframes were added in #19497, but from what I could read in the thread of the issue, it was added to fix a JSS issue

We could try to drop the global styles (I wasn't able to reproduce the problem even on the original issue, and I couldn't spot any issues with the updated implementation) cc @oliviertassinari could you help test this out, maybe I am missing something.

index 169f6c8407..30274a044c 100644
--- a/packages/material-ui/src/InputBase/InputBase.js
+++ b/packages/material-ui/src/InputBase/InputBase.js
@@ -14,7 +14,6 @@ import capitalize from '../utils/capitalize';
 import useForkRef from '../utils/useForkRef';
 import useEnhancedEffect from '../utils/useEnhancedEffect';
 import TextareaAutosize from '../TextareaAutosize';
-import GlobalStyles from '../GlobalStyles';
 import { isFilled } from './utils';
 import inputBaseClasses, { getInputBaseUtilityClass } from './inputBaseClasses';

@@ -482,13 +481,6 @@ const InputBase = React.forwardRef(function InputBase(inProps, ref) {
   inputProps = { ...inputProps, ...componentsProps.input };

   return (
-    <React.Fragment>
-      <GlobalStyles
-        styles={{
-          '@keyframes mui-auto-fill': {},
-          '@keyframes mui-auto-fill-cancel': {},
-        }}
-      />
       <Root
         {...rootProps}
         {...(!isHostComponent(Root) && {
@@ -542,7 +534,6 @@ const InputBase = React.forwardRef(function InputBase(inProps, ref) {
             })
           : null}
       </Root>
-    </React.Fragment>
   );
 });

@0xF013
Copy link
Author

0xF013 commented Aug 30, 2021

I could record a video on a project with a lot of styled tags if that is helpful

@mnajdova
Copy link
Member

@0xF013 I can see that the Global styles are re-created. Let's see @oliviertassinari response about whether we can drop them completely, as I can't reproduce the original issue that lead to adding them in the first place. If not, we could try to insert them only once (will need to see how we can do this)

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 30, 2021

@mnajdova This keyframe style is important, we need it for the autofill logic. See the same issue with emotion #26449. I had added this issue in the v5-candidate milestone.

Regarding the solution, I think that we can either:

  1. look into memoization
  2. look into a different way to render keyframe and to reference them in the JS implementation

@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! package: styled-engine-sc Specific to styled-components performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants