Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,27 @@ public void OnItemsCollectionChanged(object sender, NotifyCollectionChangedEvent
_createdShellContent.Remove(remove);
}

/// <summary>
/// Invalidates the cached item ID for the given <paramref name="shellContent"/>.
/// This causes the existing fragment for that item to be destroyed and a new fragment
/// to be created with the updated page content.
/// </summary>
internal void InvalidateShellContent(ShellContent shellContent)
{
long removeKey = -1;
foreach (var item in _createdShellContent)
{
if (item.Value == shellContent)
{
removeKey = item.Key;
break;
}
}

if (removeKey >= 0)
_createdShellContent.Remove(removeKey);
}

public int CountOverride { get; set; }

public override int ItemCount => _items.Count;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,26 @@ void OnShellContentPropertyChanged(object sender, System.ComponentModel.Property
{
UpdateTabTitle(shellContent);
}
else if (e.PropertyName == ShellContent.ContentProperty.PropertyName && sender is ShellContent changedContent)
{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Android dynamic ShellContent items are not covered by the new Content-change path. HookEvents() subscribes OnShellContentPropertyChanged only for the items present when the renderer is created, and Android's OnItemsCollectionChanged only updates the adapter. Unlike the iOS change, it does not subscribe e.NewItems or unsubscribe e.OldItems. A ShellContent added after renderer creation will therefore never reach this new ContentProperty branch when its Content changes, so its fragment will not be invalidated/recreated on Android. Removed items also remain subscribed until teardown.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[major] Navigation & Shell — This handles ShellContent.Content changes only for items subscribed in HookEvents(), which iterates the initial SectionController.GetItems() set. OnItemsCollectionChanged() updates the adapter but never subscribes e.NewItems, so a ShellContent added to a ShellSection at runtime will not raise this handler when its Content later changes. Concrete Android scenario: dynamically add a tab, navigate to it so ViewPager creates/caches its fragment, then replace that tab's Content; InvalidateShellContent()/SafeNotifyDataSetChanged() never run and the old fragment remains visible. Move the content observation into the adapter or subscribe/unsubscribe added/removed ShellContent items in the collection-changed path.

// The page inside this ShellContent changed — force ViewPager2 to recreate the
// fragment so it picks up the new content.
if (_viewPager?.Adapter is ShellFragmentStateAdapter adapter)
{
adapter.InvalidateShellContent(changedContent);
SafeNotifyDataSetChanged();
Comment thread
devanathan-vaithiyanathan marked this conversation as resolved.

// Keep toolbar state in sync when the active tab's content page is replaced.
if (ShellSection?.CurrentItem == changedContent && _toolbarTracker is not null)
{
var page = ((IShellContentController)changedContent).GetOrCreateContent();
if (page is not null)
{
_toolbarTracker.Page = page;
}
}
}
}
}

void UpdateTabTitle(ShellContent shellContent)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ public override void ViewDidLoad()
ShellSection.PropertyChanged += OnShellSectionPropertyChanged;
ShellSectionController.ItemsCollectionChanged += OnShellSectionItemsChanged;

foreach (var item in ShellSectionController.GetItems())
item.PropertyChanged += OnShellContentPropertyChanged;

_blurView = new UIView();
UIVisualEffect blurEffect = UIBlurEffect.FromStyle(UIBlurEffectStyle.ExtraLight);
_blurView = new UIVisualEffectView(blurEffect);
Expand Down Expand Up @@ -151,6 +154,11 @@ void IDisconnectable.Disconnect()
if (ShellSectionController != null)
ShellSectionController.ItemsCollectionChanged -= OnShellSectionItemsChanged;

var shellContents = ShellSectionController?.GetItems();
if (shellContents != null)
foreach (var item in shellContents)
item.PropertyChanged -= OnShellContentPropertyChanged;

if (_shellContext?.Shell != null)
_shellContext.Shell.PropertyChanged -= HandleShellPropertyChanged;

Expand Down Expand Up @@ -500,6 +508,8 @@ void OnShellSectionItemsChanged(object sender, NotifyCollectionChangedEventArgs
{
foreach (ShellContent oldItem in e.OldItems)
{
oldItem.PropertyChanged -= OnShellContentPropertyChanged;

// if current item is removed will be handled by the currentitem property changed event
// That way the render is swapped out cleanly once the new current item is set
if (_currentContent == oldItem)
Expand All @@ -524,6 +534,8 @@ void OnShellSectionItemsChanged(object sender, NotifyCollectionChangedEventArgs
{
foreach (ShellContent newItem in e.NewItems)
{
newItem.PropertyChanged += OnShellContentPropertyChanged;

if (_renderers.ContainsKey(newItem))
continue;

Expand All @@ -535,6 +547,75 @@ void OnShellSectionItemsChanged(object sender, NotifyCollectionChangedEventArgs
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💡 iOS: UpdateRendererForShellContent has no guard for an in-progress tab-switch animation

If shellContent == _currentContent but _isAnimatingOut != null (a tab-switch animation is in flight), UpdateRendererForShellContent will call oldRenderer.ViewController?.ViewIfLoaded?.RemoveFromSuperview() and DisconnectHandler() on the renderer that is currently being animated. This could cause visual artifacts or a crash.

Low probability since a user would have to change a page's content mid-swipe, but consider adding:

if (_isAnimatingOut != null && _isAnimatingOut == oldRenderer)
    return; // content change during animation — skip; animation completion will handle layout

}

void OnShellContentPropertyChanged(object sender, System.ComponentModel.PropertyChangedEventArgs e)
{
if (_isDisposed)
return;

if (e.PropertyName == ShellContent.ContentProperty.PropertyName && sender is ShellContent shellContent)
{
// INotifyPropertyChanged.PropertyChanged fires AFTER the BindableProperty's
// propertyChanged callback (OnContentChanged) in BindableObject.SetValueCore.
// For a Page, ContentCache is already updated by the time we get here.
if (shellContent.Content is Page newPage)
{
// Content is a Page — available immediately, no deferral needed.
UpdateRendererForShellContent(shellContent, newPage);
}
else if (shellContent.Content is DataTemplate)
{
// Content is a DataTemplate — ContentCache is populated by OnContentChanged,
// which has already run before this PropertyChanged notification. However,
// defer to the next run-loop iteration to ensure any async post-processing
// in GetOrCreateContent() sees a fully settled ContentCache.
BeginInvokeOnMainThread(() =>
{
if (_isDisposed)
return;
var page = ((IShellContentController)shellContent).GetOrCreateContent();
UpdateRendererForShellContent(shellContent, page);
});
}
}
}

void UpdateRendererForShellContent(ShellContent shellContent, Page newPage)
{
if (newPage == null)
return;

if (!_renderers.TryGetValue(shellContent, out var oldRenderer))
return;

// If the existing renderer is already showing the new page, nothing to do.
if (oldRenderer.VirtualView == newPage)
return;

bool isCurrentContent = shellContent == _currentContent;

// Remove the old renderer
if (isCurrentContent)
oldRenderer.ViewController?.ViewIfLoaded?.RemoveFromSuperview();

oldRenderer.ViewController?.RemoveFromParentViewController();
oldRenderer.DisconnectHandler();
_renderers.Remove(shellContent);

// Create a new renderer for the updated page
var renderer = SetPageRenderer(newPage, shellContent);
AddChildViewController(renderer.ViewController);

if (isCurrentContent)
{
_containerArea.AddSubview(renderer.ViewController.View);
renderer.ViewController.View.Frame = _containerArea.Bounds;
UpdateAdditionalSafeAreaInsets(renderer);

if (_tracker != null)
_tracker.Page = newPage;
}
}

IPlatformViewHandler SetPageRenderer(Page page, ShellContent shellContent)
{
page.Handler?.DisconnectHandler();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public override void SetVirtualView(Maui.IElement view)
{
if (_shellSection != null)
{
((IShellSectionController)_shellSection).RemoveDisplayedPageObserver(this);
((IShellSectionController)_shellSection).NavigationRequested -= OnNavigationRequested;

((IShellSectionController)_shellSection).ItemsCollectionChanged -= OnItemsCollectionChanged;
Expand Down Expand Up @@ -84,7 +85,6 @@ public override void SetVirtualView(Maui.IElement view)
if (_shellSection != null)
{
((IShellSectionController)_shellSection).NavigationRequested += OnNavigationRequested;

((IShellSectionController)_shellSection).ItemsCollectionChanged += OnItemsCollectionChanged;

var shell = _shellSection.FindParentOfType<Shell>() as IShellController;
Expand All @@ -93,9 +93,36 @@ public override void SetVirtualView(Maui.IElement view)
_lastShell = new WeakReference(shell);
shell.AddAppearanceObserver(this, _shellSection);
}

// AddDisplayedPageObserver immediately invokes the callback with the current page,
// but at that point PendingNavigationTask is already set from MapCurrentItem
// (which runs via base.SetVirtualView above), so it is safely skipped.
((IShellSectionController)_shellSection).AddDisplayedPageObserver(this, OnDisplayedPageChanged);
}
}

// Called when ShellSection.DisplayedPage changes — covers both content changes and
// navigation pushes. We only act on content changes (stack depth == 1, no pending nav).
void OnDisplayedPageChanged(Page page)
{
if (VirtualView is null)
return;

// Push/pop navigation is handled by OnNavigationRequested; skip those cases.
if (VirtualView.Stack.Count > 1)
return;

// Tab switches are handled by MapCurrentItem which sets PendingNavigationTask first
// (because the property mapper fires before the propertyChanged callback that calls
// UpdateDisplayedPage). Skip when another navigation is already in flight.
if (VirtualView.PendingNavigationTask != null)
return;

// ContentCache has been updated by OnContentChanged at this point, so
// SyncNavigationStack will correctly pick up the new page via GetOrCreateContent().
SyncNavigationStack(false, null);
}

void OnNavigationRequested(object? sender, NavigationRequestedEventArgs e)
{
SyncNavigationStack(e.Animated, e);
Expand Down Expand Up @@ -159,6 +186,9 @@ protected override void ConnectHandler(WFrame platformView)

protected override void DisconnectHandler(WFrame platformView)
{
if (_shellSection != null)
((IShellSectionController)_shellSection).RemoveDisplayedPageObserver(this);

_navigationManager?.Disconnect(VirtualView, platformView);
base.DisconnectHandler(platformView);
}
Expand Down
61 changes: 61 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue12669.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
namespace Maui.Controls.Sample.Issues;

[Issue(IssueTracker.Github, 12669, "Changing Content property of ShellContent doesn't change visual content", PlatformAffected.All)]
public class Issue12669 : TestShell
{
protected override void Init()
{
FlyoutBehavior = FlyoutBehavior.Disabled;

var shellContent = new ShellContent { Title = "Test" };

var changeButton = new Button
{
Text = "Change Content",
AutomationId = "ChangeContentButton"
};

shellContent.Content = new ContentPage
{
Content = new VerticalStackLayout
{
VerticalOptions = LayoutOptions.Center,
HorizontalOptions = LayoutOptions.Center,
Children =
{
new Label
{
Text = "Original Content",
AutomationId = "OriginalContent"
},
changeButton
}
}
};

changeButton.Clicked += (s, e) =>
{
shellContent.ContentTemplate = null;
shellContent.Content = new ContentPage
{
Content = new VerticalStackLayout
{
VerticalOptions = LayoutOptions.Center,
HorizontalOptions = LayoutOptions.Center,
Children =
{
new Label
{
Text = "Content Changed",
AutomationId = "NewContent"
}
}
}
};
};

var tab = new Tab { Title = "Main" };
tab.Items.Add(shellContent);
Items.Add(tab);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues;
public class Issue12669 : _IssuesUITest
{
public Issue12669(TestDevice device) : base(device) { }

public override string Issue => "Changing Content property of ShellContent doesn't change visual content";

[Test]
[Category(UITestCategories.Shell)]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💡 Test coverage gaps — three uncovered scenarios:

  1. Non-current tab: The test only changes content on the active (current) tab. The fix also claims to update background tabs (isCurrentContent == false path on iOS, fragment invalidation on Android). A second test case that changes content on an inactive tab and then switches to it would give much higher confidence.

  2. DataTemplate-based content: The iOS code has a special BeginInvokeOnMainThread deferral path for Content is DataTemplate. This path is not exercised by the current test.

  3. Windows Stack.Count > 1 skip: The OnDisplayedPageChanged guard skips when a navigation stack is deeper than root. A test that pushes a page then changes ShellContent.Content would confirm no regression.

public void ShellContentShouldUpdateWhenContentPropertyChanges()
{
App.WaitForElement("OriginalContent");
App.WaitForElement("ChangeContentButton");

App.Tap("ChangeContentButton");

// After clicking, the ShellContent.Content is changed to a new page.
// The visual should update to show the new content.
App.WaitForElement("NewContent");
var labelText = App.FindElement("NewContent").GetText();
Assert.That(labelText, Is.EqualTo("Content Changed"));
}
}
Loading