Conversation
🦋 Changeset detectedLatest commit: 883ad05 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 |
…n page load Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
--dialog-scrollgutter computation to avoid synchronous reflow on page load
…ScrollGutter Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves page-load performance by deferring the layout-forcing --dialog-scrollgutter computation from <dialog-helper>’s connectedCallback() to the moment a dialog is actually opened as a modal.
Changes:
- Introduces a module-level
setScrollGutter(doc)helper that computes and sets--dialog-scrollgutteronce per document. - Removes eager scroll gutter computation from
DialogHelperElement.connectedCallback(). - Invokes
setScrollGutter()immediately beforeshowModal()in both click-invocation and[open]-attribute handling paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/components/primer/dialog_helper.ts | Defers scroll gutter computation to dialog-open time via a shared helper. |
| .changeset/dialog-scrollgutter-lazy.md | Adds a patch changeset describing the lazy computation change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,8 @@ | |||
| function setScrollGutter(doc: Document) { | |||
| if (doc.body.style.getPropertyValue('--dialog-scrollgutter')) return | |||
| doc.body.style.setProperty('--dialog-scrollgutter', `${window.innerWidth - doc.body.clientWidth}px`) | |||
There was a problem hiding this comment.
setScrollGutter takes a Document but uses the global window.innerWidth. If the dialog lives in a different browsing context (e.g. inside an iframe), window.innerWidth may not correspond to doc.body.clientWidth, producing an incorrect gutter value. Use doc.defaultView?.innerWidth (or the dialog’s ownerDocument.defaultView) instead, with a safe fallback if defaultView is null.
| doc.body.style.setProperty('--dialog-scrollgutter', `${window.innerWidth - doc.body.clientWidth}px`) | |
| const view = doc.defaultView | |
| const viewportWidth = view?.innerWidth ?? window.innerWidth | |
| doc.body.style.setProperty('--dialog-scrollgutter', `${viewportWidth - doc.body.clientWidth}px`) |
There was a problem hiding this comment.
I think this exists already - so not changing it for now, but maybe this makes sense?
| if (dialogId) { | ||
| const dialog = document.getElementById(dialogId) | ||
| if (dialog instanceof HTMLDialogElement) { | ||
| setScrollGutter(dialog.ownerDocument) | ||
| dialog.showModal() |
There was a problem hiding this comment.
This change moves --dialog-scrollgutter initialization from connectedCallback() to just-in-time before showModal(). There are existing system tests for dialogs, but none here assert that opening via <dialog-helper> results in the body scroll gutter being set (even to 0px). Adding a regression assertion in the dialog system test suite would help prevent future reintroductions of eager computation or missing initialization paths.
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?
DialogHelperElement.connectedCallbackeagerly computes--dialog-scrollgutterby readingbody.clientWidth— a layout-dependent property that forces a synchronous reflow during page load. This runs for every<dialog-helper>element on the page, negatively impacting Core Web Vitals (LCP, INP).This PR defers the
--dialog-scrollguttercomputation to the moment a dialog is actually opened as a modal, avoiding the reflow cost during page load entirely.Background: PR #3947 attempted to fix this by combining lazy JS computation with a
scrollbar-gutter: stableCSS change. PR #3949 reverted that becausescrollbar-gutter: stabledoesn't work onbody(onlyhtml) and reserves scrollbar space on non-scrolling pages. This PR re-applies only the JavaScript-side deferral — no CSS changes.Changes in
app/components/primer/dialog_helper.ts:setScrollGutter(doc)helper that computes and sets--dialog-scrollgutterondoc.body.style, usinggetPropertyValueto skip the computation if the property is already set.--dialog-scrollguttercomputation fromconnectedCallback().setScrollGutter()calls just beforeshowModal()in bothdialogInvokerButtonHandlerand#handleDialogOpenAttribute().Since
--dialog-scrollgutteris only consumed bybody:has(dialog:modal.Overlay--disableScroll), it only matters when a dialog is shown as a modal — making lazy computation both safe and correct.Screenshots
N/A — no visual changes. The scroll gutter behavior is identical; only the timing of when it is computed changes.
Integration
No consuming code changes are required.
List the issues that this change affects.
Closes #3946
Risk Assessment
What approach did you choose and why?
We defer the
body.clientWidthread (which triggers synchronous layout) fromconnectedCallback(page load) to just beforeshowModal()(user interaction). A simplegetPropertyValuecheck deduplicates the work so it only runs once per document, regardless of how many<dialog-helper>elements exist.Alternative considered: Using
scrollbar-gutter: stableCSS (attempted in #3947, reverted in #3949). This approach was rejected because the property must be applied tohtmlnotbody, and doing so reserves scrollbar space on all pages including those that don't scroll.Alternative considered: Using
requestAnimationFrameto defer the read. This was rejected because the computation is truly unnecessary until a dialog opens — deferring by one frame still pays the cost during page load.Anything you want to highlight for special attention from reviewers?
setScrollGutterimplementation usesdoc.body.style.getPropertyValue('--dialog-scrollgutter')as the deduplication check instead of aWeakSet<Document>. This is simpler and avoids extra module-level state — the style property itself serves as the "already computed" flag.#handleDialogOpenAttribute()closes and re-opens the dialog (the property will already be set from the first open, so the check short-circuits).Accessibility
Merge checklist