fix(webkit): [ENG-47032] cap the dialog panel so its body scrolls at every width - #836
Open
herbert-julio-azion wants to merge 8 commits into
Open
fix(webkit): [ENG-47032] cap the dialog panel so its body scrolls at every width#836herbert-julio-azion wants to merge 8 commits into
herbert-julio-azion wants to merge 8 commits into
Conversation
herbert-julio-azion
changed the title
herbert-julio-azion
marked this pull request as ready for review
August 3, 2026 13:11
…iewport Two defects in the mobile bottom-sheet chain, both in `overlay/presets/mobile-position.js`: 1. The sheet was capped with `max-h-[80vh]`. On mobile browsers `vh` is the LARGEST viewport (toolbars retracted), so an 80vh sheet anchored to the bottom can extend under the URL bar with its bottom edge, footer actions included, off-screen and nothing able to scroll it back into view. Now `80dvh`, which tracks the currently visible area. The cap lives in the shared `overlayMobileFluidClasses`, so the drawer's mobile sheet gets the same correctness fix. 2. The cap was declared twice, and the outer copy came with a dead scroll container: the motion wrapper had `max-h-[80vh] overflow-y-auto` and the panel inside had the identical cap plus `overflow-hidden`. The wrapper could therefore never scroll, since its child clips itself at exactly the same height, so it read like a fallback scroller while doing nothing. The wrapper is now position + motion only (`max-md:w-full`); the panel is the single element that bounds the sheet and its body the single element that scrolls. Measured in real Chromium at 375x667 with real Tailwind output (v4 compile API against each source state), dialog open, body ~1256px, before and after: panel top 133 / bottom 667, cap 533.6px, overflow hidden (unchanged) panel body client 411 / scroll 1256, scrolls to the end (unchanged) header top 134, footer bottom 666, both on screen (unchanged) wrapper cap 533.6 + overflow-y auto, scrolled nothing -> no cap, no scroll So the geometry is identical in Chromium and the chain is one element simpler. Note this means the reported "dialog does not scroll on mobile" does NOT reproduce in headless Chromium at 375px: the body already scrolled there. The vh/dvh distinction is invisible without a real dynamic toolbar, which is why this needs a device check before it leaves draft. Also here: - `dialog.test.ts`: first assertion that the page scroll lock actually engages while open and is released on close. That one is real in this env because the lock writes an inline style; the panel's own scrolling is Tailwind layout and this suite runs no Tailwind, so it stays in the visual layer. - Storybook: LongContent and LongContentMobile (375px) stories with a body taller than the sheet, the regression gate for this fix. - Spec: documented the mobile sheet cap, the single scroll owner and the scroll lock, plus the new stories.
… set The canonical story set is Default + one composite per multi-option axis + one per mutually-exclusive state, and the spec's Constraints block forbids bespoke stories beyond that unless the spec justifies them. The Dialog diverged three ways: - **No story for the `size` axis.** `size` is a documented three-option prop and Drawer, the same family on the same shell, ships `Sizes`. Added, mirroring it (one dialog per size, `watch` keeping a single one open so overlays never stack). `const sizes` was already declared in the file for the argTypes and is now used by the story too. - **`LongContent` is off-name for the family.** Drawer expresses the identical concern — body taller than the surface, only `PanelContent` scrolls — as `ScrollContent`. Renamed to match; two components on one shell should not name the same behaviour twice. - **`LongContentMobile` was a duplicate whose only delta was the viewport.** Dropped: the visual runner shoots every story in six modes, mobile included in both themes, so a viewport-pinned twin adds no coverage. Its description moved into `ScrollContent`, pointing at the viewport toolbar. Also fixes the spec's `## Purpose`, which the docs lead is supposed to derive from. It described the overlay family in general and cited `packages/webkit/src/components/webkit/overlay/dialog/`, a path with no `components/webkit/` segment that does not exist in the tree.
ScrollContent mounted with `default-open`, so on the autodocs page the dialog teleported its panel to body and covered the documentation as soon as the page loaded. Every other overlay story in the tree starts closed and opens from its trigger — Drawer's ScrollContent holds `const open = ref(false)` and binds `v-model:open` — so this now matches. Note the trade-off this exposes: with the story closed by default, the visual baseline captures the trigger, not the open dialog. The scrolling body is therefore not covered by visual regression for Dialog or for Drawer, which is part of why the mobile scroll report had no failing gate to point at. Making the open state visually gated needs a play function or an interaction test, not a default-open story.
The dialog renders its overflowing body two different ways, and only one was documented. ScrollContent is the centered modal from `md` up; ScrollContentMobile is the same body below `md`, where the sheet is full-width, anchored to the bottom edge and capped at 80dvh — a different cap and position from the `max-md:` classes, not just a narrower canvas. Both open from the trigger, so neither covers the autodocs page. What these stories are and are not: they are the surface for looking at the scroll by hand, at both rendering modes. They are NOT a visual-regression gate, because the panel is teleported to `body` while the runner screenshots `#storybook-root` — an open overlay never enters a snapshot, whatever the story does. Gating the open state visually would mean screenshotting the page instead of that element, which is a change to the runner and not to this component.
…y below md
Every height cap in the overlay preset was `max-md:`-prefixed, so from `md` up
the dialog panel had no bound at all. A body taller than the viewport grew the
modal past the screen, and because the shell centres it, the overflow went off
BOTH edges: the title above the top, the footer actions below the bottom, and
nothing scrolling — `panel-content` can only scroll when the panel's own height
is bounded.
Measured in real Chromium with the real compiled CSS, dialog open, body 648px:
desktop 1280x720 before panel 771px, max-height none, body 648/648,
does not scroll, header AND footer off screen
after panel 576px (80dvh), body 453/648, scrolls to the
end, header and footer both on screen
mobile 375x667 before panel 534px, body 411/1232, scrolls (unchanged)
after identical
The cap is now one unprefixed `max-h-[80dvh]` on the panel, so mobile and
desktop share it and the mobile-only copy is gone. `dvh` rather than `vh` keeps a
mobile browser's toolbars out of the measurement; on desktop the two are equal.
The dialog panel classes no longer reuse `overlayMobileFluidClasses` for the cap:
that array stays as the drawer's fluid mobile sheet, and the dialog owns its own
composition. The fluid width and `h-fit` it contributed below `md` are declared
inline instead.
herbert-julio-azion
force-pushed
the
fix/ENG-47032-dialog-mobile-scroll
branch
from
August 4, 2026 13:52
106fe5c to
edbd792
Compare
|
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes ENG-47032. A dialog whose body is taller than the space available has to scroll that body while the title and the footer actions stay put. It did not, and the failure was different at each breakpoint.
panel-contentcarriesmin-h-0 flex-1 overflow-y-auto, so it can only scroll when the panel's own height is bounded. Every height cap inpackages/webkit/src/components/overlay/presets/mobile-position.jswasmax-md:-prefixed.1. From
mdup the panel had no cap at all. The modal grew past the viewport, and because the shell centres it the overflow went off both edges: the title above the top, the footer actions below the bottom, and nothing scrolling.2. Below
mdthe cap usedvh. On mobile browsersvhresolves against the largest viewport, toolbars retracted, so a bottom-anchored sheet can extend under the URL bar with its footer off screen.dvhtracks the visible area.3. The cap was declared twice, and the outer copy carried a dead scroll container. The motion wrapper had
max-h-[80vh] overflow-y-auto; the panel inside had the identical cap plusoverflow-hidden, so the wrapper could never scroll. It read like a fallback scroller while doing nothing.The fix is one unprefixed
max-h-[80dvh]on the panel, with the wrapper reduced to position and motion only.Measured
Real Chromium, real compiled CSS, dialog open, body 648px:
max-height: noneoverflow-y: auto, scrolled nothingThe
dvhhalf cannot be observed locally: without a real dynamic toolbardvhequalsvh, and at 375×667 in headless Chromium the body already scrolled. That is the part still to confirm on a device.Stories
The Dialog story set now follows the canonical shape (Default + one composite per axis + the scroll scenarios) and matches the Drawer, which sits on the same shell:
sizeaxis, which had no story at all even thoughconst sizeswas already declared for the argTypes.md: different cap and position from themax-md:classes, not just a narrower canvas.LongContent→ScrollContent, matching the Drawer's name for the identical behaviour.Both scroll stories open from their trigger. A dialog that mounts open teleports its panel to
bodyand covers the autodocs page, which is what the earlierdefault-opendid.What the stories do not do: gate the behaviour. The panel is teleported to
bodyand the visual runner screenshots#storybook-root(test-runner.js:137), so an open overlay never enters a snapshot, whatever the story does. These stories are the surface for checking the scroll by hand at both rendering modes; making the open state visually gated means screenshotting the page rather than that element, which is a change to the runner.Notes
dvhcap and the fluid mobile sheet no longer share one array:overlayMobileFluidClassesstays as the drawer's preset and the dialog declares its own composition, so retuning one cannot silently retune the other. The drawer's mobile sheet keeps80dvh.## Purposedescribed the overlay family in general and citedpackages/webkit/src/components/webkit/overlay/dialog/, a path that does not exist in the tree. Rewritten, andcatalog.jsonrebuilt since it embeds it.dvhhalf has not been checked on a real device. Everything above was measured in headless Chromium, wheredvhandvhare identical, so the fix for the reported symptom is reasoned from the CSS spec rather than observed. What to check: iOS Safari with the toolbars expanded and retracted, and Android Chrome with the on-screen keyboard open. If the body still will not scroll on iOS withdvhin place, the next suspect isuseScrollLock(document.body)indialog-content.vue— a different fix, and a different ticket.