Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/Reactor/Core/Reconciler.Mount.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1450,10 +1450,7 @@ private WinUI.NavigationView MountNavigationView(NavigationViewElement nav, Acti
}
if (nav.Content is not null) nv.Content = Mount(nav.Content, requestRerender);
if (nav.SelectedTag is not null)
{
foreach (var mi in nv.MenuItems.OfType<WinUI.NavigationViewItem>())
if (mi.Tag as string == nav.SelectedTag) { nv.SelectedItem = mi; break; }
}
nv.SelectedItem = FindNavItemByTag(nv.MenuItems, nav.SelectedTag);
SetElementTag(nv, nav);
if (nav.OnSelectedTagChanged is not null)
nv.SelectionChanged += (s, args) =>
Expand Down
137 changes: 127 additions & 10 deletions src/Reactor/Core/Reconciler.Update.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1750,17 +1750,17 @@ void FinalizeOldPage(UIElement? oldCtrl, Element? oldElem, object? oldRoute)
if (!double.IsNaN(n.CompactModeThresholdWidth) && nv.CompactModeThresholdWidth != n.CompactModeThresholdWidth) nv.CompactModeThresholdWidth = n.CompactModeThresholdWidth;
if (!double.IsNaN(n.ExpandedModeThresholdWidth) && nv.ExpandedModeThresholdWidth != n.ExpandedModeThresholdWidth) nv.ExpandedModeThresholdWidth = n.ExpandedModeThresholdWidth;

// Reconcile menu items in place rather than clear-and-rebuild. The old
// rebuild recreated every NavigationViewItem on each render, and since
// per-item expansion (IsExpanded) lives only on the live container — it
// is not modeled in NavigationViewItemData — every re-render snapped all
// expanded hierarchical items shut. Because consumers typically pass a
// fresh MenuItems array each render (e.g. built via LINQ), the
// ReferenceEquals guard never held and the rebuild fired constantly,
// producing the "expand then collapse" flash and "child click collapses
// parent". Reusing containers by Tag preserves IsExpanded across renders.
if (!ReferenceEquals(o.MenuItems, n.MenuItems))
{
nv.MenuItems.Clear();
foreach (var item in n.MenuItems)
{
if (item.IsHeader)
nv.MenuItems.Add(new WinUI.NavigationViewItemHeader { Content = item.Content });
else
nv.MenuItems.Add(CreateNavItem(item));
}
}
ReconcileNavMenuItems(nv.MenuItems, o.MenuItems, n.MenuItems);

// AutoSuggestBox / PaneFooter / PaneCustomContent reconcile in place
// when possible so the controls keep focus / scroll state across re-renders.
Expand Down Expand Up @@ -1835,6 +1835,123 @@ void FinalizeOldPage(UIElement? oldCtrl, Element? oldElem, object? oldRoute)
return null;
}

/// <summary>
/// Brings a live NavigationView menu-item collection into agreement with
/// <paramref name="newData"/> while reusing existing <see cref="WinUI.NavigationViewItem"/>
/// containers (matched by Tag) so their runtime <c>IsExpanded</c>/selection
/// state survives the re-render. Recurses into hierarchical children.
/// </summary>
private void ReconcileNavMenuItems(
global::System.Collections.Generic.IList<object> live,
NavigationViewItemData[]? oldData,
NavigationViewItemData[] newData)
{
// Fast path: the structure (order of headers + item Tags) is unchanged —
// the overwhelmingly common case, since menus are usually static. Update
// each container in place without detaching anything, which keeps every
// container's expansion, selection and animation state pristine.
if (NavStructureMatches(live, newData))
{
for (int i = 0; i < newData.Length; i++)
{
var data = newData[i];
if (data.IsHeader)
{
if (live[i] is WinUI.NavigationViewItemHeader h && !Equals(h.Content, data.Content))
h.Content = data.Content;
}
else if (live[i] is WinUI.NavigationViewItem nvi)
{
var oldItem = oldData is not null && i < oldData.Length ? oldData[i] : null;
UpdateNavItemInPlace(nvi, oldItem, data);
}
}
return;
}

// Structure changed (items added / removed / reordered): snapshot the
// existing containers by Tag so matches can be reused, then rebuild the
// collection in the new order. Reused containers retain their IsExpanded.
var reusable = new global::System.Collections.Generic.Dictionary<string, WinUI.NavigationViewItem>();
foreach (var item in live)
if (item is WinUI.NavigationViewItem nvi && nvi.Tag is string tag)
reusable[tag] = nvi;
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed

var oldByTag = new global::System.Collections.Generic.Dictionary<string, NavigationViewItemData>();
if (oldData is not null)
foreach (var d in oldData)
if (!d.IsHeader)
oldByTag[d.Tag ?? d.Content] = d;
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed

live.Clear();
foreach (var data in newData)
{
if (data.IsHeader)
{
live.Add(new WinUI.NavigationViewItemHeader { Content = data.Content });
continue;
}

var key = data.Tag ?? data.Content;
if (reusable.TryGetValue(key, out var nvi))
UpdateNavItemInPlace(nvi, oldByTag.GetValueOrDefault(key), data);
else
nvi = CreateNavItem(data);
live.Add(nvi);
}
}

/// <summary>True when the live collection already matches the new data in
/// count and in the sequence of headers / item Tags.</summary>
private static bool NavStructureMatches(
global::System.Collections.Generic.IList<object> live,
NavigationViewItemData[] newData)
{
if (live.Count != newData.Length) return false;
for (int i = 0; i < newData.Length; i++)
{
var data = newData[i];
if (data.IsHeader)
{
if (live[i] is not WinUI.NavigationViewItemHeader) return false;
}
else
{
if (live[i] is not WinUI.NavigationViewItem nvi) return false;
if ((nvi.Tag as string) != (data.Tag ?? data.Content)) return false;
}
}
return true;
}

/// <summary>Updates a reused NavigationViewItem's Content/Icon (only when
/// changed) and reconciles its children, leaving IsExpanded untouched.</summary>
private void UpdateNavItemInPlace(WinUI.NavigationViewItem nvi, NavigationViewItemData? oldData, NavigationViewItemData data)
{
if (!Equals(nvi.Content, data.Content)) nvi.Content = data.Content;

var newTag = data.Tag ?? data.Content;
if (!Equals(nvi.Tag, newTag)) nvi.Tag = newTag;

// Re-resolve the icon only when the icon data actually changed (IconData
// records compare by value), so the FontIcon/SymbolIcon isn't reallocated
// on every render.
bool iconChanged = oldData is null
|| !Equals(oldData.IconElement, data.IconElement)
|| oldData.Icon != data.Icon;
if (iconChanged)
{
var icon = ResolveIcon(data.IconElement, data.Icon);
if (icon is not null) nvi.Icon = icon;
else if (nvi.Icon is not null) nvi.Icon = null;
}

if (data.Children is { Length: > 0 } children)
ReconcileNavMenuItems(nvi.MenuItems, oldData?.Children, children);
else if (nvi.MenuItems.Count > 0)
nvi.MenuItems.Clear();
}

private UIElement? UpdateTitleBar(TitleBarElement o, TitleBarElement n, WinUI.TitleBar titleBar, Action requestRerender)
{
titleBar.Title = n.Title;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,9 @@ internal static class NavigationViewDescriptor

private static void ApplyMenuAndSelection(WinUI.NavigationView control, NavigationViewElement? oldElement, NavigationViewElement element)
{
if (oldElement is null || !ReferenceEquals(oldElement.MenuItems, element.MenuItems))
if (oldElement is null)
{
// Mount: build fresh.
control.MenuItems.Clear();
foreach (var item in element.MenuItems)
{
Expand All @@ -133,6 +134,13 @@ private static void ApplyMenuAndSelection(WinUI.NavigationView control, Navigati
: CreateNavItem(item));
}
}
else if (!ReferenceEquals(oldElement.MenuItems, element.MenuItems))
{
// Update: reconcile in place so reused containers keep IsExpanded.
// Mirrors Reconciler.ReconcileNavMenuItems (legacy arm) — see the note
// there on why a clear-and-rebuild collapses hierarchical items.
ReconcileMenuItems(control.MenuItems, oldElement.MenuItems, element.MenuItems);
}

if (oldElement is null
|| oldElement.SelectedTag != element.SelectedTag
Expand All @@ -142,6 +150,107 @@ private static void ApplyMenuAndSelection(WinUI.NavigationView control, Navigati
}
}

private static void ReconcileMenuItems(
global::System.Collections.Generic.IList<object> live,
NavigationViewItemData[]? oldData,
NavigationViewItemData[] newData)
{
if (StructureMatches(live, newData))
{
for (int i = 0; i < newData.Length; i++)
{
var data = newData[i];
if (data.IsHeader)
{
if (live[i] is WinUI.NavigationViewItemHeader h && !Equals(h.Content, data.Content))
h.Content = data.Content;
}
else if (live[i] is WinUI.NavigationViewItem nvi)
{
var oldItem = oldData is not null && i < oldData.Length ? oldData[i] : null;
UpdateNavItemInPlace(nvi, oldItem, data);
}
}
return;
}

var reusable = new global::System.Collections.Generic.Dictionary<string, WinUI.NavigationViewItem>();
foreach (var item in live)
if (item is WinUI.NavigationViewItem nvi && nvi.Tag is string tag)
reusable[tag] = nvi;
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed

var oldByTag = new global::System.Collections.Generic.Dictionary<string, NavigationViewItemData>();
if (oldData is not null)
foreach (var d in oldData)
if (!d.IsHeader)
oldByTag[d.Tag ?? d.Content] = d;
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed

live.Clear();
foreach (var data in newData)
{
if (data.IsHeader)
{
live.Add(new WinUI.NavigationViewItemHeader { Content = data.Content });
continue;
}

var key = data.Tag ?? data.Content;
if (reusable.TryGetValue(key, out var nvi))
UpdateNavItemInPlace(nvi, oldByTag.GetValueOrDefault(key), data);
else
nvi = CreateNavItem(data);
live.Add(nvi);
}
}

private static bool StructureMatches(
global::System.Collections.Generic.IList<object> live,
NavigationViewItemData[] newData)
{
if (live.Count != newData.Length) return false;
for (int i = 0; i < newData.Length; i++)
{
var data = newData[i];
if (data.IsHeader)
{
if (live[i] is not WinUI.NavigationViewItemHeader) return false;
}
else
{
if (live[i] is not WinUI.NavigationViewItem nvi) return false;
if ((nvi.Tag as string) != (data.Tag ?? data.Content)) return false;
}
}
return true;
}

private static void UpdateNavItemInPlace(WinUI.NavigationViewItem nvi, NavigationViewItemData? oldData, NavigationViewItemData data)
{
if (!Equals(nvi.Content, data.Content)) nvi.Content = data.Content;

var newTag = data.Tag ?? data.Content;
if (!Equals(nvi.Tag, newTag)) nvi.Tag = newTag;

bool iconChanged = oldData is null
|| !Equals(oldData.IconElement, data.IconElement)
|| oldData.Icon != data.Icon;
if (iconChanged)
{
var icon = data.IconElement is not null
? Reconciler.ResolveIconForDescriptor(data.IconElement)
: data.Icon is not null
? Reconciler.ResolveIconForDescriptor(new SymbolIconData(data.Icon))
: null;
if (icon is not null) nvi.Icon = icon;
else if (nvi.Icon is not null) nvi.Icon = null;
}

if (data.Children is { Length: > 0 } children)
ReconcileMenuItems(nvi.MenuItems, oldData?.Children, children);
else if (nvi.MenuItems.Count > 0)
nvi.MenuItems.Clear();
}

private static WinUI.NavigationViewItem CreateNavItem(NavigationViewItemData data)
{
var item = new WinUI.NavigationViewItem { Content = data.Content, Tag = data.Tag ?? data.Content };
Expand Down
14 changes: 14 additions & 0 deletions tests/Reactor.AppTests.Host/FixtureRegistry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,17 @@ internal static class FixtureRegistry
// Collections
"ListView_TypedRendering",

// TreeView expand/collapse (mirrors ReactorGallery Basic TreeView —
// E2E repro/guard for the item-body-collapse bug)
"TreeView_BasicTextTree",

// Navigation
"Navigation_TabSwitching",

// NavigationView hierarchical expand/collapse (E2E repro/guard for the
// re-render rebuild-clobber that collapsed expanded categories)
"NavigationView_Hierarchical",

// Observable
"Observable_UseObservable_Rerender",
"Observable_UseObservable_ExternalMutation",
Expand Down Expand Up @@ -205,9 +213,15 @@ internal static class FixtureRegistry
// Collections
"ListView_TypedRendering" => CollectionFixtures.ListViewTyped(ctx),

// TreeView expand/collapse
"TreeView_BasicTextTree" => TreeViewE2EFixtures.BasicTextTree(ctx),

// Navigation
"Navigation_TabSwitching" => NavigationFixtures.TabSwitching(ctx),

// NavigationView hierarchical expand/collapse
"NavigationView_Hierarchical" => NavigationViewE2EFixtures.HierarchicalNav(ctx),

// Observable
"Observable_UseObservable_Rerender" => ObservableFixtures.UseObservable_Rerender(ctx),
"Observable_UseObservable_ExternalMutation" => ObservableFixtures.UseObservable_ExternalMutation(ctx),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
using Microsoft.UI.Reactor;
using Microsoft.UI.Reactor.Core;
using Microsoft.UI.Xaml.Controls;
using static Microsoft.UI.Reactor.Factories;

namespace Microsoft.UI.Reactor.AppTests.Host.Fixtures;

/// <summary>
/// E2E fixtures for hierarchical <c>NavigationView</c> expand/collapse, mirroring
/// the ReactorGallery shell shape: a stateful component that holds the selected
/// tag, re-renders on every selection, and rebuilds its <c>MenuItems</c> array
/// fresh each render (the condition that made the old clear-and-rebuild update
/// path collapse expanded categories on every re-render).
///
/// PaneDisplayMode is pinned to <c>Left</c> so the pane stays open with text
/// labels (addressable by UIA Name) and children render inline when expanded,
/// regardless of the host window size.
/// </summary>
internal static class NavigationViewE2EFixtures
{
internal class HierarchicalNavComponent : Component
{
public override Element Render()
{
var (selectedTag, setSelectedTag) = UseState("alpha-1");

// Rebuilt fresh every render — exactly the pattern the Gallery uses
// and the one that exposed the rebuild-clobber bug.
var menuItems = new[]
{
NavItem("Alpha", tag: "alpha") with
{
Children = new[]
{
NavItem("Alpha-1", tag: "alpha-1"),
NavItem("Alpha-2", tag: "alpha-2"),
}
},
NavItem("Bravo", tag: "bravo") with
{
Children = new[]
{
NavItem("Bravo-1", tag: "bravo-1"),
}
},
};

return NavigationView(
menuItems,
content: TextBlock($"Selected: {selectedTag}").AutomationId("NavSelectedTag")
) with
{
SelectedTag = selectedTag,
PaneDisplayMode = NavigationViewPaneDisplayMode.Left,
IsSettingsVisible = false,
OnSelectedTagChanged = tag =>
{
if (tag != null) setSelectedTag(tag);
},
};
}
}

internal static Element HierarchicalNav(RenderContext ctx) =>
Component<HierarchicalNavComponent>();
}
Loading
Loading