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

Avoid full style recalculation by setting custom property only on element that uses it #597

Closed

Conversation

Inwerpsel
Copy link
Contributor

@Inwerpsel Inwerpsel commented Oct 2, 2024

Addresses #587

This PR is a safer alternative to #592 as it does not change any of the CSS logic, it only sets the custom property at a lower level in the tree.

Since the --scrolltop custom property is only used on the #toc element, we can set its value on that element instead of the root element. This avoids a full style recalculation of all elements on the page which takes very long, and causes massive framerate drops during scrolling.

This should not result in any changed behavior or appearance as no other element uses the custom property. It should be a pure optimization with no risk for regressions.

Comparing the performance recordings before and after this change, you can observe the time spent on style recalculation is reduced to a fraction. The long purple chunks shrunk became almost too small to see on the right.

colorjs-before-after-toc-optim

https://colorjs.io/docs/spaces vs https://deploy-preview-597--colorjs.netlify.app/docs/spaces

Copy link

netlify bot commented Oct 2, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 2098afd
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/66fd3cca994f6a0008175887
😎 Deploy Preview https://deploy-preview-597--colorjs.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 configuration.

…ment that uses it

Since the `--scrolltop` custom property is only used on the `#toc` element,
we can avoid a *full* style recalculation of all elements on the page which can take very long,
and causes massive framerate drops during scrolling.

This should not result in any changed behavior or appearance as no other element uses the custom property.
@Inwerpsel Inwerpsel force-pushed the optimize-scrolltop-custom-property branch from 578a637 to 2098afd Compare October 2, 2024 12:30
@Inwerpsel Inwerpsel marked this pull request as ready for review October 2, 2024 12:55
@LeaVerou
Copy link
Member

LeaVerou commented Oct 2, 2024

Thanks! Though ideally, we need to fix this even for pages that do have a TOC. @DmitrySharabin is actually investigating a potential fix right now.

@Inwerpsel
Copy link
Contributor Author

This does also address the performance issue on pages with a TOC. On those pages, in the previous version it would have to style recalculate all elements on the page before it was able to paint the scroll. With these changes, it only recalculates the TOC itself, which speeds the recalculation up by a factor of about 20 to 50 (see screenshot above of tasks run during scrolling on a page with a TOC). This effectively removes the performance issue.

So it might be a quick win if the required changes would take a bit longer.

@DmitrySharabin
Copy link
Contributor

@Inwerpsel Your fix with CSS does the job nicely, to my mind. We don't need the scroll event with that fix, so we can safely ditch it. I would follow that route. I left a comment for you in that PR. I would love to know your thoughts on it.

@Inwerpsel Inwerpsel closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants