[Windows] TabbedPage: Refresh layout when NavigationView size changes#26217
[Windows] TabbedPage: Refresh layout when NavigationView size changes#26217BagavathiPerumal wants to merge 5 commits intodotnet:mainfrom
Conversation
|
/azp run |
This comment was marked as outdated.
This comment was marked as outdated.
|
@PureWeen / @jsuarezruiz, We have analysed the test case (Issue1323Test) failure and found that the issue was caused by a button alignment-related issue. Previously, the button inside the page, which was added as a child within the TabbedPage, was not properly aligned in the center. In this test case, the button was added directly as the content of the page without any additional properties. However, when the page was added as a child page within the TabbedPage, the button was not properly centered. After the fix, the button inside the page, which was added as a child in the TabbedPage, was properly aligned in the center of the page. The fix involved calling this.InvalidateMeasure(), which triggered a layout recalculation. This allowed the element within the TabbedPage to be measured and arranged correctly during the subsequent layout cycle. As a result, the button was properly aligned in the center of the page. Previously, the layout measurements were not recalculated, which caused the button to be misaligned on the page. Based on the current behavior after the fix, can we resave the current snapshot as the expected snapshot for this test case (Issue1323Test). Button Alignment in the Page (Output image from the simple sample):
Page added as Child in the TabbedPge(Output image from the simple sample):
Testcase Image (Before Fix):
Current Testcase Image (After Fix):
|
sheiksyedm
added
the
partner/syncfusion
| { | ||
| if (_navigationView != null) | ||
| { | ||
| this.InvalidateMeasure(); |
There was a problem hiding this comment.
If you remove the call to this.Arrange and leave this.InvalidateMeasure does this all still work?
The call to arrange inside the SizeChanged method here doesn't seem correct.
Also, can you add a test?
There was a problem hiding this comment.
@PureWeen, I have implemented the suggested changes, tested the fix across all basic scenarios, and also ran UI tests.
Testcase: The test case cannot be created for this scenario as resizing the window to a minimized state is not feasible during testing. Therefore, we have not added a test case for this scenario. Could you suggest alternative ways to address this, since resizing the window to a minimized state isn't possible during testing.
There was a problem hiding this comment.
Do you need to resize the Window or minimize and maximize it?
There was a problem hiding this comment.
@jsuarezruiz, The issue occurs only when resizing the window. Could you please share your suggestions on how to resolve it.
There was a problem hiding this comment.
@PureWeen, The Arrange() call in the NavigationView SizeChanged method is necessary to prevent test failures. Without this change, the ChangingToNewMauiContextDoesntCrash test consistently fails with a timeout exception.
|
/azp run |
This comment was marked as off-topic.
This comment was marked as off-topic.
1 similar comment
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@BagavathiPerumal I've tried your pr with my app and it does appear to fix the issue, thanks! |
@Mark-NC001, Thank you so much for taking the time to test the PR with your app. We're truly glad to hear that it resolves the issue. |
jsuarezruiz
left a comment
jsuarezruiz
left a comment
There was a problem hiding this comment.
The test Issue1323Test related with TabbedPage and Navigation is failing on Windows. Small snapshot differences.
| { | ||
| if (_navigationView != null) | ||
| { | ||
| this.InvalidateMeasure(); |
There was a problem hiding this comment.
Do you need to resize the Window or minimize and maximize it?
@jsuarezruiz, It looks expected behavior, as we already mentioned here, can we resave the current snapshot as the expected snapshot for this test case (Issue1323Test). |
|
/rebase |
github-actions
bot
force-pushed
the
fix-26103
branch
from
dd46dc2 to
ea5714d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
jsuarezruiz
left a comment
jsuarezruiz
left a comment
There was a problem hiding this comment.
Could be possible to include a test case for this changes?
In Appium, can minimize and maximize the Window (even change the Window size) using:
app.Driver.Manage().Window.Maximize();
app.Driver.Manage().Window.Maximize();
Let me know if can help in someway.
@jsuarezruiz, As suggested, I have tried recreating the test case using Appium but was unable to do so for this scenario. A NotImplementedException occurs when attempting to set the size value using testApp.Driver.Manage().Window.Size. Exception details: System.NotImplementedException: Unhandled endpoint: /session/2EDC486C-4A59-4E60-8C80-3EC866B72868/window/rect -- http://127.0.0.1:10100/ with parameters { Code: I also tried creating a test case based on a button click to change the window size, but this approach did not reproduce the issue. The issue only occurs when resizing the window by dragging. Could you please share your suggestions on how to resolve it. |
There was a problem hiding this comment.
There are some TabbedPage Windows tests failing.
Issue1323Test
Snapshot different than baseline: Issue1323Test.png (0.90% difference)
(in red, the differences)
TestPageDoesntCrash
at UITest.Appium.HelperExtensions.Tap(IUIElement element) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 302
at UITest.Appium.HelperExtensions.Tap(IApp app, IQuery query) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 38
at UITest.Appium.HelperExtensions.TapMoreButton(IApp app) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2261
at Microsoft.Maui.TestCases.Tests.Issues.Issue2809.TestPageDoesntCrash() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/XFIssue/Issue2809.cs:line 19
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
Could you review if are related with the changes?
Case1: Issue1323Test It looks expected behavior, as we already mentioned here, can we resave the current snapshot as the expected snapshot for this test case (Issue1323Test). Case2: TestPageDoesntCrash I have verified that the test case failures are unrelated to the fix. This has been confirmed on a local machine. |
|
/rebase |
BagavathiPerumal
changed the title
kubaflo
added
s/agent-changes-requested
BagavathiPerumal
force-pushed
the
fix-26103
branch
from
fc7ca15 to
e4a80c4
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 26217Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 26217" |
BagavathiPerumal
changed the title
I’ve reviewed and updated the changes based on the latest findings. Specifically:
|
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #26217 | Add InvalidateMeasure() before existing Arrange(_navigationView) in OnNavigationViewSizeChanged |
⏳ PENDING | TabbedPage.Windows.cs + 6 snapshot PNGs |
Original PR |
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Remove Arrange(), keep only InvalidateMeasure() |
TabbedPage.Windows.cs |
Reviewer's preferred approach; code change is clean | |
| 2 | try-fix (claude-sonnet-4.6) | Split SizeChanged: keep Arrange() for init path, use InvalidateMeasure() for resize-only handler (unsubscribe in OnApplyTemplateFinished) |
TabbedPage.Windows.cs |
Architecturally sound; cleanly addresses reviewer concern | |
| 3 | try-fix (gpt-5.3-codex) | Replace Arrange() with InvalidateMeasure() + native platformView.InvalidateMeasure()/InvalidateArrange()/UpdateLayout() |
❌ FAIL (infra blocker: WindowsAppSDK arch issue) | TabbedPage.Windows.cs |
Interesting native approach; blocked by build infra |
| 4 | try-fix (gpt-4.1) | _templateApplied flag: Arrange if not yet applied, InvalidateMeasure after |
❌ FAIL (same infra build blocker; code compiled) | TabbedPage.Windows.cs |
Functionally same as attempt 2 but with flag |
| 5 | try-fix (claude-opus-4.6, cross-poll) | Deferred Arrange via Dispatcher.Dispatch() — synchronous InvalidateMeasure + async Arrange |
TabbedPage.Windows.cs |
Addresses PureWeen's re-entrancy concern | |
| PR | PR #26217 | Add InvalidateMeasure() before existing Arrange(_navigationView) |
⏳ PENDING (Gate skipped) | TabbedPage.Windows.cs + 6 PNGs |
Original PR |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | Deferred Arrange via Dispatcher.Dispatch() → became attempt 5 |
| claude-sonnet-4.6 | 2 | Yes | Subscribe _navigationFrame.SizeChanged (content area) instead of nav view; cascade InvalidateMeasure to CurrentPage |
| gpt-5.3-codex | 2 | Yes | One-shot LayoutUpdated/EffectiveViewportChanged hook against _navigationFrame bounds |
| gpt-4.1 | 2 | Yes | Loaded event or CompositionTarget.Rendering one-shot hook for post-render layout pass |
| All models | 3 | No | All ideas exhausted given infra constraints |
Exhausted: Yes — 5 attempts, 2 cross-poll rounds. All blocked by WindowsAppSDKSelfContained build infra on this agent. No approach can be empirically validated here.
Selected Fix: PR #26217's fix — it is the only fix validated to work (by the PR author and issue reporter), and all alternatives have stronger architectural merit but are unverifiable on this agent. However, see report for recommended changes.
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Windows-only TabbedPage layout bug; #26103, #11402, #20028 |
| Gate | No tests detected in this PR | |
| Try-Fix | ✅ COMPLETE | 5 attempts, 0 passing (all BLOCKED by Windows AppSDK build infra on agent) |
| Report | ✅ COMPLETE |
Summary
PR #26217 fixes a real, verified Windows bug: when the WinUI NavigationView hosting TabbedPage tabs resizes (window resize), MAUI's virtual view layout was not recalculated, causing ScrollView and other content to clip or hide the bottom of the page. The issue reporter confirmed the fix works. The code change is small — 4 lines added to OnNavigationViewSizeChanged. Six snapshot baselines were updated to reflect the improved TabbedPage alignment.
The PR has open reviewer concern from PureWeen that needs resolution before merge.
Root Cause
OnNavigationViewSizeChanged previously only called this.Arrange(_navigationView), which sets the MAUI virtual view's Frame directly from the NavigationView's ActualWidth × ActualHeight. This bypasses MAUI's measure phase. When the window is resized, the virtual view's dimensions were updated via Arrange, but child views (e.g., ScrollView) did not trigger their own re-measure cycle, causing stale layout dimensions to persist.
Fix Quality
What the PR does: Adds this.InvalidateMeasure() before the existing this.Arrange(_navigationView) call. This is a minimal, targeted change that triggers MAUI's re-measure cycle and then forces a synchronous arrangement.
Concerns:
-
Arrange()insideSizeChangedis architecturally questionable — Reviewer PureWeen explicitly asked: "The call to arrange inside the SizeChanged method here doesn't seem correct. If you remove the call tothis.Arrangeand leavethis.InvalidateMeasuredoes this all still work?" The author responded that removingArrange()breaks theChangingToNewMauiContextDoesntCrashdevice test (TimeoutException). However, this claim was not verified with a test run in this review session (Windows device test infra unavailable on this agent). The PR should demonstrate — via a CI run or test output — thatArrange()is genuinely required and not just coincidentally masking a timing issue. -
SizeChangedhandler has dual purpose, neither documented — The handler was originally added as an initialization fallback (fires whenTopNavArea == nullat setup). It is subscribed inSetupNavigationView()but never unsubscribed inOnApplyTemplateFinished, so it continues firing on every window resize for the TabbedPage's full lifetime. This implicit dual responsibility (initialization + resize) is undocumented and fragile. A cleaner approach (explored in Try-Fix Attempt 2) would: unsubscribe the handler inOnApplyTemplateFinishedafter initialization completes, and add a new dedicated resize handler that calls onlyInvalidateMeasure()— addressing the reviewer's concern while preserving theArrange()call for the initialization path whereChangingToNewMauiContextDoesntCrashdepends on it. -
No new test added — The PR author argues window-resize behavior cannot be automated. Reviewer jsuarezruiz suggested using Appium's
Driver.Manage().Window.Maximize()/Minimize()APIs to reproduce the resize scenario. This approach is feasible and should be attempted. The Gate was skipped due to no tests being detected. -
Snapshot updates look correct — Six Windows UI test snapshots were updated; the diffs show improved alignment of TabbedPage elements, consistent with the layout fix.
-
Comments are low quality —
"// Ensure TabbedPage layout responds to NavigationView size changes"and"// Complete layout to fix frame dimensions"don't explain why both calls are needed together, or whyArrange()is needed at all givenInvalidateMeasure()was also added. The comments should explain the dual-purpose design.
Recommended Changes
-
Verify
Arrange()is necessary: Either (a) demonstrate via a CI run that removingArrange()(keeping onlyInvalidateMeasure()) breaksChangingToNewMauiContextDoesntCrash, or (b) implement the split-handler approach (unsubscribe SizeChanged inOnApplyTemplateFinished, add resize-only handler withInvalidateMeasure()). -
Separate the initialization and resize paths (preferred by reviewer):
void OnApplyTemplateFinished(object? sender, EventArgs e) { UpdateValuesWaitingForNavigationView(); if (sender is MauiNavigationView mnv) { mnv.OnApplyTemplateFinished -= OnApplyTemplateFinished; mnv.SizeChanged -= OnNavigationViewSizeChanged; mnv.SizeChanged += OnNavigationViewResized; } } void OnNavigationViewSizeChanged(object sender, SizeChangedEventArgs e) { // Initialization path only — fired before template is applied. if (_navigationView != null) this.Arrange(_navigationView); } void OnNavigationViewResized(object sender, SizeChangedEventArgs e) { // Post-initialization resize — let MAUI layout system handle it. if (_navigationView != null) this.InvalidateMeasure(); } // Also unsubscribe OnNavigationViewResized in OnHandlerDisconnected.
-
Add a window-resize test using Appium
Driver.Manage().Window.Size = new Size(...)to resize the window and verifyScrollViewcan scroll to the bottom. -
Improve or remove the inline comments — they don't add clarity in their current form.
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Please review the ai's summary :)
I have reviewed the AI summary. The Arrange() call in the NavigationView.SizeChanged method is necessary to prevent test failures. Without this change, the ChangingToNewMauiContextDoesntCrash test consistently fails with a TimeoutException. |
#34575) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description Adds Windows platform support to the `maui-copilot` CI pipeline (AzDO definition 27723), enabling Copilot PR reviews on Windows-targeted PRs. ### Changes **`eng/pipelines/ci-copilot.yml`** - Add `catalyst` and `windows` to Platform parameter values - Add per-platform pool selection (`androidPool`, `iosPool`, `macPool`, `windowsPool`) - Skip Xcode, Android SDK, simulator setup for Windows/Catalyst - Add Windows-specific "Set screen resolution" step (1920x1080) - Add MacCatalyst-specific "Disable Notification Center" step - Fix `sed` command for `Directory.Build.Override.props` on Windows (Git Bash uses GNU sed) - Handle Copilot CLI PATH detection on Windows vs Unix - Change `script:` steps to `bash:` for cross-platform consistency **`.github/scripts/Review-PR.ps1`** - Add `catalyst` to ValidateSet for Platform parameter **`.github/scripts/BuildAndRunHostApp.ps1`** - Add Windows test assembly directory for artifact collection **`.github/scripts/post-ai-summary-comment.ps1` / `post-pr-finalize-comment.ps1`** - Various improvements for cross-platform comment posting ### Validation Successfully ran the pipeline with `Platform=windows` on multiple Windows-specific PRs: - PR #27713 — ✅ Succeeded - PR #34337 — ✅ Succeeded - PR #26217, #27609, #27880, #28617, #29927, #30068 — Triggered and running --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🚦 Gate - Test Before and After Fix📊 Expand Full Gate —
|
MauiBot
added
s/agent-fix-pr-picked
|
/rebase |
…to fix scrolling and layout issues.
…ing InvalidateMeasure() and Snapshot resaved.
github-actions
bot
force-pushed
the
fix-26103
branch
from
e4a80c4 to
4eb69f6
Compare
🏷️ Test Categories for Regression DetectionNo UI Test Categories
Pipeline filter: Device Test Categories
Recommendation: Run device tests: Yes — Windows-specific TabbedPage platform handler code changed. 🚀 Run Targeted Tests on Existing PipelinesBoth UI Tests — trigger
Device Tests — trigger
When triggered without parameters (e.g., by normal PR push), all categories run as usual. 📁 Changed files (7)Test/snapshot files (6):
Source files (1):
|
Root cause
The issue arises because the OnNavigationViewSizeChanged method fails to properly reset the layout measurements before arranging the NavigationView. As a result, the NavigationView does not correctly update its layout in response to size changes, causing misalignment or rendering issues in the ScrollView.
Description of Issue Fix
The fix involves updating the OnNavigationViewSizeChanged() method to include a call to InvalidateMeasure() before arranging the NavigationView. This ensures that the layout is accurately recalculated, allowing the ScrollView and other elements within the TabbedPage to be properly measured and arranged during the subsequent layout cycle. This effectively resolves alignment and rendering issues.
Additionally, the Arrange() call is retained within the SizeChanged handler to prevent test failures, specifically avoiding timeout issues observed in the ChangingToNewMauiContextDoesntCrash test. This combination ensures stable layout behavior while resolving the clipping and scrolling issues that occur after window resizing.
Why Tests were not added:
Regarding the test case: The issue only occurs when resizing the window, so it is not possible to add a test case for the window resizing behavior.
Tested the behavior in the following platforms.
Issues Fixed
Fixes #26103
Fixes #11402
Fixes #20028
Resaved Test snapshots
Resaved the below-mentioned test snapshot because elements in the TabbedPage were not properly aligned before the fix. The layout changes in OnNavigationViewSizeChanged (adding Arrange() after InvalidateMeasure()) now ensure proper element alignment within the TabbedPage.
Output
BeforeFix-TabbedPage-ScrollView.mp4
AfterFix-TabbedPage-ScrollView.mp4