Remove useless getComputedStyle call. #27969

emilio · 2021-08-11T16:30:52Z

See w3c/csswg-drafts#6501 to see how I found this.

This line doesn't recompute layout in browsers, because "height" is given as a pseudo-element name rather than a property.

The right way to do what it wants is getComputedStyle(document.body).height, but given nobody noticed this (and this is generally never needed, manually triggering layout should never be needed to avoid FOUC) it seems better to keep current behavior and just remove the call.

See w3c/csswg-drafts#6501 to see how I found this. This line doesn't recompute layout in browsers, because `"height"` is given as a pseudo-element name rather than a property. The right way to do what it wants is `getComputedStyle(document.body).height`, but given nobody noticed this (and this is generally never needed, manually triggering layout should never be needed to avoid FOUC) it seems better to keep current behavior and just remove the call.

stefanprobst · 2021-08-11T16:41:44Z

related: #27938

Timer · 2021-08-12T19:10:59Z

packages/next/client/index.tsx

-
-      // Force browser to recompute layout, which should prevent a flash of
-      // unstyled content:
-      getComputedStyle(document.body, 'height')


I still believe we need to force recompute here, or we should at least be very sure we want to get rid of this. It might be introducing some other form of main thread blocking that has an effect.

Can we ship this as getComputedStyle(document.body).height for now?

Why changing behavior? I'm pretty sure in all browsers this is a no-op right now.

(Source: I'm a Mozilla employee, and also a WebKit and Chromium committer)

gu-stav · 2021-08-18T13:58:58Z

@Timer @ijjk could you try to get this PR in for 11.1.1? Currently every next.js site crashes after every client side navigation on Firefox Canary because of this. And apparently there are many next.js sites out there ...

ijjk · 2021-08-18T15:56:17Z

@gu-stav does that mean a breaking change was made in Firefox canary that isn't present in other browsers? I think we could land this change per #27969 (comment) but it would also be nice to ensure this breaking change doesn't make it's way to stable Firefox if possible as this could break existing sites, potentially without them knowing since it's a client-transition until they are able to upgrade.

gu-stav · 2021-08-18T18:27:14Z

@ijjk Yes, it does. In a recent version the invalid argument triggers a TypeError after a client side navigation, which then crashes the whole app. This is reproducible in the next.js website too, in case you have to. The behavior is documented on MDN as expected: https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle but so far it doesn't happen in any other browser, so I'm not sure if the spec was updated or whether it was never implemented that way ...

In my PR I just removed the second argument, but to me @emilio argumentation makes sense. If it's evaluated lazy then the whole call could be removed.

emilio · 2021-08-19T08:47:25Z

The breaking change in Firefox was reverted a while ago, fwiw.

ijjk · 2021-09-20T02:38:25Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall decrease ✓

	vercel/next.js canary	emilio/next.js patch-1	Change
buildDuration	13.8s	13.5s	-325ms
buildDurationCached	3.4s	3.5s	⚠️ +197ms
nodeModulesSize	182 MB	182 MB	-289 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	emilio/next.js patch-1	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.962	3.003	⚠️ +0.04
/ avg req/sec	844.01	832.46	⚠️ -11.55
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.672	1.645	-0.03
/error-in-render avg req/sec	1495.12	1520	+24.88

Client Bundles (main, webpack, commons) Overall decrease ✓

	vercel/next.js canary	emilio/next.js patch-1	Change
779.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	26.8 kB	26.7 kB	-20 B
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	70.6 kB	70.6 kB	-20 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	emilio/next.js patch-1	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	emilio/next.js patch-1	Change
_app-HASH.js gzip	977 B	977 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	311 B	311 B	✓
css-HASH.js gzip	328 B	328 B	✓
dynamic-HASH.js gzip	2.67 kB	2.67 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	918 B	918 B	✓
image-HASH.js gzip	4.14 kB	4.14 kB	✓
index-HASH.js gzip	260 B	260 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	13 kB	13 kB	✓

Client Build Manifests

	vercel/next.js canary	emilio/next.js patch-1	Change
_buildManifest.js gzip	493 B	493 B	✓
Overall change	493 B	493 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	emilio/next.js patch-1	Change
index.html gzip	538 B	540 B	⚠️ +2 B
link.html gzip	550 B	552 B	⚠️ +2 B
withRouter.html gzip	532 B	534 B	⚠️ +2 B
Overall change	1.62 kB	1.63 kB	⚠️ +6 B

Diffs

Diff for main-HASH.js

@@ -1965,10 +1965,7 @@
               function(el) {
                 el.parentNode.removeChild(el);
               }
-            ); // Force browser to recompute layout, which should prevent a flash of
-            // unstyled content:
-
-            getComputedStyle(document.body, "height");
+            );
           }
 
           if (input.scroll) {

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-b119ca76bd478b712255.js"
+      src="/_next/static/chunks/main-020a4165c730e9690f21.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-b119ca76bd478b712255.js"
+      src="/_next/static/chunks/main-020a4165c730e9690f21.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-b119ca76bd478b712255.js"
+      src="/_next/static/chunks/main-020a4165c730e9690f21.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	emilio/next.js patch-1	Change
buildDuration	6.9s	6.7s	-205ms
buildDurationCached	3.2s	3.2s	-27ms
nodeModulesSize	182 MB	182 MB	-289 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	emilio/next.js patch-1	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.05	2.976	-0.07
/ avg req/sec	819.65	840.11	+20.46
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.6	1.626	⚠️ +0.03
/error-in-render avg req/sec	1562.99	1537.74	⚠️ -25.25

Client Bundles (main, webpack, commons) Overall decrease ✓

	vercel/next.js canary	emilio/next.js patch-1	Change
675-HASH.js gzip	13.8 kB	13.8 kB	✓
770.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	50.7 kB	50.7 kB	✓
main-HASH.js gzip	34.6 kB	34.6 kB	-18 B
webpack-HASH.js gzip	1.65 kB	1.65 kB	✓
Overall change	101 kB	101 kB	-18 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	emilio/next.js patch-1	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	emilio/next.js patch-1	Change
_app-HASH.js gzip	1.32 kB	1.32 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	315 B	315 B	✓
css-HASH.js gzip	331 B	331 B	✓
dynamic-HASH.js gzip	2.8 kB	2.8 kB	✓
head-HASH.js gzip	356 B	356 B	✓
hooks-HASH.js gzip	637 B	637 B	✓
image-HASH.js gzip	573 B	573 B	✓
index-HASH.js gzip	262 B	262 B	✓
link-HASH.js gzip	2.2 kB	2.2 kB	✓
routerDirect..HASH.js gzip	326 B	326 B	✓
script-HASH.js gzip	393 B	393 B	✓
withRouter-HASH.js gzip	322 B	322 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	10.1 kB	10.1 kB	✓

Client Build Manifests

	vercel/next.js canary	emilio/next.js patch-1	Change
_buildManifest.js gzip	511 B	511 B	✓
Overall change	511 B	511 B	✓

Rendered Page Sizes

	vercel/next.js canary	emilio/next.js patch-1	Change
index.html gzip	538 B	539 B	⚠️ +1 B
link.html gzip	552 B	551 B	-1 B
withRouter.html gzip	531 B	531 B	✓
Overall change	1.62 kB	1.62 kB	✓

Diffs

Diff for main-HASH.js

@@ -1226,9 +1226,6 @@
                 el.parentNode.removeChild(el);
               }
             );
-            // Force browser to recompute layout, which should prevent a flash of
-            // unstyled content:
-            getComputedStyle(document.body, "height");
           }
           if (input.scroll) {
             window.scrollTo(input.scroll.x, input.scroll.y);

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-67c2749aa505d94085cb.js"
+      src="/_next/static/chunks/main-c94ff6234e70f3312c7b.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-67c2749aa505d94085cb.js"
+      src="/_next/static/chunks/main-c94ff6234e70f3312c7b.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-67c2749aa505d94085cb.js"
+      src="/_next/static/chunks/main-c94ff6234e70f3312c7b.js"
       defer=""
     ></script>
     <script

Commit: 28f90b0

ijjk

We discussed this internally a bit more and it sounds safe to remove since it isn't actually recomputing the layout and shouldn't be a change of behavior to remove. We can investigate re-adding with the corrected property accessing in a future PR and see if it makes any difference there. Thanks for investigating this!

emilio requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners August 11, 2021 16:30

ijjk added the type: next label Aug 11, 2021

emilio mentioned this pull request Aug 11, 2021

Prevent TypeError during client side navigation #27938

Closed

styfle requested a review from Timer August 12, 2021 18:42

Timer reviewed Aug 12, 2021

View reviewed changes

Merge branch 'canary' into patch-1

d978728

Merge branch 'canary' into patch-1

2b66b7b

This comment has been minimized.

Sign in to view

Merge branch 'canary' into patch-1

28f90b0

ijjk approved these changes Sep 20, 2021

View reviewed changes

kodiakhq bot merged commit 05a732e into vercel:canary Sep 20, 2021

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

emilio deleted the patch-1 branch July 27, 2023 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove useless getComputedStyle call. #27969

Remove useless getComputedStyle call. #27969

emilio commented Aug 11, 2021

stefanprobst commented Aug 11, 2021

Timer Aug 12, 2021

emilio Aug 12, 2021

emilio Aug 12, 2021

gu-stav commented Aug 18, 2021

ijjk commented Aug 18, 2021 •

edited

Loading

gu-stav commented Aug 18, 2021 •

edited

Loading

emilio commented Aug 19, 2021

This comment has been minimized.

ijjk commented Sep 20, 2021

Diffs

Diffs

ijjk left a comment

Remove useless getComputedStyle call. #27969

Remove useless getComputedStyle call. #27969

Conversation

emilio commented Aug 11, 2021

stefanprobst commented Aug 11, 2021

Timer Aug 12, 2021

Choose a reason for hiding this comment

emilio Aug 12, 2021

Choose a reason for hiding this comment

emilio Aug 12, 2021

Choose a reason for hiding this comment

gu-stav commented Aug 18, 2021

ijjk commented Aug 18, 2021 • edited Loading

gu-stav commented Aug 18, 2021 • edited Loading

emilio commented Aug 19, 2021

This comment has been minimized.

ijjk commented Sep 20, 2021

Stats from current PR

Diffs

Diffs

ijjk left a comment

Choose a reason for hiding this comment

ijjk commented Aug 18, 2021 •

edited

Loading

gu-stav commented Aug 18, 2021 •

edited

Loading