Skip to content

Conversation

@mnajdova
Copy link
Member

@mnajdova mnajdova commented Aug 31, 2021

1. Autofill detection not working in production

Fixes #26449

The empty keyframes are being removed in production by emotion. Adding an from { display: 'block' } fixes the issue. I've decided to go with this style, as it is already defined in the styles for the InputBase component.

2. TextField rerendering causes global styles to recalculate on each keystroke with styled-components

Fixes #28051

The GlobalStyles are now hoisted in a static variable, as otherwise the <style> tag redraw on each keystroke when the Input component is rendered for example as a callback, see #28051


Reproduction and verification of the fixes

  1. Auto fill issue

Steps:

  • Enter email and password
  • Save when propt by the browser
  • Refresh the page

Previous:
Codesandbox link: https://codesandbox.io/s/ecstatic-wilbur-710ny?file=/src/App.js
Production build link: https://csb-710ny.netlify.app/?

image

Now:

Codesandbox link: https://codesandbox.io/s/upbeat-dream-5nttz?file=/package.json
Production build link: https://csb-5nttz.netlify.app/?

image


  1. styled-components redraw <style> tag on each keystroke
    Previous:
    https://codesandbox.io/s/styled-components-forked-crstj?file=/src/demo.jsx
    sc-issue-prev

Now:
https://codesandbox.io/s/styled-components-forked-gwepq?file=/src/demo.jsx
sc-issue-now

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 31, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 01d62f7

@mnajdova mnajdova marked this pull request as ready for review August 31, 2021 12:07
@oliviertassinari oliviertassinari added type: bug It doesn't behave as expected. scope: text field Changes related to the text field. labels Aug 31, 2021
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is a footgun for GlobalStyles in general with styled-components.

We should document this for GlobalStyles. Have you checked the other uses of GlobalStyles in the codebase?

Do both issues require this complete change or did you group two changes that can independently fix either issue?

@mnajdova
Copy link
Member Author

mnajdova commented Sep 1, 2021

Seems like this is a footgun for GlobalStyles in general with styled-components.

Not sure what we can do about this.. :\

We should document this for GlobalStyles. Have you checked the other uses of GlobalStyles in the codebase?

This and the CSSBaseline are the only components that use GlobalStyles. With the CSSBaseline it should not be a big problem I think, but in the InputBase is more obvious, as it is used much more, and it is more dynamic...

About the documentation, agree, I will create a PR with a note in the documentation for the GlobalStyles used with styled-components.

Do both issues require this complete change or did you group two changes that can independently fix either issue?

I've grouped the two changes, mostly because they are both affected by the global styles and they are both part of the v5 milestone.

@eps1lon
Copy link
Member

eps1lon commented Sep 1, 2021

I've grouped the two changes, mostly because they are both affected by the global styles and they are both part of the v5 milestone.

Ok. Could you make it obvious in the PR description which change fixes which problems? I'd have to guess right now but guessing is risky long-term.

@mnajdova
Copy link
Member Author

mnajdova commented Sep 1, 2021

Ok. Could you make it obvious in the PR description which change fixes which problems? I'd have to guess right now but guessing is risky long-term.

Sure, makes sense. Updated.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return (
<React.Fragment>
<GlobalStyles
styles={{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just learned that @babel/plugin-transform-constant-elements deopts mutable props. This unlocks a bunch of optimizations actually since we treat props as immutable all the time. Will investigate in a follow-up.

@mnajdova mnajdova merged commit 9831c8c into mui:next Sep 1, 2021
@bryan-hunter
Copy link
Contributor

@mnajdova stumbled across this PR when searching.

Our app is rendering a lot of text fields at once and this is injecting a global style in the <head> for each one and removing them when unmounted.

This is causing an initial render performance issue when the number of text fields to render at once is pretty big.

Is there any way to make this only inject once?

@mnajdova
Copy link
Member Author

mnajdova commented Oct 20, 2021

Is there any way to make this only inject once?

Sorry for the late response @bryan-hunter . We want the components to be able to be used in isolation without needing to setup anything else so by default I don't think so. Maybe we could add a property on the component that if set, would just not render the global styles, assuming the developers already rendered those at the top of their App. Would something like this work for your team? Would you like to submit a PR?

Also would be useful to see some benchmark that would show the perf difference we would have with this.

@bryan-hunter
Copy link
Contributor

Is there any way to make this only inject once?

Sorry for the late response @bryan-hunter . We want the components to be able to be used in isolation without needing to setup anything else so by default I don't think so. Maybe we could add a property on the component that if set, would just not render the global styles, assuming the developers already rendered those at the top of their App. Would something like this work for your team? Would you like to submit a PR?

Also would be useful to see some benchmark that would show the perf difference we would have with this.

I will attempt a PR now :)

I patched material-ui to remove the GlobalStyle and added it to createTheme with

MuiCssBaseline: {
            styleOverrides: `
                @keyframes mui-auto-fill{from{display:block;}}
                @keyframes mui-auto-fill-cancel{from{display:block;}}
                `,
        },

if anyone needs a solution in the interim

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 27, 2025

@mnajdova Is there no performant way for Inputs to query the document to see if a needed global style tag has already been added? That would be one way to keep the component working in isolation without adding hundreds of redundant style tags.

Honestly if there were no better options I would be happier to see monkeypatching like if (!document.__MUI_injected_InputBase_global_styles__) { ... } than performance tanking by default on just a few hundred Inputs mounting at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: text field Changes related to the text field. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextField rerendering causes global styles to recalculate on each keystroke with SC [TextField] Autofill detection not working in production

7 participants