Fix for Shell tab visibility not updating when navigating back multiple pages#34280
Fix for Shell tab visibility not updating when navigating back multiple pages#34280BagavathiPerumal wants to merge 7 commits intodotnet:inflight/currentfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34280Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34280" |
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Shell tab bar visibility issue where Shell.SetTabBarIsVisible() called in OnAppearing() of the root tab was ignored when navigating back multiple levels using GoToAsync("../.."). The root cause was a timing mismatch in OnPopToRootAsync(): the platform navigation was committed before OnAppearing() was fired on the root page, so UI updates made in OnAppearing() were applied too late.
Changes:
- Reorders the lifecycle event calls in
OnPopToRootAsync()so thatPresentedPageAppearing()runs beforeInvokeNavigationRequest(), aligning multi-level pop behavior with single-level pop. - Skips calling
SendDisappearing()on the top page during the post-navigation cleanup loop (since it was already handled byPresentedPageDisappearing()). - Adds a UI test (
Issue33351) with snapshot baselines for Android, iOS, and iOS 26 to verify the fix.
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Controls/src/Core/Shell/ShellSection.cs |
Core fix: reorders PresentedPageAppearing() to run before InvokeNavigationRequest() in OnPopToRootAsync; refines loop to skip SendDisappearing on top page |
src/Controls/tests/TestCases.HostApp/Issues/Issue33351.cs |
HostApp test page with Shell, TabBar, three pages, and navigation reproducing the issue |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33351.cs |
NUnit UI test that exercises the multi-level pop-to-root and verifies tab bar visibility via screenshot |
src/Controls/tests/TestCases.iOS.Tests/snapshots/ios/TabBarVisibilityAfterMultiLevelPopToRoot.png |
Baseline snapshot for iOS |
src/Controls/tests/TestCases.iOS.Tests/snapshots/ios-26/TabBarVisibilityAfterMultiLevelPopToRoot.png |
Baseline snapshot for iOS 26 |
src/Controls/tests/TestCases.Android.Tests/snapshots/android/TabBarVisibilityAfterMultiLevelPopToRoot.png |
Baseline snapshot for Android |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33351.cs
Outdated
Show resolved
Hide resolved
🤖 AI Summary📊 Expand Full Review🔍 Pre-Flight — Context & Validation📝 Review Session — fix-33351-Changes committed and updated windows and mac snapshot. ·
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34280 | Reorder PresentedPageAppearing() to run before InvokeNavigationRequest() in OnPopToRootAsync; skip duplicate SendDisappearing on top page |
⏳ PENDING (Gate) | ShellSection.cs (+15/-3), Issue33351.cs (HostApp, +160), Issue33351.cs (SharedTests, +35), 5 snapshot PNGs |
Original PR |
🚦 Gate — Test Verification
📝 Review Session — fix-33351-Changes committed and updated windows and mac snapshot. · a222541
Result: ✅ PASSED
Platform: ios
Mode: Full Verification
- Tests FAIL without fix ✅
- Tests PASS with fix ✅
Test: Issue33351.TabBarVisibilityAfterMultiLevelPopToRoot
Fix file verified: src/Controls/src/Core/Shell/ShellSection.cs
🔧 Fix — Analysis & Comparison
📝 Review Session — fix-33351-Changes committed and updated windows and mac snapshot. · a222541
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Early if (IsVisibleSection) CurrentItem?.SendAppearing() before InvokeNavigationRequest (minimal 2-line add, keeps PresentedPageAppearing at end) |
✅ PASS | ShellSection.cs (+2) |
Works but fires appearing twice; simpler than PR |
| 2 | try-fix | Move _navStack reset + PresentedPageAppearing() before InvokeNavigationRequest (mirrors OnPopAsync, no cleanup loop changes) |
✅ PASS | ShellSection.cs (+1/-1) |
Clean reorder; doesn't handle disappearing |
| 3 | try-fix | Deferred Dispatcher.Dispatch + synchronous Handler.UpdateValue after PresentedPageAppearing |
❌ FAIL | ShellSection.cs (+13) |
0.81% snapshot mismatch; post-navigation updates too late |
| 4 | try-fix | Reset _navStack, then InvokeNavigationRequest, then PresentedPageAppearing() before await args.Task |
✅ PASS | ShellSection.cs (+1/-1) |
Fires appearing after request dispatched but before await |
| 5 | try-fix | PresentedPageDisappearing() + if (IsVisibleSection) CurrentItem?.SendAppearing() before InvokeNavigationRequest |
✅ PASS | ShellSection.cs (+3) |
Good intermediate approach |
| 6 | try-fix | Synchronous Shell.Current?.Handler?.UpdateValue(TabBarIsVisibleProperty) after navigation and cleanup loop |
❌ FAIL | ShellSection.cs (+1) |
Same 0.81% mismatch; post-navigation approach fundamentally flawed |
| 7 | try-fix | SendDisappearing on ALL old pages + _navStack[0].SendAppearing() before InvokeNavigationRequest (bypasses Shell machinery) |
❌ FAIL | ShellSection.cs (+5/-5) |
0.81% mismatch; raw Page.Send* without Shell lifecycle machinery doesn't update tab bar |
| PR | PR #34280 | Full reorder: PresentedPageDisappearing() + PresentedPageAppearing() before InvokeNavigationRequest; add explanatory comment; skip duplicate SendDisappearing on top page in cleanup loop |
✅ PASS (Gate) | ShellSection.cs (+15/-3) |
Original PR; most complete solution |
Cross-Pollination
Round 2:
| Model | Response |
|---|---|
| claude-sonnet-4.6 | NO NEW IDEAS |
| claude-opus-4.6 | NO NEW IDEAS |
| gpt-5.2 | NEW IDEA: post-navigation force visual-state refresh (→ Attempt 6, FAIL) |
| gpt-5.3-codex | NEW IDEA: synchronous UpdateValue post-navigation (→ Attempt 6, FAIL) |
| gemini-3-pro-preview | NEW IDEA: UpdateValue immediately after clearing _navStack (→ Attempt 7, FAIL) |
Round 3:
| Model | Response |
|---|---|
| claude-sonnet-4.6 | NO NEW IDEAS |
| claude-opus-4.6 | NO NEW IDEAS |
| gpt-5.2 | NEW IDEA: pre-navigation Handler.UpdateValue (variant of post-navigation - not run, covered by pattern) |
| gpt-5.3-codex | NEW IDEA: SendDisappearing on full old stack + SendAppearing on root (→ Attempt 7, FAIL) |
| gemini-3-pro-preview | NO NEW IDEAS |
Round 4:
| Model | Response |
|---|---|
| gpt-5.2 | NEW IDEA: Switch Shell current state via UpdateCurrentState/OnCurrentPageChanged (not run - max rounds exceeded) |
| gpt-5.3-codex | NEW IDEA: Route multi-level pop through single-pop pipeline (not run - major refactor, animation regressions likely) |
Exhausted: Yes (exceeded 3-round maximum; remaining ideas are architectural refactors or variations of failed approaches)
Key Learning
The fix requires Shell lifecycle machinery (PresentedPageAppearing/CurrentItem.SendAppearing) to fire before InvokeNavigationRequest. This is because iOS reads HidesBottomBarWhenPushed on the root view controller when PopToRootViewController is called — if OnAppearing() (which calls SetTabBarIsVisible) hasn't fired yet, the platform commits with stale state. Post-navigation updates (even synchronous UpdateValue) cannot override the already-committed native transition.
Selected Fix
Selected Fix: PR's fix — it is the most complete solution:
- Correctly handles both disappearing (
PresentedPageDisappearing) AND appearing (PresentedPageAppearing) lifecycle in correct order - Fixes the duplicate
SendDisappearingbug in the cleanup loop (top page was already handled byPresentedPageDisappearing) - Includes explanatory code comment documenting the intentional ordering
- Aligns multi-level pop behavior with single-level pop (
OnPopAsync) - Tested on all 4 platforms by the author
Attempt 1 is simpler (2 lines) but fires OnAppearing twice (idempotent but inelegant). Attempt 2 is clean but doesn't handle the disappearing side. The PR's fix handles all aspects correctly.
📋 Report — Final Recommendation
📝 Review Session — fix-33351-Changes committed and updated windows and mac snapshot. · a222541
✅ Final Recommendation: APPROVE
Summary
PR #34280 fixes a lifecycle timing bug in ShellSection.OnPopToRootAsync() where Shell.SetTabBarIsVisible() was ignored when navigating back multiple levels with GoToAsync("../.."). The fix is correct, well-documented, tested on all 4 platforms, and independently validated by Gate (tests fail without fix, pass with fix). Five alternative approaches were explored — 4 passed — and the PR's implementation is the most complete.
Root Cause
In OnPopToRootAsync(), InvokeNavigationRequest(args) was called first, which synchronously triggers PopToRootViewController on iOS (and equivalent on other platforms). iOS reads HidesBottomBarWhenPushed at this moment to determine tab bar visibility during the animation. Only after the platform committed the navigation did PresentedPageAppearing() → OnAppearing() → Shell.SetTabBarIsVisible() fire — too late to influence the already-committed UI.
This mismatch did not occur with single-level pop (GoToAsync("..")) because OnPopAsync already called PresentedPageAppearing() before InvokeNavigationRequest().
Fix Quality: ✅ Excellent
The PR's fix is the most complete of all explored alternatives:
| Criterion | Assessment |
|---|---|
| Correctness | ✅ Lifecycle events fire before platform navigation, per the proven pattern |
| Completeness | ✅ Handles both disappearing (PresentedPageDisappearing) AND appearing (PresentedPageAppearing) |
| Loop fix | ✅ Removes duplicate SendDisappearing on top page in cleanup loop |
| Documentation | ✅ Clear code comment explaining intentional ordering and why |
| Consistency | ✅ Aligns multi-level pop with single-level pop behavior |
| Test coverage | ✅ UI test + snapshots for iOS, iOS-26, Android, Mac, Windows |
Alternative Fixes Explored (Try-Fix)
4 of 7 try-fix attempts passed, all converging on the same pattern (lifecycle before InvokeNavigationRequest). The PR's fix is the best because it handles all aspects: disappearing + appearing + cleanup loop fix + explanatory comment.
Post-navigation approaches (Attempts 3, 6, 7) all failed with 0.81% snapshot mismatch, confirming that lifecycle events must fire before InvokeNavigationRequest.
PR Finalize: Title & Description
Title Assessment:
- Current: "Fix for Shell tab visibility not updating when navigating back multiple pages"
- Suggested:
Shell: Fix tab bar visibility ignored on multi-level GoToAsync pop-to-root - The current title is accurate but "Fix for" is weak. The suggested title is more searchable and captures the specific navigation pattern (
GoToAsync("../..")).
Description Assessment:
- ✅ Root cause explained clearly
- ✅ Fix approach described accurately
- ✅ Platforms tested listed
- ✅ Issue linked correctly
- ❌ Missing required NOTE block at top of description (required by repo template)
- ❌ No "What NOT to Do" section (post-navigation UpdateValue approaches fail — worth documenting)
Code Review Findings
✅ Looks Good:
- Core fix ordering is correct and proven by Gate + 4 independent try-fix passes
- Code comment accurately explains the intentional lifecycle ordering deviation
- Cleanup loop fix correctly skips
SendDisappearingon the top page (already handled byPresentedPageDisappearing()) - HostApp test faithfully reproduces the bug: Tab hides on navigation, must re-show on pop-to-root
- Shared test navigates to Page2 then
GoToAsync("../..")and verifies tab bar viaVerifyScreenshot() - Snapshots provided for all 4 platforms (iOS, iOS-26, Android, Mac, Windows)
🟡 Minor Observations (non-blocking):
- Test:
App.Tap("Tab 1")at start — Tapping the already-selected tab at the start of the test is reasonable for ensuring correct initial state, but if Shell firesNavigatingon a no-op tab tap,MyTab.OnShellNavigatingwould hide the tab bar. In practice, Shell does not triggerNavigatingfor re-selecting the same tab, so this is safe. Low risk. - Test relies solely on
VerifyScreenshot()for visual validation of tab bar — no programmatic assertion. This is acceptable for Shell visual state but means test is sensitive to visual regressions unrelated to tab bar (e.g., text rendering differences). Consider addingretryTimeouttoVerifyScreenshot()if flakiness is observed on CI. MyTab.OnAppearingcallsShell.SetTabBarIsVisible(this.Parent, true)—this.Parentis the TabBar. This works, but it's less conventional thanShell.SetTabBarIsVisible(Shell.Current, true). Not a problem for this PR.
🔴 No critical issues.
What NOT to Do (Post-Navigation Approaches)
- ❌ Don't call
Handler.UpdateValue(TabBarIsVisible)after navigation — iOS has already committed the native transition by the time this runs. Same 0.81% snapshot mismatch regardless of sync or async dispatch. - ❌ Don't call
Page.SendDisappearing()/Page.SendAppearing()directly — These bypass Shell lifecycle machinery. The platform handler reads tab bar state through the Shell'sPresentedPageAppearing/CurrentItem.SendAppearingpath, not raw Page lifecycle. - ❌ Don't use
Dispatcher.Dispatchto defer tab bar updates — The native UITabBarController reads visibility during the animation start frame, not after.
Recommendation
✅ APPROVE — The fix is correct, complete, well-documented, independently validated, and the most comprehensive solution among all alternatives explored. Add the NOTE block and consider the title suggestion before merge.
📋 Expand PR Finalization Review
Title: ✅ Good
Current: Fix for Shell tab visibility not updating when navigating back multiple pages
Description: ✅ Good
- Does not follow the
[Platform] Component: What changedconvention - "Fix for" prefix is verbose
- "multiple pages" is vague — "multi-level pop-to-root" is more precise and searchable
- No platform prefix (the fix touches cross-platform Shell code, but the issue was iOS/Android only)
✨ Suggested PR Description
[!NOTE]
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Root cause
The issue occurs because of the execution order in ShellSection.OnPopToRootAsync(). The original code called InvokeNavigationRequest() first, which sent the navigation request to the native platform handlers. The platform committed the UI changes immediately. Only after this did PresentedPageAppearing() fire, which calls OnAppearing() on the root page. Since Shell.SetTabBarIsVisible() is typically invoked inside OnAppearing(), it ran too late — after the UI had already been rendered — causing the tab bar visibility change to be ignored.
This timing mismatch did not occur with single-level navigation (GoToAsync("..")) because that code path already called PresentedPageAppearing() before InvokeNavigationRequest().
Additionally, the original OnPopToRootAsync() was missing a PresentedPageDisappearing() call entirely, and the cleanup loop was calling SendDisappearing() on the top page even though it was never handled — leading to duplicate or inconsistent lifecycle events.
Description of Change
Reordered the execution sequence in ShellSection.OnPopToRootAsync() to match the behavior of single-level pop:
- Call
PresentedPageDisappearing()on the currently presented (top) page before modifying the nav stack. - Update
_navStackto point to the root. - Call
PresentedPageAppearing()to fireOnAppearing()on the root page — before the platform navigation is committed. - Call
InvokeNavigationRequest()so the platform reads the already-updated Shell state (including tab bar visibility) when it finalizes the animation.
Also refined the cleanup loop to skip SendDisappearing() on the top page, since step 1 already handles it, avoiding duplicate lifecycle events on intermediate pages.
Key Technical Details
Execution order (new):
PresentedPageDisappearing() ← fires OnDisappearing on top page
_navStack = [null] ← root page is now "presented"
PresentedPageAppearing() ← fires OnAppearing on root page (tab bar visibility set here)
InvokeNavigationRequest() ← platform reads Shell state and commits native navigation
await args.Task ← wait for animation to complete
cleanup loop ← SendDisappearing on intermediate pages only, RemovePage all
Why PresentedPageAppearing() fires before the animation completes:
This is intentional. Shell-level state (e.g., tab bar visibility via attached properties) must be set before the platform reads it during animation. The root page is not yet visually presented at that moment, but the Shell's logical state is correct. This matches the established single-level Pop pattern.
Issues Fixed
Fixes #33351
Platforms Tested
- Windows
- Mac
- iOS
- Android
Code Review: ✅ Passed
Code Review — PR #34280
File changed: src/Controls/src/Core/Shell/ShellSection.cs (+15/-3)
Test files: Issue33351.cs (HostApp + Shared), snapshots for all 4 platforms
✅ Core Fix — Correct and Well-Commented
The reordering in OnPopToRootAsync() is correct. The new sequence:
PresentedPageDisappearing() → _navStack reset → PresentedPageAppearing() → InvokeNavigationRequest()
matches the established pattern in both OnPopAsync() (lines 808–812) and OnPushAsync() (lines 899–903), making OnPopToRootAsync() consistent with the rest of the navigation API. The inline comment is detailed and explains the timing trade-off clearly.
🟡 Suggestions
1. SendDisappearing() skipped for top page — RemovePage() still called for all
File: src/Controls/src/Core/Shell/ShellSection.cs (line 868–874)
for (int i = 1; i < oldStack.Count; i++)
{
// Send disappearing only to intermediate pages (top page already handled by PresentedPageDisappearing)
if (i < oldStack.Count - 1)
oldStack[i].SendDisappearing();
RemovePage(oldStack[i]);
}This is correct: RemovePage() is still called for the top page, only SendDisappearing() is skipped. The comment could be slightly clearer that RemovePage() still applies to all pages — but this is cosmetic.
2. Event subscription never unsubscribed in HostApp test
File: src/Controls/tests/TestCases.HostApp/Issues/Issue33351.cs (line ~38–44)
class MyTab : ShellContent
{
protected override void OnAppearing()
{
base.OnAppearing();
if (this.Parent != null)
Shell.SetTabBarIsVisible(this.Parent, true);
Shell.Current.Navigating -= OnShellNavigating;
Shell.Current.Navigating += OnShellNavigating;
}
// No OnDisappearing override to unsubscribe
}The Navigating event is unsubscribed-then-resubscribed each time OnAppearing() fires (preventing duplicate subscriptions on the same instance), but there is no OnDisappearing() override to clean up when the tab is no longer active. If the user switches to Tab 2 and back, the subscription will be re-added.
The remove-then-add pattern (-= / +=) mitigates this to one active subscription per instance, so it won't cause duplicate handler invocations in practice. However, the handler will remain subscribed even when the tab is not visible, which could lead to Shell.SetTabBarIsVisible being called for a tab that isn't the current one.
Severity: Low — this is test-only code and the scenario is constrained. No action required for this PR, but worth noting for future test maintainability.
3. Trailing whitespace in test file
File: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33351.cs
Several lines after App.Tap(...) calls have trailing whitespace characters. Minor style issue, no functional impact.
✅ Looks Good
- All 4 platforms have snapshots — Android, iOS, iOS-26, MacCatalyst, Windows. This is thorough coverage.
- The UI test correctly validates the fix — it navigates two levels deep and then pop-to-roots, then verifies the
TabBarVisibleLabelelement is visible before capturing a screenshot. - The comment in ShellSection.cs is exceptionally clear about the timing trade-off — future maintainers will understand why
PresentedPageAppearing()fires before the native animation completes. - No breaking changes — the change only reorders calls within
OnPopToRootAsync(). External callers are unaffected. - Regression parity — the new ordering matches single-level pop (lines 808–812) and push (lines 899–903), making the overall navigation lifecycle consistent.
Summary
| Severity | Count | Items |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟡 Suggestion | 2 | Event cleanup in test; comment clarity |
| 🟢 Cosmetic | 1 | Trailing whitespace |
| ✅ Positive | 4 | All platforms covered, good comment, no breaking changes, consistent with existing patterns |
Overall: The fix is sound and production-ready. The two suggestions are minor and test-only. Safe to merge once CI passes.
…rm navigation to ensure Shell TabBar visibility is applied correctly during multi-level back navigation.
d311345 to
262fab7
Compare
|
Closing this PR because unwanted code changes were introduced during rebasing. |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Root cause
The issue occurs because of the navigation sequence used in OnPopToRootAsync() where the platform navigation is triggered before the root page’s lifecycle events are fired. When Shell.Current.GoToAsync("../..") is used, the method InvokeNavigationRequest() executes first, which sends the navigation request to the native platform handlers.
The platform then commits the UI changes immediately. Only after this process, the root page’s OnAppearing() method is called. Since Shell.SetTabBarIsVisible() is typically invoked inside OnAppearing(), it runs too late, after the UI has already been rendered, causing the TabBar visibility change to be ignored.
This timing mismatch does not occur with single-level navigation (GoToAsync("..")) because that flow correctly triggers lifecycle events before the platform navigation begins.
Description of Issue Fix
The fix involves reordering the execution sequence within OnPopToRootAsync() so that PresentedPageAppearing() runs before InvokeNavigationRequest(), ensuring OnAppearing() is triggered early enough for updates such as Shell.SetTabBarIsVisible() to be applied before the platform finalizes the navigation UI.
Additionally, the stack cleanup loop was refined to skip calling SendDisappearing() on the top page since it is already handled by PresentedPageDisappearing(), thereby avoiding duplicate lifecycle events while still notifying intermediate pages. This change aligns multi level pop to root behavior with single level pop, providing consistent lifecycle handling without introducing breaking changes.
Tested the behavior in the following platforms.
Issues Fixed
Fixes #33351
Output
33351-BeforeFix.mov
33351-AfterFix.mov