-
Notifications
You must be signed in to change notification settings - Fork 116
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
reduce cost of style calc+layout in --dialog-scrollgutter #3144
Merged
keithamus
merged 2 commits into
main
from
reduce-cost-of-style-calc-layout-in-dialog-scrollgutter
Oct 11, 2024
Merged
reduce cost of style calc+layout in --dialog-scrollgutter #3144
keithamus
merged 2 commits into
main
from
reduce-cost-of-style-calc-layout-in-dialog-scrollgutter
Oct 11, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: fa39d1e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as off-topic.
This comment was marked as off-topic.
keithamus
force-pushed
the
reduce-cost-of-style-calc-layout-in-dialog-scrollgutter
branch
from
October 9, 2024 15:30
9a3a670
to
b1c4117
Compare
dgreif
reviewed
Oct 9, 2024
langermank
approved these changes
Oct 9, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool! 😄
mattcosta7
reviewed
Oct 9, 2024
Co-authored-by: Aaron Ballard <ansballard@github.com> Co-authored-by: AdamShwert <adamshwert@github.com> Co-authored-by: Arelia Jones <2359538+arelia@users.noreply.github.com> Co-authored-by: Ariel Valentin <arielvalentin@users.noreply.github.com> Co-authored-by: Chris Gavin <chris@chrisgavin.me> Co-authored-by: Cody Bodfield <cbodfield@github.com> Co-authored-by: Daniel Colson <composerinteralia@github.com> Co-authored-by: Daniel Garman <garman@github.com> Co-authored-by: Dusty Greif <dgreif@users.noreply.github.com> Co-authored-by: Erinna Chen <erinnachen@users.noreply.github.com> Co-authored-by: George Brocklehurst <george@georgebrock.com> Co-authored-by: Jibran Garcia <jibrang@github.com> Co-authored-by: Jonathan Fuchs <21195+jfuchs@users.noreply.github.com> Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> Co-authored-by: Manuel Puyol <manuelpuyol@github.com> Co-authored-by: Matthew Costabile <mattcosta7@github.com> Co-authored-by: Matthew Reyes <another-mattr@github.com> Co-authored-by: Melissa Pastore <64283754+MelissaPastore@users.noreply.github.com> Co-authored-by: Ned Schwartz <theinterned@github.com> Co-authored-by: Phill MV <phillmv@github.com> Co-authored-by: Raffaele Di Fazio <raffo@github.com> Co-authored-by: Sarah Vessels <82317+cheshire137@users.noreply.github.com> Co-authored-by: Simon Taranto <srt32@users.noreply.github.com>
keithamus
force-pushed
the
reduce-cost-of-style-calc-layout-in-dialog-scrollgutter
branch
from
October 11, 2024 09:37
708d0c2
to
fa39d1e
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
keithamus
deleted the
reduce-cost-of-style-calc-layout-in-dialog-scrollgutter
branch
October 11, 2024 09:44
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Authors: Please fill out this form carefully and completely.
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
In our mob pairing session called "Making Dotcom Fast" we ran into an issue where the Primer Dialog component is causing an expensive recalc operation on every page load; this is due to it setting the
--dialog-scrollgutter
css property, which is a property derived from theinnerWidth
&body.clientWidth
properties.In order to prevent this cost on page load, we decided to move the operation to the first
click
event that can be handled by thedialogInvokerButtonHandler
. This moves the cost of the recalc away from document load to document click:However this change alone still leaves the recalc cost during the first click event which is still costly and could impact INP.
So we added some CSS - inspired by this article on the
@property
rule and how it can lower recalc costs by disabling inheritance - which is perfect for this use case. This reduced recalc cost from ~70-90ms on my machine to under 2ms:We've not moved the code away from the hot-path, and into the click event listener, due to compat risk with the Dialog element and its use in dotcom. This can come at a later date, perhaps.
Screenshots
(See above perf traces)
Integration
List the issues that this change affects.
Risk Assessment
What approach did you choose and why?
Anything you want to highlight for special attention from reviewers?
Accessibility
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.
/cc the commit co-authors:
@ansballard, @AdamShwert, @arelia, @arielvalentin, @chrisgavin, @Cbodfield, @composerinteralia, @garman, @dgreif, @erinnachen, @georgebrock, @jibrang, @jfuchs, @khiga8, @manuelpuyol, @mattcosta7, @another-mattr, @MelissaPastore, @theinterned, @phillmv, @Raffo, @cheshire137, @srt32