From 36f8cd2b1bf891213c90434fdc69c250f85a25b3 Mon Sep 17 00:00:00 2001 From: rubyycheung <254546175+rubyycheung@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:08:45 -0500 Subject: [PATCH] fix(Dialog): address responsive review feedback Follow-up to #5352, which merged before these review nits were applied. Fullscreen safe-area inline padding mapped physical env() insets straight onto logical properties. That holds in LTR and inverts in RTL, where inline-start is the right edge, so a notch on the physical left padded the edge away from it and left the notched edge unprotected. Each physical inset now feeds the logical edge that faces it, through a dir-scoped variant. Safe-area protection also applied unconditionally, so it beat an explicit padding prop and a theme's dialog: {padding: 0} alike and a deliberately full-bleed fullscreen dialog could not be expressed. The max() now sits in the innermost fallback of the same --astryx-dialog-padding* chain that container already resolves, so an explicit value -- 0 included -- is honored as written and the safe-area floor only applies when no padding is set anywhere. Fullscreen motion coverage now asserts on the keyframes themselves rather than on the order styles are spread: opacity-only for fullscreen, directional translate and scale for standard. The changeset whose credit the fourth nit corrected was consumed by the v0.5.0 release, which already credits @rubyycheung, so this carries a new changeset describing the fix against the released behavior instead. Co-authored-by: Cursor --- .../dialog-fullscreen-safe-area-padding.md | 13 ++ packages/core/src/Dialog/Dialog.test.tsx | 203 ++++++++++++++++-- packages/core/src/Dialog/Dialog.tsx | 46 +++- 3 files changed, 233 insertions(+), 29 deletions(-) create mode 100644 .changeset/dialog-fullscreen-safe-area-padding.md diff --git a/.changeset/dialog-fullscreen-safe-area-padding.md b/.changeset/dialog-fullscreen-safe-area-padding.md new file mode 100644 index 0000000000000..49a22d582a5ef --- /dev/null +++ b/.changeset/dialog-fullscreen-safe-area-padding.md @@ -0,0 +1,13 @@ +--- +'@astryxdesign/core': patch +--- + +[fix] Dialog: fullscreen safe-area padding follows writing direction and defers to explicit padding + +Two corrections to the fullscreen safe-area padding that shipped in 0.5.0. + +**The insets were mapped to the wrong edges in RTL.** `env(safe-area-inset-left)` and `env(safe-area-inset-right)` are physical, but they were assigned straight to `padding-inline-start` and `padding-inline-end`, which are logical. That holds in LTR and inverts in RTL, where inline-start is the right edge — so a device notch on the physical left padded the edge away from it and left the notched edge unprotected. Each physical inset now feeds the logical edge that actually faces it, in both directions. + +**Safe-area protection overrode explicit padding.** The `max()` was applied to the fullscreen surface unconditionally, so it beat both a `padding` prop and a theme's `dialog: {padding: 0}`, and a deliberately full-bleed fullscreen dialog could not be expressed. It now sits in the innermost fallback of the same `--astryx-dialog-padding*` chain `container` already resolves, so it applies only when no padding is set anywhere. An explicit value, `0` included, is honored as written. + +@rubyycheung diff --git a/packages/core/src/Dialog/Dialog.test.tsx b/packages/core/src/Dialog/Dialog.test.tsx index 7e3d1922d84e3..433caa8371d17 100644 --- a/packages/core/src/Dialog/Dialog.test.tsx +++ b/packages/core/src/Dialog/Dialog.test.tsx @@ -12,8 +12,87 @@ import {readFileSync} from 'node:fs'; import {describe, it, expect, vi, beforeEach} from 'vitest'; import {render, screen, fireEvent} from '@testing-library/react'; -import {Dialog, resolveDialogPositionOffsets} from './Dialog'; +import { + Dialog, + dialogFullscreenSafeAreaPaddingContract, + resolveDialogPositionOffsets, +} from './Dialog'; import {DialogHeader} from './DialogHeader'; +import {defineTheme, generateThemeCSS} from '../theme'; + +function generateThemeTestCSS( + theme: Parameters[0], +): string { + return Object.values(generateThemeCSS(theme)).join('\n'); +} + +function extractConstDeclaration(source: string, name: string): string { + const start = source.indexOf(`const ${name} = stylex.keyframes({`); + if (start === -1) { + throw new Error(`Missing ${name} keyframes`); + } + + const bodyStart = source.indexOf('{', start); + let depth = 0; + for (let index = bodyStart; index < source.length; index += 1) { + const char = source[index]; + if (char === '{') { + depth += 1; + } else if (char === '}') { + depth -= 1; + if (depth === 0) { + return source.slice(start, index + 3); + } + } + } + + throw new Error(`Unterminated ${name} keyframes`); +} + +function extractThemeVars(css: string): Map { + const vars = new Map(); + for (const match of css.matchAll(/(--astryx-dialog-[\w-]+):\s*([^;]+);/g)) { + vars.set(match[1], match[2].trim()); + } + return vars; +} + +function resolveCSSVarFallback( + value: string, + vars: ReadonlyMap, +): string { + const trimmed = value.trim(); + if (!trimmed.startsWith('var(') || !trimmed.endsWith(')')) { + return trimmed; + } + + const inner = trimmed.slice(4, -1); + let depth = 0; + let commaIndex = -1; + for (let index = 0; index < inner.length; index += 1) { + const char = inner[index]; + if (char === '(') { + depth += 1; + } else if (char === ')') { + depth -= 1; + } else if (char === ',' && depth === 0) { + commaIndex = index; + break; + } + } + + const varName = ( + commaIndex === -1 ? inner : inner.slice(0, commaIndex) + ).trim(); + const fallback = commaIndex === -1 ? '' : inner.slice(commaIndex + 1).trim(); + return vars.has(varName) + ? vars.get(varName)! + : resolveCSSVarFallback(fallback, vars); +} + +function normalizeCSSValue(value: string): string { + return value.replace(/\s+/g, ''); +} // Mock showModal and close methods since they're not fully implemented in jsdom beforeEach(() => { @@ -396,36 +475,57 @@ describe('Dialog', () => { expect(inlineStyle).toContain('--x-maxHeight: 70dvh'); }); - it('uses a fullscreen-specific fade animation instead of centered dialog movement', () => { + it('uses opacity-only fullscreen keyframes while standard dialogs keep directional movement', () => { const source = readFileSync( 'packages/core/src/Dialog/Dialog.tsx', 'utf8', ); - const standardOpen = source.slice( - source.indexOf(' open: {'), - source.indexOf(' // Backdrop using ::backdrop'), + const enterDirectional = extractConstDeclaration( + source, + 'enterDirectional', + ); + const enterFullscreen = extractConstDeclaration( + source, + 'enterFullscreen', ); const fullscreenOpen = source.slice( source.indexOf(' fullscreenOpen: {'), source.indexOf(' fullscreenSafeArea: {'), ); - const modalStyleOrder = source.slice( - source.indexOf('focusOutlineProps.focusVisible('), - source.indexOf( - ' xstyle,', - source.indexOf('focusOutlineProps.focusVisible('), - ), - ); - expect(standardOpen).toContain('enterDirectional'); + expect(enterDirectional).toContain('transform'); + expect(enterDirectional).toContain('translate(var(--dialog-dir-x'); + expect(enterDirectional).toContain('scale(0.95)'); + expect(enterFullscreen).toContain('opacity'); + expect(enterFullscreen).not.toContain('transform'); + expect(enterFullscreen).not.toContain('translate'); + expect(enterFullscreen).not.toContain('scale('); expect(fullscreenOpen).toContain('enterFullscreen'); expect(fullscreenOpen).not.toContain('enterDirectional'); - expect(modalStyleOrder.indexOf('styles.open')).toBeLessThan( - modalStyleOrder.indexOf('styles.fullscreenOpen'), + }); + + it('maps fullscreen safe-area insets to logical sides in LTR and RTL', () => { + expect(dialogFullscreenSafeAreaPaddingContract.inlineStart.ltr).toContain( + 'safe-area-inset-left', + ); + expect(dialogFullscreenSafeAreaPaddingContract.inlineEnd.ltr).toContain( + 'safe-area-inset-right', + ); + expect(dialogFullscreenSafeAreaPaddingContract.inlineStart.rtl).toContain( + 'safe-area-inset-right', + ); + expect(dialogFullscreenSafeAreaPaddingContract.inlineEnd.rtl).toContain( + 'safe-area-inset-left', + ); + expect(dialogFullscreenSafeAreaPaddingContract.inlineStart.rtl).not.toBe( + dialogFullscreenSafeAreaPaddingContract.inlineStart.ltr, + ); + expect(dialogFullscreenSafeAreaPaddingContract.inlineEnd.rtl).not.toBe( + dialogFullscreenSafeAreaPaddingContract.inlineEnd.ltr, ); }); - it('protects fullscreen content with safe-area padding', () => { + it('protects default fullscreen content with safe-area padding', () => { render( { const wrapper = screen.getByTestId('child').parentElement!; const computed = window.getComputedStyle(wrapper); - expect(computed.paddingInlineStart).toContain('safe-area-inset-left'); - expect(computed.paddingInlineEnd).toContain('safe-area-inset-right'); + expect(normalizeCSSValue(computed.paddingInlineStart)).toBe( + normalizeCSSValue( + dialogFullscreenSafeAreaPaddingContract.inlineStart.ltr, + ), + ); + expect(normalizeCSSValue(computed.paddingInlineEnd)).toBe( + normalizeCSSValue( + dialogFullscreenSafeAreaPaddingContract.inlineEnd.ltr, + ), + ); expect(wrapper.parentElement!.tagName).toBe('DIALOG'); }); + + it('applies RTL fullscreen safe-area padding to the opposite logical edges', () => { + render( +
+ {}} + variant="fullscreen" + aria-label="Fullscreen RTL dialog"> +
Content
+ +
, + ); + + const wrapper = screen.getByTestId('rtl-child').parentElement!; + const computed = window.getComputedStyle(wrapper); + expect(normalizeCSSValue(computed.paddingInlineStart)).toBe( + normalizeCSSValue( + dialogFullscreenSafeAreaPaddingContract.inlineStart.rtl, + ), + ); + expect(normalizeCSSValue(computed.paddingInlineEnd)).toBe( + normalizeCSSValue( + dialogFullscreenSafeAreaPaddingContract.inlineEnd.rtl, + ), + ); + }); + + it('resolves theme zero padding before the default fullscreen safe-area fallback', () => { + const zeroPaddingTheme = defineTheme({ + name: 'dialog-zero-padding-test', + components: { + dialog: { + base: {padding: '0'}, + }, + }, + }); + + const vars = extractThemeVars(generateThemeTestCSS(zeroPaddingTheme)); + expect(vars.get('--astryx-dialog-padding')).toBe('0'); + expect( + resolveCSSVarFallback( + dialogFullscreenSafeAreaPaddingContract.blockStart, + vars, + ), + ).toBe('0'); + expect( + resolveCSSVarFallback( + dialogFullscreenSafeAreaPaddingContract.inlineStart.ltr, + vars, + ), + ).toBe('0'); + expect( + resolveCSSVarFallback( + dialogFullscreenSafeAreaPaddingContract.inlineEnd.rtl, + vars, + ), + ).toBe('0'); + }); }); describe('position prop', () => { diff --git a/packages/core/src/Dialog/Dialog.tsx b/packages/core/src/Dialog/Dialog.tsx index 0aa495ee55cf0..9f7ae585c7923 100644 --- a/packages/core/src/Dialog/Dialog.tsx +++ b/packages/core/src/Dialog/Dialog.tsx @@ -11,8 +11,9 @@ * The standard variant treats `width` as the preferred surface width, then * clamps it to the dynamic viewport with spacing-token gutters so narrow * viewports keep content and controls on screen without changing the public API. - * Fullscreen dialogs preserve the same padding floor while honoring safe-area - * insets, and fade in without the centered-dialog translate/scale motion. + * Fullscreen dialogs add safe-area protection to the default padding fallback + * while preserving explicit prop/theme padding overrides, and fade in without + * the centered-dialog translate/scale motion. * * SYNC: When modified, update these files to stay in sync: * - /packages/core/src/Dialog/Dialog.doc.mjs (props table, features, implementation notes) @@ -131,6 +132,27 @@ const enterFullscreen = stylex.keyframes({ to: {opacity: 1}, }); +const dialogFullscreenSafeAreaBlockStartPadding = `var(--astryx-dialog-padding-block-start, var(--astryx-dialog-padding, max(${spacingVars['--spacing-4']}, env(safe-area-inset-top, 0px))))`; +const dialogFullscreenSafeAreaBlockEndPadding = `var(--astryx-dialog-padding-block-end, var(--astryx-dialog-padding, max(${spacingVars['--spacing-4']}, env(safe-area-inset-bottom, 0px))))`; +const dialogFullscreenSafeAreaInlineStartPaddingLtr = `var(--astryx-dialog-padding-inline-start, var(--astryx-dialog-padding-inline, var(--astryx-dialog-padding, max(${spacingVars['--spacing-4']}, env(safe-area-inset-left, 0px)))))`; +const dialogFullscreenSafeAreaInlineStartPaddingRtl = `var(--astryx-dialog-padding-inline-start, var(--astryx-dialog-padding-inline, var(--astryx-dialog-padding, max(${spacingVars['--spacing-4']}, env(safe-area-inset-right, 0px)))))`; +const dialogFullscreenSafeAreaInlineEndPaddingLtr = `var(--astryx-dialog-padding-inline-end, var(--astryx-dialog-padding-inline, var(--astryx-dialog-padding, max(${spacingVars['--spacing-4']}, env(safe-area-inset-right, 0px)))))`; +const dialogFullscreenSafeAreaInlineEndPaddingRtl = `var(--astryx-dialog-padding-inline-end, var(--astryx-dialog-padding-inline, var(--astryx-dialog-padding, max(${spacingVars['--spacing-4']}, env(safe-area-inset-left, 0px)))))`; + +/** @internal Verified by Dialog.test.tsx; not re-exported from the package entry point. */ +export const dialogFullscreenSafeAreaPaddingContract = { + blockStart: dialogFullscreenSafeAreaBlockStartPadding, + blockEnd: dialogFullscreenSafeAreaBlockEndPadding, + inlineStart: { + ltr: dialogFullscreenSafeAreaInlineStartPaddingLtr, + rtl: dialogFullscreenSafeAreaInlineStartPaddingRtl, + }, + inlineEnd: { + ltr: dialogFullscreenSafeAreaInlineEndPaddingLtr, + rtl: dialogFullscreenSafeAreaInlineEndPaddingRtl, + }, +} as const; + /** * Dialog styles using native element * Uses ::backdrop pseudo-element for overlay @@ -191,14 +213,16 @@ const styles = stylex.create({ }, }, fullscreenSafeArea: { - paddingBlockStart: - 'max(var(--container-padding-block-start), env(safe-area-inset-top, 0px))', - paddingBlockEnd: - 'max(var(--container-padding-block-end), env(safe-area-inset-bottom, 0px))', - paddingInlineStart: - 'max(var(--container-padding-inline-start), env(safe-area-inset-left, 0px))', - paddingInlineEnd: - 'max(var(--container-padding-inline-end), env(safe-area-inset-right, 0px))', + paddingBlockStart: dialogFullscreenSafeAreaBlockStartPadding, + paddingBlockEnd: dialogFullscreenSafeAreaBlockEndPadding, + paddingInlineStart: { + default: dialogFullscreenSafeAreaInlineStartPaddingLtr, + ':is([dir="rtl"] *)': dialogFullscreenSafeAreaInlineStartPaddingRtl, + }, + paddingInlineEnd: { + default: dialogFullscreenSafeAreaInlineEndPaddingLtr, + ':is([dir="rtl"] *)': dialogFullscreenSafeAreaInlineEndPaddingRtl, + }, }, inner: { display: 'flex', @@ -628,7 +652,7 @@ export function Dialog({ !useThemeDefault && effectivePadding !== 4 && containerPaddingBlockEndVarStyles[effectivePadding], - isFullscreen && styles.fullscreenSafeArea, + isFullscreen && useThemeDefault && styles.fullscreenSafeArea, )}> {children}