[XAML] Make OnPlatform/OnIdiom markup extensions data-driven for custom backends#35901
[XAML] Make OnPlatform/OnIdiom markup extensions data-driven for custom backends#35901kubaflo wants to merge 3 commits into
Conversation
The {OnPlatform} and {OnIdiom} XAML markup extensions resolved values via a
hardcoded if-chain over DevicePlatform/DeviceIdiom, and the GTK, macOS and WPF
properties on OnPlatformExtension were internal. As a result, custom backends
(e.g. GTK/Linux) could not express platform-conditional values the way
first-party platforms can, even though the element form (OnPlatform<T>/On) is
already data-driven and supports arbitrary platform strings.
- Resolve values by comparing DeviceInfo.Platform.ToString() /
DeviceInfo.Idiom.ToString() (case-insensitive) against a lookup built from the
named properties, mirroring the data-driven element form. UWP/WinUI
precedence is preserved.
- Make GTK, macOS and WPF public on OnPlatformExtension.
Fixes dotnet#35695
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35901Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35901" |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
🤖 Multimodal code review summaryI ran an independent multi-model review of this PR with three top models — Claude Opus 4.8, GPT‑5.5, and Gemini 3.1 Pro. Consolidated findings: ✅ Verified correct (unanimous)
🔧 Finding to address — case sensitivity (GPT‑5.5: Major, Opus: Minor)The old chain compared via ➡️ Fix: switch both lookups to 📝 Considered — per‑call dictionary allocation (Gemini: Critical; GPT‑5.5 & Opus: acceptable)Gemini flagged the Applying the case‑sensitivity fix now and re‑reviewing afterward. |
Multimodal review (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro) found the new lookups used StringComparer.OrdinalIgnoreCase, which was the only behavioral delta versus the old DevicePlatform/DeviceIdiom comparison (Ordinal) and was inconsistent with the case-sensitive element form (OnPlatform<T>/On) and the compile-time SimplifyOnPlatformVisitor. Switch both lookups to StringComparer.Ordinal for exact behavioral parity and runtime/compile-time consistency, and correct the related comments and tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Round-2 multimodal review — all clear
Re-reviewed the case-sensitivity fix with Claude Opus 4.8 and GPT-5.5:
- ✅ Case-sensitivity finding resolved. Both confirm the switch to
StringComparer.Ordinalrestores exact parity with the oldDevicePlatform/DeviceIdiom(Ordinal) comparison and is consistent with the element form + compile-timeSimplifyOnPlatformVisitor. - ✅ Exhaustive parity (Opus): old
if-chain vs. new lookup across 15,360 platform + 640 idiom combinations → 0 mismatches. - ✅ Tests updated correctly (case-variant strings →
Default); comments accurate; the 7PublicAPI.Unshipped.txtentries correct; no new issues. - 📝 Dictionary allocation (Gemini round-1 Critical): both round-2 reviewers confirm it's acceptable —
ProvideValueruns once per inflation and is dominated by the surrounding reflection/ConvertTo, and #35695 explicitly asks for a data-driven dictionary. Kept intentionally.
Blocking issues: none. Local tests: all 145 OnPlatform/OnIdiom cases pass; no regressions (the 2 unrelated failures in the wider suite are pre-existing culture-dependent decimal-binding tests).
Inline notes on the key lines below. 👇
| // Keyed by platform string using Ordinal (case-sensitive) comparison, matching | ||
| // DevicePlatform equality, the element form (OnPlatform<T>/On) and the compile-time | ||
| // SimplifyOnPlatformVisitor, so runtime and compiled XAML resolve identically. | ||
| var lookup = new Dictionary<string, object>(StringComparer.Ordinal); |
There was a problem hiding this comment.
Round-2 fix (multimodal review). Switched from OrdinalIgnoreCase to Ordinal after GPT-5.5 (Major) and Opus (Minor) flagged that case-insensitive matching was the only behavioral delta vs. the old DevicePlatform comparison and was inconsistent with the case-sensitive element form (OnPlatform<T>) and compile-time SimplifyOnPlatformVisitor.
Opus exhaustively simulated the old if-chain vs. this lookup across 15,360 platform cases → 0 mismatches. The dictionary is kept intentionally: #35695 asks for a data-driven lookup, and the per-inflation allocation is negligible next to the reflection/ConvertTo already in ProvideValue.
| // "UWP" still matches a custom backend reporting the legacy "UWP" platform string. | ||
| lookup["UWP"] = UWP; | ||
|
|
||
| // UWP is a backwards-compatible alias for WinUI: only fall back to it for the |
There was a problem hiding this comment.
Reviewers verified this preserves the original precedence exactly: explicit WinUI wins (added first; this UWP block only fills the WinUI key via !ContainsKey), UWP is the fallback for the WinUI platform when WinUI is unset, and the legacy "UWP" platform string still resolves UWP. Covered by the WinUI=25, UWP=20 → 25 test case.
| { | ||
| // Keyed by idiom string using Ordinal (case-sensitive) comparison, matching | ||
| // DeviceIdiom equality and the data-driven element form. | ||
| var lookup = new Dictionary<string, object>(StringComparer.Ordinal); |
There was a problem hiding this comment.
Mirror of the OnPlatform change — Ordinal for case-sensitive parity with DeviceIdiom equality and the data-driven element form. AddIfSet skips null, preserving the old Idiom ?? Default fallback (verified across all idiom combinations).
Automated fleet code review — PR #35901Reviewed head SHA: Verdict: NEEDS_DISCUSSION Independent assessmentThe code replaces hardcoded Findings❌ ErrorsNone found in the changed code.
|
A third multimodal review round (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro) noted the per-call Dictionary built in ProvideValue: it allocates on the runtime resolution path (Windows/Tizen, and any runtime-loaded XAML, since SimplifyOnPlatformVisitor only compile-simplifies android/ios/macos/maccatalyst), including the DataTemplate inflation path. It also added no extensibility over a plain comparison, because the markup-extension parser only accepts properties declared on the type. Replace the Dictionary with a zero-allocation, string-keyed if-chain over the cached DeviceInfo.Platform.ToString() / DeviceInfo.Idiom.ToString() value. This keeps the data-driven string matching (Ordinal, matching DevicePlatform/ DeviceIdiom equality and the element form), preserves exact behavior including UWP/WinUI precedence, and removes the now-unused System.Collections.Generic import. All 145 OnPlatform/OnIdiom unit tests still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Round-3 multimodal review + CI triage
Ran a third independent review with Claude Opus 4.8, GPT-5.5, and Gemini 3.1 Pro, plus a full CI investigation.
CI failures — NOT caused by this PR ✅
The 7 red checks are pre-existing net11.0 issues, not this change:
- AOT macOS + RunOniOS_MauiRelease / TrimFull / TrimFull_CoreCLR fail in
BuildWarningsUtilities.AssertWarnings— the AOT/trim expected-warnings baseline drifted because the template now emitsHybridWebViewHandlerIL2026/IL3050(System.Text.Json) warnings unrelated to OnPlatform/OnIdiom. The identical failures appear on unrelated net11.0 PRs (#35892, #35891, #35870). - Build windows was canceled, not failed (known issue dnceng#3008).
- All Build, Pack, and Helix Unit Test jobs (which run the XAML unit tests exercising this change, under trim/AOT analyzers +
TreatWarningsAsErrors) passed.
All three models independently concluded the change cannot explain the AOT/Trim/Windows failures (ref-type generic instantiation is AOT-safe, static local function lowers to a private static method, additive public API).
Finding addressed — per-call Dictionary allocation
Gemini reaffirmed Critical; Opus rated it Minor but agreed "a zero-alloc string-keyed if-chain is a reasonable micro-opt"; GPT-5.5 had no objection. I verified Gemini's two supporting claims:
SimplifyOnPlatformVisitoronly compile-simplifies android/ios/macos/maccatalyst →{OnPlatform}/{OnIdiom}do run at runtime on Windows/Tizen (incl. theDataTemplateinflation path).MarkupExtensionParseronly accepts properties declared on the type → the dictionary added no extensibility over a direct comparison.
➡️ Fix applied (commit 7a59777): replaced the per-call Dictionary with a zero-allocation, string-keyed if-chain over the cached DeviceInfo.Platform.ToString() / DeviceInfo.Idiom.ToString(). Keeps the data-driven Ordinal matching and exact UWP/WinUI precedence; removed the now-unused using. Source builds with 0 warnings under warnings-as-errors; all 145 OnPlatform/OnIdiom tests still pass.
Everything else — clean
Correctness, UWP/WinUI precedence, OnIdiom null sentinel, nullability/NRT, trimming/AOT, SourceGen↔runtime consistency, public API (7 TFMs), and edge cases (Unknown/empty/tvOS/watchOS → Default, Converter path) all verified across the three models. No remaining blocking issues.
| // a value as long as DeviceInfo.Platform.ToString() matches one of the keys below. | ||
| // Ordinal (case-sensitive), matching DevicePlatform equality and the compile-time | ||
| // SimplifyOnPlatformVisitor, so runtime and compiled XAML resolve identically. | ||
| var platform = DeviceInfo.Platform.ToString(); |
There was a problem hiding this comment.
Round-3 fix. Replaced the per-call Dictionary<string,object> with this zero-allocation, string-keyed if-chain over the cached DeviceInfo.Platform.ToString().
Rationale (verified): SimplifyOnPlatformVisitor only compile-simplifies android/ios/macos/maccatalyst, so this path runs at runtime on Windows/Tizen including DataTemplate inflation; and MarkupExtensionParser only accepts declared properties, so the dictionary added no extensibility over direct comparison — it was pure allocation overhead.
Behavior is identical: Ordinal (matches DevicePlatform equality + the element form), and UWP/WinUI precedence preserved (explicit WinUI wins; UWP falls back for the WinUI platform only when WinUI is unset; legacy "UWP" string still matches). Locked in by the existing 145 tests.
Automated fleet code review — PR #35901Reviewed head SHA: Verdict: NEEDS_DISCUSSION Independent assessmentThe new commit replaces the prior per-call lookup/dictionary approach with a zero-allocation string-keyed if-chain. The behavior remains exact-case ( Reconciliation with prior review / PR narrativeRound22's allocation concern appears addressed by the new zero-allocation matching commit. However, the PR description is still stale: it says matching is case-insensitive and that tests cover Findings by severity❌ ErrorsNone found in the changed code.
|
✅ Final multimodal merge-readiness review — unanimous
|
| Model | Verdict |
|---|---|
| Claude Opus 4.8 | MERGEABLE |
| Claude Opus 4.6 | MERGEABLE |
| Gemini 3.1 Pro | MERGEABLE |
| GPT‑5.5 | MERGEABLE |
What all four models independently confirmed:
- Behavioral equivalence is exhaustive — Opus 4.8 simulated 13,312
{OnPlatform}combinations (13 platform strings × all 2¹⁰ set/unset states) and 576{OnIdiom}combinations against the old hardcoded if‑chain → 0 mismatches. - UWP/WinUI precedence preserved exactly —
WinUIwins overUWP;UWPstill matches both the WinUI platform and a legacy"UWP"string. (Including the trap thatDevicePlatform.UWP's internal string is actually"WinUI".) Ordinalis the correct comparison — it matchesDevicePlatform/DeviceIdiom.Equals(which useStringComparison.Ordinal), the runtime element form (List<string>.ContainsonDeviceInfo.Platform.ToString()), and the compile‑timeSimplifyOnPlatformVisitor(case‑sensitive==). Case‑variant inputs correctly fall through toDefault— covered by new"android"/"gtk"/"phone"tests.OnIdiomnull‑sentinel maps exactly to the old?? Defaultcoalescing; matching is allocation‑free (ToString()returns the backing string field; theDictionaryfrom an earlier round was replaced by a zero‑alloc if‑chain).- Public
GTK/macOS/WPFis justified, not speculative — these are functional value properties wired intoTryGetValueForPlatform, sitting alongside already‑public Android/iOS/MacCatalyst/WinUI/UWP, enabling community backends via{OnPlatform GTK=…}; new tests demonstrate real resolution.PublicAPI.Unshipped.txtis correct and identical across all 7 TFMs.
CI note: red AOT‑macOS / RunOniOS legs are the pre‑existing net11 HybridWebViewHandler baseline drift shared by sibling PRs; Build / Pack / Helix Unit Tests are green, and 145 OnPlatform/OnIdiom unit tests pass locally.
|
@StephaneDelcroix — design question before we take this further. cc @PureWeen Context. This PR makes the The limitation I want your call on. The matching is now data-driven, but the property surface of the markup extension is still a fixed set of CLR properties ( The element form already supports arbitrary platforms today — <Label.Text>
<OnPlatform x:TypeArguments="x:String">
<On Platform="Web" Value="..." />
</OnPlatform>
</Label.Text>So the gap is specifically the attribute markup-extension syntax Questions:
Same question applies to |
|
@StephaneDelcroix — concrete follow-up to the dynamic-key question, now traced through all three code paths so we can pick a direction. Goal: let a custom backend (e.g. a future Where the hardcoded surface bites:
So the attribute form is blocked on paths (1) and (3); the element form Proposed design (opt-in, scoped to these two extensions): interface IAcceptArbitraryKeys { bool TrySetKey(string key, object value, IServiceProvider sp); }
Questions for you:
Happy to implement whichever direction you prefer on |
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!
Description of Change
Fixes #35695. Part of #34099.
The
{OnPlatform ...}and{OnIdiom ...}XAML markup extensions matched values with a hardcodedif-chain overDevicePlatform/DeviceIdiom, and theGTK,macOSandWPFproperties onOnPlatformExtensionwereinternal. As a result, custom/community backends (e.g. GTK/Linux) couldn't express platform-conditional values in the markup-extension form the way first-party platforms can — even though the element form (OnPlatform<T>/On) already resolves values by platform string.This PR generalizes the markup extensions to resolve values by platform/idiom string, mirroring the element form:
DeviceInfo.Platform.ToString()/DeviceInfo.Idiom.ToString()against the per-property keys usingStringComparison.Ordinal(exact, case-sensitive). This matchesDevicePlatform/DeviceIdiomequality and the compile-timeSimplifyOnPlatformVisitor, so runtime and compiled XAML resolve identically. The match is a zero-allocationif-chain (no per-call dictionary/lookup allocation); the named properties are unchanged.GTK,macOSandWPFare now public onOnPlatformExtension, so a backend whoseDeviceInfo.Platform.ToString()isGTK,macOSorWPFcan resolve those values from{OnPlatform ...}.Scope note: this lets the markup-extension form resolve the predefined platform keys (now including the newly-public
GTK/macOS/WPF). It does not add arbitrary user-defined platform/idiom keys to the markup-extension form — a backend reporting a string with no matching property (e.g.Web,Linux) still falls back toDefault. Use the element form for fully arbitrary platform strings.Backwards-compatibility is preserved:
UWP/WinUIprecedence is unchanged: an explicitWinUIvalue wins, andUWPis used as the fallback for the WinUI platform when noWinUIvalue is provided. The legacy"UWP"platform string still resolves theUWPvalue.Default.For first-party platform builds (
android/ios/macos/maccatalyst),SimplifyOnPlatformVisitorstill resolves the value at compile time, so the new runtime matching path only runs for other/custom platforms — keeping the change low-risk.Why the markup extension and not just the element form?
The element form already resolves by platform string, but the markup-extension form (which most XAML uses) hardcoded a closed set and kept
GTK/macOS/WPFinternal. This change closes that gap (issue options 1 + 3) without changing any first-party named-property behavior.Issues Fixed
Fixes #35695
API Changes
Microsoft.Maui.Controls.Xaml.OnPlatformExtension:Tests
Added unit tests in
MarkupExpressionParserTestsandOnPlatformTestscovering:GTK,macOSandWPFresolving through{OnPlatform ...}(newly-public properties).GTK) resolving its value, and an unknown platform (Web) falling back toDefault, via the markup extension (MarkupExtensionResolvesCustomPlatform, including an end-to-endLoadFromXamlcase).Ordinal) matching: a non-matching-case string (e.g.android) falls back toDefaultrather than matchingAndroid.UWP/WinUIprecedence cases still pass.All
OnPlatform/OnIdiomunit tests pass locally; no regressions in the wider XAML unit-test suite.