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 all commits
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
13 changes: 12 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,17 @@ internal static void IgnoreLayoutSafeArea(this Layout layout)
{
IgnoreLayoutSafeArea(childLayout);
}
else if (child is IContentView contentView)

@MauiBot MauiBot Jul 17, 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] Safe Area — This only handles IContentView nodes after IgnoreLayoutSafeArea(Layout) has already been entered. TitleBar still starts suppression with (newValue as Layout)?.IgnoreLayoutSafeArea() for its Leading/Content/Trailing slots, so a slot whose root is an IContentView but not a Layout (for example TrailingContent = new Border { Content = new StackLayout(...) }) never reaches this new recursion and the inner layout keeps obeying the title-bar safe area. Please add an entry point that handles IView/IContentView roots and call that from the TitleBar property changed handlers.

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)

[major] Logic and Correctness — This only handles IContentView when it is a child of a Layout, but TitleBar.LeadingContent, Content, and TrailingContent are typed as IView and TitleBar still calls the helper only via (newValue as Layout)?.IgnoreLayoutSafeArea(). A valid title-bar slot such as TrailingContent = new Border { Content = new StackLayout(...) } has an IContentView root, so the helper is never invoked and the nested layout still receives safe-area adjustment. Please invoke the helper for IContentView roots as well, or otherwise normalize all title-bar slot roots through the same recursive path.

@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] Logic and Correctness — The new else if (child is IContentView contentView) block only traverses one IContentView level. If PresentedContent/Content is itself an IContentView rather than a Layout, recursion stops and any Layout nested within is silently skipped.

Concrete gap: HorizontalStackLayout { ContentView { Border { StackLayout } } } — ContentView.PresentedContent == ContentView.Content == Border (not a Layout), so neither branch fires and StackLayout never receives IgnoreSafeArea = true. The fix is correct for the reported one-level case (HorizontalStackLayout > Border > StackLayout) but leaves two-level IContentView chains unprotected.

Consider adding a recursive helper so that when PresentedContent/Content is an IContentView (rather than a Layout), the same walk continues into it:

static void IgnoreContentViewSafeArea(IContentView contentView)
{
    var inner = contentView.PresentedContent ?? contentView.Content;
    if (inner is Layout innerLayout)
        IgnoreLayoutSafeArea(innerLayout);
    else if (inner is IContentView innerContentView)
        IgnoreContentViewSafeArea(innerContentView);
}

@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)

[major] Logic and Correctness — This only processes IContentView when it is already a child of a Layout, but TitleBar's LeadingContent/Content/TrailingContent properties are typed as IView and their handlers still enter this helper only via (newValue as Layout)?.IgnoreLayoutSafeArea(). A valid slot value such as TrailingContent = new Border { Content = new StackLayout { ... } } is therefore skipped entirely, so the Mac titlebar safe-area/alignment bug still reproduces for root IContentView wrappers. Please make the entry point handle IContentView (for example on IView) instead of only Layout roots.

@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] Logic and Correctness — The traversal is still rooted on Layout, so the new IContentView branch only helps when the content view is a direct child of a Layout. The only callers are TitleBar.cs:111/176/201, which all do (newValue as Layout)?.IgnoreLayoutSafeArea(). That means the reported scenario written the natural way — TrailingContent = new Border { Content = new HorizontalStackLayout { ... } } or TrailingContent = new ContentView { ... } — never enters this method at all and is still broken after the fix. The added HostApp repro (Issue29516.cs) sidesteps this by wrapping everything in a HorizontalStackLayout first, so the test shape hides the gap. Either change the recursion to accept IView/IContentView (e.g. internal static void IgnoreLayoutSafeArea(this IView view) with a Layout / IContentView dispatch) and update the three TitleBar call sites, or document why only Layout-rooted content is supported.

@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] Logic and Correctness / Layer Placement — The new IContentView branch only helps nested children; the entry points into this helper are still gated on as Layout, so a root-level IContentView slot value is still never traversed.

TitleBar.LeadingContent, Content, and TrailingContent are typed IView, and all three callbacks call (newValue as Layout)?.IgnoreLayoutSafeArea() (TitleBar.cs:111, :176, :201). Concrete failing scenario — the reported shape with one less wrapper level:

TrailingContent = new Border { Content = new StackLayout { Children = { new Label { Text = "Trailing" } } } }

Border is an IContentView, not a Layout, so newValue as Layout is null, the helper is never invoked, and the inner StackLayout keeps IgnoreSafeArea == false — i.e. the exact macOS misalignment from #29516 still reproduces. The same applies to ContentView, Frame, and ContentPresenter roots. The helper now knows how to walk IContentView but the callers cannot reach it, so the fix is shape-dependent: it works only because the repro happens to wrap the Border in a 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)

⚠️ Warning — Formatting: this hunk breaks the repo's tab indentation and drops the final newline.

Raw bytes of the added lines (verified with od -c):

  • L17 is \t\t + 4 spaces + else if (...) instead of \t\t\t, so the else if no longer lines up with the if (child is Layout ...) on L13.
  • L20–L26 each start with a leading space before their tabs ( \t\t\t\t{, \t\t\t\t\tIgnoreLayoutSafeArea(...), …).
  • The file now ends ...\t}\n} with no trailing newline (the diff shows \ No newline at end of file), a gratuitous change to a line this PR otherwise doesn't touch.

.editorconfig sets [*.cs] indent_style = tab, and every pre-existing line in this file uses tabs only. This is mechanical to fix (re-indent L17–L26 with tabs, restore the EOF newline) but it will show up in whitespace/format verification and makes the new block visually misaligned for every subsequent reader.

@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] Logic and Correctness — the recursion still terminates at any non-Layout node, so the fix only covers exactly one IContentView hop and only when that hop's content is a Layout. Two cases in the same bug class remain broken:

  1. Border → ContentView → Grid (ContentView-in-ContentView): the inner ContentView is not a Layout, so IgnoreLayoutSafeArea is never called for the Grid.
  2. TitleBar.TrailingContent = new Border { Content = new HorizontalStackLayout { ... } }: the three call sites (TitleBar.cs:111, :176, :201) do (newValue as Layout)?.IgnoreLayoutSafeArea(), so a non-Layout root is skipped entirely and nothing in the subtree is touched.

This is why the added UI test passes: HorizontalStackLayout → Border → StackLayout is the single shape the new else if handles. Recommend making the walk operate on IView (handle Layout and IContentView uniformly, recursing until leaves) and having the TitleBar call sites pass any IView, so arbitrary nesting is covered instead of one special case.

@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] Architectural Layer / Cross-Platform Consistency — The root entry point was not updated, so a non-Layout container assigned directly to a TitleBar slot still gets no traversal at all.

All three call sites are guarded by an as Layout cast:

  • src/Controls/src/Core/TitleBar/TitleBar.cs:111 — OnLeadingChanged: (newValue as Layout)?.IgnoreLayoutSafeArea();
  • src/Controls/src/Core/TitleBar/TitleBar.cs:176 — OnContentChanged: same
  • src/Controls/src/Core/TitleBar/TitleBar.cs:201 — OnTrailingContentChanged: same

This PR teaches the walker to step through IContentView children, but the root is still required to be a Layout. So the very common shape

<TitleBar.TrailingContent>
  <Border>
    <HorizontalStackLayout>...</HorizontalStackLayout>
  </Border>
</TitleBar.TrailingContent>

is silently a no-op — Border is View, IContentView (Border.cs:20), not a Layout, so as Layout yields null and nothing is walked. The user-visible symptom is identical to the reported bug.

The fix is incomplete/asymmetric: either change the extension to accept IView/Element and update the three TitleBar call sites, or add an IContentView overload. Without this, whether the bug reproduces depends on whether the user happened to wrap their content in a StackLayout first.

@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] Logic and Correctness / Safe Area — The new IContentView branch only descends one hop and only when the content happens to be a Layout. If the content is itself a non-Layout IContentView, the walk terminates and IgnoreSafeArea never reaches the inner layout.

Concrete failing scenario (a small variation of this PR's own repro):

TrailingContent = new HorizontalStackLayout {
    Children = {
        new Border { Content = new Border { Content = new StackLayout { ... } } }
    }
}

Outer Border → IContentView branch → Content is the inner Border, which is neither a Layout (it is Border : View, IContentView, ...) nor matched by any branch → recursion stops → the inner StackLayout keeps IgnoreSafeArea == false and is still inset on macCatalyst. Same for ContentView/Frame/ContentPresenter wrapping another content view.

Recommendation: replace the type-specific if/else if chain with a single recursive helper over IView that dispatches on both Layout and IContentView (and recurses into IContentView content regardless of whether it is a Layout), so the traversal is depth-agnostic:

static void IgnoreSafeAreaCore(IView? view)
{
    switch (view)
    {
        case Layout layout:
            layout.IgnoreSafeArea = true;
            foreach (var child in layout.Children)
                IgnoreSafeAreaCore(child);
            break;
        case IContentView cv:
            IgnoreSafeAreaCore((cv.PresentedContent ?? cv.Content) as IView);
            break;
    }
}

@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] Logic and Correctness — This traversal only descends one IContentView hop, and only when that hop's content is already a Layout. Two reachable shapes are still not covered:

  1. Wrapper chains: HorizontalStackLayout > Border > ContentView > StackLayout. The outer Border is an IContentView whose Content is a ContentView (not a Layout), so recursion stops and the inner StackLayout never gets IgnoreSafeArea = true.
  2. Root slot that is not a Layout: the only three call sites are TitleBar.cs:111 / 176 / 201, all of which are (newValue as Layout)?.IgnoreLayoutSafeArea(). Setting TitleBar.TrailingContent = new Border { Content = new StackLayout { ... } } — which is exactly the Border -> StackLayout shape reported in Issue with the TitleBar TrailingContent not being properly aligned on macOS. #29516 — is a Border, not a Layout, so the extension is never invoked at all and this fix does nothing.

The added HostApp repro (Issue29516.cs) happens to wrap everything in a HorizontalStackLayout root, so it is the one shape that does enter the method — the test cannot detect either gap. Suggest making the traversal a general IView walk (recurse on IContentView regardless of whether its content is a Layout, and on Layout children), and changing the TitleBar call sites to accept any IView root rather than as Layout.

@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] Logic and Correctness / Layout Safe Area: the walk still only re-enters through Layout, so the traversal terminates at the first non-Layout container in a chain.

Concrete failing scenarios:

  1. TrailingContent = new Border { Content = new Border { Content = new StackLayout {...} } } — the outer Border's Content is a Border, not a Layout, so neither branch matches and the inner StackLayout never gets IgnoreSafeArea. Same for ContentView → Border → Grid, Frame → ScrollView → VerticalStackLayout, etc.
  2. More importantly, the three entry points are (newValue as Layout)?.IgnoreLayoutSafeArea() (src/Controls/src/Core/TitleBar/TitleBar.cs:145, :210, :235). If LeadingContent/Content/TrailingContent is set to a Border, ContentView, or ScrollView directly — a very common TitleBar shape, and arguably the shape the reported issue describes — the extension is never invoked at all and this PR changes nothing for it.

The fix only covers the exact one-level Layout → IContentView → Layout shape used by the new HostApp repro. Suggest restructuring as an IView-based recursive walk (set IgnoreSafeArea when the node is a Layout, then recurse into Layout.Children, IContentView.PresentedContent/Content, and IBorderView content regardless of whether the intermediate node is a Layout), and changing the TitleBar call sites so a non-Layout root is also traversed.

@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] Logic and Correctness — The traversal still terminates on nested content-view chains. The new branch only recurses when the content view's content is a Layout; when it is another IContentView, no recursion happens and the subtree below it never gets IgnoreSafeArea = true.

Concrete failing scenario (same shape as #29516, one level deeper):

TrailingContent = new HorizontalStackLayout {
    Children = { new Border { Content = new ContentView { Content = new StackLayout {
        Children = { new Label { Text = "TrailingContent" } } } } } }
};

Border matches IContentView, but its content is a ContentView (not a Layout), so both new if/else if arms miss and the inner StackLayout keeps IgnoreSafeArea = false — the original misalignment reproduces. Border → Border → layout fails the same way.

Suggested shape: make the walk element-typed rather than layout-typed, so Layout and IContentView are handled by the same recursive step:

static void IgnoreSafeAreaForView(IView? view)
{
    if (view is Layout layout)
    {
        layout.IgnoreSafeArea = true;
        foreach (var child in layout.Children)
            IgnoreSafeAreaForView(child as IView);
    }
    else if (view is IContentView contentView)
    {
        IgnoreSafeAreaForView((contentView.PresentedContent ?? contentView.Content) as IView);
    }
}

@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)

[major] Logic and Correctness Verification — The traversal is extended one hop deep, but the three entry points that call it are still gated on Layout: TitleBar.cs:145, TitleBar.cs:210, and TitleBar.cs:235 all do (newValue as Layout)?.IgnoreLayoutSafeArea();. Concrete failing scenario: <TitleBar.TrailingContent><Border><StackLayout>… (a Border assigned directly as TrailingContent, without an outer HorizontalStackLayout) — newValue as Layout is null, the extension method is never invoked, and the nested StackLayout keeps IgnoreSafeArea = false, reproducing exactly the misalignment #29516 reports. This is the same class of bug the PR is fixing, left unfixed one level up. The repro page added in this PR happens to wrap the Border in a HorizontalStackLayout (Issue29516.cs:34), so the test cannot detect this gap. Fixing this requires either changing the callers to accept IView/IContentView, or moving the entry-point dispatch into this helper.

{
if (contentView.PresentedContent is Layout presentedLayout)

@MauiBot MauiBot Jul 18, 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] Safe Area / Logic and Correctness — This only handles the Layout > IContentView > Layout shape. The TitleBar slots accept any IView, but the callers still invoke this helper via (newValue as Layout)?.IgnoreLayoutSafeArea(), so a valid root such as TitleBar.TrailingContent = new Border { Content = new StackLayout(...) } never enters the traversal. It also stops after one wrapper: HorizontalStackLayout > Border > ContentView > StackLayout misses the inner layout because neither PresentedContent nor Content of the Border is itself a Layout. Please make the traversal operate recursively on IView/IContentView and call it for all non-null TitleBar content, not only Layout roots.

@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)

[major] Safe Area and Window Insets — The recursion stops unless the immediate PresentedContent/Content is a Layout. Nested wrapper chains that are valid MAUI content, for example HorizontalStackLayout > Border > ContentView > StackLayout or HorizontalStackLayout > Border > Border > StackLayout, leave the inner layout unchanged because the first wrapper's presented content is another IContentView, not a Layout. The traversal needs to continue through IContentView nodes until it reaches layouts, otherwise common TitleBar compositions still render with the unwanted safe-area offset.

@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] Logic and Correctness — Recursion terminates at the first non-Layout node, so nesting deeper than one content level is silently skipped. Concrete failure: HorizontalStackLayout > Border > ContentView > VerticalStackLayout > Label. The Border is matched here, its PresentedContent is the ContentView which is not a Layout, so neither IgnoreLayoutSafeArea nor the else if on Content runs, and the inner VerticalStackLayout never gets IgnoreSafeArea = true — exactly the bug this PR claims to fix, one level deeper. The added repro uses precisely one content level (HorizontalStackLayout > Border > StackLayout), so it cannot detect this. The recursion should walk any IContentView/IView child rather than only re-entering on Layout.

@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] Logic and Correctness — Traversal terminates at the first IContentView whose content is not itself a Layout, so nested content chains are only handled exactly one level deep.

Both branches require is Layout before recursing, and there is no IContentView -> IContentView recursion. Concrete failing scenario:

TrailingContent = new HorizontalStackLayout {
    Children = { new Border { Content = new ContentView { Content = new StackLayout { /* ... */ } } } }
}

Border.PresentedContent is a ContentView (not a Layout), Border.Content is also not a Layout, so both if/else if fail and the inner StackLayout never gets IgnoreSafeArea = true. Extracting the walk over IView/IContentView/Layout (mutually recursive, not Layout-rooted) would cover the general case.

Secondary issue on the else if at line 23: when PresentedContent is non-null but not a Layout (a ControlTemplate-driven ContentView, or any templated content whose root is a Border/Label), the code silently falls back to the logical Content, which is not the tree that is actually rendered. That walks the wrong subtree while leaving the presented one untouched. For an untemplated ContentView, PresentedContent and Content are the same object, so the fallback is dead in the common case and only fires in exactly the situation where it is wrong.

Also note this helper permanently mutates user-owned content and is never reverted when the slot is cleared or the subtree is reparented (oldValue path does not reset IgnoreSafeArea). That was already true for direct Layout slots, but this change pushes the sticky mutation deeper into arbitrary nested user content.

@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)

⚠️ Warning — The new content-view descent is only one hop deep: it stops at the first non-Layout content, so nested content-view chains still keep IgnoreSafeArea = false.

Both new branches require the content to be a Microsoft.Maui.Controls.Layout. When it is any other IContentView (or a Compatibility.Layout), recursion terminates and the subtree below is never visited — the same class of gap this PR is fixing.

Concrete failure modes (all valid TitleBar.TrailingContent/Content/LeadingContent trees):

  1. Two content-view hops — a small variation on the very tree added in Issue29516.cs:
TrailingContent = new HorizontalStackLayout {          // IgnoreSafeArea = true
    Children = { new Border {                          // IContentView -> PresentedContent = inner Border
        Content = new Border {                          // not a Layout -> recursion stops here
            Content = new StackLayout { ... }           // IgnoreSafeArea stays false
        } } } }

Border.PresentedContent => Content (Border.cs:360), so presentedLayout fails, and contentView.Content is that same inner Border — also not a Layout — so the fallback fails too and the inner StackLayout is skipped.

  1. HorizontalStackLayout { Children = { new ScrollView { Content = new Border { Content = new Grid { ... } } } } } — same termination at the Border.

  2. ContentPresenter derives from Compatibility.Layout, not Controls.Layout (ContentPresenter.cs:14), so any templated content reached through a presenter also fails the is Layout test.

Suggested shape: recurse on the IView itself rather than only on Layout, e.g. a helper that takes IView and dispatches — Layout → set flag + walk Children; IContentView → walk PresentedContent ?? Content — so the walk continues through arbitrary nesting instead of exactly one content-view level. (The same reasoning applies to the three (newValue as Layout)?.IgnoreLayoutSafeArea() call sites in TitleBar.cs L111/L176/L201, which silently no-op when the assigned content is itself a Border/ContentView rather than a Layout.)

@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] Handler Mapper and Property Patterns / Logic and Correctness — this walk is a one-shot executed from the TitleBar property-changed callbacks (TitleBar.cs:111/176/201), i.e. at the moment Content/LeadingContent/TrailingContent is assigned. Extending it into IContentView makes it timing-dependent in a way the previous Layout-only walk was not:

  • PresentedContent for a templated ContentView is TemplateRoot ?? Content — TemplateRoot is null until the control template is applied (parent/handler attach), which normally happens after the property assignment. At callback time this silently falls through to a possibly-null Content and the whole subtree is skipped.
  • In XAML, object-initializer, and data-bound flows the inner ContentView.Content is frequently set after the outer container is assigned to the TitleBar; nothing re-runs the walk, so the deep nodes never get IgnoreSafeArea.

This produces order-dependent behavior (works in the codebehind sample added here, silently no-ops for XAML/templated content). Consider applying the walk on child-added / content-changed / handler-connect instead of only at assignment time.

@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)

❌ [critical] Logic and Correctness / Layout — Recursion terminates on the first non-Layout presented content, so nested content containers are still missed.

This branch only descends when PresentedContent is itself a Layout. Concrete failing scenario:

<TitleBar.TrailingContent>
  <HorizontalStackLayout>
    <Border>
      <ContentView>          <!-- Border.PresentedContent == this ContentView -->
        <Grid>...</Grid>      <!-- never reached -->
      </ContentView>
    </Border>
  </HorizontalStackLayout>
</TitleBar.TrailingContent>

Border implements IContentView with PresentedContent => Content (src/Controls/src/Core/Border/Border.cs:360). Here PresentedContent is a ContentView, which is not a Microsoft.Maui.Controls.Layout (ContentView : TemplatedView : Compatibility.Layout, see src/Controls/src/Core/ContentView/ContentView.cs:18). So neither the is Layout test on line 19 nor the Content is Layout test on line 23 matches, and the inner Grid never gets IgnoreSafeArea = true — the exact bug this PR is fixing, one nesting level deeper.

The traversal should be a single generic recursion that handles IView and re-dispatches on both Layout and IContentView at every level, e.g.:

static void IgnoreSafeArea(IView? view)
{
    switch (view)
    {
        case Layout layout:
            layout.IgnoreSafeArea = true;
            foreach (var child in layout.Children)
                IgnoreSafeArea(child);
            break;
        case IContentView contentView:
            IgnoreSafeArea((contentView.PresentedContent ?? contentView.Content as IView));
            break;
    }
}

As written, the fix is scoped to exactly the one shape covered by the new test (Layout → IContentView → Layout).

@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] Safe Area — The fix descends past the intermediate content view without neutralizing that node's own safe-area participation. ContentView is declared public partial class ContentView : TemplatedView, IContentView, ISafeAreaView2, ISafeAreaElement (src/Controls/src/Core/ContentView/ContentView.cs:18) and derives from Compatibility.Layout, not Microsoft.Maui.Controls.Layout — so it never matches the child is Layout branch and never gets IgnoreSafeArea = true.

Result: for TrailingContent = new HorizontalStackLayout { Children = { new ContentView { Content = new StackLayout {...} } } }, the inner StackLayout is opted out, but the enclosing ContentView still reports its own safe-area regions via ISafeAreaView2.GetSafeAreaRegionsForEdge, so insets can still be applied at that node and the trailing content stays mis-aligned. This is a plausible root cause for the gate/UI-test failure on this PR.

Recommendation: when the intermediate node implements ISafeAreaElement/ISafeAreaView2, explicitly set SafeAreaEdges to SafeAreaRegions.None on it (guarded by HasExplicitSafeAreaEdges so an author-set value is not clobbered) in addition to recursing into its content.

@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)

[moderate] Logic and Correctness — The PresentedContent / Content pair is treated as an either-or fallback, but they are not interchangeable. IContentView.PresentedContent is the templated/presented IView that actually participates in layout; IContentView.Content is object? and is the logical value. When a templated ContentView/Border has a non-Layout PresentedContent (e.g. the template root is a Border), the else if falls through and recurses into Content instead — mutating a subtree that is not the one being laid out, while still skipping the presented tree that needed the adjustment. The presented tree should be traversed unconditionally when it is non-null (recursing through non-Layout presented wrappers), rather than silently switching to the logical Content.

@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)

[moderate] Handler Mapper and Property Patterns — This walk is one-shot: IgnoreLayoutSafeArea runs only from TitleBar.OnLeadingChanged/OnContentChanged/OnTrailingContentChanged (TitleBar.cs:145,210,235) at the moment the property is assigned, and it is not re-run afterwards. Two consequences for the newly added IContentView path specifically:

  1. PresentedContent is generally not materialized yet at assignment time (the element is not parented and no ControlTemplate has been applied when a XAML/code-constructed TrailingContent is set), so the first arm silently falls through to Content — the branch the PR relies on is mostly untested in practice.
  2. Any subtree mutated after assignment — border.Content = new StackLayout(...) set later, children appended to the HorizontalStackLayout, or a ControlTemplate applied later — never receives IgnoreSafeArea = true, so the bug returns for dynamically built title-bar content.

If this walk is meant to be authoritative, it needs to re-run on descendant changes (e.g. on ChildAdded/content change of the assigned subtree), or IgnoreSafeArea needs to propagate at layout time rather than being pushed once at property-set time.

@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)

[major] Logic and Correctness Verification — Recursion terminates when an IContentView's content is itself an IContentView rather than a Layout. Concrete failing scenario: TrailingContent = new HorizontalStackLayout { new Border { Content = new Border { Content = new StackLayout { … } } } }. First Border: PresentedContent is a Border (not a Layout) → falls to the else if, where Content is also a Border (not a Layout) → both branches miss and the walk stops. The inner StackLayout never gets IgnoreSafeArea = true. Same failure for Border > ContentView > Layout, ContentView > Border > Layout, and Border > ScrollView > Layout (ScrollView.PresentedContent => Content, ScrollView is not a Controls.Layout). The recursion needs to descend through IContentView chains, not only IContentView → Layout.

{

@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)

[minor] Complexity / hygiene — The added block uses malformed mixed indentation: line 17 is \t\t + 4 spaces before else if, and lines 20-26 are prefixed with a leading space before the tabs. Additionally this PR deletes the trailing newline from the file (\ No newline at end of file on the closing }), which is an unnecessary modification to a production file's last line and adds diff noise. The same missing-EOF-newline applies to both new test files. dotnet format in CI will likely flag this.

@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)

[minor] Complexity Reduction — Mechanical cleanups on the new block: lines 20-26 are indented with a leading space followed by tabs (" \t\t\t\t{") while the rest of the file uses tabs only, and line 17 mixes spaces with tabs ("\t\t else if"). The diff also deletes the trailing newline at end of file (\ No newline at end of file), which is an unrelated change to a line the PR does not otherwise touch. Both trip dotnet format --verify-no-changes. The two new test files have the same issues (4-space indentation in TestCases.Shared.Tests/Tests/Issues/Issue29516.cs, missing EOF newline in both).

@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 — Indentation is inconsistent with the rest of the file and will not survive dotnet format: line 17 begins with two tabs followed by four spaces (\t\t else if), and lines 20–26 each begin with a space before their tabs (\u0020\t\t\t\t{). The file otherwise uses tabs exclusively. This is a mechanical diff artifact, not a preference — it will show up as a formatting-check failure and as spurious whitespace noise in git blame for this block.

IgnoreLayoutSafeArea(presentedLayout);

@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] Backward Compatibility / Logic and Correctness — the walk permanently mutates a user-owned public (obsolete) property and never restores it. Previously the blast radius was "direct Layout descendants of a Layout assigned to TitleBar"; this change extends it to layouts nested inside any IContentView — Border, Frame, ContentView, ContentPresenter, RadioButton, SwipeView, ScrollView, etc.

Failure mode: a user builds a reusable Border-wrapped panel, assigns it to TitleBar.TrailingContent, later removes it and reparents it into a Page. IgnoreSafeArea is still true on the inner layouts (the TitleBar old-value branches only reset BindingContext), so that content now silently draws under the notch/status bar on iOS/MacCatalyst with no way for the user to see why. Recommend restoring the previous value when the content is detached, or tracking this via internal/attached state instead of writing the public property.

@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] Safe Area / Handler Lifecycle — This traversal runs exactly once, synchronously, from OnLeadingChanged/OnContentChanged/OnTrailingContentChanged (src/Controls/src/Core/TitleBar/TitleBar.cs:111,176,201) at the moment the property is assigned. PresentedContent for templated views (ContentView, ContentPresenter, TemplatedView) is only populated when the ControlTemplate is applied — which normally happens after the property setter runs, on parent/handler attach. So the branch added here will observe PresentedContent == null in exactly the templated cases it is meant to cover, and the content realized later is never visited.

Same class of gap for children appended to a titlebar layout after assignment (stack.Children.Add(...)), or border.Content = ... set later — they never receive IgnoreSafeArea. If the intent is to make titlebar content edge-to-edge, this needs to be re-applied on descendant-added / template-applied, not only at property-set time. Please confirm the templated path with a test before merge.

@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)

[moderate] Native Platform Defaults Preservation / Backward Compatibility — This helper mutates user-owned objects by force-setting the obsolete public Layout.IgnoreSafeArea = true, and the mutation is never undone. TitleBar.OnTrailingContentChanged / OnContentChanged / OnLeadingContentChanged (TitleBar.cs:111/176/201) set it on the new value but never clear it on oldValue or when the slot is set to null.

This PR widens the blast radius of that asymmetry: previously only Layout descendants were touched; now arbitrary IContentView content (Border, ContentView, ScrollView, Frame subtrees) is reached too. Concrete scenario: a user builds a Border > StackLayout panel, assigns it to TitleBar.TrailingContent, later reassigns it into the page body — the subtree permanently keeps IgnoreSafeArea = true and will now clip under the notch/home indicator in its new location, with no API for the user to discover why.

Either make it symmetric (clear the flag on the outgoing value in the TitleBar property-changed callbacks) or stop mutating public bindable state and apply the safe-area suppression at layout/handler time instead.

@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] Handler Mapper and Property Patterns / lifecycle: this is a one-shot snapshot taken at TitleBar.LeadingContent/Content/TrailingContent assignment time (TitleBar.cs:145/210/235). Descendants materialized after that assignment never receive IgnoreSafeArea.

Failing scenario: titleBar.TrailingContent = stack; then later border.Content = new StackLayout {...} or stack.Children.Add(new Grid()) — the new subtree keeps IgnoreSafeArea = false and the alignment defect reappears. This pre-exists for Layout.Children, but the new IContentView branch widens the set of shapes where the propagation silently does not apply. Consider driving this from a descendant-added hook (or applying it at layout/attach time) rather than only at property-set time.

@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 — This propagation runs exactly once, at TitleBar.LeadingContent/Content/TrailingContent assignment time. Any subtree mutated afterwards is not covered: e.g. border.Content = new StackLayout{…} assigned after the TitleBar was built, or stack.Children.Add(new Border{ Content = new VerticalStackLayout{…} }). The pre-existing Layout-only walk had the same one-shot characteristic, but this change materially widens the surface (content views are commonly populated lazily / via BindingContext), so the intermittent "works on first render, broken after a content swap" failure mode becomes reachable. There is no test covering re-assignment after the title bar is set. Consider hooking Children/Content change notifications, or documenting the limitation.

}
else if (contentView.Content is Layout contentLayout)

@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)

[major] Logic and Correctness — The new traversal only unwraps one IContentView level when its immediate PresentedContent/Content is a Layout. If users compose a titlebar slot as HorizontalStackLayout -> Border -> ContentView -> StackLayout, the inner layout never gets IgnoreSafeArea applied and the misalignment remains. Recurse through IContentView chains until the layout descendants are reached.

@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] Logic and Correctness — This else if branch is dead for every in-box IContentView and simultaneously masks a real miss. All framework implementations return TemplateRoot ?? Content or Content for PresentedContent (ContentView.cs:78, Border.cs:360, ScrollView.cs:447, ContentPresenter.cs:63, ContentPage.cs:91, RadioButton.cs:744), so whenever Content is a Layout and no template is applied, PresentedContent is that same Layout and the first branch already took it — this line never executes. Conversely, when a ControlTemplate is applied and TemplateRoot is not a Layout, PresentedContent is non-null-but-not-Layout, the else if is skipped, and the user's Content layout is never visited — the opposite of the intent. Both branches should be evaluated (or the traversal should walk PresentedContent generically), not chained with else.

@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] Logic and Correctness — PresentedContent / Content precedence is wrong. The fallback to Content fires whenever PresentedContent is non-null but not a Layout, instead of only when PresentedContent is null.

Concrete scenario: a ContentView with a ControlTemplate whose template root is a Border/ContentPresenter (not a Layout) and whose raw Content is a VerticalStackLayout. ContentView.IContentView.PresentedContent returns TemplateRoot ?? Content (ContentView.cs:78), so here it returns the non-Layout template root. The code then walks the raw Content, which is not the element rendered at that position, and never walks the actually-presented subtree. Result: IgnoreSafeArea is set on a view that is not in the rendered tree while the rendered layout still respects safe area — the exact bug this PR is fixing, just one level deeper.

Suggested shape:

var content = contentView.PresentedContent ?? contentView.Content as IView;

and then walk content once, rather than branching on is Layout twice.

@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] Handler Mapper / Timing & Lifecycle — This traversal runs once, eagerly, from the TitleBar property-changed callbacks; for IContentView the state it reads is frequently not populated yet.

IgnoreLayoutSafeArea is only invoked from OnLeadingChanged / OnContentChanged / OnTrailingContentChanged (TitleBar.cs:111,176,201). For a plain Layout tree built in XAML that is mostly fine (children exist before the parent property is assigned). But the new IContentView branch depends on PresentedContent, which for ContentView is ((this as IControlTemplated).TemplateRoot as IView) ?? Content (src/Controls/src/Core/ContentView/ContentView.cs:78) — TemplateRoot is only populated once the ControlTemplate is applied, which happens on parent/handler attach, i.e. after this callback has already run.

Concrete failure: a ContentView subclass with a ControlTemplate inside the trailing HorizontalStackLayout. At callback time TemplateRoot is null, so line 19 misses and line 23 applies IgnoreSafeArea to the raw Content only; the template root layout (the thing actually measured/arranged) never gets it. Likewise, any content assigned to the ContentView after TitleBar.TrailingContent is set (code-behind, DataTemplate, binding) is never visited — there is no re-walk hook.

Please confirm the intended lifecycle, or move the walk to a point where the content tree is realized (e.g. on descendant-added / handler-connect) rather than a one-shot property-changed pass.

@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] Logic and Correctness — else if (contentView.Content is Layout contentLayout) is unreachable whenever PresentedContent is non-null but not a Layout. For a templated content view whose ControlTemplate root is, e.g., a Border or ContentPresenter, PresentedContent is non-null and non-Layout, so the fallback to Content is silently skipped and neither subtree is visited. The PresentedContent/Content relationship is not mutually exclusive here — prefer (contentView.PresentedContent ?? contentView.Content as IView) and recurse once, or visit both when they differ.

@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] Logic and Correctness: PresentedContent and Content are treated as mutually exclusive alternatives, but they are not.

When a templated ContentView/Border has a ControlTemplate, PresentedContent is the template root, and Content is the user content hosted by the ContentPresenter. If the template root is not a Layout (e.g. the template root is a Border or a Label), PresentedContent is Layout is false, but because it is an else if the Content branch is evaluated — which is accidental and inconsistent; conversely when the template root is a Layout the user Content layout subtree is skipped entirely even though it is a distinct subtree that also needs IgnoreSafeArea.

Both references should be visited (deduplicated by reference equality), not chained with else.

@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] Logic and Correctness Verification — PresentedContent is not resolved yet at the moment this code runs, so the fallback silently no-ops for templated content. ContentView.PresentedContent is ((this as IControlTemplated).TemplateRoot as IView) ?? Content (ContentView.cs:78) and TemplatedView.PresentedContent (TemplatedView.cs:159) is template-root-only; TemplateRoot is null until the ControlTemplate is applied during parenting/measure. This method is invoked synchronously from the TitleBar property-changed callback, i.e. before the assigned subtree is parented. Concrete failing scenario: a ContentView subclass with a ControlTemplate (and no plain Content) inside TrailingContent — at call time PresentedContent is null and Content is null, both branches miss, and when the template root is later materialized nothing re-runs the walk. Same applies to a nested TitleBar (TitleBar.cs:345, PresentedContent => TemplateRoot as IView with no Content fallback).

{
IgnoreLayoutSafeArea(contentLayout);
}
}
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