Skip to content

Commit 591ea07

Browse files
kubafloPureWeen
kubaflo
and
PureWeen
authored
[iOS] Fix Shell long-press back button not triggering navigation events (#33380)
### Description of Change Fixes long-press back button navigation not triggering `OnAppearing` and other navigation events in Shell. **Problem:** Test expected `OnAppearing count: 2`, got `OnAppearing count: 1` **Root cause:** PR #29825 replaced `SendPop()` with manual stack synchronization (`SyncStackDownTo()`), which doesn't trigger navigation events. **Fix:** Simplified `DidPopItem` to use stack-sync detection: - Stacks in sync → Shell already handled pop → return early - Stacks out of sync → user-initiated (long-press) → call `SendPop()` **Key insight:** Tab tap updates Shell's stack BEFORE `DidPopItem` is called, but iOS long-press pops directly without notifying Shell first. **Regression chain:** | PR | What happened | |-----|---------------| | #24003 | Fixed #23892 with `_popRequested` flag | | #29825 | Removed flag, added `SyncStackDownTo()` - **broke long-press** | | #32456 | Added null checks, maintained broken state | | #33380 | **This PR** - simplified fix using stack-sync detection | **Removed:** `SyncStackDownTo()` method (44 lines) **What to avoid:** Don't remove stack count comparison - distinguishes user vs programmatic navigation. ### Issues Fixed Fixes #33379 **Related:** #23892, #29798 (verified not regressed ✅) --------- Co-authored-by: Shane Neuville <[email protected]>
1 parent 8eab1e5 commit 591ea07

File tree

3 files changed

+230
-62
lines changed

3 files changed

+230
-62
lines changed

‎.github/agent-pr-session/pr-33380.md‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
# PR Review: #33380 - [PR agent] Issue23892.ShellBackButtonShouldWorkOnLongPress - test fix
2+
3+
**Date:** 2026-01-07 | **Issue:** [#33379](https://github.com/dotnet/maui/issues/33379) | **PR:** [#33380](https://github.com/dotnet/maui/pull/33380)
4+
5+
## ✅ Final Recommendation: APPROVE
6+
7+
| Phase | Status |
8+
|-------|--------|
9+
| Pre-Flight | ✅ COMPLETE |
10+
| 🧪 Tests | ✅ COMPLETE |
11+
| 🚦 Gate | ✅ PASSED |
12+
| 🔧 Fix | ✅ COMPLETE |
13+
| 📋 Report | ✅ COMPLETE |
14+
15+
---
16+
17+
<details>
18+
<summary><strong>📋 Issue Summary</strong></summary>
19+
20+
**Issue #33379**: The UI test `Issue23892.ShellBackButtonShouldWorkOnLongPress` started failing after PR #32456 was merged.
21+
22+
**Test Expectation**: `OnAppearing count: 2`
23+
**Test Actual**: `OnAppearing count: 1`
24+
25+
**Original Issue #23892**: Using long-press navigation on the iOS back button in Shell does not update `Shell.Current.CurrentPage`. The `Navigated` and `Navigating` events don't fire.
26+
27+
**Platforms Affected:**
28+
- [x] iOS
29+
- [ ] Android
30+
- [ ] Windows
31+
- [ ] MacCatalyst
32+
33+
</details>
34+
35+
<details>
36+
<summary><strong>🔍 Deep Regression Analysis - Full Timeline</strong></summary>
37+
38+
## The Regression Chain
39+
40+
This PR addresses a **double regression** - the same functionality was broken twice by subsequent PRs.
41+
42+
### Timeline of Changes to `ShellSectionRenderer.cs`
43+
44+
| Date | PR | Purpose | Key Change | Broke Long-Press? |
45+
|------|-----|---------|------------|-------------------|
46+
| Feb 2025 | #24003 | Fix #23892 (long-press back) | Added `_popRequested` flag + `DidPopItem` | ✅ Fixed it |
47+
| Jul 2025 | #29825 | Fix #29798/#30280 (tab blank issue) | **Removed** `_popRequested`, expanded `DidPopItem` with manual sync | ❌ **Broke it** |
48+
| Jan 2026 | #32456 | Fix #32425 (navigation hang) | Added null checks, changed `ElementForViewController` | ❌ Maintained broken state |
49+
50+
### PR #24003 - The Original Fix (Feb 2025)
51+
52+
**Problem solved**: Long-press back button didn't trigger navigation events.
53+
54+
**Solution**: Added `_popRequested` flag to distinguish:
55+
- **User-initiated navigation** (long-press): Call `SendPop()` → triggers `GoToAsync("..")` → fires `OnAppearing`
56+
- **Programmatic navigation** (code): Skip `SendPop()` to avoid double-navigation
57+
58+
**Key code added**:
59+
```csharp
60+
bool _popRequested;
61+
62+
bool DidPopItem(UINavigationBar _, UINavigationItem __)
63+
=> _popRequested || SendPop(); // If not requested, call SendPop
64+
```
65+
66+
### PR #29825 - The First Regression (Jul 2025)
67+
68+
**Problem solved**: Tab becomes blank after specific navigation pattern (pop via tab tap, then navigate again, then back).
69+
70+
**What went wrong**: The PR author expanded `DidPopItem` with manual stack synchronization logic (`_shellSection.SyncStackDownTo()`) and **removed the `_popRequested` flag entirely**.
71+
72+
**Result**: `DidPopItem` now ALWAYS does manual sync, never calls `SendPop()` for user-initiated navigation. Long-press navigation stopped triggering `OnAppearing`.
73+
74+
**Why the test didn't catch it**: Unclear - possibly the test wasn't run or was flaky at the time.
75+
76+
### PR #32456 - Maintained the Broken State (Jan 2026)
77+
78+
**Problem solved**: Navigation hangs after rapidly opening/closing pages (iOS 26 specific).
79+
80+
**What it did**: Added null checks to prevent crashes in `DidPopItem` and changed `ElementForViewController` pattern matching.
81+
82+
**Maintained the regression**: The PR kept the broken `DidPopItem` logic from #29825 (no `_popRequested` flag).
83+
84+
**This triggered the test failure**: When #32456 merged to `inflight/candidate`, the existing `Issue23892` test started failing.
85+
86+
</details>
87+
88+
<details>
89+
<summary><strong>📁 Files Changed</strong></summary>
90+
91+
| File | Type | Changes |
92+
|------|------|---------|
93+
| `src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs` | Fix | -20 lines (simplified) |
94+
| `src/Controls/src/Core/Shell/ShellSection.cs` | Fix | -44 lines (removed `SyncStackDownTo`) |
95+
96+
**Net change:** -49 lines (code reduction)
97+
98+
</details>
99+
100+
<details>
101+
<summary><strong>💬 PR Discussion Summary</strong></summary>
102+
103+
**Key Comments:**
104+
- Issue #33379 was filed by @sheiksyedm pointing to the test failure after #32456 merged
105+
- @kubaflo (author of both #32456 and #33380) created this fix
106+
107+
**Reviewer Feedback:**
108+
- None yet
109+
110+
**Disagreements to Investigate:**
111+
| File:Line | Reviewer Says | Author Says | Status |
112+
|-----------|---------------|-------------|--------|
113+
| (none) | | | |
114+
115+
**Author Uncertainty:**
116+
- None expressed
117+
118+
</details>
119+
120+
<details>
121+
<summary><strong>🧪 Tests</strong></summary>
122+
123+
**Status**: ✅ COMPLETE
124+
125+
- [x] PR includes UI tests (existing test from #24003)
126+
- [x] Tests reproduce the issue
127+
- [x] Tests follow naming convention (`IssueXXXXX`) ✅
128+
129+
**Test Files:**
130+
- HostApp: `src/Controls/tests/TestCases.HostApp/Issues/Issue23892.cs`
131+
- NUnit: `src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue23892.cs`
132+
133+
</details>
134+
135+
<details>
136+
<summary><strong>🚦 Gate - Test Verification</strong></summary>
137+
138+
**Status**: ✅ PASSED
139+
140+
- [x] Tests PASS with fix
141+
142+
**Test Run:**
143+
```
144+
Platform: iOS
145+
Test Filter: FullyQualifiedName~Issue23892
146+
Result: SUCCESS ✅
147+
```
148+
149+
**Result:** PASSED ✅ - The `Issue23892.ShellBackButtonShouldWorkOnLongPress` test now passes with the PR's fix.
150+
151+
</details>
152+
153+
<details>
154+
<summary><strong>🔧 Fix Candidates</strong></summary>
155+
156+
**Status**: ✅ COMPLETE
157+
158+
| # | Source | Approach | Test Result | Files Changed | Notes |
159+
|---|--------|----------|-------------|---------------|-------|
160+
| 1 | try-fix | Simplified `DidPopItem`: Always call `SendPop()` when stacks are out of sync | ✅ PASS (Issue23892 + Issue29798 + Issue21119) | `ShellSectionRenderer.cs` (-17, +6) | **Simpler AND works!** |
161+
| PR | PR #33380 (original) | Restore `_popRequested` flag + preserve manual sync from #29825/#32456 | ✅ PASS (Gate) | `ShellSectionRenderer.cs` (+11) | Superseded by update |
162+
| PR | PR #33380 (updated) | **Adopted try-fix #1** - Stack sync detection, removed `SyncStackDownTo` | ✅ PASS (CI pending) | `ShellSectionRenderer.cs`, `ShellSection.cs` (-49 net) | **CURRENT - matches recommendation** |
163+
164+
**Update (2026-01-08):** Developer @kubaflo adopted the simpler approach recommended in try-fix #1.
165+
166+
**Exhausted:** Yes
167+
**Selected Fix:** PR #33380 (updated) - Now implements the recommended simpler approach
168+
169+
</details>
170+
171+
---
172+
173+
## 📋 Final Report
174+
175+
### Recommendation: ✅ APPROVE
176+
177+
**Update (2026-01-08):** Developer @kubaflo has adopted the recommended simpler approach.
178+
179+
### Changes Made by Developer
180+
181+
The PR now implements exactly the simplified stack-sync detection approach:
182+
183+
**ShellSectionRenderer.cs** - Simplified `DidPopItem`:
184+
```csharp
185+
bool DidPopItem(UINavigationBar _, UINavigationItem __)
186+
{
187+
if (_shellSection?.Stack is null || NavigationBar?.Items is null)
188+
return true;
189+
190+
// If stacks are in sync, nothing to do
191+
if (_shellSection.Stack.Count == NavigationBar.Items.Length)
192+
return true;
193+
194+
// Stacks out of sync = user-initiated navigation
195+
return SendPop();
196+
}
197+
```
198+
199+
**ShellSection.cs** - Removed `SyncStackDownTo` method (44 lines deleted)
200+
201+
### Why This Approach Works
202+
203+
| Scenario | What Happens |
204+
|----------|--------------|
205+
| **Tab tap pop** | Shell updates stack BEFORE `DidPopItem` → stacks ARE in sync → returns early (no `SendPop()`) |
206+
| **Long-press back** | iOS pops directly → Shell stack NOT updated → stacks out of sync → calls `SendPop()` |
207+
208+
### Benefits of Updated PR
209+
210+
| Aspect | Before (Original PR) | After (Updated PR) |
211+
|--------|---------------------|-------------------|
212+
| Lines changed | +11 | **-49 net** |
213+
| New fields | `_popRequested` bool | **None (stateless)** |
214+
| Complexity | State tracking | **Simple sync check** |
215+
| `SyncStackDownTo` | Preserved | **Removed** |
216+
217+
### Conclusion
218+
219+
The PR now:
220+
- ✅ Fixes Issue #33379 (long-press back navigation)
221+
- ✅ Uses the simpler stateless approach
222+
- ✅ Removes 49 lines of code
223+
- ✅ No new state tracking required
224+
- ⏳ Pending CI verification
225+
226+
**Approve once CI passes.**u

‎src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs‎

Lines changed: 4 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -108,35 +108,15 @@ public bool ShouldPopItem(UINavigationBar _, UINavigationItem __)
108108
[Internals.Preserve(Conditional = true)]
109109
bool DidPopItem(UINavigationBar _, UINavigationItem __)
110110
{
111-
// Check for null references
112111
if (_shellSection?.Stack is null || NavigationBar?.Items is null)
113112
return true;
114113

115-
// Check if stacks are in sync
114+
// If stacks are in sync, nothing to do
116115
if (_shellSection.Stack.Count == NavigationBar.Items.Length)
117116
return true;
118117

119-
var pages = _shellSection.Stack.ToList();
120-
121-
// Ensure we have enough pages and navigation items
122-
if (pages.Count == 0 || NavigationBar.Items.Length == 0)
123-
return true;
124-
125-
// Bounds check: ensure we have a valid index for pages array
126-
int targetIndex = NavigationBar.Items.Length - 1;
127-
if (targetIndex < 0 || targetIndex >= pages.Count || pages[targetIndex] is null)
128-
return true;
129-
130-
_shellSection.SyncStackDownTo(pages[targetIndex]);
131-
132-
for (int i = pages.Count - 1; i >= NavigationBar.Items.Length; i--)
133-
{
134-
var page = pages[i];
135-
if (page != null)
136-
DisposePage(page);
137-
}
138-
139-
return true;
118+
// Stacks out of sync = user-initiated navigation
119+
return SendPop();
140120
}
141121

142122
internal bool SendPop()
@@ -577,7 +557,7 @@ Element ElementForViewController(UIViewController viewController)
577557

578558
foreach (var child in ShellSection.Stack)
579559
{
580-
if (child?.Handler is IPlatformViewHandler { ViewController: var vc } && viewController == vc)
560+
if (child?.Handler is IPlatformViewHandler handler && viewController == handler.ViewController)
581561
return child;
582562
}
583563

‎src/Controls/src/Core/Shell/ShellSection.cs‎

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -110,44 +110,6 @@ void IShellSectionController.SendInsetChanged(Thickness inset, double tabThickne
110110
_lastInset = inset;
111111
_lastTabThickness = tabThickness;
112112
}
113-
114-
internal void SyncStackDownTo(Page page)
115-
{
116-
if (_navStack.Count <= 1)
117-
{
118-
throw new Exception("Nav Stack consistency error");
119-
}
120-
121-
var oldStack = _navStack;
122-
123-
int index = oldStack.IndexOf(page);
124-
_navStack = new List<Page>();
125-
126-
// Rebuild the stack up to the page that was passed in
127-
// Since this now represents the current accurate stack
128-
for (int i = 0; i <= index; i++)
129-
{
130-
_navStack.Add(oldStack[i]);
131-
}
132-
133-
// Send Disappearing for all pages that are no longer in the stack
134-
// This will really only SendDisappearing on the top page
135-
// but we just call it on all of them to be sure
136-
for (int i = oldStack.Count - 1; i > index; i--)
137-
{
138-
oldStack[i].SendDisappearing();
139-
}
140-
141-
UpdateDisplayedPage();
142-
143-
for (int i = index + 1; i < oldStack.Count; i++)
144-
{
145-
RemovePage(oldStack[i]);
146-
}
147-
148-
(Parent?.Parent as IShellController)?.UpdateCurrentState(ShellNavigationSource.Pop);
149-
}
150-
151113
async void IShellSectionController.SendPopping(Task poppingCompleted)
152114
{
153115
if (_navStack.Count <= 1)

0 commit comments

Comments
 (0)