Skip to content

[Mac] Fixed the Issue with the TitleBar TrailingContent not being properly aligned #36541

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.

Sign up for GitHub

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
kubaflo merged 2 commits into from
Sep 13, 2026
Merged

[Mac] Fixed the Issue with the TitleBar TrailingContent not being properly aligned #36541

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b0bf433
Fixed the Issue with the TitleBar TrailingContent not being properly…
Ahamed-Ali Jul 13, 2026
fba7afb
Added the fix for controltemplate scenario
Ahamed-Ali Jul 17, 2026
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
6 changes: 5 additions & 1 deletion src/Controls/src/Core/Layout/LayoutExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ internal static void IgnoreLayoutSafeArea(this Layout layout)
{
IgnoreLayoutSafeArea(childLayout);
}
else if (child is IContentView contentView && contentView.Content is Layout contentLayout)
Comment thread
Ahamed-Ali marked this conversation as resolved.
Outdated
Comment thread
Ahamed-Ali marked this conversation as resolved.
Outdated
Comment thread
Ahamed-Ali marked this conversation as resolved.
Outdated
Comment thread
Ahamed-Ali marked this conversation as resolved.
Outdated
Comment thread
Ahamed-Ali marked this conversation as resolved.
Outdated
{
IgnoreLayoutSafeArea(contentLayout);
}
Comment thread
Ahamed-Ali marked this conversation as resolved.
Outdated
Comment on lines +17 to +27

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[minor] Diff hygiene — This change removes the trailing newline from a previously well-formed file (\ No newline at end of file on all three added/modified files) and introduces mixed tab/space indentation inside the new block (line 17 is indented with tabs+spaces; lines 20-26 are prefixed with a space before the tabs). Both will be flagged by the repo's formatting check. Please restore the trailing newline and re-indent with tabs only.

}
}
}
}

@MauiBot MauiBot Aug 7, 2026

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.

🔍 AI-Generated Review (multi-model)

[minor] Build & MSBuild — This change deletes the trailing newline at end of file (\ No newline at end of file in the diff) and the new block mixes indentation styles: line 17 starts with tabs+spaces (\t\t else if) and lines 20-26 start with a space before the tabs. The repo enforces formatting in CI (dotnet format / whitespace verification), so this will produce avoidable build noise on an already-red PR. Restore the final newline and use tab-only indentation consistent with the rest of the file.

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[minor] Build & MSBuild hygiene — This edit drops the trailing newline at end of file (\ No newline at end of file in the diff) and the added block at lines 17-26 mixes indentation styles (line 17 is space-indented, lines 20-26 are space+tab). Neither affects behavior, but it will surface as an unrelated dotnet format / whitespace diff and makes the real one-line change harder to read in blame. Restoring the EOF newline and using tabs consistently keeps the diff to the intended hunk.

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[minor] Build & MSBuild — mechanical formatting issues introduced by this hunk that will trip the format/whitespace check rather than being a style preference: line 17 is indented with spaces instead of a tab, lines 20–26 carry a stray leading space before their tabs, and the trailing newline at end of file was removed (\ No newline at end of file). Please run dotnet format whitespace on the file.

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

💡 [minor] Build & Formatting — Two mechanical issues introduced by this hunk that dotnet format / the .editorconfig check will flag:

  1. The trailing newline at end of file was removed (\ No newline at end of file in the diff). The previous revision ended with a newline; this is an unnecessary churn line in the diff.
  2. Mixed indentation in the new block — line 17 is indented with \t\t + 4 spaces (\t\t else if), and lines 20–26 begin with a leading space before the tabs ( \t\t\t\t{). The rest of the file uses tabs only.

Running dotnet format (or just re-indenting the block with tabs and restoring the final newline) will clear this.

@MauiBot MauiBot Aug 19, 2026

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.

🔍 AI-Generated Review (multi-model)

💡 Suggestion — [minor] Build: the added block mixes leading spaces and tabs (\t\t else if, and \t\t\t\t{ on the inner braces), and the trailing newline at end-of-file was removed here and in both new Issue29516.cs files. dotnet format --verify-no-changes runs in CI and will flag these, turning the lane red for a reason unrelated to the fix. Please re-run dotnet format / restore the final newlines.

@MauiBot MauiBot Aug 24, 2026

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.

🔍 AI-Generated Review (multi-model)

[minor] Complexity Reduction — The trailing newline was removed from the end of the file (\ No newline at end of file), which turns the unchanged closing brace into a modified line in the diff and violates the repo's .editorconfig insert_final_newline. Same issue in src/Controls/tests/TestCases.HostApp/Issues/Issue29516.cs and src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29516.cs. Please restore the final newline in all three files.

67 changes: 67 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue29516.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
namespace Maui.Controls.Sample.Issues;

[Issue(IssueTracker.Github, 29516, "Issue with the TitleBar TrailingContent not being properly aligned on macOS", PlatformAffected.macOS)]
public class Issue29516 : ContentPage
{
TitleBar _titleBar;
public Issue29516()
{
Content = new VerticalStackLayout
{
Children =
{
new Label
{
Text = "This is a test for the TitleBar TrailingContent alignment issue on macOS.",
AutomationId="ContentLabel",
}
}
};
_titleBar = new TitleBar
{
Title = ".NET MAUI",
BackgroundColor = Colors.Blue,
HeightRequest = 50,
Content = new SearchBar

@MauiBot MauiBot Aug 19, 2026

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.

🔍 AI-Generated Review (multi-model)

⚠️ Warning — [major] Regression Prevention: the repro puts a SearchBar in TitleBar.Content, which is unrelated to the trailing-content alignment being fixed and injects platform-variable rendering (placeholder metrics, focus ring, native search-field chrome, and a blinking caret on Catalyst) into a pixel-exact snapshot baseline. This is a flakiness source that will cause unrelated future failures on this baseline. Recommend replacing it with a static Label, or omitting Content entirely so the snapshot isolates the trailing content.

{

@MauiBot MauiBot Aug 7, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The repro is built entirely in the object initializer, so all children exist before TitleBar.TrailingContent is assigned and OnTrailingContentChanged (TitleBar.cs:201) sees the complete subtree. IgnoreLayoutSafeArea is a one-shot snapshot with no ChildAdded/ContentChanged hook, so the very common runtime pattern — assign TrailingContent first, then stack.Add(...) or set border.Content = ... later, or swap content via a binding/DataTemplate — will leave the new layouts with IgnoreSafeArea = false and reproduce the original misalignment. The test as written cannot catch that. Either add a case that mutates the content after assignment, or make the propagation reactive to tree changes.

Placeholder = "TitleBar Content",
PlaceholderColor = Colors.White,
BackgroundColor = Colors.Blue,
MaximumWidthRequest = 300,
HorizontalOptions = LayoutOptions.Fill,
VerticalOptions = LayoutOptions.Center
},
Comment thread
Ahamed-Ali marked this conversation as resolved.
TrailingContent = new HorizontalStackLayout

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The repro page exercises exactly one shape: HorizontalStackLayout -> Border -> StackLayout, which is the single case the new branch handles. There is no negative/adjacent coverage for the two shapes that remain broken:

  1. root-level IContentView slot — TrailingContent = new Border { Content = new StackLayout { ... } } (never reaches the helper, see LayoutExtensions.cs:17);
  2. nested IContentView chain — Border -> ContentView -> StackLayout (recursion stops, see LayoutExtensions.cs:19).

Adding those two slots to this same page would make the screenshot baseline discriminate between "safe-area ignored everywhere in the title bar" and "safe-area ignored only for the one-level-deep case". As written, a future regression that re-narrows the traversal to the one covered shape would still pass.

Minor test-robustness note (TestCases.Shared.Tests/Tests/Issues/Issue29516.cs): the test waits on ContentLabel, which lives in the page body, then captures with includeTitleBar: true. The wait does not prove the title bar has been realized/laid out, so the capture can race title-bar realization. Waiting on an AutomationId inside the trailing content would tie the precondition to what is being asserted.

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

⚠️ [major] Regression Prevention — The repro covers only the single shape the fix happens to handle; the adjacent shapes are untested and (per the other comments) still broken.

This page builds TrailingContent = HorizontalStackLayout → Border → StackLayout, which is exactly Layout → IContentView → Layout — the one path the new else if covers. Missing negative/adjacent cases:

  • TrailingContent set directly to a Border/ContentView (no outer Layout) — still a no-op because of the as Layout cast at TitleBar.cs:201.
  • Two levels of content nesting (Border → ContentView → Grid) — recursion stops at the inner ContentView.
  • LeadingContent and Content, which call the same helper (TitleBar.cs:111,176) and are equally affected. Content here is a SearchBar (not a Layout), so that slot exercises nothing.

Per the MAUI regression guidance, a bug fix should enumerate and cover the adjacent scenarios, not just the reported one — most reverts come from the neighbouring case. Please extend this page (or add sibling cases) so the test suite would actually catch the gaps above.

@MauiBot MauiBot Aug 24, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The repro wraps the Border in a HorizontalStackLayout, which is the one shape the new code handles. It therefore cannot detect the two gaps flagged in LayoutExtensions.cs: (a) TrailingContent assigned directly to a Border/ContentView (entry points still do newValue as Layout), and (b) Border > Border > StackLayout nesting where the IContentView → Layout hop breaks. Please extend this page (or add sibling cases) to cover a direct-Border trailing content and a two-level content-view nest, so the regression test actually pins the fixed behavior rather than only the single reported arrangement.

{
Spacing = 8,
Margin = new Thickness(4),
Children =
{
new Border

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention — coverage only exercises the single nesting shape the new else if happens to handle (HorizontalStackLayout → Border → StackLayout). The adjacent cases in the same bug class are untested and, per the traversal as written, still broken: (a) TrailingContent that is itself a Border/ContentView rather than a Layout, and (b) two-level content nesting (Border → ContentView → Grid). Because the screenshot baselines are newly generated from the fixed build, the suite would stay green for both.

The changed method is internal in Controls.Core, so a cheap Controls.Core.UnitTests test asserting IgnoreSafeArea propagation across a nesting matrix (Layout-only, Layout→ContentView→Layout, ContentView root, ContentView→ContentView→Layout, templated ContentView) would cover the recursive behavior far better than one screenshot and would fail today for the uncovered shapes.

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention — The repro exercises only the single shape the fix happens to handle: HorizontalStackLayout (Layout) → Border (IContentView) → StackLayout (Layout). It never exercises the cases the new branch does not handle — IContentView → IContentView → Layout, or a ContentView/templated content view (which derives from Compatibility.Layout and so matches neither branch). Because the fixture conflates "content view nesting works" with "one-hop Border nesting works", a green result here does not discriminate.

Recommendation: add a fast unit test (Controls.Core.UnitTests) that builds a nested tree and asserts IgnoreSafeArea == true on every descendant Layout, including a two-level content-view chain and a ContentView node. That is far cheaper than a screenshot test and directly covers the new branch.

@MauiBot MauiBot Aug 19, 2026

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.

🔍 AI-Generated Review (multi-model)

💡 Suggestion — [moderate] Regression Prevention and Test Coverage: the repro exercises exactly one shape — HorizontalStackLayout → Border → StackLayout — which is precisely the single case the new else if (child is IContentView) branch handles. There is no coverage for the adjacent shapes that the fix still leaves broken (see the LayoutExtensions.cs comment): a Border assigned directly as TrailingContent (the extension is never called, because the call site is (newValue as Layout)?), and a two-level Border → Border → Layout chain (traversal terminates at the intermediate non-Layout). Adding those two variants to this page would show whether the fix is complete or only covers the demo shape.

{
Content = new StackLayout
{
VerticalOptions = LayoutOptions.Center,
BackgroundColor = Colors.Red,
Children =
{
new Label { Text = "TrailingContent" }

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention — new Label { Text = "TrailingContent" } has no AutomationId, so the UI test has no way to wait for or assert on the trailing content it is validating (see the paired comment on the shared test). Add AutomationId = "TrailingContentLabel" here.

@MauiBot MauiBot Aug 24, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The reproduction subtree that the fix is supposed to correct carries no AutomationId, so nothing in the UI test can query or assert on it. Add AutomationId to this Label (and/or the parent Border) so the test can wait on it and, ideally, assert its rect is inside the title bar's 50px height — which would give a deterministic pre-fix failure instead of relying solely on a pixel snapshot.

}
}
}
}
}
};

}

protected override void OnAppearing()
{
base.OnAppearing();

if (Window is not null)
{
Window.TitleBar = _titleBar;

@MauiBot MauiBot Aug 7, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — Window.TitleBar is assigned here but never cleared, and the HostApp reuses a single Window across issue pages for the whole run. Once this test executes, the blue 50pt TitleBar (with the SearchBar and red TrailingContent) stays attached for every subsequent page in that session, which is a cross-test contamination risk specifically for other screenshot tests that run afterwards. Issue24489_2.xaml.cs:28 sets Window.TitleBar = null on teardown for exactly this reason — please add an OnDisappearing override that restores it.

@MauiBot MauiBot Aug 24, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — Window.TitleBar is assigned in OnAppearing but never cleared, and TestCases.HostApp runs all issue pages in one long-lived app session. After this test navigates away, the blue 50px title bar with the SearchBar and trailing content stays attached to the Window and will appear in unrelated pages' screenshots on Mac Catalyst and Windows — a cross-test contamination source for every subsequent VerifyScreenshot(includeTitleBar: true) test in the same run. Add OnDisappearing that restores Window.TitleBar = null.

}
}
}
Binary file added ...stCases.Mac.Tests/snapshots/mac/TitleBarTrailingContentShouldRenderProperly.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
23 changes: 23 additions & 0 deletions src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29516.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#if MACCATALYST || WINDOWS

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

💡 [minor] Platform Scoping / Test Cost — The test is compiled for WINDOWS as well, but the fix cannot change anything on Windows.

The production change only sets Layout.IgnoreSafeArea, which is a safe-area concept that is a no-op outside iOS/MacCatalyst. So the added TestCases.WinUI.Tests/snapshots/windows/TitleBarTrailingContentShouldRenderProperly.png baseline does not validate the fix — it is a pure "nothing changed" guard that adds a new WinUI snapshot to the maintenance/flake surface (title-bar screenshots with includeTitleBar: true are chrome-sensitive and churn on Windows App SDK bumps).

If the Windows coverage is intentional (guarding against a cross-platform regression), a one-line comment saying so would be helpful; otherwise consider narrowing to #if MACCATALYST and dropping the WinUI baseline.

Also a nit: this file uses 4-space indentation while the rest of TestCases.Shared.Tests uses tabs.

@MauiBot MauiBot Aug 24, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Cross-Platform Behavioral Consistency — The issue is scoped to macOS (the HostApp page declares PlatformAffected.macOS), but this #if MACCATALYST || WINDOWS also introduces a new Windows screenshot test plus a 1010x761 Windows baseline. The gate ran catalyst only, so the Windows path and its baseline are entirely unvalidated by this PR. Since the production change (LayoutExtensions.cs) is shared code that now also alters Windows TitleBar content safe-area state, either (a) validate the Windows run and keep it, or (b) scope the test to MACCATALYST and drop the unverified Windows baseline. Committing an unverified snapshot baseline adds a flake source to a required check.

// TitleBar is only available on Mac Catalyst and Windows. https://learn.microsoft.com/en-us/dotnet/maui/user-interface/controls/titlebar?view=net-maui-9.0
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues;

public class Issue29516 : _IssuesUITest
{
public Issue29516(TestDevice device) : base(device) { }

public override string Issue => "Issue with the TitleBar TrailingContent not being properly aligned on macOS";

[Test]

@MauiBot MauiBot Aug 20, 2026

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.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention and Test Coverage — The only coverage for this fix is a screenshot comparison, which is both currently unrunnable (see baseline size mismatch below) and non-discriminating: a screenshot diff cannot tell a reviewer why it changed, and if it ever regresses the usual remedy is re-recording the baseline, which silently erases the regression signal.

The changed code (LayoutExtensions.IgnoreLayoutSafeArea) is internal and Controls.Core.UnitTests already has IVT, so a deterministic regression test is cheap and platform-independent:

[Fact]
public void IgnoreLayoutSafeAreaRecursesThroughContentViews()
{
    var inner = new StackLayout { Children = { new Label() } };
    var root = new HorizontalStackLayout { Children = { new Border { Content = inner } } };
    root.IgnoreLayoutSafeArea();
    Assert.True(inner.IgnoreSafeArea);
}

Please add the unit test covering the exact nesting from #29516 (and, per the finding on LayoutExtensions.cs:17, a Border → ContentView → Layout case, which currently fails).

[Category(UITestCategories.Window)]

@MauiBot MauiBot Aug 9, 2026

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.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention and Test Coverage — The production change is pure shared, platform-agnostic C# (IgnoreSafeArea propagation through a visual-tree walk) with no platform dependency, yet the only coverage is an expensive, environment-sensitive screenshot UI test on two platforms.

A Controls.Core.UnitTests test that builds TitleBar.TrailingContent = <shape> and asserts IgnoreSafeArea == true on each nested Layout would be deterministic, would run on every PR, and — critically — would have caught the two traversal gaps noted on LayoutExtensions.cs:17, because it can cheaply cover the negative/adjacent shapes the screenshot test cannot:

  • TrailingContent = Border { Content = StackLayout } (root is not a Layout → helper never called)
  • TrailingContent = HStack { Border { ContentView { StackLayout } } } (chain stops before the inner layout)
  • the reset case: after TrailingContent = null or reassignment, the old subtree should no longer be flagged

Per the repo's test-type preference (unit > device > UI), please add the unit test; the screenshot test can stay as a visual smoke check once its baseline is validated.

public void TitleBarTrailingContentShouldRenderProperly()

@MauiBot MauiBot Aug 7, 2026

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.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention and Test Coverage — The behavior changed here is pure shared managed code in LayoutExtensions.IgnoreLayoutSafeArea, but the only coverage added is a device screenshot test. A deterministic unit test in Controls.Core.UnitTests that builds a TitleBar with TrailingContent and asserts IgnoreSafeArea == true on each nested Layout would run on every PR, cost nothing, and would immediately expose the two traversal gaps flagged on LayoutExtensions.cs:17 and :19 (top-level IContentView content, and content nested more than one level deep). It would also cover the negative case — a non-IContentView, non-Layout child must not be touched — which the screenshot test cannot express. Please add unit coverage for the propagation matrix rather than relying solely on a snapshot.

@MauiBot MauiBot Aug 24, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Gestures / Regression Prevention — The test's precondition does not cover what it claims to verify. WaitForElement("ContentLabel") waits on the page body label (Issue29516.cs:16 in the HostApp), not on anything inside the TitleBar. The trailing content (Border > StackLayout > Label "TrailingContent") carries no AutomationId, so the screenshot can be taken before the title bar has laid out, and — more importantly — the test would still pass its wait even if the trailing content failed to render at all. Add an AutomationId to the trailing Label/Border and WaitForElement on it before VerifyScreenshot, so the assertion is anchored to the element the issue is about.

{
App.WaitForElement("ContentLabel");
Comment thread
Ahamed-Ali marked this conversation as resolved.
Comment thread
Ahamed-Ali marked this conversation as resolved.
Comment thread
Ahamed-Ali marked this conversation as resolved.
Comment thread
Ahamed-Ali marked this conversation as resolved.

@MauiBot MauiBot Jul 20, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — This waits for ContentLabel in the page body, then screenshots the title bar. Window.TitleBar is assigned in OnAppearing and its native title-bar content can be realized later than the page content, so the test can capture before the trailing/title-bar content is actually present. Please give a TitleBar sentinel (for example the trailing label) an AutomationId and wait for that before VerifyScreenshot(includeTitleBar: true).

@MauiBot MauiBot Jul 21, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention — The test waits for ContentLabel, which is in the page body, not in the titlebar being verified. That allows the screenshot to be captured before Window.TitleBar and its trailing content have finished loading/layout, making this regression test flaky and not proving the fixed element is present. Add an AutomationId/sentinel to the titlebar trailing content and wait for that element before VerifyScreenshot(includeTitleBar: true).

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention — the test's precondition is not verified. ContentLabel lives in the page body (Issue29516.cs:16), which is present regardless of whether Window.TitleBar was assigned and laid out — the assignment happens in OnAppearing and the platform title bar is realized asynchronously in the window chrome. So WaitForElement("ContentLabel") can return before the TitleBar (the actual subject) has rendered, making the screenshot both flaky and non-discriminating: a run where the title bar never appeared would still reach VerifyScreenshot.

Give the trailing Label an AutomationId and wait on that (or otherwise assert the title bar is present) so the test cannot pass without the thing under test being on screen.

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

⚠️ [moderate] Regression Prevention / Test Correctness — The test's synchronization point does not gate on the thing under test, and the assertion cannot discriminate the bug.

App.WaitForElement("ContentLabel") waits for a Label in the page body (Issue29516.cs HostApp, AutomationId="ContentLabel"), which is unrelated to the TitleBar. Worse, the TitleBar is assigned in OnAppearing (Window.TitleBar = _titleBar;), i.e. potentially after ContentLabel is already present — so the wait can succeed while the title bar is still unrealized. VerifyScreenshot's retry may paper over this, but the test is timing-dependent by construction.

Please give the trailing content an AutomationId and wait on that element instead, so the precondition ("the trailing content was actually rendered") is proven before the screenshot is taken. Without it, a regression that drops the trailing content entirely could still produce a stable-but-wrong baseline.

@MauiBot MauiBot Aug 8, 2026

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.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention / Test Coverage — The test does not verify its own precondition. App.WaitForElement("ContentLabel") waits on a label in the page body, not on anything inside TitleBar.TrailingContent. If the trailing content fails to render at all — the exact bug under test — this wait still succeeds and VerifyScreenshot fires, so the test can pass trivially or race titlebar layout and become flaky.

Recommendation: give the trailing-content label an AutomationId and wait on it (App.WaitForElement("TrailingContentLabel")) before VerifyScreenshot, so the assertion is anchored to the region being validated.

@MauiBot MauiBot Aug 9, 2026

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.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention / Gestures-style precondition verification — The test's only synchronization point is App.WaitForElement("ContentLabel"), which is the Label in the page body, not in the TitleBar. Nothing in the test proves the TrailingContent subtree was ever laid out or rendered before VerifyScreenshot captures.

Two consequences: (a) the screenshot can be taken while the title bar content is still settling, which is a plausible contributor to the 1.59% diff and to future flakiness; (b) the assertion cannot discriminate the bug — the page-body label renders identically whether or not the TrailingContent fix works.

Give the Border/Label inside TrailingContent an AutomationId and wait on that element (a sentinel positioned inside the title-bar bounds), so the test verifies its own precondition before asserting on pixels.

@MauiBot MauiBot Aug 19, 2026

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.

🔍 AI-Generated Review (multi-model)

⚠️ Warning — [major] Regression Prevention / Gestures precondition rule: the only synchronization before the screenshot is App.WaitForElement("ContentLabel"), which waits on the page body, not on the TitleBar. The TitleBar is attached in OnAppearing (Issue29516.cs:64 in the HostApp), so the body label can be present and queryable before the title bar is attached, measured, and drawn — the screenshot can be taken mid-attach.

There is also no retryTimeout. Per the repo guidance, prefer VerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2)) over relying on a single capture, and wait on an AutomationId placed on the trailing content itself so the test's precondition (the thing under test is actually on screen) is verified rather than assumed.

@MauiBot MauiBot Aug 24, 2026

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.

🔍 AI-Generated Review (multi-model)

[critical] Regression Prevention and Test Coverage — This test does not currently demonstrate the bug or the fix. Gate evidence (Mac Catalyst, -TestFilter "Issue29516"): the test failed without the production fix and still failed with it. The with-fix failure was a screenshot-cropping mismatch — expected baseline 789x592 vs. a retained full-screen capture of 2560x1600, with the harness reporting the app window at a negative X coordinate and therefore unccroppable. Two independent problems follow: (1) the committed Mac baseline TestCases.Mac.Tests/snapshots/mac/TitleBarTrailingContentShouldRenderProperly.png (789x592) was captured under window conditions the CI harness does not reproduce, so it can never match; (2) because the test never passes, it provides zero evidence that LayoutExtensions.cs corrects the reported misalignment. A pure VerifyScreenshot assertion also cannot distinguish "trailing content correctly aligned" from "trailing content missing/blank" without a human diffing pixels. Please re-capture the baseline from a green harness run and add a deterministic assertion (see the AutomationId comment below) so the test fails pre-fix and passes post-fix.

VerifyScreenshot(includeTitleBar: true);
Comment thread
Ahamed-Ali marked this conversation as resolved.

@MauiBot MauiBot Jul 20, 2026

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.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — WaitForElement("ContentLabel") only confirms the page body is loaded; it does not guarantee the TitleBar has finished its layout pass. Window.TitleBar = _titleBar is set in OnAppearing, which may trigger an asynchronous layout pass on the native side. Without retryTimeout:, VerifyScreenshot retries only once (with a 500 ms delay), which may not be enough to capture a fully-rendered TitleBar, leading to a flaky baseline mismatch.

The recommended fix is to add AutomationId = "TrailingContentLabel" to the Label inside the Border's StackLayout in the host-app fixture, then call App.WaitForElement("TrailingContentLabel") before the screenshot so the test blocks until the TitleBar content is actually visible. Alternatively, pass retryTimeout: TimeSpan.FromSeconds(5) here:

VerifyScreenshot(includeTitleBar: true, retryTimeout: TimeSpan.FromSeconds(5));

@MauiBot MauiBot Aug 7, 2026

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.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention and Test Coverage — This test does not discriminate the fix. Per the pipeline gate run (BuildAndRunHostApp.ps1 -Platform catalyst -TestFilter "Issue29516"), VerifyScreenshot failed without the change and then failed three consecutive times with the change at an identical 21.64% mismatch against the committed mac/TitleBarTrailingContentShouldRenderProperly.png baseline. An identical delta on both sides means the committed baseline does not correspond to the rendering the code produces in either state, so this test can neither prove the bug nor prove the fix — it is a permanently-red test rather than a regression guard. The baseline PNGs must be regenerated from an actual passing run on the fixed build (and the same re-verified for the Windows baseline) before this can be merged, and the pre-fix/post-fix screenshots should be shown to differ.

@MauiBot MauiBot Aug 9, 2026

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.

🔍 AI-Generated Review (multi-model)

[critical] Regression Prevention and Test Coverage — The gate is a definitive failure on this assertion: TitleBarTrailingContentShouldRenderProperly fails both without the fix and with the fix, with the committed Mac baseline differing by 1.59% (two confirmation reruns, gate/content.md). That means one of two things, and the PR must resolve which before merge:

  • the committed TestCases.Mac.Tests/snapshots/mac/TitleBarTrailingContentShouldRenderProperly.png baseline was captured in a different environment/OS version than CI renders, i.e. the baseline is invalid; or
  • the production change does not actually produce the intended rendering, and the test is correctly reporting that the fix is ineffective.

Given the traversal gap flagged on LayoutExtensions.cs:17, the second possibility is credible and must be ruled out empirically — please do not resolve this by regenerating the baseline or by raising tolerance, which would convert a real signal into a permanent green. A deterministic unit-level assertion (see the comment on line 16) would disambiguate this in seconds.

@MauiBot MauiBot Aug 19, 2026

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.

🔍 AI-Generated Review (multi-model)

❌ Error — [critical] Regression Prevention and Test Coverage: this test does not discriminate the fix, per the gate run on Mac Catalyst:

  • Without the LayoutExtensions.cs change: TitleBarTrailingContentShouldRenderProperly FAILED (expected).
  • With the change applied: FAILED on all three executions — Snapshot different than baseline: TitleBarTrailingContentShouldRenderProperly.png (size differs - baseline is 789x592 pixels, actual is 2560x1600 pixels).

Two separate problems:

  1. The baseline does not match the capture method. 789x592 is the logical-point Catalyst window size (existing snapshots/mac/*.png are 789x563 = the same window with the default cropFromTop: 29, which includeTitleBar: true sets to 0 — see UITest.cs:432). The actual capture is 2560x1600, i.e. the full physical/Retina screen. A size mismatch means the baseline was produced by a different capture path than the one CI executes, so the comparison never even reaches pixel diffing. The .png baselines must be regenerated from an actual run of this test on the CI capture path.
  2. Even once the sizes align, this assertion cannot prove the fix. A whole-window pixel diff of a page whose only body content is a wrapping paragraph label is dominated by unrelated pixels; it will fail for any incidental rendering change and passes/fails for reasons unrelated to trailing-content safe-area padding. Consider cropping to the title-bar strip (cropBottom) or asserting the trailing element's rect/position via Appium instead of a full-window snapshot.

As submitted, CI cannot be green and the change is unverified.

@MauiBot MauiBot Aug 20, 2026

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.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention and Test Coverage — The committed Mac baseline cannot match what this call captures, so the test fails regardless of the fix. TestCases.Mac.Tests/snapshots/mac/TitleBarTrailingContentShouldRenderProperly.png is 789x592, while the run captures 2560x1600 (the gate run failed with exactly this size mismatch, with the fix applied, in all three attempts). A size mismatch means the capture method differs from how the baseline was produced — includeTitleBar: true captures the full window/screen at device resolution, not the scaled app-view image the baseline was recorded from. The Windows baseline is a third size (1010x761), which reinforces that these were not recorded through the CI capture path.

Re-record both baselines from an actual VerifyScreenshot(includeTitleBar: true) run on the CI image before this can be considered validated; as submitted, the PR's only evidence of the fix is a test that never passes.

}
}
#endif
Binary file added ...s.WinUI.Tests/snapshots/windows/TitleBarTrailingContentShouldRenderProperly.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading