search

emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.42k stars 1.11k forks source link

Emotion causes large Layout Shift (with critical CSS extraction) #2154

Open neefrehman opened 3 years ago

neefrehman commented 3 years ago

Current behavior:

I just migrated a project from Linaria to Emotion, but am seeing a massive increase in Cumulative Layout Shift, and I'm not sure why. I'm using Next.js and can confirm that the "out of the box" critical CSS extraction is working fine after looking at the HTML output. It's my understanding that this should reduce layout shift as the styles will load before paint.

Inspecting the network panel shows a small increase in bundle size, but the performance panel shows hugely elongated period of layout shift (seen in light red below). To my eye it looks like there's a re-painting of all the CSS upon rehydration, which causes a flicker.

Before (deployed url):

image

After (deployed url | repo at commit):

image

Using devtools' break on subtree modifications I can see that the SSR styles are initially in a style tag inside Next's App container element in the body (above the other dom content). Then they are deleted from their place and placed in the head, which looks to be what's causing the shift. It seems like the styles should already be placed in the head during the SSR process, and not have to be moved later?

To reproduce:

  1. Fork the repo and ensure you are at commit e44151d
  2. Run npm run build && npm run start
  3. Refresh the homepage to see the layout shift occur

Expected behavior:

The page should load, being render-blocked by the critical css, and then painting to the screen without any subsequent layout shift or style changes.

Environment information:

Andarist commented 3 years ago

I understand that this is not the perfect answer to your problem but a workaround for the issue seems to be this:

diff --git a/src/pages/_document.tsx b/src/pages/_document.tsx
index ef8dfa2..01e82ff 100755
--- a/src/pages/_document.tsx
+++ b/src/pages/_document.tsx
@@ -31,6 +31,14 @@ class MyDocument extends Document {
                     <meta property="twitter:title" content={title} />
                     <meta property="twitter:description" content={description} />
                     <meta property="twitter:image" content={imageUrl} />
+                    <style>
+                        {`@font-face {
+                            font-family: "Fleuron";
+                            font-weight: normal;
+                            font-display: block;
+                            src: url("/static/fonts/fleuronregular.woff2");
+                        }`}
+                    </style>
                 </Head>

                 <body>
diff --git a/src/styles/GlobalStyles.tsx b/src/styles/GlobalStyles.tsx
index 372ade2..52088a4 100644
--- a/src/styles/GlobalStyles.tsx
+++ b/src/styles/GlobalStyles.tsx
@@ -5,13 +5,6 @@ export const GlobalStyles = () => {
     const { colors } = useTheme();

     const globalStyles = css`
-        @font-face {
-            font-family: "Fleuron";
-            font-weight: normal;
-            font-display: block;
-            src: url("/static/fonts/fleuronregular.woff2");
-        }
-
         * {
             margin: 0;
         }

For some reason, this font is causing a layout shift - when the rule for it is temporarily removed from the DOM. Usually, this doesn't cause any problems - this is the very first report about this problem and I haven't encountered it in the past.

neefrehman commented 3 years ago

@Andarist Thanks! This also makes the performance panel make a bit more sense. It does look like the loading of the font is interrupted, which I'm guessing now is because of the styles removal. I'll implement this workaround for now.

Is the DOM node switch for the style tag necessary for some reason? It feels like a strange way to handle inlining styles so I'm curious. I can imagine it might be prone to other issues like this.

Andarist commented 3 years ago

Is the DOM node switch for the style tag necessary for some reason? It feels like a strange way to handle inlining styles so I'm curious. I can imagine it might be prone to other issues like this.

We render styles "inline" on the server so you don't have to do any extra config step (nor runtime work) to get a workable output. On the client, we need to move those styles to head though so React doesn't see them in the SSRed output so it won't complain about SSR-client mismatch.

While somewhat strange this solution has been proven to be quite effective for a couple of years now. There are Emotion-based libraries that can be just installed and used right away - without having to document how to actually do the SSR.

We provide alternative ways to do SSR - so you could also use them if only you don't mind some extra config: https://emotion.sh/docs/ssr#advanced-approach

neefrehman commented 3 years ago

@Andarist Thanks for the details! I'm not that experienced with critical extraction so this is interesting. I assumed you could extract the styles to the head immediately at build time, but now I'm realising that would have to be a next-specific implementation whereas the current one is framework agnostic, which is cool. I can see a next implementation in an old version of their example repo, so may move to that at some point.

On this layout shift, any ideas why this is the first report? I don't think the font is that large. Do other users usually add their @font-face rules elsewhere? Does there have to be a moment where the styles aren't in the DOM at all for react to not complain? Maybe delaying their removal from body until the head injection is confirmed them could solve things if not.

I also have one last question:

Server side rendering works out of the box in Emotion 10 and above if you’re only using @emotion/react and @emotion/styled

In my app I'm having to use css and cx from @emotion/css in a couple of places — as cx won't accept classNames made with css from /react. I'm assuming that this means those styles will be unavailable for critical extraction?

Andarist commented 3 years ago

I can see a next implementation in an old version of their example repo, so may move to that at some point.

Right - this is using that "advanced" setup I've been mentioning.

On this layout shift, any ideas why this is the first report? I don't think the font is that large. Do other users usually add their @font-face rules elsewhere?

I honestly don't know and this has surprised me a little bit. Maybe your font is too different from the fallback one? And the reserved space for it is just not enough for the final one which results in a layout shift to be reported? Could you try with different fonts or with a smaller heading? Just to get more information about how this behaves in browsers. This would be a valuable information.

Does there have to be a moment where the styles aren't in the DOM at all for react to not complain?

We need to move the styled before .hydrate() is called.

Maybe delaying their removal from body until the head injection is confirmed them could solve things if not.

That's interesting idea that also has crossed my mind. Technically this shouldn't be a problem if have the same rules twice isnerted into the DOM for a brief moment of time - it would require copying those nodes though. I might check this out later but no promises if that would get into Emotion even if it would solve the problem.

In my app I'm having to use css and cx from @emotion/css in a couple of places — as cx won't accept classNames made with css from /react. I'm assuming that this means those styles will be unavailable for critical extraction?

Yes, I think they might not be available because extractCritical extracts those styles from the cache and you basically have 2 caches if you are mixing those. You could fix this by doing this:

import { cache } from '@emotion/css'
// ...

<CacheProvider value={cache}>
  <App/>
</CacheProvider>

This would use a "compat" cache from the @emotion/css but if you plan to use extractCritical then it shouldn't be a problem (it potentially could be a problem if you would like to use the 0-config approach).

Other question for this one would be - why do u need cx? Couldn't you just use ClassNames component?

neefrehman commented 3 years ago

Maybe your font is too different from the fallback one? And the reserved space for it is just not enough for the final one which results in a layout shift to be reported?

Yes this looks to be the case. Selecting a larger fallback font reduces the layout shift. Maybe because the font-display: block property is required to avoid this, but is temporarily missing.

Could you try with different fonts or with a smaller heading? Just to get more information about how this behaves in browsers. This would be a valuable information.

I've tried swapping fonts around and can confirm it's an issue with other fonts and sizes too, only the ratio of the layout shift changes. I've also tried with a font from Google Fonts (with a preloaded link), and am having the same issue. I've updated my minimal repro with that test. You can see the relevant bits _document.tsx and GlobalStyles.tsx

Technically this shouldn't be a problem if have the same rules twice isnerted into the DOM for a brief moment of time

That sounds promising! Let me know if you need me to test any potential fixes.

Other question for this one would be - why do u need cx? Couldn't you just use ClassNames component?

I much prefer the cx() pattern from @emotion/css to the renderprops pattern from ClassNames. I guess my dream solution here would be for @emotion/react to export it's own cx function that can accept the package's SerializedStyles and grab that className from it. Then I can remove @emotion/css alltogether.

Andarist commented 3 years ago

I much prefer the cx() pattern from @emotion/css to the renderprops pattern from ClassNames. I guess my dream solution here would be for @emotion/react to export it's own cx function that can accept the package's SerializedStyles and grab that className from it. Then I can remove @emotion/css altogether.

@emotion/react handles composition of the SerializedStyles just fine - you can pass arrays to the css prop. I assume that you want to use non-emotion class names though, right? And this is why you need the cx?

neefrehman commented 3 years ago

I assume that you want to use non-emotion class names though, right? And this is why you need the cx?

Yes exactly!

neefrehman commented 3 years ago

I've just implemented the workaround by importing the @font-face rule as JSX into the Head from globalStyles.tsx so the styles are still co-located. I had to use dangerouslySetInnerHTML however, as otherwise the output would include escaped quotes (&quot;) which would not load the font:

// globalStyles.tsx
+ export const FontFaceWorkaround = () => (
+     <style>
+        // eslint-disable-next-line react/no-danger
+        dangerouslySetInnerHTML={{
+            __html: `
+            @font-face {
+                font-family: "Fleuron";
+                font-weight: normal;
+                font-display: block;
+                src: url("/static/fonts/fleuronregular.woff2");
+            }`,
+        }}
+    />
+ );

// _document.tsx
+     <Head>
          {...otherElements}
+         <FontFaceWorkaround />
+     </Head>

~And now there is 0 layout shift! So even better than the static css extraction from Linaria.~

UPDATE: After fixing the issue with dangerouslySetInnerHTML there is now still a small amount of layout shift. But in line with the old result from Linaria. The @font-face rule wasn't being parsed due to the unescaped quotes, so the local version of my font was being used.

UPDATE 2: I just tried implementing the advanced extraction method in this commit, which resulted in more layout shift and a secondary flash of changed styles, despite the styles being applied to the head at build time. Strange! Deployed site here.

neefrehman commented 3 years ago

Since I've attempted a few different ways of CSS extraction I thought I'd put a bit more rigour into a test. Here are the links for each strategy as well as a (roughly calculated) average latest layout shift from the performance panel.

CSS Strategy & Link to deploy Average latest layout shift
Linaria 360 ms
Emotion - out of the box critical extraction 950 ms
Emotion - out of the box critical extraction + @font-face fix 310 ms
Emotion - advanced critical extraction 400 ms
ivan-kleshnin commented 2 years ago

I have a layout shift with :where selectors in NextJS applied after an inappropriate delay. I'm curious if extractCritical behavior is controllable. I mean – can we hint Emotion that some components are "critical" for the layout? For some reason I can't find any information on that. What is considered "critical" and how to modify it?