Skip to content

Commit 62d1bed

Browse files
committed
fix(ui): bring the blog's lock copy fully in lockstep, unrig two tests
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.
1 parent 0feb745 commit 62d1bed

3 files changed

Lines changed: 69 additions & 14 deletions

File tree

examples/blog/components/ui/dialog.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,46 @@
6262
* within the dialog (native focus trap).
6363
*
6464
* Design tokens used: --background, --border, --muted-foreground.
65+
*
66+
* Scroll lock (#1144): opening the dialog locks body scroll, which hides the
67+
* page scrollbar and would widen the viewport. The lock reserves the scrollbar
68+
* gutter so the width never changes and a `position: fixed` header stays put.
69+
* Where the engine ignores `scrollbar-gutter` (WebKit today) it publishes the
70+
* leftover width on <html> as `--wj-scrollbar-compensation`, so a fixed header
71+
* opts in with `border-right: var(--wj-scrollbar-compensation, 0px) solid transparent`
72+
* (a transparent border composes with existing padding, and a background paints
73+
* across it, so a header keeps its own chrome full bleed).
74+
*
75+
* @example
76+
* ```html
77+
* <ui-dialog>
78+
* <ui-dialog-trigger>
79+
* <button class=${buttonClass({ variant: 'outline' })}>Edit profile</button>
80+
* </ui-dialog-trigger>
81+
* <ui-dialog-content>
82+
* <div class=${dialogHeaderClass()}>
83+
* <h2 data-slot="dialog-title" class=${dialogTitleClass()}>Edit profile</h2>
84+
* <p data-slot="dialog-description" class=${dialogDescriptionClass()}>Make changes and click save.</p>
85+
* </div>
86+
* <div class="grid gap-3">
87+
* <label class=${labelClass()} for="dlg-name">Name</label>
88+
* <input class=${inputClass()} id="dlg-name" placeholder="Your name">
89+
* </div>
90+
* <div class=${dialogFooterClass()}>
91+
* <ui-dialog-close><button class=${buttonClass({ variant: 'outline' })}>Cancel</button></ui-dialog-close>
92+
* <button class=${buttonClass()}>Save</button>
93+
* </div>
94+
* </ui-dialog-content>
95+
* </ui-dialog>
96+
*
97+
* <!-- Suppress the auto-injected top-right X close button. -->
98+
* <ui-dialog-content show-close-button="false">
99+
* <div class=${dialogHeaderClass()}>
100+
* <h2 data-slot="dialog-title" class=${dialogTitleClass()}>Quiet dialog</h2>
101+
* </div>
102+
* </ui-dialog-content>
103+
* ```
104+
65105
*/
66106
import { WebComponent, html, unsafeHTML, prop } from '@webjsdev/core';
67107
import { ref, createRef } from '@webjsdev/core/directives';
@@ -137,7 +177,7 @@ function installStyles(): void {
137177
// 2. Where the engine ignores it (measured on WebKit), fall back to
138178
// padding <html> and publish the leftover width as
139179
// `--wj-scrollbar-compensation` on it, so a fixed element can opt in with
140-
// `padding-right: var(--wj-scrollbar-compensation, 0px)`.
180+
// `border-right: var(--wj-scrollbar-compensation, 0px) solid transparent`.
141181
//
142182
// Everything below is MEASURED rather than assumed, because engines disagree
143183
// about both scrollbar geometry and gutter support. When mechanism 1 works the

packages/ui/test/components/browser/ui-overlay.test.js

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -463,12 +463,17 @@ suite('ui-dialog scroll lock layout', () => {
463463
}
464464
});
465465

466-
// A dialog with no content child returns from _setup() BEFORE locking, but
467-
// _teardown() unlocks unconditionally, so an unlock can arrive with no lock
468-
// behind it. Treating that as the last release would replay a stale snapshot
469-
// onto whatever the page owns now, and while another dialog is open it would
470-
// drop that dialog's compensation mid-flight.
471-
test('an unlock with no matching lock does not disturb an open dialog', async function () {
466+
// A dialog with no content child returns from _setup() BEFORE locking, so it
467+
// must not release anything on the way out. It used to: _teardown() unlocked
468+
// unconditionally, which consumed the OPEN dialog's refcount and restored the
469+
// page mid-flight, dropping its compensation.
470+
//
471+
// What this pins is the `_scrollLocked` guard in _teardown(). The count-zero
472+
// early return in unlockScroll() is now belt-and-braces rather than the thing
473+
// under test, because no path through the components can reach unlockScroll
474+
// with a zero count any more. Deleting that early return would leave this
475+
// green, and that is expected.
476+
test('a contentless dialog does not release an open dialog\'s lock', async function () {
472477
const page = await buildFixedHeaderPage();
473478
try {
474479
if (!requireClassicScrollbar(this)) return;

website/test/ssr/fixed-header-scroll-lock.test.ts

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,13 +68,23 @@ test('the compensation targets the header, not the wrapper or the centring bar',
6868
new RegExp(`\\.site-top\\s*>\\s*header\\s*\\{[^}]*var\\(${COMPENSATION}`).test(body),
6969
'a `.site-top > header` rule consumes the compensation',
7070
);
71-
for (const wrong of ['\\.site-top', '\\.site-bar']) {
72-
assert.equal(
73-
new RegExp(`${wrong}\\s*\\{[^}]*var\\(${COMPENSATION}`).test(body),
74-
false,
75-
`${wrong.replace('\\', '')} must not be the element inset`,
76-
);
77-
}
71+
// `.site-top` on its own is the wrong target and IS a reachable mistake, since
72+
// it is the fixed element and the obvious thing to reach for.
73+
assert.equal(
74+
new RegExp(`\\.site-top\\s*\\{[^}]*var\\(${COMPENSATION}`).test(body),
75+
false,
76+
'the non-painting fixed wrapper must not be the element inset',
77+
);
78+
// Exactly ONE rule may consume it. The other wrong target was the centring
79+
// div, which carries no class, so it cannot be named in a negative selector
80+
// check; counting the consuming rules catches it anyway, and catches any future
81+
// second consumer that would double-compensate.
82+
const consumers = body.match(new RegExp(`\\{[^{}]*var\\(${COMPENSATION}`, 'g')) ?? [];
83+
assert.equal(
84+
consumers.length,
85+
1,
86+
`exactly one rule may consume the compensation, found ${consumers.length}`,
87+
);
7888
});
7989

8090
test('the compensation resolves to zero when no lock is active', () => {

0 commit comments

Comments
 (0)