Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

Improve navigation drawer performance, remove/update old polyfills #9409

Merged
merged 4 commits into from
Feb 8, 2023

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Jan 13, 2023

web.dev has "needs improvement" INP for both the home page and the origin as a whole

PageSpeed Insights INP diagram showing a 'needs improvement' INP of 288 milliseconds

@philipwalton identified the cookie consent banner and navigation drawer as two sources of INP from web-vitals.js element attribution data, and noticed that both the focus-visible and wicg-inert polyfills were major parts of the task blocking the main thread on interactions with those components. This PR focuses on the navigation drawer first because its performance appears worse than the consent banner and (in theory) it will be interacted with much more often than the consent banner.

@tunetheweb and I failed to coordinate, so this overlaps with #9408

Changes proposed in this pull request (split up by commit for easier review):

Remove focus-visible polyfill

This polyfill runs whether or not the browser had built-in support for :focus-visible, causing extra work for no benefit. There was discussion about how to make it polyfill only when needed, but as of March 2022, all the major browsers support :focus-visible and it's unnecessary (the readme now even tells you to not use it).

I also removed the polyfill-specific CSS rules that would not have any effect after the polyfill is gone. I was trying to be careful and not break any side effects of the rules, but then I noticed that none of them are actually shipped in the current web.dev anyways and next.css seems written for current browsers that all support :focus-visble. As a result, there shouldn't be any observable changes.

Update wicg-inert polyfill

Unfortunately Firefox's implementation of inert is still ongoing, so this polyfill can't be removed without breaking behavior. However, there was a bug that caused this polyfill to also run whether or not the browser had built-in support for inert, so e.g. Chrome and Safari have been paying the cost for both the built-in and polyfill inert implementations. This commit updates the polyfill after that issue was fixed, and so (in updated browsers) the polyfill should only run in Firefox.

(this does mean that Firefox is still running the relatively slow polyfill, but the next commit will help ameliorate that)

Async page inert when nav drawer is opened

Turns out even the built-in inert causes pretty significant main-thread work. It just shows up as a large "Recalculate Style" block in DevTools due to changing main.inert, and it's not clear why it's so slow (I did not expect going inert would require a large style recalc). Assuming we still want the rest of the page to be inert when the drawer is open, the best thing to do is then break the long task up. Here it's using a double requestAnimationFrame to schedule the inert change on the next frame so that the click handler can return control of the main thread immediately. This will also help Firefox with the polyfill, as that work will also happen async.

Comparison

Both screenshots are the nav drawer open interaction on my desktop machine with 4x CPU slowdown, roughly directly comparable because they're zoomed to show ~130ms.

Before:
DevTools trace showing a 108 millisecond interaction and a flame chart of js and style tasks below it
(view trace)

After:
DevTools trace showing a 44 millisecond interaction and a flame chart of js and style tasks below it, with considerably less depth to the CPU tasks
(view trace)

The light green blocks (mostly wicg-inert) is gone. The large "Recalculate Stye" block still occurs due to applying inert, but it's now moved out of the interaction and so multiple frames are no longer dropped.


Alternative to consider:

  • Drop inert for the nav drawer. While it's probably the right thing to use here, AFAIK it's relatively uncommon to use that for menus as long as the focus is correctly placed in the menu, the menu is dismissible, etc. The performance hit also seems weirdly high and can hopefully be fixed at the browser level to encourage use for large portions of the DOM like this.

    The current nav drawer has also been buggy for a while, trying to make the <footer> also inert, but using a selector for the old footer.njk instead of the newer site-footer.njk and so the footer has not been inert since switching.

Remaining issues:

  • There's still a decent chunk of (I believe) lit code running on every open/close of the drawer. It's hard to follow what it's doing, but it seems like it would be unnecessary for a static drawer reveal/hide
  • On the homepage, recaptcha also shows up in the trace of these interactions, tracking attribute changes (like inert) across the whole page with mutation observers. It's not a huge amount of work, but again seems completely unnecessary for this interaction. Should recaptcha be loaded more on demand, maybe once a user starts interacting with the form at the bottom of the page?
  • I need to measure this, but I suspect the blur and shadow being applied to page contents behind the drawer are also a perf hit we might consider reducing (especially since it's almost always only seen on mobile devices)

@netlify
Copy link

netlify bot commented Jan 13, 2023

Deploy Preview for web-dev-staging ready!

Name Link
🔨 Latest commit c7184c5
🔍 Latest deploy log https://app.netlify.com/sites/web-dev-staging/deploys/63dc357d242a4b0009552d67
😎 Deploy Preview https://deploy-preview-9409--web-dev-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

OK @brendankenny let's go with this one over mine and see impact.

Can you look at the merge conflicts?

main.inert = true;
footer.inert = true;
// Setting the majority of the page as inert can have a significant perf hit when
// trying to animate e.g. the navigation drawer, so do it in the frame after this.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// trying to animate e.g. the navigation drawer, so do it in the frame after this.
// trying to animate e.g. the navigation drawer, so do it in the frame after this
// to avoid any INP delays.

@brendankenny
Copy link
Member Author

brendankenny commented Feb 2, 2023

Updated! @tunetheweb, do you know who else we need PR approval from?

@tunetheweb
Copy link
Member

@devnook can you approve this one? I’ve had a look and am happy with it.

@devnook
Copy link
Contributor

devnook commented Feb 8, 2023

Tracking recaptcha proposal here: #9528

Copy link
Contributor

@devnook devnook left a comment

Choose a reason for hiding this comment

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

Thanks @brendankenny and @tunetheweb !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants