-
Notifications
You must be signed in to change notification settings - Fork 69
fix(ui): stop the dialog scroll lock shifting a fixed header #1146
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
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
c29a3ad
fix(ui): stop the dialog scroll lock shifting a fixed header
vivek7405 6618e68
test(ui): isolate the scroll-lock tests from each other on failure
vivek7405 e758a73
docs(ui): document the dialog scroll lock's fixed-header contract
vivek7405 052b954
fix(ui): stop the scroll lock clobbering a page's own layout choices
vivek7405 8fa72b5
fix(ui): make the scroll lock safe to release out of order
vivek7405 7dd22bd
fix(website): keep the header chrome full bleed while compensating it
vivek7405 98c4f03
fix(ui): release only the scroll lock a dialog actually took
vivek7405 d1fda36
refactor(ui): opt in with a transparent border, not a padding calc
vivek7405 611239f
chore: drop two scratch probe scripts committed by accident
vivek7405 0feb745
fix(ui): make the lock idempotent and inset the element that actually…
vivek7405 62d1bed
fix(ui): bring the blog's lock copy fully in lockstep, unrig two tests
vivek7405 bfd75e3
test(blog): pin the fixed header's scroll-lock opt-in
vivek7405 977a8b0
docs(ui): stop the unlock comment contradicting the guard above it
vivek7405 2cd07e6
test(ui): make the left-aligned probe actually able to catch the bug
vivek7405 0d307cf
test(ui): give the fixture the flex-1 shape that makes both probes count
vivek7405 f03144d
test(ui): cover a dialog whose own content scrolls
vivek7405 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
86 changes: 86 additions & 0 deletions
86
examples/blog/test/layout/fixed-header-scroll-lock.test.ts
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| /** | ||
| * The blog's fixed header opts into the dialog scroll lock's compensation (#1144). | ||
| * | ||
| * Opening a `<ui-dialog>` locks page scroll, which hides the scrollbar and widens | ||
| * the viewport on any engine that ignores `scrollbar-gutter`. The header here is | ||
| * `position: fixed`, so it lays out against the viewport and cannot be reached by | ||
| * the padding the lock puts on `<html>` to hold in-flow content still. It opts in | ||
| * instead, via `--wj-scrollbar-compensation`. | ||
| * | ||
| * This app owns its copy of the kit's dialog (`components/ui/dialog.ts`, the | ||
| * shadcn model), so the header and the lock have to stay in step by hand. That is | ||
| * exactly how the app kept the #1144 shift for two review rounds after the kit | ||
| * was fixed. The marketing site's copy is a gitignored mirror, so it picked the | ||
| * fix up automatically; this one is real tracked source and did not. | ||
| * | ||
| * The opt-in is a single declaration in the root layout's inline `<style>`, so | ||
| * nothing else on the page would notice it going missing, and the shift would | ||
| * come back silently. This is what makes that fail instead. | ||
| * | ||
| * Run: node --test test/layout/fixed-header-scroll-lock.test.ts | ||
| */ | ||
| import { test, before } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { resolve, dirname } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { createRequestHandler } from '@webjsdev/server'; | ||
|
|
||
| const APP_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); | ||
| const COMPENSATION = '--wj-scrollbar-compensation'; | ||
|
|
||
| let body: string; | ||
|
|
||
| before(async () => { | ||
| const app = await createRequestHandler({ appDir: APP_ROOT, dev: false }); | ||
| await app.warmup?.(); | ||
| const res = await app.handle(new Request('http://localhost/')); | ||
| assert.equal(res.status, 200, 'the home page should render'); | ||
| body = await res.text(); | ||
| }); | ||
|
|
||
| test('the fixed header opts into the scroll-lock compensation', () => { | ||
| assert.ok( | ||
| body.includes(COMPENSATION), | ||
| `the served HTML must reference ${COMPENSATION}, or the fixed header goes back ` + | ||
| 'to sliding right by half the scrollbar width when a dialog opens', | ||
| ); | ||
| }); | ||
|
|
||
| test('the opt-in is on the header itself, which is both viewport-width and painting', () => { | ||
| // Unlike the marketing site, this app's fixed element IS the painting element, | ||
| // so it takes the opt-in directly rather than delegating to a child. Both | ||
| // properties matter. Viewport-width, or a left-aligned child (the brand) still | ||
| // moves while a centred one looks fine. Painting, or the background stops short | ||
| // of the widened edge, since insetting a wrapper insets whatever child paints. | ||
| assert.ok( | ||
| new RegExp(`\\.site-header\\s*\\{[^}]*var\\(${COMPENSATION}`).test(body), | ||
| 'a `.site-header` rule consumes the compensation', | ||
| ); | ||
| // Exactly one consumer, or two rules would double-compensate. | ||
| const consumers = body.match(new RegExp(`\\{[^{}]*var\\(${COMPENSATION}`, 'g')) ?? []; | ||
| assert.equal( | ||
| consumers.length, | ||
| 1, | ||
| `exactly one rule may consume the compensation, found ${consumers.length}`, | ||
| ); | ||
| // The element the rule targets has to actually be the fixed one. | ||
| assert.ok( | ||
| /<header class="site-header fixed /.test(body), | ||
| 'the .site-header element is the fixed header', | ||
| ); | ||
| }); | ||
|
|
||
| test('the compensation resolves to zero when no lock is active', () => { | ||
| // The `0px` fallback is what keeps the header identical to its unpatched self | ||
| // at every other moment. Without it the border width is invalid and the whole | ||
| // declaration is dropped, which fails silently rather than visibly. | ||
| assert.ok( | ||
| body.includes(`var(${COMPENSATION},0px)`) || body.includes(`var(${COMPENSATION}, 0px)`), | ||
| 'the compensation must carry a 0px fallback for the un-locked case', | ||
| ); | ||
| // A transparent border, so the chrome still paints across the inset. | ||
| assert.ok( | ||
| new RegExp(`border-right:\\s*var\\(${COMPENSATION},\\s*0px\\)\\s*solid\\s*transparent`).test(body), | ||
| 'the opt-in is a transparent right border', | ||
| ); | ||
| }); |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.