Skip to content

fix(Dialog): reset inherited edge signals from ancestor containers - #1237

Merged
czarandy merged 1 commit into
mainfrom
fix/dialog-edge-signal-isolation
Apr 10, 2026
Merged

fix(Dialog): reset inherited edge signals from ancestor containers#1237
czarandy merged 1 commit into
mainfrom
fix/dialog-edge-signal-isolation

Conversation

@czarandy

@czarandy czarandy commented Apr 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Bug: When XDSDialog is rendered as a DOM descendant of a container that sets edge compensation signals (--edge-start/--edge-end), ghost-variant buttons inside the dialog (like the close button in XDSDialogHeader) incorrectly apply edge compensation negative margins. This happens because CSS custom properties inherit through the DOM tree even for top-layer elements rendered via showModal().
  • Root cause: XDSTopNav sets --edge-end: 1 on its endContent wrapper via edgeSignals.end. When a dialog is rendered inside endContent, the close button's edgeCompensation.self style reads the inherited --edge-end: 1 and applies an unwanted -8px margin, making it visually different from dialogs rendered outside the TopNav.
  • Fix: Reset --edge-start and --edge-end to 0 on the <dialog> element so edge compensation only applies within intentional container boundaries.

Repro scenario

<XDSTopNav
  endContent={
    <>
      <XDSButton label="Open" onClick={() => setOpen(true)} />
      <XDSDialog isOpen={open} onOpenChange={setOpen} purpose="form" width={900}>
        <XDSLayout
          header={<XDSDialogHeader title="Title" onOpenChange={setOpen} />}
          content={<XDSLayoutContent>Content</XDSLayoutContent>}
        />
      </XDSDialog>
    </>
  }
/>

The close button in the dialog has a visible margin shift compared to an identical dialog rendered outside the TopNav.

Test plan

  • Existing Dialog tests pass (22/22)
  • Visually verify Dialog close button spacing is identical when rendered inside vs outside TopNav

…gin shift

When XDSDialog is rendered inside a container that sets edge signals
(e.g. TopNav's endContent sets --edge-end: 1), ghost buttons inside
the dialog (like the close button) inherit the signal and apply
unwanted edge compensation margins. CSS custom properties inherit
through the DOM tree even for top-layer elements.

Reset --edge-start and --edge-end to 0 on the <dialog> element so
edge compensation only applies within intentional container boundaries.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Apr 9, 2026
@czarandy

czarandy commented Apr 9, 2026

Copy link
Copy Markdown
Collaborator Author

Not sure if we have a better way to reset these? LMK

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

Dialog · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A 752B N/A
Lines of Code N/A 397 -
Complexity N/A Very High (51) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@xds/core 14.8KB 23.3KB 3.5KB

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

@cixzhang

Copy link
Copy Markdown
Contributor

Hmm I was thinking to get rid of this kind of edge compensation and then just have the containers that want it adjust the negative margins around the content. But I guess it's tricky for the top nav. That might be the last one I need to think about converting.

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, this is a reasonable fix to unblock.

@czarandy
czarandy merged commit 5d69974 into main Apr 10, 2026
17 checks passed
@czarandy
czarandy deleted the fix/dialog-edge-signal-isolation branch April 10, 2026 04:27
cixzhang pushed a commit that referenced this pull request Apr 26, 2026
…gin shift (#1237)

When XDSDialog is rendered inside a container that sets edge signals
(e.g. TopNav's endContent sets --edge-end: 1), ghost buttons inside
the dialog (like the close button) inherit the signal and apply
unwanted edge compensation margins. CSS custom properties inherit
through the DOM tree even for top-layer elements.

Reset --edge-start and --edge-end to 0 on the <dialog> element so
edge compensation only applies within intentional container boundaries.

Co-authored-by: Andrey Goder <agoder@fb.com>
cixzhang pushed a commit that referenced this pull request Jun 21, 2026
…gin shift (#1237)

When XDSDialog is rendered inside a container that sets edge signals
(e.g. TopNav's endContent sets --edge-end: 1), ghost buttons inside
the dialog (like the close button) inherit the signal and apply
unwanted edge compensation margins. CSS custom properties inherit
through the DOM tree even for top-layer elements.

Reset --edge-start and --edge-end to 0 on the <dialog> element so
edge compensation only applies within intentional container boundaries.

Co-authored-by: Andrey Goder <agoder@fb.com>
cixzhang pushed a commit that referenced this pull request Jun 21, 2026
…gin shift (#1237)

When XDSDialog is rendered inside a container that sets edge signals
(e.g. TopNav's endContent sets --edge-end: 1), ghost buttons inside
the dialog (like the close button) inherit the signal and apply
unwanted edge compensation margins. CSS custom properties inherit
through the DOM tree even for top-layer elements.

Reset --edge-start and --edge-end to 0 on the <dialog> element so
edge compensation only applies within intentional container boundaries.

Co-authored-by: Andrey Goder <agoder@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants