fix(ui): stop the dialog scroll lock shifting a fixed header - #1146
Conversation
|
Design rationale: I measured the gutter before trusting it, and WebKit does not honour it
The issue flagged the double-compensation interaction as the crux and asked for it to be settled empirically rather than by reading the spec. That was the right call, because reasoning would have got two things wrong. I drove Chromium, Firefox and WebKit through Playwright on a 1000px viewport, applied each candidate lock, and measured a fixed header, a centred in-flow box, and whether the wheel was actually blocked. A classic scrollbar can be forced on Chromium (drop Playwright's Finding 1. The classic probe is not a reliable residual measure. Under a reserved gutter Chromium reports Finding 2. WebKit ignores That is why the fix is not "set the gutter and delete the padding". It is "set the gutter, then measure what is left and handle it". Rejected: Rejected: Known limit, stated plainly. On an engine that ignores the gutter, an app that does not opt in still sees the old shift. It is never worse than today anywhere, and it is fixed with no opt-in wherever the gutter is honoured. Real Safari 18.2+ may well honour it, in which case those users get the no-opt-in fix automatically and the property is simply never set. I could not verify that: Playwright's headless WebKit only exposes a classic scrollbar via |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went back over the lock with fresh eyes and found four things worth fixing, all in the new code rather than inherited. Three of them are the same shape: the lock writes to <html> and <body> without accounting for what the page already put there.
The one I care most about is the cross-module case. dialog.ts and alert-dialog.ts keep separate refcounts by design, and a confirm opened from inside a dialog is the canonical way those two get used together. The inner lock unlocks first, so anything it removes outright is removed while the outer dialog is still open. Every other piece of state got a saved* companion; the custom property was the one that only got removed.
The position: fixed probe question is the interesting one. I reached for one to measure the ICB and it reads a stale box on WebKit, which is exactly the engine the fallback exists for, so it would have looked correct and done nothing. The root element's own border box turns out to be the right measure: it tracks the viewport and is laid out synchronously on both engines. That also made the whole thing simpler, one measurement applied two ways instead of two deltas.
Everything below is measured, not reasoned, across Chromium and WebKit with a forced classic scrollbar. All four now have a test that fails when the fix is reverted.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, this time on everything outside the lock function, and the most serious thing on the PR turned up here rather than in the first round.
The refcount was per module, and the two components ship as separate copies, so two counters were writing the same <html>. Save-and-restore only survives that if releases are LIFO, and they are not: disconnectedCallback fires in tree order and the #766 before-cache close runs in registration order, so a confirm inside a dialog releases outer-first and the inner unlock re-applies what it captured with nothing left to clear it. The page then stays padded for the rest of the session. One shared refcount on globalThis makes order irrelevant, and neither file imports the other.
The test holes matter as much. Three of the new tests could pass without proving anything, and the teardown I added specifically to stop a failure cascading did not actually stop it, because hide() only queues the unlock and my finally reset the app's root styles before it ran. So a red run wrote the app's values back onto <html> and every later test in the page inherited them. Fixed by awaiting the unlock inside teardown, asserting the lock engaged in each test, and making a zero-width scrollbar fail rather than skip whenever it could be a leak or a runner-config regression.
Also caught the website opt-in being on the wrong element. Verified with the property forced to 15px in Chromium rather than reasoned about.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third pass, zoomed out to cross-file consistency, and it found the two things a per-file read cannot see.
The first is a lock/unlock asymmetry that predates this PR but that this PR made dangerous. _setup() returns before locking when there is no content child while _teardown() unlocked unconditionally, so a contentless dialog could unlock without ever locking. That was survivable when the lock only touched body overflow; now that it writes the root's padding, gutter, and the published property, an unmatched unlock consumes another open dialog's count and restores the page out from under it. My first attempt at a guard was wrong (it only caught the count-already-zero case), and the test caught that, which is the whole reason to write the test before believing the fix.
The second is that examples/blog keeps its own hand-maintained copy of the dialog. The marketing site tracked the fix automatically because its copy is a gitignored mirror, but the blog's is real source, so example-blog.webjs.dev still had the original bug while webjs.dev was fixed. That is the more embarrassing failure of the two: fixing the kit and leaving a dogfood app broken.
Also narrowed a claim I should not have made. I wrote that Chromium and Firefox both honour the reserved gutter, but I never measured Firefox with a scrollbar that takes layout width, because it cannot be forced off overlay scrollbars in headless Playwright here. Nothing branches on the engine, so the code is fine either way; the docs just should not assert what was not established.
Hiding the page scrollbar widens the viewport by its width. The lock compensated by padding the body, which holds in-flow content still, but a position:fixed header lays out against the initial containing block and never sees that padding, so it slid right by half the scrollbar width. That hits the pinned-header pattern WebJs itself recommends (#610), and webjs.dev/ui is a live example. The lock now reserves the scrollbar gutter for its duration, so the viewport width never changes and nothing moves, in flow or fixed, with no cooperation from the page. Chromium and Firefox honour that. WebKit ignores scrollbar-gutter today, so the residual is measured rather than assumed and falls back to the old body padding plus a published --wj-scrollbar-compensation custom property a fixed element can opt into. When the gutter works the residual is zero, so the two paths cannot double-compensate. The test forces a classic scrollbar, because headless browsers default to overlay scrollbars where the whole scenario is inert and the assertion would pass vacuously. Playwright's --hide-scrollbars is dropped for Chromium for the same reason.
A failed assertion left the lock engaged, so the next test saw a page with no scrollbar and skipped itself instead of running, and the dropdown-menu tests failed as collateral. Closing every tracked dialog in teardown keeps a red run reporting only what actually broke.
The kit recommends a position: fixed header and its dialog used to move one, so the guidance and the component disagreed. Records the reserved gutter, the --wj-scrollbar-compensation opt-in, and the deliberate divergence from shadcn's body-padding-only approach. Opts webjs.dev's own fixed navbar in, which is the reported case.
Review of the first pass turned up four real problems, all measured in Chromium and WebKit before and after. An app that already reserves both gutters had its choice overwritten with the single-edge value, which dropped one gutter and shifted content by the full scrollbar width, a regression the previous code could not cause. The gutter is now only reserved when the page has not chosen one. The compensation now lands on <html> rather than <body>, and is added to any padding already there. Padding the body replaced the page's own value, and it missed the shift entirely on a max-width body, which does not widen when the viewport does, so a fixed header opting in got no compensation at all. The residual is measured from the root's border box. A position:fixed probe reads its OLD box on WebKit until the next rendering update, so it reported a zero delta and the fallback never fired. The published property is now restored on unlock rather than removed. The two components keep separate refcounts on purpose, so a confirm opened from inside a dialog unlocked first and stripped the dialog's compensation while it was still open.
The refcount was per module, and dialog.ts and alert-dialog.ts ship as separate copies, so two counters were mutating the same <html>. That only works if they release LIFO, and they do not: disconnectedCallback fires in tree order and the before-cache close runs in registration order, so a confirm opened inside a dialog releases OUTER first. The inner unlock then re-applied the values it had captured with nothing left to clear them, leaving the page padded and the compensation published for the rest of the session. One shared refcount on globalThis makes release order irrelevant, and keeps both files importing nothing from each other. Three test holes closed alongside it. Teardown now awaits the unlock before restoring the app's own root styles, because hide() only queues the unlock and the old order let it write those values straight back. Each test now asserts the lock actually engaged, so an assertion cannot pass because nothing happened. And a zero-width scrollbar is no longer always a skip: a leaked lock fails on every engine, and Chromium fails outright, since its scrollbar is guaranteed by the runner config rather than by the platform.
The opt-in was on .site-top, the fixed wrapper, which paints nothing. Padding it insets the header element that carries the background and bottom border, so with the compensation applied the painted chrome ended 15px short of the edge: measured 1265 to 1250 in Chromium with the property forced. Moving it to the centring bar holds the nav centre exactly as before and leaves the chrome edge-to-edge. Also documents what the lock actually does now, on all four surfaces. They each said it reserves the gutter, full stop, and none mentioned that it skips a page that declared its own gutter, or that the fallback pads <html>, which is otherwise a mystery inline style on your root element. The SSR test pins both halves, because the opt-in is one declaration in a layout and nothing else would notice it going missing.
_setup() returns before locking when there is no content child, but _teardown() unlocked unconditionally, so a contentless dialog could unlock without ever locking. That consumed another open dialog's refcount and ran the full restore mid-flight, dropping its compensation and jumping a fixed header until it closed. Rounds 1 and 2 moved the lock's writes onto <html>, which widened what this asymmetry could damage, so it is fixed here rather than left alone: each element releases only what it took. examples/blog kept its own copy of the lock, so example-blog.webjs.dev still had the original #1144 shift while webjs.dev was fixed. The website's copy tracked automatically because it is a gitignored mirror; the blog's is hand-maintained and was missed. Its header opts in directly, since unlike the marketing site's wrapper it paints its own chrome. The global key follows the framework's existing prefix and carries a typed cast, matching sonner.ts in the same directory instead of inventing a second convention. Docs now claim only what was measured. Firefox cannot be forced off overlay scrollbars in headless Playwright here, so it was never measured with a scrollbar that takes layout width, and asserting it honours the gutter was not something this PR established. Nothing branches on the engine anyway. Also drops an illustrative Tailwind class from a CSS comment: the scanner reads inline styles, so it was emitting a real rule with an invalid value onto every page.
The opt-in was `pr-[calc(1rem+var(--wj-scrollbar-compensation,0px))]` plus an `sm:` twin, which restates the element's own base padding once per responsive variant. A transparent right border does the same job in one declaration: it composes with whatever padding is already there, and a background still paints across a border, so a header carrying its own chrome stays full bleed instead of ending short. Verified equivalent on both shapes that ship here, the fixed element that paints its own chrome (the blog) and the wrapper whose child paints (the marketing site): 0.00px centre shift, chrome right edge unchanged, and the border resolves transparent against the border-* utilities that also set border-right-color. Also drops a Tailwind arbitrary-value class from the markup, which is worth something on its own: the previous form only worked because Tailwind happens to order pr after px.
Throwaway measurement helpers from working #1144. They were never meant to land; a git add -A caught them before I deleted them.
46dd818 to
611239f
Compare
… moves Two real defects from review, one in the lock and one in my own fix. _setup() was not idempotent. The refcount is an integer but the per-instance flag is a boolean, so a second _setup() with no _teardown() between double-counted and the lock could never be released: the page stayed locked and padded for the rest of the session. Two reachable paths. A hide() then show() in one task coalesces into ONE update whose changedProperties still reports open, so updated() runs _setup() again on an already-open dialog. And an element removed between updated()'s queueMicrotask and its execution runs _teardown() first as a no-op, then locks from the microtask on a detached element that will never disconnect again. Locking is now guarded on the flag, and a detached element does not lock. The website opt-in was on the centring bar, which is capped by max-width, so insetting it held the centred nav still and left the LEFT-ALIGNED logo shifting the full scrollbar width. My earlier check missed it twice over: it forced the custom property while the scrollbar was still present, so the viewport never widened and only half the effect showed, and it measured only a centred element. Re-measured properly at a 1400px viewport with the gutter suppressed: logo +7.5 on the bar, 0.0 on the header. The target has to be both viewport-width (or a leading child still moves) and painting (or the background stops short), and `.site-top > header` is the only element there that is both. The browser test was asserting the abandoned padding form, so the shape every doc surface prescribes was untested, and it measured only a centred child, which is exactly how the placement bug got through. It now uses the border form, pins a left-aligned child, and checks the painted chrome still spans the viewport.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth pass, and it caught me shipping a fix that did not fix the reported symptom. Worth writing down properly.
The lock defect first. _setup() was not idempotent, and the count is an integer while the per-instance flag is a boolean, so a second setup with no teardown between double-counts and the lock can never be released. hide() then show() in one task coalesces into a single update whose changedProperties still reports open, so updated() calls _setup() on an already-open dialog. That leaves the page locked and padded for the rest of the session, which is the worst outcome in this whole PR. Also reachable by removing the element between updated()'s queueMicrotask and its execution: _teardown() runs first as a no-op, then the microtask locks a detached element that will never disconnect again. Fixed by guarding the lock on the flag and refusing to lock when detached.
The one I am less happy about is my own, on website/app/layout.ts. The opt-in was on the centring bar, and the bar is capped by max-width, so insetting it holds the CENTRED nav still and does nothing for the left-aligned logo. My verification missed it twice: I forced the custom property while the scrollbar was still present, so the viewport never widened and I was looking at half the effect, and I only measured a centred element. Both mistakes point the same way, which is that I was measuring the thing I expected to move rather than the thing a user would notice.
Re-measured at a 1400px viewport with the gutter suppressed to emulate WebKit: logo +7.5px with the bar, 0.0px with the header, chrome spanning 0..1400 either way. The rule that generalises is that the target must be BOTH viewport-width (or a leading child still moves) and painting (or the background stops short of the widened edge). That is now in the guidance rather than left for the next person to rediscover, and the browser test pins a left-aligned child so this class of miss cannot repeat.
The test itself was also asserting the abandoned padding-right form, so the border-right shape every doc surface prescribes was untested.
One finding was already fixed before this round ran: the two scratch files came out in 611239f5, which the reviewer's checkout predated.
The blog copy's lock banner still prescribed the padding form that was removed for cause, which made it the one line out of step across the three copies and the only place a reader was pointed at the rejected approach. It also lacked the scroll-lock paragraph the registry copies carry in their module JSDoc. The executable block was already byte-identical and stays so. Two tests were claiming more than they proved. The SSR placement check asserted that no rule targets `.site-bar`, a class that no longer exists anywhere, so it could never fail; it now asserts exactly one rule consumes the property, which also catches the unclassed centring div that no selector-based check can name. And the contentless-dialog test was named and explained after a path the `_scrollLocked` guard made unreachable, presenting a defensive branch as the thing under test; it now says what it actually pins, and says plainly that deleting the count-zero early return would leave it green.
|
State of play and how to resume this cold, with the measurement traps that cost me the most time Writing this so the PR carries everything needed to pick the work up with no memory of the session that produced it. Paused mid review loop, so the "what is left" section at the bottom is the actual starting point. Where it stands11 commits, CI was green on all 10 checks at How to reproduce the bug at all, which is most of the difficultyThe bug is invisible unless the scrollbar takes LAYOUT WIDTH, and every headless browser hides scrollbars by default. Anything measured without dealing with that proves nothing.
Two measurement traps, both of which caught meTrap 1: forcing the custom property without widening the viewport. The WebKit path is TWO things happening at once, the scrollbar disappearing (viewport widens) and the property being published. I verified the website opt-in by setting the property on a page whose scrollbar was still present, saw the number I expected, and shipped a fix that did not work. To emulate the fallback honestly you must suppress the gutter AND remove the scrollbar, or you are looking at half the effect. Trap 2: measuring only a centred element. A centred child and a left-aligned child move for different reasons. My opt-in sat on a Three measurement probes and why two of them lie
The design, and what was rejectedThe lock reserves the scrollbar gutter for its duration, so the viewport width never changes and nothing moves at all. Skipped when the page already declared its own
Rejected, with reasons:
The placement rule for the opt-in, which took two attempts
Three copies of the lock, all tracked
The first two are deliberately not shared so The refcount lives on What the review rounds found, 24 issues over five roundsRound 1 (4): clobbering a page's own Round 2 (5): the per-module refcount leaking permanently on out-of-order release; teardown not awaiting the deferred unlock so a red run poisoned later tests; assertions that passed without the lock engaging; a zero-width scrollbar silently skipping instead of failing; the website opt-in on a non-painting wrapper. Round 3 (8): an unmatched unlock consuming another dialog's count; Round 4 (3): Round 5 (4): the blog copy's banner still prescribing the padding form; a vacuous SSR assertion naming a class that no longer exists; a test named after a path a later guard made unreachable; the PR body omitting the third copy. Every fix has a test that fails when the source is reverted. The counterfactual procedure: commit first, then Verification as of the pauseNode 3392, browser 596 per engine, website 131 server plus 35 browser, all four dogfood apps booting with no broken modulepreloads, live dialog on webjs.dev/ui/alert-dialog holding both logo and nav at 0.00px where it was 7.50px. Not re-run since the last two commits: the full node suite and the dogfood boot check. Both commits are comments and tests, so no movement is expected, but it is unverified rather than verified. What is left
Related: #1147 is the same class of bug in the website's docs drawer, filed rather than fixed here because it is a CSS-only lock in a different app and cannot measure a residual. One note on the commit links in the older commentsThe branch was rebased onto main partway through (main had picked up #1141 and #1149), so several commit links in the review replies above point at PRE-rebase SHAs that no longer appear in |
The marketing site got this test on the argument that the opt-in is a single declaration nothing else would notice going missing. That argument applies to this app identically, and more so: it owns its copy of the kit's dialog, so the header and the lock stay in step by hand. That is exactly how this app kept the #1144 shift for two review rounds after the kit was fixed, because the site's copy is a gitignored mirror that tracked automatically and this one is not. Pins the same three properties as the website's: the compensation reaches the served HTML, exactly one rule consumes it and it targets the element that is both viewport-width and painting, and the 0px fallback is present so the declaration is not silently dropped. All three fail when the declaration is removed.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fifth pass. The lock itself came back clean this time, which is the first round that has been true: no surviving refcount desync path, the executable block byte-identical across all three copies, and both opt-ins landing on an element that is genuinely viewport-width and painting. Everything found was in the layer around it.
Two of the four were tests claiming more than they proved, which is the failure mode I care most about here, because a test that cannot fail is worse than no test. The SSR placement check asserted that no rule targets .site-bar, but I had removed that class in an earlier round, so the assertion was unconditionally true and could never fire. It now asserts that exactly ONE rule consumes the property, which also catches the unclassed centring div that no selector-based negative check can name. And the contentless-dialog test was named and explained after a code path a later guard made unreachable, presenting a defensive branch as the thing under test; it now says what it actually pins and says plainly that deleting the count-zero early return would leave it green.
The other two were the blog copy. Its lock banner still prescribed the padding-right form removed for cause, making it the single line out of lockstep across the three copies, and it lacked the scroll-lock paragraph the registry copies carry.
I also closed the gap that made that copy easy to miss twice. website/components/ui/ is a gitignored mirror that regenerates, so it tracked every fix for free; the blog's is real tracked source that only changes when someone remembers it. It now has the same SSR test the marketing site got, and all three of its assertions fail when the declaration is removed. The PR body says three copies rather than two.
The comment justified the count-zero early return as reachable, citing a _teardown() that unlocks unconditionally. It has not done that since 98c4f03, when each element started releasing only what it locked. 62d1bed corrected the same claim in the test and left the three source copies asserting the opposite, so the PR shipped two comments disagreeing about one branch with the source half wrong. It now reads as belt-and-braces, matching the test, and says why it is kept anyway. Also drops a duplicated example from the blog copy. That copy already had its own Usage section, so lifting the registry's @example across left it documenting itself twice, plus a bare unprefixed line where the splice landed. Its own section stays; only the duplicate goes. alert-dialog's banner gains the paragraph break dialog.ts gets from its separator rule, so the two prose blocks no longer run together.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Sixth pass. No functional findings, and it independently verified the things I most wanted a second opinion on: the new blog test's regexes against the actual served markup, that webjs test --server discovers test/layout/, that CI migrates and seeds the blog DB before that step so the 200 precondition holds, and that all four doc surfaces agree with the lock and with each other. Three problems, all in the comment and body layer.
The first is the most instructive, and it is the exact failure mode of iterating a design in public. The unlockScroll() comment justified the count-zero early return as REACHABLE, citing a _teardown() that unlocks unconditionally. That stopped being true in 98c4f037, when each element started releasing only what it locked. I corrected exactly that claim in the test comment in 62d1bede and left the three source copies asserting the opposite, so the PR was shipping two comments about one branch that disagreed, with the source half wrong. Fixed to read as belt-and-braces, with the reason it is kept anyway.
The second is my own sloppiness in a fix. When I lifted the scroll-lock paragraph into the blog copy I took the registry's @example with it, and that copy already had its own Usage: section, so it ended up documenting itself twice, with a bare unprefixed line where the splice landed.
The third was two stale claims in the PR body, both now corrected: the byte-identity claim was true but described the banner difference imprecisely, and the test plan had no line for the blog test added in bfd75e30.
Worth recording plainly: this round's findings, and most of round 5's, exist because I changed the design three times mid-review (body padding to root padding, padding opt-in to border opt-in, centring bar to header). Each change invalidated comments and tests written against the previous shape. The functional work was settled by round 4; everything since has been cleaning up after my own churn.
The probe was `position: absolute; left: 0` inside a header whose left edge is viewport 0, so its edge could never move and it could not catch the placement bug its own comment cited. Both the comment and the PR body were claiming coverage that did not exist, which is worse than having no probe. It now mirrors the shape the bug turned on: a viewport-width painting header wrapping a max-width centring bar, with the leading child in flow inside the bar, so the bar re-centres when the viewport widens. Measured across all three placements at a 1000px viewport with a 15px scrollbar: no opt-in moves the leading child 7.5px, the opt-in on the bar STILL moves it 7.5px (the wrong placement that reached review), and the opt-in on the header holds it at 0.0. Also moves the blog layout's new rule below the glow-layer block, since it had landed between an existing comment and the rule that comment documents, and fixes a comma splice in the blog test's docblock.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Seventh pass. No functional findings again, but one of the three is the sharpest thing any round has caught, so it is worth stating plainly.
The left-aligned probe I added in round 4 could not catch the bug I added it for. I made it position: absolute; left: 0 inside a header whose left edge is viewport 0, so its edge could never move no matter what the lock did. Meanwhile both the test comment and the PR body claimed it guarded the max-width-bar placement failure. That is a test asserting coverage it does not have, which is worse than no test, and it is the second time in this PR I have written an assertion that could not fail.
It now mirrors the shape the bug actually turned on, a viewport-width painting header wrapping a max-width centring bar with the leading child in flow inside it, so the bar re-centres when the viewport widens. Measured across all three placements at a 1000px viewport with a 15px scrollbar:
| opt-in placement | leading child | bar centre |
|---|---|---|
| none | +7.5px | +7.5px |
| on the bar (the wrong placement) | +7.5px | +7.5px |
| on the header (what ships) | 0.0px | 0.0px |
So the probe now genuinely distinguishes the correct placement from the one that reached review. I checked this with a standalone measurement rather than the counterfactual, because the centred assertion throws first and would have masked whether the leading one ever fires.
The other two were cosmetic: the blog layout's new rule had landed between an existing comment and the rule that comment documents, so the glow-layer note read as a preamble to the wrong rule, and the blog test's docblock had a comma splice.
Round 7's restructure was still wrong, for a reason my own verification hid: I measured it on a page with a box-sizing reset, and the WTR page has none. Under content-box `max-width` caps the content box, so the right border grew the border box instead of insetting the content and the leading probe stayed inert. Pinning border-box was necessary but not sufficient. With space-between children both probes moved together, so the leading one still caught nothing the centred one did not. The real site's nav is a flex-1 centred region, and that is the whole point: insetting the bar shrinks flex-1 and pulls its content back to where it started, so a centred probe reports nothing while the leading child keeps moving. The fixture now has that shape, and it discriminates. Moving the opt-in from the header to the bar makes exactly ONE assertion fail, the left-aligned one, with the centred one passing blind. Measured at a 1000px viewport with a 15px scrollbar: no opt-in moves both 7.5, the bar placement moves leading 7.5 and holds centred at 0.0, the header placement holds both. Also removes .fix8.py, a scratch script that got committed because the script failed so the `&& rm` after it never ran. Second time; the lesson is to delete before staging, not after.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Eighth pass, and it caught my verification method rather than my code, which is the most useful thing a review has done here.
I claimed in round 7 that the restructured fixture made the left-aligned probe catch the bar misplacement, and I backed it with a measurement. The measurement was of a page with its own box-sizing: border-box reset. The WTR page has no reset, so the fixture actually ran under content-box, where max-width caps the CONTENT box: a right border grows the border box instead of insetting the content, and the leading probe stayed inert. I verified the right behaviour on the wrong page.
Pinning border-box was necessary but not sufficient, and this is the part I would have missed without measuring again. With space-between children both probes moved together, so the leading one still caught nothing the centred one did not. The real site's nav is a flex: 1 centred region, and that is the whole mechanism: insetting the bar shrinks flex-1 and pulls its content back to where it started, so a centred probe reports zero movement while the leading child keeps riding the bar's shift.
The fixture now has that shape and it discriminates. Moving the opt-in from the header to the bar makes exactly ONE assertion fail, the left-aligned one, with the centred one passing blind:
| opt-in placement | leading | centred |
|---|---|---|
| none | +7.5px | +7.5px |
| on the bar | +7.5px | 0.0px |
| on the header (what ships) | 0.0px | 0.0px |
Also removed .fix8.py, a scratch script that got committed for the second time in this PR. Both times the cause was the same: I chained && rm after the script, the script failed on an assertion, so the delete never ran and a later git add -A swept it in. Deleting before staging rather than after is the fix.
Three lessons from this round worth keeping: a counterfactual only proves the FIRST failing assertion in a test is load-bearing, a standalone measurement must run in the same environment as the test it is vouching for, and a probe is only load-bearing if some wrong implementation makes it and only it fail.
|
Review: round 9 clean on the code, one false claim of mine in the body Ninth pass, and the useful part is that it did not take my word for anything. It drove a browser itself, rebuilt the fixture byte-for-byte, forced the fallback path, and reproduced my table exactly: header opt-in holds both probes at 0.0, the bar placement moves the leading child 7.5 and holds the centred one at 0.0, no opt-in moves both. It also confirmed the in-flow probe holds via the root padding and that the header's right edge tracks the root's. The one finding was a claim of mine, not the code: the test plan said 596 passing on all three engines, and Firefox is 587 plus 10 skipped. The 9-test delta is exactly the new scroll-lock tests it cannot run. That also contradicted the body's own later paragraph saying Firefox skips them, so the body was internally inconsistent. I corrected it, then corrected the correction. My local Firefox reports 584 passing and CI reports 587, both with the same 10 skips, because other tests there are environment-gated. So an exact pass total for Firefox is brittle regardless of which number I pick. The body now states zero failures on all three, the exact totals only for the two engines where they are stable, and the skip count with its reason, since that is the number that actually means something. Small thing, but it is the third body claim across nine rounds that was more confident than the evidence supported, and the pattern is the same each time: I wrote the number I expected rather than the number I had just seen. |
|
Review: round 10 clean, loop converged Tenth pass found nothing. It re-derived every number in the test plan against CI run 30340586109 rather than taking the body's word for it, including checking main's own last run to confirm the 9-test delta and that the single remaining skip is genuinely pre-existing. It confirmed the three lock copies are byte-identical over the executable region, that the docs-surfaces list matches the actual file list exactly, that the cited Next.js path exists, and that the head tree carries no stray scratch files. One thing it raised that I want to leave standing rather than paper over. Four claims in the body are attested by me with no CI artifact behind them: the reverted-source counterfactual, the live 7.50px to 0.00px measurement on webjs.dev/ui, the four-app dogfood boot, and the note about Firefox pass totals differing between CI and local. Those are all things I ran and reported, and none of them contradicts anything the reviewer could independently measure, but they are not reproducible from the PR alone. The counterfactual in particular is the load-bearing one, since it is the evidence that the tests actually fail without the fix, and the honest position is that a reader has to either trust it or re-run it. The procedure is written down in the handoff comment above so it can be re-run. It also checked the commit trail for messages later commits made false. Several early ones do make claims that were subsequently reversed, on Firefox honouring the gutter, on padding the body, and on the centring-bar placement. In each case the correcting commit names the earlier one and says what it got wrong, so Ten rounds, 34 issues. Eight were functional, and the two that mattered most were session-long lock leaks a user would have experienced as a permanently broken page. The rest were mine to answer for: tests that could not fail, comments describing code that had moved, and body claims more confident than the evidence. The functional work settled at round 4; rounds 5 through 10 were cleaning up after three mid-review design changes I chose to make in public. |
Raised in review and genuinely untested: the case where the page's scrollbar is being removed at the same moment the dialog's own appears. The residual is measured off the root element's border box, which a scrollbar inside the top layer does not touch, so the two should be independent, but that was an assumption rather than a checked fact. Asserts the header holds on both paths (the reserved gutter on Chromium and the published compensation on WebKit), that the tall dialog actually overflows so the check is not vacuous, and that it stays scrollable while the page underneath is locked. A lock that also froze the dialog would be a worse bug than the shift this PR fixes.
1 participant
Closes #1144
Opening a
<ui-dialog>or<ui-alert-dialog>shifted aposition: fixedheader right by half the scrollbar width. Visible on webjs.dev/ui itself: the Delete-account demo moved the site navbar. Measured on the live page, 7.50px at a 1280px viewport with a 15px scrollbar, now 0.00px.Why it happened
The scroll lock sets
overflow: hiddenon the body, which removes the page scrollbar. A classic scrollbar takes real layout width, so removing it widens the viewport. The lock compensated by padding the body, which holds in-flow content still, but aposition: fixedbox lays out against the initial containing block and never sees a padding box. So it widened with the viewport and its centred content slid right.This hits the pinned-header pattern WebJs explicitly recommends (
position: fixed, neversticky, per #610), so the kit's own guidance and the kit's own dialog disagreed.Worth noting where this sits in the ecosystem, because it is not a WebJs-specific wart. Next.js's own dev overlay carries the identical algorithm (
packages/next/src/next-devtools/dev-overlay/components/overlay/body-locker.ts: same probe, same refcount, same body padding), with the same two defects; it just does not matter behind a full-screen overlay. Radix, the library Next apps actually use for dialogs, needs more machinery than we do: it always pads, always publishes--removed-body-scroll-bar-size, and ships two magic classes (zeroRightClassName,fullWidthClassName) with!importantfor exactly this fixed-element case.The fix
Reserve the scrollbar gutter for the duration of the lock. The viewport width then never changes, so nothing moves, in flow or fixed, with no cooperation from the page. It is skipped when the page already declared its own
scrollbar-gutter, since overwritingstable both-edgeswith the single-edge value would drop a gutter and cause the shift it exists to prevent.The issue proposed putting
scrollbar-gutter: stablepermanently in the theme CSS. Measuring first ruled that out: it only reaches apps on the kit's theme, and it changes every page permanently to fix a transient moment.Engines differ and the lock does not branch on them. The residual is measured from
<html>'s own border box, which tracks the viewport and is laid out synchronously. Where the gutter holds (measured on Chromium) the residual is zero and the lock does nothing else. Where it is ignored (measured on WebKit) the lock pads<html>by the residual, added to any padding already there, and publishes it as--wj-scrollbar-compensationfor a fixed element to opt into with one declaration:A transparent border rather than padding, because it composes with existing padding instead of restating it, and a background still paints across a border, so a header carrying its own chrome stays full bleed.
Placement is the part that is easy to get wrong, and it took two attempts here. The target must be BOTH viewport-width and painting. Viewport-width, or a left-aligned child still moves: insetting a
max-widthcontainer holds its centred children still while its leading ones keep shifting, since the container's own box is not what widened. Painting, or the background stops short of the widened edge. Measured at a 1400px viewport with the gutter suppressed: logo +7.5px on the centring bar, 0.0px on the header.Several structural fixes came out of review. One refcount, shared across both component copies via
globalThis, becausedisconnectedCallbackfires in tree order and the #766 before-cache close runs in registration order, so a confirm inside a dialog releases OUTER first and two independent counters left the page padded permanently. Each element releases only the lock it took, because_setup()returns before locking when there is no content child while_teardown()unlocked unconditionally, which consumed another open dialog's count. And locking is idempotent per element, because the count is an integer while the flag is a boolean: a coalescedhide()thenshow()produces one update that still reports open, so_setup()could run twice and leave the page locked permanently.Three tracked copies of the lock, not two:
packages/ui/packages/registry/components/{dialog,alert-dialog}.tsandexamples/blog/components/ui/dialog.ts. The first two are deliberately unshared sowebjs ui add alert-dialogstays self-contained; the blog's is the shadcn "you own it" model. The executable block is byte-identical across all three, md5-verified over the region frominterface ScrollLockStatethrough the end ofunlockScroll. The surrounding banner prose differs only where it must: alert-dialog's second line names its own reason for being a separate copy.website/components/ui/is a gitignored mirror and tracked automatically, which is why the blog's copy is the one that kept the bug for two review rounds after the kit was fixed.Test plan
webjs checkclean on the websiteNine new browser tests. They force a classic scrollbar with
::-webkit-scrollbarand refuse to pass vacuously: an overlay-only engine skips, Chromium fails outright (its scrollbar is guaranteed by the runner config, so a zero width means that config broke), and a lock leaked by an earlier test fails everywhere. They pin a left-aligned child as well as a centred one, and the fixture is shaped so that matters: it mirrors the site's viewport-width painting header wrapping a max-width bar with aflex: 1centred region. Insetting the bar shrinks that region and pulls its content back to where it started, so a centred probe reports nothing while the leading child keeps moving. Moving the opt-in from the header to the bar makes exactly one assertion fail, the left-aligned one. That is the placement that reached review, and measuring only a centred element is what let it look correct. Playwright passes--hide-scrollbarsby default, which is why nothing caught this before; the root WTR config now drops it for Chromium.Firefox skips these assertions and says so. It cannot be forced off overlay scrollbars in headless Playwright here, so it was never measured with a scrollbar that takes layout width, and this PR does not claim it was.
Docs surfaces
.agents/skills/webjs/references/styling.md, the fixed-header section, with the scroll-lock contract, the opt-in, and what to do when rolling your own lockwebsite/app/docs/styling/page.ts, the same for the user-facing docspackages/ui/AGENTS.md, the shadcn divergence recorded under invariant 5, plus the dialog inventory rowwebjs ui viewand the MCPuitool serve, and in the blog's own copywebsite/app/layout.tsandexamples/blog/app/layout.ts, the two in-repo consumerswebsite/test/ssr/fixed-header-scroll-lock.test.tsandexamples/blog/test/layout/fixed-header-scroll-lock.test.ts, since the opt-in is one declaration nothing else would notice going missing. Each pins that exactly one rule consumes the property and that it targets an element which is both viewport-width and paintingFollow-up filed
#1147, the website's docs drawer has the same shift from a separate CSS-only scroll lock. Confirmed by measurement (right-hand header controls jump the full 15px at viewports under 860px) rather than assumed. Left out of this PR because it is a different mechanism in a different app, and a CSS-only lock cannot measure a residual, so it needs its own approach.