[net11.0] Trimmable element handlers#29952
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR transitions built-in controls from DI registration to using [ElementHandler] attributes, introduces IElementHandlerWithAndroidContext<T> for Android renderers, and adds a new MSBuild property to toggle CSS support.
- Apply
[ElementHandler]to core controls and cells instead of DI - Introduce
IElementHandlerWithAndroidContext<T>andCreateHandler(Context)on Android renderers - Add
MauiCssEnabledMSBuild property and corresponding runtime host configuration
Reviewed Changes
Copilot reviewed 120 out of 120 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/src/Core/ContentPage/ContentPage.Mapper.cs | Added static ctor calling RemapForControls and VisualElement.RemapIfNeeded |
| src/Controls/src/Core/Compatibility/Handlers/TableView/Android/TableViewRenderer.cs | Implemented IElementHandlerWithAndroidContext<TableViewRenderer> and CreateHandler |
| src/Controls/src/Core/Compatibility/Handlers/ListView/Android/ListViewRenderer.cs | Implemented IElementHandlerWithAndroidContext<ListViewRenderer> and CreateHandler |
| src/Controls/src/Core/Compatibility/Handlers/Android/FrameRenderer.cs | Implemented IElementHandlerWithAndroidContext<FrameRenderer> and CreateHandler |
| src/Controls/src/Core/Cells/*.cs | Added [ElementHandler<...>] attributes for compatibility cell renderers |
| src/Controls/src/Core/Button/Button.cs | Added [ElementHandler<ButtonHandler>] |
| src/Controls/src/Core//.Mapper.cs | Added static ctors calling RemapForControls / RemapIfNeeded |
| src/Controls/src/Build.Tasks/nuget/.../Microsoft.Maui.Controls.targets | Added MauiCssEnabled property and runtime host configuration option |
| src/Compatibility/Core/src/{iOS,Android}/Platform.cs | Updated GetHandler calls to pass MauiContext |
Comments suppressed due to low confidence (1)
src/Controls/src/Build.Tasks/nuget/buildTransitive/netstandard2.0/Microsoft.Maui.Controls.targets:240
- Added the
MauiCssEnabledbuild property but no corresponding public documentation under/docs/; please update the XML docs to describe this new feature.
<MauiCssEnabled Condition="'$(MauiCssEnabled)' == '' and '@(MauiCss->Count())' == '0'">false</MauiCssEnabled>
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
33afe18 to
c49e57e
Compare
|
@copilot there are merge conflicts in this branch. please resolve them. When you're done, write summary of what this PR does and what functionality is missing to fully convert the "old" way of implementing handlers to the "new" way. |
|
@simonrozsival I've opened a new pull request, #32861, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot oops, try now! |
|
@simonrozsival I've opened a new pull request, #32862, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot there are several errors popping up:
Fix them and make sure the product can be built. |
|
@simonrozsival I've opened a new pull request, #32933, to work on those changes. Once the pull request is ready, I'll request review from you. |
…#35666) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Description Workaround for #35665. The `[ElementHandler<CheckBoxHandler>]` attribute on `CheckBox` causes handler resolution to fail on .NET 11 preview MAUI templates on Android **Release** builds across all runtime flavors: - **CoreCLR Default** — `System.TypeLoadException` when the generic attribute is enumerated (`GenericArguments[0] ... violates the constraint of type parameter 'THandler'`). - **Mono Default** / **NativeAOT** — `HandlerNotFoundException`, since the attribute isn't found and no DI registration exists. `CheckBox` was the **only** type using `[ElementHandler<T>]`. This PR removes the attribute and restores the regular DI handler registration (`AddHandler<CheckBox, CheckBoxHandler>()`), which unblocks the failing template builds. The `[ElementHandler<T>]` attribute mechanism itself will undergo a major refactoring in #29952, so this is intentionally a minimal, targeted workaround rather than a fix of the attribute path. ### Issues Fixed Fixes #35665 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AI code review for net11.0 targetAutomated, non-approval review comment. This does not constitute a human approval and intentionally does not use GitHub's Approve/Request-changes. Verdict: Needs changes (architecture is sound; blocked on rebase + fresh CI) What this PR doesMoves Controls handler registration from imperative Findings
No blocking correctness defect was found in the slices I reviewed, but the breadth (≈all controls) plus the lazy-cctor ordering change make this merge-risky without a fully green on-device run. CI noteBuild 1436697 is stale and red, but Build/Pack macOS (Release) failed on infrastructure ( Confidence: Medium — sound design and good trimming handling, but high blast radius, subtle init-ordering change, and no current green CI. |
|
@copilot address latest review |
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
AI code review refresh for net11.0 targetAutomated, non-approval review comment. This does not constitute a human approval and intentionally does not use GitHub's Approve / Request-changes. Head reviewed: Verdict: Needs discussion — code looks ready; CI signal needs differential analysis vs. baseline before merge. What changed since the prior round-1 fleet review (
|
| # | PureWeen finding | Status at a2cd361 |
|---|---|---|
| 1 🔴 | Material3 handlers silently broken on Android | ✅ Resolved. Each affected control (Slider.cs, Switch.cs, Editor.cs, Picker.cs, TimePicker.cs, RadioButton.cs, Label.cs, Entry.cs, SearchBar.cs, Image.cs, DatePicker.cs, ActivityIndicator.cs, ProgressBar.cs) declares an internal sealed XxxHandlerAttribute : ElementHandlerAttribute under #if ANDROID whose GetHandlerType() returns the Handler2 variant when RuntimeFeature.IsMaterial3Enabled. New src/Controls/tests/DeviceTests/Material3HandlerResolutionTests.Android.cs explicitly asserts both GetHandlerType and platform-view instantiation for all 13 controls under each switch value. |
| 2 🔴 | SwipeItemView crashes on Windows |
✅ Resolved. [ElementHandler(typeof(SwipeItemViewHandler))] is now wrapped in #if ANDROID || IOS || MACCATALYST || TIZEN, so Windows falls through to the IContentView → ContentViewHandler fallback (step 4). |
| 3 🔴 | DI base-type overrides silently ignored | ✅ Resolved. MauiHandlersFactory.GetHandler/GetHandlerType now run the assignable-DI lookup before the attribute walk (step 2 with an explicit code comment about AddHandler<Button, …>() winning for FancyButton : Button). New unit test RegisteredBaseControlHandlerOverridesInheritedElementHandlerAttribute exercises exactly this case. |
| 4 🟡 | Uncached reflection on hot path | ✅ Resolved. ConcurrentDictionary<Type, ElementHandlerAttribute?> _elementHandlerAttributeCache with GetOrAdd covers both GetHandler and GetHandlerType. A dedicated regression test was added (36f3926a). |
| 5 🟡 | Opaque MissingMethodException |
✅ Resolved. CreateAttributeHandler catches MissingMethodException and rethrows HandlerNotFoundException with a message naming the parameterless-ctor requirement and pointing to AddHandler for constructor-injected handlers. |
| 6 🟡 | GetCollection() obsoletion w/ incomplete migration path |
✅ Resolved. IMauiHandlersFactory.GetCollection() is no longer obsoleted (signature restored, per PR description). |
| 7 🟢 | ContainerChangedFiresWhenMapContainerIsCalled removed without replacement |
➖ Acceptable. Replaced by broader handler-resolution and _elementHandlerAttributeCache coverage; the underlying MapContainerView/PlatformContainerViewChanged wiring is still exercised by Controls device-tests. |
All three CRITICAL findings are now resolved with targeted tests. The earlier round-1 fleet-review caveats (lazy-cctor ordering risk, behavioural change from explicit static Foo() cctors removing beforefieldinit) remain intentional and observable — they're worth a final on-device pass but I did not find any new code path that breaks them.
Architecture re-spot-check at head
MauiHandlersFactoryresolution order is consistent across bothGetHandlerandGetHandlerType: exact DI → assignable DI →[ElementHandler]base-walk →IContentViewfallback.TryGetElementHandlerAttributewalksBaseTypechain (Inherited = falseon the attribute) with cache.RemappingHelper.EnsureBaseTypeRemapped(derived, base)is invoked from every Controls-layer per-typestatic Xxx()cctor (e.g.Slider.Mapper.cs,WebView.Mapper.cs,VisualElement.Mapper.cs→Element.Mapper.cs). The chain bottoms out atElementwhich has no furtherEnsureBaseTypeRemappedcall — correct.[ElementHandler]attribute isinternal, sealed-pattern subclasses areinternal sealedand live inside each view type for trim-friendly conditional dispatch.- HybridWebView is the only view without an
[ElementHandler]attribute on it; its handler is registered only whenRuntimeFeature.IsHybridWebViewSupportedis true, with scoped#pragma warning disable IL2026, IL3050.WebViewitself remains attribute-driven withWebViewHandler. No new hard references toHybridWebViewHandlerwere introduced. Controls/src/Core/RemappingHelper.csis a tinyinternal statichelper with oneDebug.AssertplusRuntimeHelpers.RunClassConstructor— minimal surface, scopedIL2059suppression with a clear justification.
CI status (build 1459262, in-progress)
| Bucket | Count | Notes |
|---|---|---|
| ✅ Pass | 25 | All Windows builds, both Packs, macOS Release, all Helix unit-test legs that have reported so far, all RunOnAndroid + most RunOniOS (BlazorDebug/Release ± CoreCLR, MauiDebug ± CoreCLR, MauiNativeAOT, MauiRelease_CoreCLR), all Integration buckets except trim/AOT. |
| ❌ Fail | 5 | (1) Build macOS (Debug) — infrastructure MSB4276 "default SDK resolver failed to resolve Microsoft.Build.NoTargets because /opt/hostedtoolcache/dotnet/sdk/10.0.100/Sdks/... did not exist". Same SDK-resolver infra failure pattern as round-1. Not a code defect. (2) AOT macOS, (3) RunOniOS_MauiReleaseTrimFull ARM64, (4) RunOniOS_MauiRelease ARM64, (5) RunOniOS_MauiReleaseTrimFull_CoreCLR ARM64 — all share the same root cause: ILLink emits IL2026 against <Module>..cctor() for HybridWebViewHandler.SchemeHandler.* / WebViewScriptMessageHandler.* (members carry RequiresUnreferencedCodeAttribute), and TreatWarningsAsErrors=true + TrimMode=full upgrades that to NETSDK1144. |
| ⏳ Pending | 4 | Windows Helix Unit Tests (Debug/Release), the maui-pr rollup, Build Analysis. |
Baseline differential. The two most recent net11.0 baseline runs (1455686 @ 74e739f8 — exactly this PR's merge target — and 1455594 @ d3684fa8) both fail with the same AOT macOS + RunOniOS_MauiReleaseTrimFull ARM64 jobs. So 2 of 5 failures are pre-existing net11.0 baseline failures. The remaining three (Build macOS Debug, RunOniOS_MauiRelease ARM64, RunOniOS_MauiReleaseTrimFull_CoreCLR ARM64) deserve a closer look before merge: the macOS Debug failure is clearly infra, but the two extra iOS legs also fail with the same <Module>..cctor() → HybridWebViewHandler IL2026 cascade — strongly suggesting the same root cause is just being amplified across more trim configurations, not a PR-introduced regression. A simple way to confirm is /azp run after a baseline re-merge once arcade/infra stabilizes.
Blast radius
- All built-in controls (~50 view types) now declare
[ElementHandler]and shift mapper remapping into per-typestaticcctors, removingbeforefieldinit. Initialization is now triggered lazily on first handler resolution rather than eagerly during host build.EnsureBaseTypeRemappedforces the named base type's cctor each level — coverage relies on each control having (and correctly chaining) its ownstaticcctor. - The
Controls/Hosting/AppHostBuilderExtensions.csshrink from 211→137 lines is the biggest single-file simplification;AddControlsHandlers,RemapForControls, andIElementHandlerWithAndroidContextare gone. Any third-party code callingAddControlsHandlers(it wasinternal) is unaffected. - The
internal sealed XxxHandlerAttributepattern inside view classes is repeated in 13 places — consistent and trim-friendly, but a future maintenance burden if more conditional handler families appear.
Concise findings (new vs. prior round)
- No new code-level findings. Every prior-round critical/moderate finding I re-checked was addressed in source with corresponding test coverage.
- CI: 2/5 failures match net11.0 baseline today. The other 3 likely share the HybridWebView IL2026 root cause and a macOS image SDK-resolver hiccup. Recommend re-baselining (re-run after current net11.0 trim issues are fixed) before treating any of these as PR-blocking.
Confidence
Medium-high on code, low on CI as a merge signal. The handler-resolution architecture is well-tested at the unit-test and (newly added) Material3 device-test level. The trim/AOT failure delta vs. baseline is the open question; everything else looks merge-ready pending a clean run.
Non-approval disclaimer
This is an automated, non-approval review comment. It does not constitute a human approval and intentionally does not use GitHub's "Approve" or "Request changes" actions. Please defer to human reviewers for final merge sign-off.
kubaflo
left a comment
There was a problem hiding this comment.
Verdict: NEEDS_CHANGES (confidence: high)
Model split was 3 NEEDS_CHANGES / 1 NEEDS_DISCUSSION; the lone ND (opus-4.6) assumed all findings resolved but missed a confirmed regression. The architecture is sound and the prior multi-model review's findings (Material3 routing, uncached attribute reflection, SwipeItemView Windows guard, DI base-type override, MissingMethodException, GetCollection obsoletion) are already FIXED at this HEAD and were re-confirmed — they are not re-raised here. The one surviving code defect is a Tizen [ElementHandler] guard regression on ViewCell/SwitchCell.
Key findings
- [warning] Tizen guard regression —
SwitchCell.cs:9&ViewCell.cs:11(consensus: gpt-5.5 + opus-4.8 on SwitchCell; opus-4.8 on ViewCell — both verified). Both addTIZENto their[ElementHandler]guard, butHandlers.Compatibility.SwitchCellRenderer/ViewCellRendererexist only for Android, iOS/MacCatalyst, and Windows (theCompatibility/Handlers/ListView/Tizen/folder has neither). The removedAddControlsHandlers()registered all six cell renderers under#if !TIZEN; the migration correctly dropped Tizen for the 4 sibling cells (Cell/EntryCell/ImageCell/TextCell) but erroneously added it for these two. References an undefined type → CS0246 onnet11-tizen. Dormant today only because Tizen TFMs are disabled. Novel finding (prior TIZEN comments were about the now-resolved SwipeItemView guard). - [dropped]
RemappingHelper.RunClassConstructoras the AOT root cause (gemini, speculative). Contradicted by evidence:RunOniOS_MauiNativeAOTPASSES while exercising the exact sameEnsureBaseTypeRemappedpath; the failing legs are trim/Release configs, not a runtime cctor failure. Not raised. - [dropped/minor] Design-doc drift (opus-4.6, suggestion).
docs/design/HandlerResolution.mddeletesGetCollection()from the shownIMauiHandlersFactorylisting while the real interface keeps it. Real but pure-deletion (no added/modified line to anchor an inline comment), lowest severity, and a defensible illustrative-doc simplification — non-blocking, not posted inline.
CI
maui-pr is PR-failing. Failing legs are trim/AOT: AOT macOS, RunOniOS_MauiRelease, RunOniOS_MauiReleaseTrimFull, RunOniOS_MauiReleaseTrimFull_CoreCLR. Per the prior round's baseline differential, ~2/5 match the net11.0 baseline and the remainder trace to the HybridWebView IL2026 <Module>..cctor() trim cascade (TrimMode=full + TreatWarningsAsErrors → NETSDK1144) plus a macOS SDK-resolver infra hiccup — not the new attribute path (RunOniOS_MauiNativeAOT and AOT windows both pass). A clean re-baseline run is needed to confirm, but CI cannot be treated as green as-is.
| { | ||
| /// <summary>A <see cref="Microsoft.Maui.Controls.Cell"/> with a label and an on/off switch.</summary> | ||
| [Obsolete("The controls which use SwitchCell (ListView and TableView) are obsolete. Please use CollectionView instead.")] | ||
| #if WINDOWS || ANDROID || IOS || MACCATALYST || TIZEN |
There was a problem hiding this comment.
This [ElementHandler] guard adds TIZEN, but Handlers.Compatibility.SwitchCellRenderer is only defined for Android, iOS/MacCatalyst, and Windows — there is no Tizen implementation (Compatibility/Handlers/ListView/Tizen/ contains no SwitchCellRenderer/CellRenderer). The removed AddControlsHandlers() registered this renderer under #if !TIZEN, and the four sibling cells (Cell, EntryCell, ImageCell, TextCell) all use #if WINDOWS || ANDROID || IOS || MACCATALYST without TIZEN. Including TIZEN here references an undefined type and will fail to compile (CS0246) on net11-tizen; it is dormant today only because the Tizen TFMs are disabled. Drop TIZEN to match the sibling cells and the renderer's actual platform availability.
| #if WINDOWS || ANDROID || IOS || MACCATALYST || TIZEN | |
| #if WINDOWS || ANDROID || IOS || MACCATALYST |
There was a problem hiding this comment.
Addressed in c110c98554: removed TIZEN from the SwitchCell [ElementHandler] guard, so it now matches the platforms where Handlers.Compatibility.SwitchCellRenderer actually exists: WINDOWS || ANDROID || IOS || MACCATALYST. I also rebuilt Controls.Core.UnitTests and reran HostBuilderHandlerTests successfully.
| /// <summary>A <see cref="Microsoft.Maui.Controls.Cell"/> containing a developer-defined <see cref="Microsoft.Maui.Controls.View"/>.</summary> | ||
| [Obsolete("The controls which use ViewCell (ListView and TableView) are obsolete. Please use CollectionView instead.")] | ||
| [ContentProperty("View")] | ||
| #if WINDOWS || IOS || MACCATALYST || ANDROID || TIZEN |
There was a problem hiding this comment.
Same regression as SwitchCell: this guard adds TIZEN, but Handlers.Compatibility.ViewCellRenderer is only defined for Android, iOS/MacCatalyst, and Windows — there is no Tizen ViewCellRenderer. The old AddControlsHandlers() registered it under #if !TIZEN, and the sibling cells exclude TIZEN. On net11-tizen this references an undefined type and breaks the build (CS0246); it is dormant only because the Tizen TFMs are currently disabled. Remove TIZEN from the condition.
| #if WINDOWS || IOS || MACCATALYST || ANDROID || TIZEN | |
| #if WINDOWS || IOS || MACCATALYST || ANDROID |
There was a problem hiding this comment.
Addressed in c110c98554: removed TIZEN from the ViewCell [ElementHandler] guard, so it now matches the available Handlers.Compatibility.ViewCellRenderer platforms: WINDOWS || IOS || MACCATALYST || ANDROID. I also rebuilt Controls.Core.UnitTests and reran HostBuilderHandlerTests successfully.
Replace static constructor forcing with an instance remap hook that runs when handlers attach to remappable Controls elements. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove Tizen from SwitchCell and ViewCell compatibility handler attributes because those renderers are not available on Tizen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review pass. I pushed two follow-up commits that address the latest comments and clean up the remapping design:
I also updated the PR description so it no longer describes the old static-constructor / Validation run locally:
|
Register command parameter dependencies during type initialization instead of delaying them until handler attachment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
One additional follow-up after the earlier review replies: I pushed Specifically, I also updated the PR description to document this distinction. Validation:
|
kubaflo
left a comment
There was a problem hiding this comment.
Re-review verdict: LGTM (confidence: medium)
Re-review after your update: the prior blocking issue is fixed, and the one remaining item is a non-blocking test-coverage nit.
Fixed (prior NEEDS_CHANGES)
- ViewCell / SwitchCell
[ElementHandler]TIZEN guard — resolved. At HEAD (86ee494):SwitchCell.cs:#if WINDOWS || ANDROID || IOS || MACCATALYST(no TIZEN)ViewCell.cs:#if WINDOWS || IOS || MACCATALYST || ANDROID(no TIZEN)- These now match the sibling cells (
Cell/EntryCell/ImageCell/TextCell) and the renderers' real platform availability, so the Tizen CS0246 risk is gone.
Still open
- None blocking.
- Non-blocking (test coverage) —
HostBuilderHandlerTests.BuiltInHandlerTypes:ListView/TableViewcarry[ElementHandler]forWINDOWS || IOS || MACCATALYST || TIZEN || ANDROID(Windows renderers exist), but the new test data asserts them only under#if ANDROID || IOS || MACCATALYST, leaving the Windows attribute path unverified.ShellItem/ShellSectionareWINDOWS || TIZENin source but asserted only under#if WINDOWS. TIZEN branches are dormant (Tizen TFMs disabled), so only the Windows gap is active. See inline comment.
Resolving the split
- gpt-5.5 LGTM, opus-4.6 LGTM, gemini APPROVED (1 non-blocking nit) — all correct against the code.
- opus-4.8 NEEDS_CHANGES (2 findings) — both are stale false positives: they re-flag the TIZEN guard on ViewCell/SwitchCell, which is already removed at HEAD (verified above). Its summary also claims iOS-TrimFull/AOT-macOS CI legs are failing; that is not reflected in the current checks (see below).
- gemini's coverage nit is real and code-confirmed, but explicitly optional and non-blocking — kept as a single inline suggestion.
CI
Pending, no failing checks: license/cla pass; maui-pr and all build/Helix legs pending; Build Analysis pending; add-dogfood-comment pass; Bump global.json skipping. No PR-failing legs in the current check set (the new commit's CI is still running). Recommend confirming maui-pr (incl. iOS Release/TrimFull and AOT macOS legs) goes green before merge.
| { typeof(ViewCell), typeof(Handlers.Compatibility.ViewCellRenderer) }, | ||
| #endif | ||
| #if ANDROID || IOS || MACCATALYST | ||
| { typeof(ListView), typeof(Handlers.Compatibility.ListViewRenderer) }, |
There was a problem hiding this comment.
Non-blocking (test coverage): the newly added [ElementHandler] attributes on ListView/TableView are compiled for #if WINDOWS || IOS || MACCATALYST || TIZEN || ANDROID, and the Compatibility renderers do exist on Windows (Compatibility/Handlers/ListView/Windows/ListViewRenderer.cs, .../TableView/Windows/TableViewRenderer.cs). However this BuiltInHandlerTypes data only asserts their resolution under #if ANDROID || IOS || MACCATALYST, so VariousControlsGetCorrectHandler never exercises the new Windows attribute path for these two controls. Consider adding Windows entries so the Windows resolution is covered. (Similarly, ShellItem/ShellSection carry [ElementHandler] under #if WINDOWS || TIZEN but are asserted only under #if WINDOWS.) The TIZEN branches are dormant while Tizen TFMs are disabled, so the Windows gap is the only active one. Not blocking.
✅ LGTM — no blocking issues foundRe-review verdict: LGTM (confidence: medium)Re-review after your update: the prior blocking issue is fixed, and the one remaining item is a non-blocking test-coverage nit. Fixed (prior NEEDS_CHANGES)
Still open
Resolving the split
CIPending, no failing checks: Multi-model review (gpt-5.5 · opus-4.8 · opus-4.6 · gemini-3.1-pro). Comments only — not a formal approval. |
Remove the element-side remap interface and use per-type Interlocked gates invoked during handler attribute resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit ea04ec8.
|
@kubaflo one more remapping cleanup pushed after the last follow-up. We backed out the attribute-class dispatch experiment and kept remapping on the Controls virtual view types, which is where the old static constructors lived. The final shape is:
I updated the PR description to reflect this final design. Validation:
|
Keep mapper remapping on Controls virtual views with per-type Interlocked gates and remove the shared HashSet remap state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
There was a problem hiding this comment.
Re-review verdict: NEEDS_CHANGES (confidence: high)
Re-review of the new commit ea04ec8 ("Trigger Controls remaps from handler attributes"). The prior Tizen fix still holds, but this commit introduces a new regression: Controls mapper remaps no longer run for DI-registered handler overrides.
New commit: regressed
gpt-5.5's finding is CONFIRMED (real regression), not a false positive.MauiHandlersFactory.cs:119—RemapForControls()is now triggered only fromFindElementHandlerAttribute()on the[ElementHandler]path (step 3).GetHandler()/GetHandlerType()return at step 1 (exact DI) or step 2 (assignable DI) before that path, so a view type that has a DI registration never gets remapped.- This same commit deleted the attach-time hook that previously covered all paths:
IControlsMapperRemappable.cswas removed andElementHandler.SetVirtualView'sControlsMapperRemapper.EnsureRemapped(...)call was deleted. That hook was keyed on the view, so it fired for every handler attach — including DI handlers. The parent commit86ee494(the prior LGTM HEAD) still had it. - Concrete impact:
AddHandler<Button, ButtonHandler>()(the override pattern documented indocs/design/HandlerResolution.md:26) or anyCustomButtonHandler : ButtonHandlerthat uses the inherited staticButtonHandler.Mapper(its default ctor passes that static mapper to base) never triggersButton.RemapForControls(). So Controls-only mappings — ContentLayout, TextTransform, Text, LineBreakMode, plus iOS/Android/Windows platform specifics — silently stop being applied. This directly contradicts the author's stated requirement in-thread: "Any custom handler registered via DI should still work." - One inline comment posted on the added line
MauiHandlersFactory.cs:119with a suggested fix (also invoke the view type'sElementHandlerAttribute.RemapForControls()when a DI registration wins, preserving DI precedence for the handler instance).
Why the other models missed it
- opus-4.8 reviewed the parent commit (
86ee494, "Decouple command dependencies from mapper remaps"), which still had theSetVirtualViewhook — so its LGTM does not coverea04ec8's change. - opus-4.6 described
ea04ec8's architecture correctly but did not trace the DI-resolution → static-mapper interaction (it concluded "mappers are only mutated by their owning type" without noting the owning type's remap never fires under DI). - gemini APPROVED with one malformed/empty coverage finding (ignored).
Confirmed fixed (prior NEEDS_CHANGES)
- ViewCell / SwitchCell
[ElementHandler]TIZEN guard — still fixed at HEAD. All six cells guard on#if WINDOWS || ANDROID || IOS || MACCATALYST(no TIZEN), matching the real renderer availability. The CS0246 risk is gone.
Not duplicating prior comments
This DI-skip regression is new to ea04ec8 and was not present at the prior LGTM HEAD (86ee494 remapped on attach). It is distinct from the already-fixed prior findings (Material3 routing, uncached reflection, SwipeItemView guard, DI base-type ordering, MissingMethodException, GetCollection obsoletion, Tizen guard).
CI
Not PR-failing, but not yet green: license/cla and add-dogfood-comment pass; maui-pr and all build/Helix legs pending; Build Analysis pending; Bump global.json skipping. No failing legs in the current set. The fix above should be validated against maui-pr (incl. iOS Release/TrimFull and AOT macOS legs) plus a unit test asserting a DI-registered override still receives Controls mappings.
| var elementHandlerAttribute = type.GetCustomAttribute<ElementHandlerAttribute>(); | ||
| if (elementHandlerAttribute is not null) | ||
| { | ||
| return elementHandlerAttribute; |
There was a problem hiding this comment.
Controls remaps are skipped for DI-registered handler overrides. RemapForControls() is now reached only here, inside FindElementHandlerAttribute() on the [ElementHandler] resolution path (step 3). But GetHandler() returns earlier at step 1 (exact DI registration, lines 26-30) or step 2 (assignable DI, lines 35-39), and GetHandlerType() mirrors that same order — so whenever a view type has a DI registration, this remap never runs for it. This commit (ea04ec8) also removed the ElementHandler.SetVirtualView hook (if (VirtualView is IControlsMapperRemappable) ControlsMapperRemapper.EnsureRemapped(...)) that previously fired on every handler attach, including DI-registered handlers. Net effect: AddHandler<Button, ButtonHandler>() (the override pattern documented in docs/design/HandlerResolution.md) or any CustomButtonHandler : ButtonHandler that uses the inherited static ButtonHandler.Mapper never triggers Button.RemapForControls(), so Controls-only mappings (ContentLayout, TextTransform, Text, LineBreakMode, plus the iOS/Android/Windows platform specifics) silently stop being applied. That contradicts the stated design goal that “any custom handler registered via DI should still work,” and it is a behavior change versus the parent commit 86ee494, which still remapped on attach. Consider running the requested view type’s ElementHandlerAttribute.RemapForControls() even when a DI registration wins (e.g. resolve the attribute for the view type and invoke its remap before returning the DI handler), while preserving DI precedence for the handler instance itself.
There was a problem hiding this comment.
Good catch. The ea04ec8f18 factory/attribute-dispatch approach was wrong for exactly this reason: DI could win before attribute lookup, so Controls remaps could be skipped. We backed that out and kept remapping on the virtual view attach path instead. Current head calls the internal Controls remap hook from ElementHandler.SetVirtualView(), so it runs regardless of whether the handler came from exact DI, assignable DI, [ElementHandler], or fallback. Each remappable Controls type owns its own Interlocked.CompareExchange gate and calls base.RemapForControls() before applying its own mappings. I also added regression coverage in d6697750f7: DiRegisteredHandlerOverrideStillRunsControlsMapperRemapOnAttach resolves a handler through a DI override that wins over [ElementHandler], attaches the remappable view via SetVirtualView, and verifies the Controls remap still runs. Validation: Core.UnitTests build passed and the focused HostBuilderHandlerTests run passed (34 tests).
Verify Controls mapper remapping still runs when a handler is resolved through a DI override instead of ElementHandlerAttribute. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Align remap method bodies with the previous mapper static constructor bodies after switching to early-return guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the command dependency static constructors back to the mapper partial files to reduce churn in the main control files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ LGTM — no blocking issues foundPR #29952 — [net11.0] Trimmable element handlersVerdict: LGTM (confidence: high) — HEAD Re-review: The prior blocking DI-handler Is the DI-handler regression fixed or still present? — FIXEDThe prior NEEDS_CHANGES was: Evidence at
Reconciling the model split
Net on the adjudicated regression: fixed (code + test + 3 models; the lone dissent reviewed an obsolete commit). Non-blocking note (not the adjudicated regression)gpt-5.5 (error) and opus-4.6 (warning) independently observe a timing change: Controls remaps moved from eager ( CINot a blocker: Multi-model review (gpt-5.5 · opus-4.8 · opus-4.6 · gemini-3.1-pro). Comments only — not a formal approval. |
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!
This is a follow-up to #28357.
Summary
Built-in controls now primarily use
[ElementHandler]attributes instead of DI registration for handler resolution. Handler associations are visible at the type level, improving trimmability because unused controls no longer need to be rooted by the default handler-registration table.HybridWebViewremains an exception: its handler is registered through theRuntimeFeature.IsHybridWebViewSupported-guarded DI path so NativeAOT can eliminate trim-unsafe HybridWebView code when the feature switch is disabled.Key changes
[ElementHandler]attributeElementHandlerAttributeon view types declaratively associates a view with its handler.Inherited = false— each concrete type that needs a different handler declares its own attribute.BaseTypechain, so derived types without their own attribute inherit the nearest ancestor's handler.Handler resolution in
MauiHandlersFactoryBoth
GetHandler(Type)andGetHandlerType(Type)follow the same resolution order:AddHandler<Button, MyButtonHandler>(), that wins.[ElementHandler]attribute — walk the type'sBaseTypechain looking for the attribute.IContentViewfallback — if the type implementsIContentView, returnContentViewHandler.GetHandler()is the primary API — it returns an instantiatedIElementHandler.GetHandlerType()returns only theTypeand is used by code that needs to compare handler types without instantiating them.Attribute lookup is cached per view type to avoid repeated reflection walks during handler resolution.
ElementExtensions.ToHandler()/SetHandler()These methods call
GetHandler()directly. When a handler requires constructor parameters,ToHandler()catches theMissingMethodExceptionand falls back toActivatorUtilities.CreateInstance()with DI injection. This fallback path is cached per type.Controls mapper remapping
Controls-layer mapper remaps no longer rely on static constructors or
RuntimeHelpers.RunClassConstructor.Controls remapping stays on the Controls virtual view types, matching where the old mapper static constructors lived.
ElementHandler.SetVirtualView()invokes an internal Controls remap hook before mapper updates; each remappable control override uses its ownInterlocked.CompareExchangefield to run once, callsbase.RemapForControls()first, then applies its own mapper changes.This preserves the existing base-before-derived mapper ordering without forcing arbitrary class constructors to run, without putting Controls-specific remap logic on Core handlers, and without handwritten per-control handler-attribute classes.
Non-mapper command dependency setup (
CommandProperty.DependsOn(...)) remains in the relevant control type initialization so binding behavior is independent of handler creation.Cleanup
IElementHandlerWithAndroidContextandElementHandlerWithAndroidContextAttribute.RemapForControls(this MauiAppBuilder)and oldRemapForControls()/RemapIfNeeded()patterns.RemappingHelperand the static-constructor-based mapper-remap helper path.Layout.Mapper.csremap file.IMauiHandlersFactory.GetHandler()to its original signature.MauiContext/HandlersContextStubdeclarations from tests and benchmarks.