Skip to content

Fix ShellContent badge propagation#35462

Open
jfversluis wants to merge 1 commit into
net11.0from
jfversluis/shell-badges-net11
Open

Fix ShellContent badge propagation#35462
jfversluis wants to merge 1 commit into
net11.0from
jfversluis/shell-badges-net11

Conversation

@jfversluis
Copy link
Copy Markdown
Member

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Summary

  • Bind BadgeText, BadgeColor, and BadgeTextColor from ShellContent to the implicit ShellSection created for TabBar items.
  • Add unit coverage for initial and runtime badge property propagation.

Fixes #35448

Testing

  • bash ./eng/common/dotnet.sh test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj -f net11.0 --filter FullyQualifiedName~ShellBadgeTests --verbosity minimal -p:IncludeAndroidTargetFrameworks=false -p:IncludeIosTargetFrameworks=false -p:IncludeMacCatalystTargetFrameworks=false -p:IncludeWindowsTargetFrameworks=false -p:IncludeTizenTargetFrameworks=false -p:IncludeMacOSTargetFrameworks=false

Bind badge properties when ShellContent is converted into an implicit ShellSection so badges set directly on ShellContent update the generated section.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35462

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35462"

@github-actions github-actions Bot added the t/bug Something isn't working label May 15, 2026
@PureWeen PureWeen added the area-controls-shell Shell Navigation, Routes, Tabs, Flyout label May 15, 2026
PureWeen added a commit that referenced this pull request May 15, 2026
Addresses multi-model analysis of eval failures:
- Rewrite all rubrics to test final label output, not reasoning technique
- Remove duplicate PR #35462 usage; replace cross-platform scenario
  with issue #35448 (Shell badge, no platform specified)
- Broaden noop assertion regex to accept more valid phrasings
- Remove skill-specific vocabulary from rubric criteria
- Merge Shell + no-platform assertions into one scenario

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 19, 2026

/review -b feature/regression-check -p android

1 similar comment
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 19, 2026

/review -b feature/regression-check -p android

@MauiBot MauiBot added s/agent-review-incomplete s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels May 19, 2026
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 19, 2026

/review -b feature/regression-check -p android

@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 20, 2026

/review -b feature/regression-check -p ios

@MauiBot
Copy link
Copy Markdown
Collaborator

MauiBot commented May 20, 2026

🤖 AI Summary

👋 @jfversluis — new AI review results are available. Please review the latest session below.

📊 Review Session48a1da6 · Fix ShellContent badge propagation · 2026-05-20 19:49 UTC
🚦 Gate — Test Before & After Fix

Gate Result: ❌ FAILED

Platform: IOS · Base: net11.0 · Merge base: a8f27932

🩺 Fix does not pass the tests — every test still fails after applying the fix. The PR's change does not resolve the failure(s).

Test Without Fix (expect FAIL) With Fix (expect PASS)
🧪 ShellBadgeTests ShellBadgeTests ✅ FAIL — 5s ❌ FAIL — 4s
🔴 Without fix — 🧪 ShellBadgeTests: FAIL ✅ · 5s
  Determining projects to restore...
/Users/cloudtest/vss/_work/1/s/.dotnet/sdk/10.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(188,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 11.0.  Either target .NET 10.0 or lower, or use a version of the .NET SDK that supports .NET 11.0. Download the .NET SDK from https://aka.ms/dotnet/download [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]

🟢 With fix — 🧪 ShellBadgeTests: FAIL ❌ · 4s
  Determining projects to restore...
/Users/cloudtest/vss/_work/1/s/.dotnet/sdk/10.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(188,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 11.0.  Either target .NET 10.0 or lower, or use a version of the .NET SDK that supports .NET 11.0. Download the .NET SDK from https://aka.ms/dotnet/download [/Users/cloudtest/vss/_work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]

⚠️ Failure Details

  • ShellBadgeTests FAILED with fix (should pass)
📁 Fix files reverted (1 files)
  • src/Controls/src/Core/Shell/ShellSection.cs

🧪 UI Tests — Shell

Detected UI test categories: Shell

🧪 UI Test Execution Results

⏭️ SKIPPED — 0 passed, 0 failed, 1 skipped (platform: ios)

Category Result Tests Duration Notes
Shell ⏭️ SKIPPED 0s Runner threw an exception

Failures here are informational only — they do not block the gate or affect try-fix candidate scoring.


🔍 Pre-Flight — Context & Validation

Issue: #35448 - Shell Badge propagation isn't working
PR: #35462 - Fix ShellContent badge propagation
Base branch: net11.0 · Author: @jfversluis
Platforms Affected: iOS, Android (per issue), Windows (uses same pattern)
Files Changed: 1 implementation (src/Controls/src/Core/Shell/ShellSection.cs, +3), 1 test (src/Controls/tests/Core.UnitTests/ShellBadgeTests.cs, +48)

Issue summary

When badge properties are set directly on a ShellContent element nested in a TabBar (without an explicit Tab/ShellSection wrapper), the badge does not display. With an explicit <Tab> wrapper that carries the badge properties, it works. Reported on .NET 11 Preview 4, iOS 26.2 simulator. Issue also annotated as affecting Android.

Root cause

When ShellContent is added directly to a TabBar (or ShellItem), ShellSection.CreateFromShellContent(...) wraps it in an implicit ShellSection. The platform code (iOS ShellItemRenderer.UpdateTabBarItemBadge, Android ShellItemRenderer.UpdateShellSectionBadge) reads badge properties from the ShellSection, not the contained ShellContent. Existing code wires bindings for Title, Icon, and FlyoutIcon from ShellContent → ShellSection, but not for BadgeText, BadgeColor, BadgeTextColor. Hence the badges set on ShellContent are silently dropped.

PR's fix (Candidate PR)

Adds three SetBinding calls in ShellSection.CreateFromShellContent mirroring the existing Title/Icon/FlyoutIcon bindings — propagates BadgeText, BadgeColor, BadgeTextColor from ShellContent to the implicit ShellSection. Pure Core/Controls change; no platform touches; consistent with established pattern.

Code review (light, by main agent)

  • Strengths: minimal, surgical, matches existing convention for sibling properties; unit tests cover initial set, runtime changes, and clear-to-null cases.
  • Concerns: None significant. A user who explicitly wraps ShellContent in a <Tab> AND sets badge on both will now hit the existing current.CurrentItem = shellContent branch (no binding wired — preserves prior behavior). Acceptable.
  • Test coverage gap: no UI/device test that actually verifies the badge renders on iOS/Android via the platform renderers — but unit tests sufficiently cover the Core fix mechanism.

Gate result

❌ FAILED due to environment blocker (no .NET 11 SDK installed: NETSDK1045: The current .NET SDK does not support targeting .NET 11.0). Both "without fix" and "with fix" runs failed at restore time, not at the assertion layer. The fix's correctness cannot be empirically confirmed locally in this environment.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #35462 Add SetBinding for BadgeText/BadgeColor/BadgeTextColor in ShellSection.CreateFromShellContent ⏳ Gate blocked (env: no net11 SDK) ShellSection.cs Original PR — matches existing Title/Icon/FlyoutIcon convention

🔧 Fix — Analysis & Comparison

Try-Fix Aggregated Narrative — PR #35462

Gate context

Gate ❌ FAILED due to environment blocker (NETSDK1045: The current .NET SDK does not support targeting .NET 11.0). Both without-fix and with-fix runs failed at SDK restore, not at the assertion layer. No candidate can be empirically validated locally; all candidates below are analytically reviewed by the maui-expert-reviewer agent.

Summary of candidates

# Approach Files Covers reported case Covers explicit-<Tab> case Verdict vs PR
PR Add SetBinding for BadgeText/Color/TextColor in CreateFromShellContent (mirrors Title/Icon pattern) 1 Core Baseline
1 OnChildAdded forwarding in ShellSection (PropertyChanged subscription + IsSet-guarded mirror) 1 Core Strictly better if explicit-<Tab> coverage is in scope; otherwise equivalent at higher line count
2 Pull-model in platform renderers (resolve badge from CurrentItem when section unset) 3 platform Worse — breaks PR unit tests, large surface, lifecycle hazards
3 Push from ShellContent via BaseShellItem propertyChanged callback + Routing.IsImplicit parent check 1 Core Worse — layering violation, no scenario gain over PR

Iteration story

  1. Candidate 1 (OnChildAdded) generated by expert reviewer as the most architecturally clean alternative. Provides scope expansion (covers explicit <Tab> wrap) at one-file cost. No new empirical failures predicted; would pass the PR's two new unit tests.
  2. Candidate 2 (pull model) explored to validate whether avoiding Core mirroring entirely was viable. Concluded it isn't worth the surface and breaks the PR's existing unit-test assertion shape.
  3. Candidate 3 (push from BaseShellItem) explored as an alternative single-file fix. Rejected for layering and zero coverage gain over PR. No further meaningfully-different approaches identified — additional variations (e.g., custom propagation extension, attached property, separate BadgePropagator service) all reduce to one of these three patterns or add complexity without behavioral benefit. Stopping here to avoid trivial variations.

Recommendation

Accept PR as-is. It is a minimal, surgical, conventional fix mirroring an established pattern (Title/Icon/FlyoutIcon bindings in the same factory). Unit tests are appropriate.

Optional improvement (single follow-up): If symmetric coverage for the explicit <Tab><ShellContent BadgeText="3"/></Tab> form is desired, swap PR's 3-line binding addition for Candidate 1's ~25-line OnChildAdded forwarding. The single-file scope remains the same; coverage doubles.

Empirical caveat: Local gate could not run because of the missing .NET 11 SDK. The PR author validated locally per the PR description (running Controls.Core.UnitTests on net11.0). Recommend re-running gate in a CI environment with the correct SDK to confirm.

Key risk flags raised by expert reviewer (apply to ANY candidate above)

  • Unit tests are Core-property-mirroring only; no device-level snapshot confirming the badge actually renders on iOS/Android. Issue specifically calls out iOS 26.2 simulator + Xcode 26.4 behavior, which may have its own rendering-path quirks unrelated to property propagation.
  • Windows path (ShellItemHandler.Windows.cs:284–351) reads from bsi.BadgeText (the section); PR's fix populates that correctly, but no explicit Windows test exists.
  • Memory lifetime: PR's bindings hold a strong source reference from the implicit ShellSection to ShellContent, but ShellContent is already held as a Logical child (Items.Add(shellContent) at line 307), so net effect is no new leak.

See try-fix-1/content.md, try-fix-2/content.md, try-fix-3/content.md for full per-candidate analyses including diffs, pros/cons, and failure-mode reasoning.


📋 Report — Final Recommendation

Comparative Report — PR #35462 Candidates

Issue: #35448 — Shell Badge propagation isn't working
PR: #35462 — Fix ShellContent badge propagation (@jfversluis, base net11.0)
Platform: iOS (also Android, Windows by inspection)
Gate: ❌ FAILED — environment blocker (NETSDK1045, no .NET 11 SDK locally). All test verdicts are analytical, not empirical.

Candidates

# Candidate Files Lines Covers implicit <TabBar><ShellContent> Covers explicit <Tab><ShellContent> Existing PR tests pass New tests added Risk
A pr 1 Core +3 3 (Core unit) Very low
B pr-plus-reviewer 1 Core + 1 test +14 / +~30 test 3 + 3 (Core unit) Low
C try-fix-1 (OnChildAdded) 1 Core ~+25 (none added) Low-medium (subscription lifecycle)
D try-fix-2 (platform pull-model) 3 platform large ❌ — breaks PR's assertions (none added) High (3 platforms, lifecycle, breaks tests)
E try-fix-3 (BaseShellItem propertyChanged) 1 Core ~+15 (none added) Medium (layering violation)

Empirical test status (gate-derived)

  • Local gate could not run any candidate end-to-end (missing .NET 11 SDK is a pre-condition for both "without fix" and "with fix" runs). No candidate failed regression tests empirically.
  • Analytical prediction of regression-test failure:
    • Candidate D (try-fix-2) is the only one that would fail the PR's existing unit tests (BadgePropertiesPropagateFromShellContentToImplicitShellSection asserts shellSection.BadgeText == "3" — a pull-model leaves that as null). Per the task's ranking rule, D is ranked last.
    • All other candidates would pass the PR's three new unit tests.

Per-candidate evaluation

A — pr (the as-submitted fix)

  • Pros: Surgical 3-line change. Matches the existing Title/Icon/FlyoutIcon pattern on the lines directly above. No new lifecycle machinery, no new bindings beyond what already lives in the same factory. Tests cover initial set, runtime mutation, and clear-to-null.
  • Cons: Doesn't cover the symmetric <Tab><ShellContent BadgeText=…></Tab> form — the same bug class can be re-reported by wrapping the ShellContent in a Tab.
  • Verdict: Solid minimum-viable fix. Approve-as-is is defensible.

B — pr-plus-reviewer (PR + reviewer feedback applied)

  • Pros: Extends PR's bindings into the early-return branch (current.Items.Contains(shellContent) path), guarded by !current.IsSet(...) so author-set badges on <Tab> are preserved. Adds 3 regression tests (Shell-shaped + explicit-Tab + author-wins). Adds an explanatory comment so future maintainers don't strip the OneWay bindings as a perceived copy-paste mistake. Still single production file, still binding-based — no new lifecycle code.
  • Cons: Slightly larger diff (+14 prod, +~30 test). The IsSet guard runs once at construction time, so a user who first creates the Tab with an explicit value and then clears it to null won't get the ShellContent's value falling through — same quirk as PR.
  • Verdict: Strictly better than PR for the same risk profile.

C — try-fix-1 (OnChildAdded forwarding)

  • Pros: Single-file, covers both implicit and explicit cases. Reacts to CurrentItem swaps naturally.
  • Cons: Replaces a 3-line binding addition with ~25 lines of subscription machinery (OnChildAdded/OnChildRemoved + handler + SyncBadgesFromCurrentItem). Manual +=/-= lifecycle is more error-prone than declarative bindings. Doesn't add new tests. Less idiomatic given that the file already uses bindings for Title/Icon/FlyoutIcon two lines above.
  • Verdict: Architecturally fine but pays a higher complexity tax for the same coverage that Candidate B achieves declaratively.

D — try-fix-2 (platform pull-model)

  • Pros: No Core mutation — ShellSection.BadgeText reads exactly what was written. Per-property IsSet semantics allow mixed authoring.
  • Cons: Touches three platform files (iOS + Android + Windows). Requires re-subscription to CurrentItem.PropertyChanged on every CurrentItem change — high leak risk in three independent code paths. Breaks the PR's existing unit tests that assert Core-property mirroring. Subtle debuggability hazard: ShellSection.BadgeText would report a different value from what's rendered.
  • Verdict: Worst candidate. Per the task ranking rule (regression-test failures rank lower than passes), D goes to the bottom.

E — try-fix-3 (BaseShellItem propertyChanged)

  • Pros: Single-file, no subscriptions to manage. Works even if ShellContent is reparented later (rare).
  • Cons: Layering violation — BaseShellItem (the base) inspecting ShellSection (a derived) and Routing.IsImplicit to perform a structural check reverses the dependency direction. Explicitly excludes the <Tab> case (via Routing.IsImplicit), so no coverage gain over PR. Adds property-changed callback overhead to every BaseShellItem subclass on every badge mutation.
  • Verdict: Worse than PR. No upside, real downside.

Ranking (best → worst)

  1. pr-plus-reviewer — same risk profile as PR, broader coverage, better tests, clarifying comment.
  2. pr — minimum-viable, surgical, conventional. Acceptable as-is.
  3. try-fix-1 — equivalent coverage to B but higher line count and lifecycle complexity for no extra benefit.
  4. try-fix-3 — no scenario gain over PR, layering violation.
  5. try-fix-2 — breaks PR's existing unit-test assertions (analytical), 3-platform blast radius, lifecycle hazards. Ranked last per the regression-test-failure rule.

Winner

pr-plus-reviewer — it is Pareto-superior to pr: same one-file production scope, same declarative binding mechanism the file already uses, but it (a) closes the symmetric <Tab> gap that would predictably generate a follow-up issue, (b) preserves author-set explicit values via IsSet guards, (c) adds 3 Shell-shaped regression tests, and (d) leaves a maintainer-facing comment. The empirical caveat (gate blocked on missing .NET 11 SDK) applies equally to every candidate, so it does not break the tie.

Recommended action

Apply Candidate B's diff on top of PR #35462 before merging (or leave inline comments via inline-findings.json requesting these changes). If shipping speed is critical, accepting pr as-is is acceptable with a follow-up issue tracking the explicit-<Tab> symmetric case.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-controls-shell Shell Navigation, Routes, Tabs, Flyout s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) t/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants