fix: dialogs on safari#94
Conversation
laravdiemen
requested review from
FreakyWizard,
JasperS20,
Rovasch,
ShunLuk,
SimonvanWijhe,
WybeBosch,
Yannicvanveen,
YvetteNikolov,
dtakken,
ictbeheer,
mvdhoek1 and
rivanuff
There was a problem hiding this comment.
Pull request overview
Deze PR verhelpt Safari-problemen met dialog-animaties door Tailwind’s translate-property output te vermijden en expliciet transform: translateX/Y(...) te gebruiken voor de open/close transitions.
Changes:
- Vervangt
translate-x/y-*utilities doortransform-[translateX/Y(...)]voor betere Safari-compatibiliteit. - Past de
starting:open:*enopen:*states aan zodat de animatie viatransformloopt. - Update mobile filters dialog layout/z-index (o.a.
size-full,z-1) en maakt sticky footer ookz-1.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| web/app/themes/sage/resources/views/components/header/search-bar.blade.php | Zet de zoekbalk slide-in/out om naar transform: translateY(...) i.p.v. translate. |
| web/app/themes/sage/resources/views/components/header/mobile-menu.blade.php | Zet het mobiele menu slide-in/out om naar transform: translateX(...) i.p.v. translate. |
| web/app/themes/sage/resources/views/components/facetwp/mobile-filters.blade.php | Zet filters bottom-sheet animatie om naar transform: translateY(...) en voegt sizing/z-index toe voor correcte stacking/positioning. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| <x-brave-dialog :id="$dialogId" :ariaLabel="$label" @class([ | ||
| 'mobile-menu transition-discrete top-0 bottom-0 left-10 right-0 h-full max-h-full w-auto max-w-full translate-x-full bg-white opacity-0 transition-all duration-500', | ||
| 'mobile-menu transition-discrete top-0 bottom-0 left-10 right-0 h-full max-h-full w-auto max-w-full transform-[translateX(100%)] bg-white opacity-0 transition-all duration-500', |
There was a problem hiding this comment.
🟣 This PR fixes Safari-broken CSS translate utilities in the three dialog components, but misses the same issue in sections/header.blade.php, which uses headroom-pinned:translate-y-0 and headroom-unpinned:-translate-y-full. In Tailwind v4, these generate the CSS translate property (translate: 0 0 / translate: 0 -100%) — the exact same pattern this PR identifies as broken on Safari — meaning the Headroom.js sticky header hide/show animation will remain broken on Safari. This is a pre-existing issue not introduced by this PR, but the fix pattern applied here should be applied there too.
Extended reasoning...
Root Cause
In Tailwind v4, shorthand translate utilities like -translate-y-full and translate-y-0 compile to the CSS translate shorthand property (e.g., translate: 0 -100%) rather than transform: translateY(-100%). This PR correctly identifies that Safari does not handle the CSS translate shorthand correctly in the context of @starting-style / transition-discrete dialog animations, and replaces all affected utilities with explicit transform-[translateY(...)] Tailwind arbitrary values.
Missed Location
While the PR fixes three dialog components, web/app/themes/sage/resources/views/sections/header.blade.php line 2 still contains:
headroom-pinned:translate-y-0 headroom-unpinned:-translate-y-full
In Tailwind v4, headroom-pinned:translate-y-0 generates translate: 0 0 and headroom-unpinned:-translate-y-full generates translate: 0 -100%. Both use the CSS translate shorthand property — the same broken pattern.
Code Path
Headroom.js applies headroom--pinned / headroom--unpinned classes to the sticky header element as the user scrolls. The custom headroom-pinned: and headroom-unpinned: Tailwind variants respond to those classes. On Safari, the CSS translate property generated by these utilities fails to animate correctly, so the sticky header's slide-in/slide-out animation breaks.
Why Existing Code Doesn't Prevent It
The PR author fixed the translate-x-full, -translate-y-full, and translate-y-0 utilities inside the three <dialog> components but did not audit all other translate utility uses in the project. The Headroom.js header uses the same underlying Tailwind utilities and will generate the same broken CSS output.
Impact
On Safari, the sticky header will not animate correctly when scrolling — it may snap instead of slide, or fail to hide/show entirely, depending on Safari's exact handling of the translate shorthand in this context.
Fix
Replace the two utilities on the header element:
headroom-pinned:translate-y-0→headroom-pinned:transform-[translateY(0)]headroom-unpinned:-translate-y-full→headroom-unpinned:transform-[translateY(-100%)]
Step-by-Step Proof
- Tailwind v4 processes
-translate-y-full: generates.\-translate-y-full { translate: 0 -100%; } - Headroom.js adds class
headroom--unpinnedto the header element as user scrolls down. - The
headroom-unpinned:variant activates, applyingtranslate: 0 -100%to the header. - Safari incorrectly handles the CSS
translateshorthand in animated transitions → header does not slide up. - Same issue the PR author already observed and fixed for dialogs applies here verbatim.
There was a problem hiding this comment.
headroom is een custom variant is, het probleem lijkt alleen met starting-style te zijn
|
Ter referentie voor later tailwindlabs/tailwindcss#18304 |
laravdiemen
force-pushed
the
fix/dialogs-safari
branch
from
5209941 to
b59b4cc
Compare
There was a problem hiding this comment.
LGTM — the previous concern about size-full doesn't apply here; the actual code uses h-fit, which correctly auto-sizes the dialog to its content up to the max-h-3/4 cap.
Extended reasoning...
Overview
The PR replaces Tailwind v4 translate-* shorthand utilities (which compile to the CSS translate property) with explicit transform-[translateY/X(...)] arbitrary values across three dialog components: mobile filters bottom sheet, mobile menu slide-in, and search bar overlay. It also swaps h-full for h-fit on the mobile-filters dialog and bumps animation durations from 300ms to 500ms.
Correction of prior review
A previous run of this reviewer left a 🔴 inline comment claiming the PR added size-full to the mobile-filters dialog. Re-reading the actual diff confirms the PR uses h-fit (not size-full). h-fit sets height: fit-content, which preserves auto-sizing behavior up to the max-h-3/4 cap — there is no forced 75vh height. The inner flex flex-col div also correctly drops its h-full since no fixed parent height exists. That prior concern was based on a misread and does not apply.
Security risks
None. These are purely presentational CSS class changes in Blade view templates.
Level of scrutiny
Low. This is a well-understood browser compatibility workaround (see the linked Tailwind discussion). The fix pattern — replacing CSS translate shorthand with explicit transform: translateY() — is mechanical and self-contained. The 🟣 note about the Headroom.js header is a valid pre-existing gap but is out of scope for this PR.
Other factors
No logic, data, or security-sensitive code is touched. Changes follow the existing pattern established in the three components.
translate-y-fullwordt onderwatertranslate: 0 100%en dat werkt blijkbaar niet goed op safari. Daarom doe ik nutransform: translateY(100%). Ook moest de filters nog wat aanpassingen qua height enz-1krijgen. Waarom dit eerst wel goed heeft gewerkt is mij een raadsel.