Skip to content

Commit 98c4f03

Browse files
vivek7405vivek7405
committed
fix(ui): release only the scroll lock a dialog actually took
_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.
1 parent 7dd22bd commit 98c4f03

11 files changed

Lines changed: 347 additions & 51 deletions

File tree

‎.agents/skills/webjs/references/styling.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ Anything that locks page scroll (a modal, a drawer, an off-canvas menu) hides th
213213

214214
`@webjsdev/ui`'s `<ui-dialog>` / `<ui-alert-dialog>` handle this for you (#1144). Their scroll lock reserves the scrollbar gutter for its duration, so the viewport width never changes and nothing moves. It leaves the gutter alone if your page already declared its own `scrollbar-gutter`, on the assumption that a page which made that choice meant it.
215215

216-
Two engines, two outcomes. Chromium and Firefox honour the reserved gutter, so nothing moves and the lock does nothing else. WebKit ignores `scrollbar-gutter`, so the viewport does widen there; the lock measures how much, pads `<html>` by it to hold in-flow content still (added to any padding you already had, and restored on close), and publishes the amount as `--wj-scrollbar-compensation` so a fixed element can opt in with one line:
216+
Engines differ, and the lock does not need to know which one it is on. Where the gutter is honoured (measured on Chromium) nothing moves and the lock does nothing else. Where it is ignored (measured on WebKit) the viewport does widen, so the lock measures how much, pads `<html>` by it to hold in-flow content still (added to any padding you already had, and restored on close), and publishes the amount as `--wj-scrollbar-compensation` so a fixed element can opt in with one line:
217217

218218
```css
219219
header { position: fixed; inset-inline: 0; top: 0; padding-right: var(--wj-scrollbar-compensation, 0px); }

‎.ff-probe.mjs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { firefox } from 'playwright';
2+
const PAGE = `<!doctype html><html><head><style>
3+
* { margin:0; padding:0; box-sizing:border-box }
4+
body { min-height: 10px }
5+
#hdr { position: fixed; inset: 0 0 auto 0; height: 40px; background:#333 }
6+
</style></head><body><div id="hdr"></div></body></html>`;
7+
const CFGS = [
8+
['default', {}],
9+
['gtk overlay off', { 'widget.gtk.overlay-scrollbars.enabled': false }],
10+
['non-native theme', { 'widget.non-native-theme.enabled': true, 'widget.gtk.overlay-scrollbars.enabled': false }],
11+
['ui.useOverlayScrollbars 0', { 'ui.useOverlayScrollbars': 0 }],
12+
];
13+
for (const [name, prefs] of CFGS) {
14+
const b = await firefox.launch({ firefoxUserPrefs: prefs, ignoreDefaultArgs: ['--hide-scrollbars'] });
15+
const p = await b.newPage({ viewport: { width: 1000, height: 600 } });
16+
await p.setContent(PAGE);
17+
const before = await p.evaluate(() => ({ sb: innerWidth - document.documentElement.clientWidth, hdr: document.getElementById('hdr').getBoundingClientRect().width }));
18+
await p.evaluate(() => { document.documentElement.style.scrollbarGutter = 'stable'; });
19+
const after = await p.evaluate(() => ({ hdr: document.getElementById('hdr').getBoundingClientRect().width }));
20+
console.log(`${name.padEnd(26)} scrollbarProbe=${before.sb} hdr ${before.hdr} -> ${after.hdr} gutterHonoured=${after.hdr !== before.hdr}`);
21+
await b.close();
22+
}

‎.sync-blog2.py‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
"""Re-sync examples/blog's copied dialog with the registry source.
2+
3+
The blog owns its copy (the shadcn model), and it has its own local divergences,
4+
so this ports ONLY the pieces #1144 changed: the lock block, and the per-instance
5+
guard in _setup / _teardown.
6+
"""
7+
import pathlib
8+
9+
src = pathlib.Path('packages/ui/packages/registry/components/dialog.ts').read_text()
10+
dst_path = pathlib.Path('examples/blog/components/ui/dialog.ts')
11+
dst = dst_path.read_text()
12+
13+
14+
def block(text, start, end_marker):
15+
a = text.index(start)
16+
b = text.index(end_marker, a)
17+
return text[a:b]
18+
19+
20+
# 1. The lock block, verbatim.
21+
canon_lock = block(src, '// Page scroll lock, refcounted', 'function unlockScroll')
22+
canon_lock += src[src.index('function unlockScroll'):]
23+
canon_lock = canon_lock[: canon_lock.index('\n}\n') + 2]
24+
25+
old_lock = block(dst, '// Page scroll lock, refcounted', 'function unlockScroll')
26+
old_lock += dst[dst.index('function unlockScroll'):]
27+
old_lock = old_lock[: old_lock.index('\n}\n') + 2]
28+
dst = dst.replace(old_lock, canon_lock)
29+
30+
# 2. The per-instance guard.
31+
old_setup = """ _setup(): void {
32+
const content = this._content;
33+
if (!content) return;
34+
lockScroll();
35+
content.showModal();
36+
}"""
37+
new_setup = """ _setup(): void {
38+
const content = this._content;
39+
if (!content) return;
40+
lockScroll();
41+
this._scrollLocked = true;
42+
content.showModal();
43+
}"""
44+
assert dst.count(old_setup) == 1
45+
dst = dst.replace(old_setup, new_setup)
46+
47+
old_td = """ _teardown(): void {
48+
unlockScroll();
49+
this._content?.close();
50+
}"""
51+
new_td = """ _teardown(): void {
52+
// Release only what THIS element locked. _setup() returns before locking
53+
// when there is no content child, so an unconditional unlock here would
54+
// consume ANOTHER open dialog's count and restore the page out from under
55+
// it, dropping its compensation while it is still open.
56+
if (this._scrollLocked) {
57+
this._scrollLocked = false;
58+
unlockScroll();
59+
}
60+
this._content?.close();
61+
}"""
62+
assert dst.count(old_td) == 1
63+
dst = dst.replace(old_td, new_td)
64+
65+
if '_scrollLocked?: boolean;' not in dst:
66+
marker = ' _disposeBeforeCache?: () => void;'
67+
assert dst.count(marker) >= 1
68+
dst = dst.replace(marker, marker + '\n _scrollLocked?: boolean;', 1)
69+
70+
dst_path.write_text(dst)
71+
print('blog copy re-synced')

‎examples/blog/app/layout.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ export default function RootLayout({ children }: LayoutProps) {
282282
283283
<div class="glow-layer" aria-hidden="true"></div>
284284
285-
<header class="site-header fixed inset-x-0 top-0 z-20 flex items-center justify-between gap-4 px-4 sm:px-6 py-3 border-b border-border bg-[color-mix(in_oklch,var(--background)_75%,transparent)] backdrop-blur-[18px] backdrop-saturate-[180%]">
285+
<header class="site-header fixed inset-x-0 top-0 z-20 flex items-center justify-between gap-4 px-4 sm:px-6 pr-[calc(1rem+var(--wj-scrollbar-compensation,0px))] sm:pr-[calc(1.5rem+var(--wj-scrollbar-compensation,0px))] py-3 border-b border-border bg-[color-mix(in_oklch,var(--background)_75%,transparent)] backdrop-blur-[18px] backdrop-saturate-[180%]">
286286
<a href="/" class="inline-flex items-center gap-2 no-underline text-foreground font-semibold text-[15px] leading-none tracking-tight">
287287
<span class="inline-block w-[22px] h-[22px] rounded-[7px] bg-gradient-to-br from-[var(--logo-from)] to-[var(--logo-to)] shadow-[inset_0_0_0_1px_oklch(1_0_0/0.15),0_2px_10px_var(--primary-tint)]"></span>
288288
<span>webjs</span>

‎examples/blog/components/ui/dialog.ts‎

Lines changed: 125 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -122,30 +122,128 @@ function installStyles(): void {
122122
}
123123

124124
// --------------------------------------------------------------------------
125-
// Body scroll lock. Refcounted so nested dialogs unlock in order. Native
126-
// <dialog> does not lock body scroll, only inert-ifies the background.
125+
// Page scroll lock, refcounted so nested dialogs release safely in ANY order.
126+
// Native <dialog> does not lock scrolling, it only inert-ifies the background.
127+
//
128+
// Hiding the page scrollbar widens the viewport by the scrollbar's width, so
129+
// anything laid out against the viewport moves. Padding the page holds in-flow
130+
// content still, but a `position: fixed` header lays out against the initial
131+
// containing block, never against any padding box, so padding alone leaves it
132+
// sliding right by half the scrollbar width. Two mechanisms fix it:
133+
//
134+
// 1. Reserve the scrollbar gutter for the duration of the lock, so the
135+
// viewport width never changes and NOTHING moves, in flow or fixed. This
136+
// needs no cooperation from the page. Measured honoured on Chromium.
137+
// 2. Where the engine ignores it (measured on WebKit), fall back to
138+
// padding <html> and publish the leftover width as
139+
// `--wj-scrollbar-compensation` on it, so a fixed element can opt in with
140+
// `padding-right: var(--wj-scrollbar-compensation, 0px)`.
141+
//
142+
// Everything below is MEASURED rather than assumed, because engines disagree
143+
// about both scrollbar geometry and gutter support. When mechanism 1 works the
144+
// measured residual is zero, so no padding is applied and the custom property is
145+
// never set: the two mechanisms cannot double-compensate.
127146
// --------------------------------------------------------------------------
128147

129-
let scrollLockCount = 0;
130-
let savedOverflow = '';
131-
let savedPaddingRight = '';
148+
// The lock's state is DOCUMENT level, so it is keyed on globalThis rather than
149+
// module scope. dialog.ts and alert-dialog.ts ship as separate copies (so
150+
// `webjs ui add alert-dialog` stays self contained), and two independent
151+
// counters mutating the same <html> can only be released safely in LIFO order.
152+
// They are not: `disconnectedCallback` fires in tree order and the #766
153+
// before-cache close runs in registration order, so a confirm inside a dialog
154+
// releases OUTER first, and the inner unlock would then re-apply the values it
155+
// captured with nothing left to clear them, leaving <html> padded for good. One
156+
// shared counter makes release order irrelevant.
157+
interface ScrollLockState {
158+
count: number;
159+
overflow: string;
160+
rootPaddingRight: string;
161+
scrollbarGutter: string;
162+
compensation: string;
163+
}
164+
165+
const SCROLLBAR_COMPENSATION = '--wj-scrollbar-compensation';
166+
167+
function scrollLockState(): ScrollLockState {
168+
// The key follows the framework's global-key prefix (see `__webjsSonnerBus` in
169+
// sonner.ts, the same pattern for the same reason), and the cast is typed
170+
// rather than a string-keyed bag, so a change to the shape is a type error in
171+
// both copies instead of a silent undefined at runtime.
172+
const store = globalThis as unknown as { __webjsScrollLock?: ScrollLockState };
173+
let state = store.__webjsScrollLock;
174+
if (!state) {
175+
state = { count: 0, overflow: '', rootPaddingRight: '', scrollbarGutter: '', compensation: '' };
176+
store.__webjsScrollLock = state;
177+
}
178+
return state;
179+
}
132180

133181
function lockScroll(): void {
134-
if (scrollLockCount === 0) {
135-
const scrollbarWidth = window.innerWidth - document.documentElement.clientWidth;
136-
savedOverflow = document.body.style.overflow;
137-
savedPaddingRight = document.body.style.paddingRight;
138-
document.body.style.overflow = 'hidden';
139-
if (scrollbarWidth > 0) document.body.style.paddingRight = `${scrollbarWidth}px`;
182+
const state = scrollLockState();
183+
if (state.count === 0) {
184+
const root = document.documentElement;
185+
const body = document.body;
186+
const rootStyle = getComputedStyle(root);
187+
188+
state.overflow = body.style.overflow;
189+
state.rootPaddingRight = root.style.paddingRight;
190+
state.scrollbarGutter = root.style.scrollbarGutter;
191+
state.compensation = root.style.getPropertyValue(SCROLLBAR_COMPENSATION);
192+
193+
// <html> is an in-flow block filling the initial containing block, so its
194+
// border box IS the viewport width, and it is re-laid-out synchronously.
195+
// A `position: fixed` probe is NOT a substitute: WebKit keeps reporting its
196+
// old box until the next rendering update, so it reads a zero delta and the
197+
// compensation below never fires.
198+
const widthBefore = root.getBoundingClientRect().width;
199+
const padBefore = parseFloat(rootStyle.paddingRight) || 0;
200+
// An engine with no `scrollbar-gutter` at all reads back undefined, which
201+
// must be treated as "the page has not chosen" so the gutter is still
202+
// attempted (setting an unsupported property is a harmless no-op, and the
203+
// measured residual below is what actually decides the fallback).
204+
const chosenGutter = rootStyle.scrollbarGutter || 'auto';
205+
206+
// Reserve the gutter, but only when the page has not already made its own
207+
// choice. An app running `stable both-edges` keeps both gutters through the
208+
// lock, and overwriting that with the single-edge value would drop one and
209+
// introduce the very shift this exists to prevent.
210+
if (chosenGutter === 'auto' && window.innerWidth > root.clientWidth) {
211+
root.style.scrollbarGutter = 'stable';
212+
}
213+
body.style.overflow = 'hidden';
214+
215+
const grew = root.getBoundingClientRect().width - widthBefore;
216+
if (grew > 0) {
217+
// Padding the ROOT holds in-flow content still whatever the body's own
218+
// width is (a `max-width` body does not widen with the viewport, so
219+
// padding the body would miss it), and it leaves the page's own body
220+
// padding untouched.
221+
root.style.paddingRight = `${padBefore + grew}px`;
222+
// A fixed box lays out against the viewport, so no padding here can reach
223+
// it. Publish what it moved by and let it opt in.
224+
root.style.setProperty(SCROLLBAR_COMPENSATION, `${grew}px`);
225+
}
140226
}
141-
scrollLockCount++;
227+
state.count++;
142228
}
143229

144230
function unlockScroll(): void {
145-
scrollLockCount = Math.max(0, scrollLockCount - 1);
146-
if (scrollLockCount === 0) {
147-
document.body.style.overflow = savedOverflow;
148-
document.body.style.paddingRight = savedPaddingRight;
231+
const state = scrollLockState();
232+
// An unlock with no matching lock is a no-op, NOT the last release. Clamping
233+
// to zero and restoring would replay a stale snapshot onto whatever the page
234+
// owns now, and it is reachable: a dialog with no content child returns from
235+
// _setup() before locking, while _teardown() always unlocks.
236+
if (state.count === 0) return;
237+
state.count--;
238+
if (state.count === 0) {
239+
const root = document.documentElement;
240+
document.body.style.overflow = state.overflow;
241+
root.style.paddingRight = state.rootPaddingRight;
242+
root.style.scrollbarGutter = state.scrollbarGutter;
243+
// Restored rather than removed, so a value the PAGE set before any dialog
244+
// opened survives the lock.
245+
if (state.compensation) root.style.setProperty(SCROLLBAR_COMPENSATION, state.compensation);
246+
else root.style.removeProperty(SCROLLBAR_COMPENSATION);
149247
}
150248
}
151249

@@ -212,15 +310,25 @@ export class UiDialog extends WebComponent({
212310
return this.querySelector('ui-dialog-content') as UiDialogContent | null;
213311
}
214312

313+
_scrollLocked?: boolean;
314+
215315
_setup(): void {
216316
const content = this._content;
217317
if (!content) return;
218318
lockScroll();
319+
this._scrollLocked = true;
219320
content.showModal();
220321
}
221322

222323
_teardown(): void {
223-
unlockScroll();
324+
// Release only what THIS element locked. _setup() returns before locking
325+
// when there is no content child, so an unconditional unlock here would
326+
// consume ANOTHER open dialog's count and restore the page out from under
327+
// it, dropping its compensation while it is still open.
328+
if (this._scrollLocked) {
329+
this._scrollLocked = false;
330+
unlockScroll();
331+
}
224332
this._content?.close();
225333
}
226334
}

‎packages/ui/AGENTS.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -310,9 +310,11 @@ when the caller passes an explicit custom `--registry <url>`.
310310
`position: fixed` header sliding right by half the scrollbar width. Since
311311
WebJs recommends exactly that pinned-header pattern (#610), our lock instead
312312
reserves the scrollbar gutter (unless the page already declared its own), and
313-
where the engine ignores `scrollbar-gutter` it pads `<html>` by the measured
314-
residual and publishes it as `--wj-scrollbar-compensation` for a fixed element
315-
to opt into. The refcount is shared across both component copies via
313+
where the engine ignores `scrollbar-gutter` (measured on WebKit, honoured on
314+
Chromium) it pads `<html>` by the measured residual and publishes it as
315+
`--wj-scrollbar-compensation` for a fixed element to opt into. Nothing branches
316+
on the engine: the residual is measured, so an engine nobody tested behaves
317+
correctly either way. The refcount is shared across both component copies via
316318
`globalThis`, because release order is not guaranteed to be LIFO. This is
317319
presentation and lifecycle, not API: every tag, variant, and data attribute
318320
still matches shadcn.

‎packages/ui/packages/registry/components/alert-dialog.ts‎

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,8 @@ function installStyles(): void {
111111
document.head.appendChild(style);
112112
}
113113

114-
// Body scroll lock. Refcounted so nested dialogs unlock in order. Kept in
115-
// lockstep with dialog.ts (not imported) so `webjs ui add alert-dialog`
114+
// Page scroll lock, refcounted so nested dialogs release safely in ANY order.
115+
// Kept in lockstep with dialog.ts (not imported) so `webjs ui add alert-dialog`
116116
// doesn't pull in the full dialog component.
117117
//
118118
// Hiding the page scrollbar widens the viewport by the scrollbar's width, so
@@ -123,8 +123,8 @@ function installStyles(): void {
123123
//
124124
// 1. Reserve the scrollbar gutter for the duration of the lock, so the
125125
// viewport width never changes and NOTHING moves, in flow or fixed. This
126-
// needs no cooperation from the page. Chromium and Firefox honour it.
127-
// 2. Where the engine ignores `scrollbar-gutter` (WebKit today), fall back to
126+
// needs no cooperation from the page. Measured honoured on Chromium.
127+
// 2. Where the engine ignores it (measured on WebKit), fall back to
128128
// padding <html> and publish the leftover width as
129129
// `--wj-scrollbar-compensation` on it, so a fixed element can opt in with
130130
// `padding-right: var(--wj-scrollbar-compensation, 0px)`.
@@ -151,14 +151,17 @@ interface ScrollLockState {
151151
}
152152

153153
const SCROLLBAR_COMPENSATION = '--wj-scrollbar-compensation';
154-
const SCROLL_LOCK_KEY = '__wjScrollLock';
155154

156155
function scrollLockState(): ScrollLockState {
157-
const store = globalThis as unknown as Record<string, ScrollLockState | undefined>;
158-
let state = store[SCROLL_LOCK_KEY];
156+
// The key follows the framework's global-key prefix (see `__webjsSonnerBus` in
157+
// sonner.ts, the same pattern for the same reason), and the cast is typed
158+
// rather than a string-keyed bag, so a change to the shape is a type error in
159+
// both copies instead of a silent undefined at runtime.
160+
const store = globalThis as unknown as { __webjsScrollLock?: ScrollLockState };
161+
let state = store.__webjsScrollLock;
159162
if (!state) {
160163
state = { count: 0, overflow: '', rootPaddingRight: '', scrollbarGutter: '', compensation: '' };
161-
store[SCROLL_LOCK_KEY] = state;
164+
store.__webjsScrollLock = state;
162165
}
163166
return state;
164167
}
@@ -214,7 +217,12 @@ function lockScroll(): void {
214217

215218
function unlockScroll(): void {
216219
const state = scrollLockState();
217-
state.count = Math.max(0, state.count - 1);
220+
// An unlock with no matching lock is a no-op, NOT the last release. Clamping
221+
// to zero and restoring would replay a stale snapshot onto whatever the page
222+
// owns now, and it is reachable: a dialog with no content child returns from
223+
// _setup() before locking, while _teardown() always unlocks.
224+
if (state.count === 0) return;
225+
state.count--;
218226
if (state.count === 0) {
219227
const root = document.documentElement;
220228
document.body.style.overflow = state.overflow;
@@ -244,6 +252,7 @@ export class UiAlertDialog extends WebComponent({
244252
}
245253

246254
_disposeBeforeCache?: () => void;
255+
_scrollLocked?: boolean;
247256

248257
connectedCallback(): void {
249258
installStyles();
@@ -295,11 +304,19 @@ export class UiAlertDialog extends WebComponent({
295304
const content = this._content;
296305
if (!content) return;
297306
lockScroll();
307+
this._scrollLocked = true;
298308
content.showModal();
299309
}
300310

301311
_teardown(): void {
302-
unlockScroll();
312+
// Release only what THIS element locked. _setup() returns before locking
313+
// when there is no content child, so an unconditional unlock here would
314+
// consume ANOTHER open dialog's count and restore the page out from under
315+
// it, dropping its compensation while it is still open.
316+
if (this._scrollLocked) {
317+
this._scrollLocked = false;
318+
unlockScroll();
319+
}
303320
this._content?.close();
304321
}
305322
}

0 commit comments

Comments
 (0)