Skip to content

Commit 1737b74

Browse files
fix(dialog): ContentDialog opens without manual XamlRoot (#246) (#247)
* fix(dialog): resolve ContentDialog XamlRoot from in-tree placeholder (#246) ShowContentDialog mounted dialog content into a detached subtree, then read contentUi.XamlRoot — which is always null because the content was never attached. ShowAsync() then threw on the null XamlRoot, the empty catch swallowed the error, and apps saw no dialog. Workaround was to manually .Set(d => d.XamlRoot = ...). Resolve XamlRoot from the in-tree placeholder (which the reconciler already returned to the parent), falling back to PrimaryWindow. For the mount-time IsOpen=true case the placeholder isn't attached yet, so defer via Loaded. ApplySetters runs last so caller .Set(...) still wins. Adds two regression fixtures (mount-time and state-flip paths) that verify the dialog appears as a popup via GetOpenPopupsForXamlRoot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address CR: defer on anchor.XamlRoot=null, re-read current element, log swallowed errors - ShowContentDialog: only consult anchor.XamlRoot at mount-time; defer to Loaded whenever the anchor isn't attached, so dialogs mounted in a secondary window can't be misrouted to PrimaryWindow's XamlRoot. The PrimaryWindow fallback is now only a last resort after Loaded. - In the Loaded handler, re-read the current element from the anchor's Tag and bail unless IsOpen is still true — guards against a stale show if state flipped back to false (or the element was replaced) before the placeholder attached. - Revert ShowContentDialog from internal to private (no external caller). - Log ShowAsync exceptions via Debug.WriteLine instead of silently swallowing, matching the rest of the framework's best-effort pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2bec028 commit 1737b74

5 files changed

Lines changed: 130 additions & 9 deletions

File tree

src/Reactor/Core/Reconciler.Mount.cs

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1732,30 +1732,60 @@ private UIElement MountContentDialog(ContentDialogElement cdEl, Action requestRe
17321732
{
17331733
var placeholder = new WinUI.StackPanel { Visibility = Visibility.Collapsed };
17341734
SetElementTag(placeholder, cdEl);
1735-
if (cdEl.IsOpen) ShowContentDialog(cdEl, requestRerender);
1735+
if (cdEl.IsOpen) ShowContentDialog(cdEl, placeholder, requestRerender);
17361736
return placeholder;
17371737
}
17381738

1739-
private async void ShowContentDialog(ContentDialogElement cdEl, Action requestRerender)
1739+
private void ShowContentDialog(ContentDialogElement cdEl, FrameworkElement anchor, Action requestRerender)
1740+
{
1741+
// Source XamlRoot from the placeholder so the dialog routes to the
1742+
// window that owns the anchor. If the anchor isn't attached yet
1743+
// (mount-time IsOpen=true) defer via Loaded — falling back to
1744+
// PrimaryWindow here would misroute the dialog when the anchor lives
1745+
// in a secondary window.
1746+
if (anchor.XamlRoot is null)
1747+
{
1748+
void OnLoaded(object sender, RoutedEventArgs _)
1749+
{
1750+
anchor.Loaded -= OnLoaded;
1751+
// Re-read the current element from the anchor's Tag in case
1752+
// IsOpen was toggled back to false (or the element was
1753+
// replaced) before Loaded fired.
1754+
if (GetElementTag(anchor) is not ContentDialogElement current || !current.IsOpen)
1755+
return;
1756+
var deferredRoot = anchor.XamlRoot
1757+
?? ReactorApp.PrimaryWindow?.NativeWindow.Content?.XamlRoot;
1758+
ShowContentDialogCore(current, deferredRoot, requestRerender);
1759+
}
1760+
anchor.Loaded += OnLoaded;
1761+
return;
1762+
}
1763+
ShowContentDialogCore(cdEl, anchor.XamlRoot, requestRerender);
1764+
}
1765+
1766+
private async void ShowContentDialogCore(ContentDialogElement cdEl, XamlRoot? xamlRoot, Action requestRerender)
17401767
{
17411768
var dialog = new WinUI.ContentDialog
17421769
{
17431770
Title = cdEl.Title, PrimaryButtonText = cdEl.PrimaryButtonText,
17441771
DefaultButton = cdEl.DefaultButton,
1745-
XamlRoot = null,
17461772
};
17471773
if (cdEl.SecondaryButtonText is not null) dialog.SecondaryButtonText = cdEl.SecondaryButtonText;
17481774
if (cdEl.CloseButtonText is not null) dialog.CloseButtonText = cdEl.CloseButtonText;
17491775
dialog.Content = Mount(cdEl.Content, requestRerender);
1776+
if (xamlRoot is not null) dialog.XamlRoot = xamlRoot;
1777+
// ApplySetters last so caller .Set(...) wins (including overriding XamlRoot).
17501778
ApplySetters(cdEl.Setters, dialog);
17511779
try
17521780
{
1753-
if (dialog.Content is UIElement contentUi && contentUi.XamlRoot is not null)
1754-
dialog.XamlRoot = contentUi.XamlRoot;
17551781
var winUiResult = await dialog.ShowAsync();
17561782
cdEl.OnClosed?.Invoke(winUiResult);
17571783
}
1758-
catch { }
1784+
catch (Exception ex)
1785+
{
1786+
global::System.Diagnostics.Debug.WriteLine(
1787+
$"[Reactor.ContentDialog] ShowAsync failed: {ex.GetType().Name}: {ex.Message}");
1788+
}
17591789
}
17601790

17611791
private UIElement? MountFlyout(FlyoutElement flyEl, Action requestRerender)

src/Reactor/Core/Reconciler.Update.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2426,7 +2426,7 @@ private static bool StringArrayEquals(string[] a, string[] b)
24262426

24272427
private UIElement? UpdateContentDialog(ContentDialogElement o, ContentDialogElement n, FrameworkElement fe, Action requestRerender)
24282428
{
2429-
if (n.IsOpen && !o.IsOpen) ShowContentDialog(n, requestRerender);
2429+
if (n.IsOpen && !o.IsOpen) ShowContentDialog(n, fe, requestRerender);
24302430
SetElementTag(fe, n);
24312431
return null;
24322432
}

tests/Reactor.AppTests.Host/SelfTest/Fixtures/CoreCoverageFixtures.cs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,91 @@ public override async Task RunAsync()
320320
}
321321
}
322322

323+
// Regression for #246: declarative IsOpen=true at first mount must actually
324+
// show the dialog. Previously the framework tried to read XamlRoot from the
325+
// dialog's just-mounted (detached) content, which is always null, so
326+
// ShowAsync() threw and the empty catch swallowed it.
327+
internal class ContentDialogOpensAtMount(Harness h) : SelfTestFixtureBase(h)
328+
{
329+
public override async Task RunAsync()
330+
{
331+
var host = H.CreateHost();
332+
host.Mount(_ => VStack(
333+
TextBlock("anchor"),
334+
ContentDialog("AutoOpen", TextBlock("dialog-body"), "OK") with { IsOpen = true }
335+
));
336+
337+
await Harness.Render(100);
338+
339+
var dialog = FindOpenContentDialog(H, "AutoOpen");
340+
H.Check("ContentDialog_OpensAtMount_#246", dialog is not null);
341+
342+
dialog?.Hide();
343+
await Harness.Render(50);
344+
}
345+
}
346+
347+
// Regression for #246: flipping IsOpen false→true via state must show the
348+
// dialog without requiring the app to set XamlRoot manually.
349+
internal class ContentDialogOpensOnStateFlip(Harness h) : SelfTestFixtureBase(h)
350+
{
351+
public override async Task RunAsync()
352+
{
353+
var host = H.CreateHost();
354+
host.Mount(ctx =>
355+
{
356+
var (open, setOpen) = ctx.UseState(false);
357+
return VStack(
358+
Button("Show", () => setOpen(true)),
359+
ContentDialog("Toggled", TextBlock("dialog-body"), "OK") with { IsOpen = open }
360+
);
361+
});
362+
363+
await Harness.Render();
364+
H.Check("ContentDialog_NotOpenBeforeClick_#246", FindOpenContentDialog(H, "Toggled") is null);
365+
366+
H.ClickButton("Show");
367+
await Harness.Render(50);
368+
369+
var dialog = FindOpenContentDialog(H, "Toggled");
370+
H.Check("ContentDialog_OpensOnStateFlip_#246", dialog is not null);
371+
372+
dialog?.Hide();
373+
await Harness.Render(50);
374+
}
375+
}
376+
377+
private static Microsoft.UI.Xaml.Controls.ContentDialog? FindOpenContentDialog(Harness h, string title)
378+
{
379+
var xamlRoot = (h.Window.Content as UIElement)?.XamlRoot
380+
?? ReactorApp.PrimaryWindow?.NativeWindow.Content?.XamlRoot;
381+
if (xamlRoot is null) return null;
382+
foreach (var popup in VisualTreeHelper.GetOpenPopupsForXamlRoot(xamlRoot))
383+
{
384+
// Popup.Child is not enumerated by VisualTreeHelper.GetChildrenCount,
385+
// so descend into it explicitly before recursing.
386+
if (popup.Child is DependencyObject child)
387+
{
388+
var found = WalkForContentDialog(child, title);
389+
if (found is not null) return found;
390+
}
391+
}
392+
return null;
393+
}
394+
395+
private static Microsoft.UI.Xaml.Controls.ContentDialog? WalkForContentDialog(DependencyObject node, string title)
396+
{
397+
if (node is Microsoft.UI.Xaml.Controls.ContentDialog cd && cd.Title as string == title)
398+
return cd;
399+
int count = VisualTreeHelper.GetChildrenCount(node);
400+
for (int i = 0; i < count; i++)
401+
{
402+
var hit = WalkForContentDialog(VisualTreeHelper.GetChild(node, i), title);
403+
if (hit is not null) return hit;
404+
}
405+
return null;
406+
}
407+
323408
// ════════════════════════════════════════════════════════════════════════
324409
// 6. CommandBar — mount + update (primary/secondary commands)
325410
// Targets: Reconciler.Mount.cs lines 1636+, Reconciler.Update.cs lines 1593+

tests/Reactor.AppTests.Host/SelfTest/Fixtures/RareControlFixtures.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,10 @@ public override async Task RunAsync()
8181
// ContentDialog mount + update (~15 lines each)
8282
// ════════════════════════════════════════════════════════════════════
8383

84-
// ContentDialog is rendered as a popup overlay and requires ShowAsync() to appear
85-
// in the visual tree — tested via interactive tests instead of self-tests.
84+
// ContentDialog mount + open coverage lives in CoreCoverageFixtures
85+
// (ContentDialogMount + ContentDialogOpensAtMount + ContentDialogOpensOnStateFlip).
86+
// The dialog appears as a popup; ShowAsync() is fire-and-forget here so the
87+
// fixtures probe VisualTreeHelper.GetOpenPopupsForXamlRoot() after a render.
8688

8789
// ════════════════════════════════════════════════════════════════════
8890
// GridView mount + update

tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,8 @@ internal static class SelfTestFixtureRegistry
349349
"CoreCov_TemplatedFlipViewMountUpdate",
350350
"CoreCov_LazyVStackMountUpdate",
351351
"CoreCov_ContentDialogMount",
352+
"CoreCov_ContentDialogOpensAtMount",
353+
"CoreCov_ContentDialogOpensOnStateFlip",
352354
"CoreCov_CommandBarMountUpdate",
353355
"CoreCov_CommandHostMountUpdate",
354356
"CoreCov_MenuBarMountUpdate",
@@ -1137,6 +1139,8 @@ internal static class SelfTestFixtureRegistry
11371139
"CoreCov_TemplatedFlipViewMountUpdate" => new CoreCoverageFixtures.TemplatedFlipViewMountUpdate(harness),
11381140
"CoreCov_LazyVStackMountUpdate" => new CoreCoverageFixtures.LazyVStackMountUpdate(harness),
11391141
"CoreCov_ContentDialogMount" => new CoreCoverageFixtures.ContentDialogMount(harness),
1142+
"CoreCov_ContentDialogOpensAtMount" => new CoreCoverageFixtures.ContentDialogOpensAtMount(harness),
1143+
"CoreCov_ContentDialogOpensOnStateFlip" => new CoreCoverageFixtures.ContentDialogOpensOnStateFlip(harness),
11401144
"CoreCov_CommandBarMountUpdate" => new CoreCoverageFixtures.CommandBarMountUpdate(harness),
11411145
"CoreCov_CommandHostMountUpdate" => new CoreCoverageFixtures.CommandHostMountUpdate(harness),
11421146
"CoreCov_MenuBarMountUpdate" => new CoreCoverageFixtures.MenuBarMountUpdate(harness),

0 commit comments

Comments
 (0)