[net11.0] Trimmable element handlers by simonrozsival · Pull Request #29952 · dotnet/maui

simonrozsival · 2025-06-12T07:43:43Z

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.

Key changes

`[ElementHandler]` attribute

New internal ElementHandlerAttribute on view types declaratively associates a view with its handler.
Inherited = false — each concrete type that needs a different handler declares its own attribute.
Handler resolution walks the BaseType chain, so derived types without their own attribute inherit the nearest ancestor's handler.
Android Material3 controls use control-specific nested attributes so the handler type can be selected conditionally without unconditionally rooting both handler implementations.
The same nested-attribute technique selects the handler conditionally where a runtime feature switch applies: Android Material3 controls, and CollectionView on Windows (CollectionViewHandler2 vs. the legacy handler, honoring RuntimeFeature.IsWindowsCollectionView2HandlerEnabled).

Handler resolution in `MauiHandlersFactory`

Both GetHandler(Type) and GetHandlerType(Type) follow the same resolution order:

Exact DI registration — if the concrete type is registered via AddHandler<Button, MyButtonHandler>(), that wins.
Assignable DI registration — base/interface registrations still override inherited attribute defaults.
[ElementHandler] attribute — walk the type's BaseType chain looking for the attribute.
IContentView fallback — if the type implements IContentView, return ContentViewHandler.

GetHandler() is the primary API — it returns an instantiated IElementHandler. GetHandlerType() returns only the Type and 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 the MissingMethodException and falls back to ActivatorUtilities.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 own Interlocked.CompareExchange field to run once, calls base.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

Removed IElementHandlerWithAndroidContext and ElementHandlerWithAndroidContextAttribute.
Removed dead RemapForControls(this MauiAppBuilder) and old RemapForControls() / RemapIfNeeded() patterns.
Removed RemappingHelper and the static-constructor-based mapper-remap helper path.
Removed an empty Layout.Mapper.cs remap file.
Fixed Slider not having mapper remapping coverage.
Restored IMauiHandlersFactory.GetHandler() to its original signature.
Removed unused MauiContext / HandlersContextStub declarations from tests and benchmarks.
Matched cell compatibility handler attributes to the platforms where those renderers actually exist.
Kept command dependency registration separate from mapper remapping so it is not delayed until handler attachment.
Migrated HybridWebView to [ElementHandler(typeof(HybridWebViewHandler))] and removed the RuntimeFeature.IsHybridWebViewSupported feature switch, now that [AOT] Make HybridWebView AOT safe using source generator #35626 made HybridWebView NativeAOT-safe via a source generator. IHybridWebViewTaskManager is now registered unconditionally.
Preserved the default Windows CollectionViewHandler2 selection during the migration (a Windows-specific [ElementHandler] attribute honors RuntimeFeature.IsWindowsCollectionView2HandlerEnabled).

mattleibow · 2025-06-17T15:11:42Z

/azp run

azure-pipelines · 2025-06-17T15:11:49Z

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.

simonrozsival · 2025-11-25T16:24:08Z

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

Copilot · 2025-11-25T16:24:20Z

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

simonrozsival · 2025-11-25T16:25:08Z

@copilot oops, try now!

Copilot · 2025-11-25T16:25:24Z

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

simonrozsival · 2025-12-01T10:54:02Z

@copilot there are several errors popping up:

src\Controls\src\Core\Shell\Shell.cs(29,25): Error CS0246: The type or namespace name 'ShellHandler' could not be found (are you missing a using directive or an assembly reference?)
src\Controls\src\Core\Shell\ShellItem.cs(18,25): Error CS0246: The type or namespace name 'ShellItemHandler' could not be found (are you missing a using directive or an assembly reference?)
buildtasks/Microsoft.Maui.Controls.Build.Tasks.After.targets(15,3): error MSB4024: The imported project file "/mnt/vss/_work/1/s/.buildtasks/Microsoft.Maui.Controls.targets" could not be loaded. Name cannot begin with the '<' character, hexadecimal value 0x3C. Line 325, position 2. [/mnt/vss/_work/1/s/src/BlazorWebView/tests/DeviceTests/MauiBlazorWebView.DeviceTests.csproj]

Fix them and make sure the product can be built.

Copilot · 2025-12-01T10:54:15Z

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

Validate attribute handler types consistently, make the mapper test self-contained, and correct the no-op handler registration documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a8d4a631-257f-49c2-b002-eaf82ac7a057

simonrozsival · 2026-07-14T16:36:17Z

@PureWeen @kubaflo I pushed 7d6af056f3 to address the final review feedback:

validate attribute-resolved handler types consistently in both GetHandler and GetHandlerType, with focused regression coverage;
make the PlatformContainerViewChanged mapper test self-contained under lazy remapping;
correct the AddMauiControlsHandlers XML documentation to describe its no-op behavior.

The earlier substantive handler-selection, DI-precedence, and remap-concurrency findings are also addressed at the current head. Please take another look when the fresh CI run completes.

simonrozsival · 2026-07-14T16:36:19Z

/azp run maui-pr,maui-pr-devicetests,maui-pr-uitests

azure-pipelines · 2026-07-14T16:36:37Z

Azure Pipelines: Successfully started running 3 pipeline(s).

Copilot

Copilot's findings

Comments suppressed due to low confidence (2)

src/Controls/src/Core/TimePicker/TimePicker.cs:1

[TimePickerHandler] will not bind to the nested TimePickerHandlerAttribute type (TimePicker.TimePickerHandlerAttribute). Attribute type name lookup for a type-level attribute won’t resolve nested types here, so this is likely a compile-time or runtime resolution bug. Fix by moving the custom attribute types (e.g., TimePickerHandlerAttribute, SwitchHandlerAttribute, LabelHandlerAttribute, etc.) to namespace scope (internal is fine) so [TimePickerHandler] resolves correctly, or apply the fully qualified attribute type if you keep it non-nested.
src/Core/src/Hosting/Internal/MauiHandlersFactory.cs:1
Only MissingMethodException is wrapped with an actionable HandlerNotFoundException. Other common activation failures (e.g., abstract handler type => MemberAccessException, ctor throws => TargetInvocationException) will currently bubble out with less context than the MissingMethod path. Consider catching these activation-related exceptions and wrapping them in HandlerNotFoundException with the same {viewType} / {handlerType} context so failures remain actionable and consistent.

Files reviewed: 123/125 changed files
Comments generated: 0 new

Copilot

Copilot's findings

Files reviewed: 123/125 changed files
Comments generated: 2

+				catch (HandlerNotFoundException ex) when (ex.InnerException is MissingMethodException)
+				{
+					handler = viewType.CreateTypeWithInjection(context);
+					if (handler != null)
+						handlersWithConstructors.Add(view.GetType());


+				throw new HandlerNotFoundException(
+					$"Unable to create the {nameof(IElementHandler)} {handlerType} declared by {nameof(ElementHandlerAttribute)} for {viewType}. " +
+					$"Handlers declared with {nameof(ElementHandlerAttribute)} must have a public parameterless constructor when resolved directly through {nameof(IMauiHandlersFactory.GetHandler)}. " +
+					$"Use `Microsoft.Maui.Hosting.MauiHandlersCollectionExtensions.AddHandler` to register handlers that require constructor arguments, or resolve through `ToHandler()` so constructor arguments can come from the DI container.",
+					ex);


MauiBot

Expert Review — 2 findings

See inline comments for details.

MauiBot · 2026-07-14T17:58:41Z

-		});
+		// Ensure the handlers factory is registered even though most built-in handlers are resolved
+		// via [ElementHandler] attributes on the view types.
+		builder.ConfigureMauiHandlers(configureDelegate: null);


🔍 AI-Generated Review (multi-model)

[major] Backward Compatibility — Switching the built-in Controls defaults to attribute-only registration means TryAddHandler<Button, LibraryButtonHandler>() now succeeds after UseMauiApp() instead of no-oping against the existing MAUI default. Because exact DI registrations win over [ElementHandler] defaults, libraries that defensively call TryAddHandler can now globally replace built-in handlers. Please preserve the old TryAddHandler semantics for exact attributed built-ins, or add an explicit compatibility decision plus regression test.

MauiBot · 2026-07-14T17:58:41Z

+			{
+				lock (s_controlsMapperRemapLock)
+				{
+					remappable.RemapForControls();


🔍 AI-Generated Review (multi-model)

[major] Handler Mapper and Property Patterns — Running the Controls remap from the first SetVirtualView() happens after app/library startup mapper customizations. A common UseMauiApp().ConfigureMauiHandlers(_ => ButtonHandler.Mapper.AppendToMapping(nameof(Button.Text), ...)) customization can be discarded when this late remap calls ReplaceMapping for Button.Text, because ReplaceMapping does not invoke the previous mapping for matching Button handlers. Please ensure Controls remaps run before user mapper customizations, or preserve prior append/prepend mappings when the late remap replaces a key.

PureWeen

Adversarial re-review (round 3) — HEAD `7d6af0` ✅ prior findings resolved

Ran a fresh 3-model adversarial pass (claude-opus-4.8, gpt-5.6-sol, claude-sonnet-5) focused on the 7 commits pushed since my round-2 review. Both of my prior open findings are fixed at this head, verified against source.

⚠️ Stale-head correction

My round-2 review was posted against 4fd577, but the real PR head was already 7d6af0 (a 7-commit descendant). So my round-2 "still open" claims were against superseded code. At the current head, both are resolved — apologies for the confusion.

Prior findings — status at `7d6af0`

Finding (prior round)	Status	Verification
`RemapForControls()` first-attach race on the shared static mapper	✅ Resolved	`SetVirtualView` now wraps the remap in `lock (s_controlsMapperRemapLock)`. The winner runs the full `Interlocked.CompareExchange` + mapper mutation inside the lock; a losing thread blocks until release, then observes the fully-mutated mapper (lock release/acquire = full barrier) and early-returns. Global scope is correct here — `base.RemapForControls()` mutates the process-wide shared `ViewMapper`, so a per-type lock would leave that unprotected. No deadlock: all `RemapForControls` overrides were audited and none re-enter `SetVirtualView`; and `System.Threading.Lock` is reentrant/recursive anyway, so same-thread re-entry would not self-deadlock.
`ToHandler()` ActivatorUtilities fallback broken for `[ElementHandler]` handlers needing ctor args	✅ Resolved	New `catch (HandlerNotFoundException ex) when (ex.InnerException is MissingMethodException)` routes to `CreateTypeWithInjection` (DI). The filter is precise and does not over-catch: the new non-handler-type validation error throws with `InnerException == null`, so it correctly propagates. New test `ToHandlerUsesActivatorUtilitiesForElementHandlerAttributeWithoutDefaultConstructor` asserts `Assert.Same(dependency, handler.Dependency)` — proving real DI injection, not just non-null.

Also verified clean

GetValidatedHandlerType — validates the attribute's type implements IElementHandler, throws an actionable HandlerNotFoundException (null inner) otherwise; DAM PublicConstructors annotation threads correctly through to Activator/ActivatorUtilities. Covered by two new tests on both the GetHandler and GetHandlerType paths. Making GetHandlerType throw (instead of returning an invalid type that fails later deep in DI) is a desirable behavior change, not a regression.
inherit: false on the BaseType-walk attribute lookup — behaviorally identical (ElementHandlerAttribute is [AttributeUsage(Inherited = false)]); a clarity improvement.
Netstandard Lock alias (using Lock = System.Object → Monitor) under #if NET9_0_OR_GREATER — sound.
Material3 test hardening — DisableParallelization=true genuinely serializes the process-global AppContext switch against the rest of the assembly; no residual cross-class race since the switch is only touched in that one class.

Remaining note (non-blocking, pre-existing)

One ⚠️ surfaced by all three reviewers — see the inline comment on handlersWithConstructors. It's a pre-existing unsynchronized-HashSet issue, not introduced here; the delta just adds a second writer path to it. Real-world impact is low because ToHandler/SetVirtualView are normally UI-thread-bound. Optional to harden in this PR since it's now touched twice as often.

Net: the 7-commit delta is high quality and closes both of my prior findings with regression tests. No new blocking (❌) defects. Round-3 verdict: clean (one optional pre-existing hardening note). CI status was out of scope for this pass.

(Comment-only review — approval is a human decision.)

PureWeen · 2026-07-16T19:41:00Z

+				{
+					handler = viewType.CreateTypeWithInjection(context);
+					if (handler != null)
+						handlersWithConstructors.Add(view.GetType());


⚠️ Race Condition (pre-existing, non-blocking) — This new catch adds a second unsynchronized writer to the static handlersWithConstructors HashSet<Type> (declared at line 35; also written in the MissingMethodException catch just above, and read at line 78). HashSet<T> is not thread-safe: concurrent Add/Contains from parallel first-time ToHandler() calls can corrupt the set's internal buckets (not merely miss an entry), potentially throwing during resolution.

This predates the PR, so it's not a regression and not a blocker — ToHandler() is normally UI-thread-bound so a live trigger is unlikely. But since the delta now doubles the write paths, this is a reasonable spot to harden: switch to ConcurrentDictionary<Type, byte> or guard all three accesses with a lock. All three round-3 reviewers independently flagged this.

Restore the current Android Shell, ShellItem, and ShellSection handlers after migrating built-in registrations to attributes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4542e8a1-05ad-484c-9852-9e86c3a5c250

Copilot

Copilot's findings

Files reviewed: 123/125 changed files
Comments generated: 1

+			if (VirtualView is IControlsMapperRemappable remappable)
+			{
+				lock (s_controlsMapperRemapLock)
+				{
+					remappable.RemapForControls();


simonrozsival · 2026-07-19T07:56:24Z

/azp run maui-pr-uitests,maui-pr-devicetests

azure-pipelines · 2026-07-19T07:56:41Z

Azure Pipelines: Successfully started running 2 pipeline(s).

MauiBot

AI Review Summary

ℹ️ The review agent did not produce a full summary on this run (an infrastructure issue on the CI agent), but the deep UI tests completed — their results are below. Re-comment /review for a fresh full review.

❌ Deep UI tests — 1049 passed, 3 failed across 16 categories on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category	Tests	Snapshot diffs
`Border`	48/48 ✓	—
`BoxView`	24/25 ✓	—
`Button`	67/69 ✓	—
`CheckBox`	23/23 ✓	—
`DatePicker`	44/44 ✓	—
`Editor`	66/66 ✓	—
`Entry`	111/113 (1 ❌)	—
`FlyoutPage`	60/62 (2 ❌)	—
`Frame`	6/6 ✓	—
`ImageButton`	52/52 ✓	—
`IndicatorView`	39/39 ✓	—
`Label`	96/98 ✓	—
`Layout`	191/194 ✓	—
`ListView`	108/109 ✓	—
`Navigation`	88/89 ✓	—
`Page`	26/26 ✓	—

🔍 AI analysis of failures — PR-related vs unrelated

🔍 AI-generated triage (GitHub Copilot CLI) — a heuristic judgement of whether each deep UI test failure is connected to this PR's changes. Verify before relying on it.

Likely PR-related: one or more failures appear connected to this PR's changes.

✗ PR-related — Entry clear-button interaction on Android (~1 test): the PR changes shared Entry handler resolution/remapping code compiled into the Android run, and the failure is an Appium null-click while exercising the Entry clear-button theme behavior.
✗ PR-related — FlyoutPage gesture opening on Android (~2 tests): the PR changes shared FlyoutPage and mapper code, and both failures assert that the Android flyout gesture no longer opens the flyout.

Strongest signal: every failing category maps directly to shared control files modified by the PR and compiled for Android, with no clear infrastructure-only error pattern.

❌ Entry — 1 failed test

EntryClearButtonColorShouldUpdateOnThemeChange

System.NullReferenceException : Object reference not set to an instance of an object.
at UITest.Appium.HelperExtensions.Click(IUIElement element) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 416
   at UITest.Appium.HelperExtensions.<>c__DisplayClass2_0.<Tap>b__0() in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 36
   at UITest.Appium.HelperExtensions.<>c__DisplayClass186_0.<RunWithTimeout>b__0() in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 3079
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Tas
...

❌ FlyoutPage — 2 failed tests

VerifyFlyoutPage_IsGestureEnabled

Flyout did not open after multiple drag attempts
Assert.That(flyoutOpened, Is.True)
  Expected: True
  But was:  False
at Microsoft.Maui.TestCases.Tests.FlyoutPageFeatureTests.VerifyFlyoutPage_IsGestureEnabled() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/FlyoutPageFeatureTests.cs:line 92

1)    at Microsoft.Maui.TestCases.Tests.FlyoutPageFeatureTests.VerifyFlyoutPage_IsGestureEnabled() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/FlyoutPageFeatureTests.cs:line 92

VerifyFlyoutPage_IsGestureEnabled_FlyoutBehaviorPopover

Flyout did not open after multiple drag attempts
Assert.That(flyoutOpened, Is.True)
  Expected: True
  But was:  False
at Microsoft.Maui.TestCases.Tests.FlyoutPageFeatureTests.VerifyFlyoutPage_IsGestureEnabled_FlyoutBehaviorPopover() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/FlyoutPageFeatureTests.cs:line 149

1)    at Microsoft.Maui.TestCases.Tests.FlyoutPageFeatureTests.VerifyFlyoutPage_IsGestureEnabled_FlyoutBehaviorPopover() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/FeatureMatrix/FlyoutPageFeatureTests.cs:line 149

📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

MauiBot

AI Review Summary

@simonrozsival — new AI review results are available based on this last commit: 3fb4cbd.

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ INCONCLUSIVE

Platform: ANDROID · Base: net11.0 · Merge base: 381ab4fe

Test	Without Fix (expect FAIL)	With Fix (expect PASS)
🧪 CommandSourceTests `CommandSourceTests`	🛠️ BUILD ERROR	🔍 NO MATCH
🧪 HostBuilderHandlerTests `HostBuilderHandlerTests`	🛠️ BUILD ERROR	✅ PASS — 20s
🧪 VisualElementTests `VisualElementTests`	🛠️ BUILD ERROR	✅ PASS — 20s
📱 Material3HandlerResolutionTestsCollection (ResolvesMaterial3HandlerWhenFeatureSwitchEnabled, InstantiatesMaterial3HandlerAndPlatformViewWhenFeatureSwitchEnabled) `Category=Mapper`	⚠️ ENV ERROR	⚠️ ENV ERROR
📱 MapperTests `MapperTests`	⚠️ ENV ERROR	⚠️ ENV ERROR

🔴 Without fix — 🧪 CommandSourceTests: 🛠️ BUILD ERROR · 73s

Error-relevant lines (filtered from the build log):

/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/VisualElementTests.cs(93,4): error CS0176: Member 'VisualElement.RemapForControls()' cannot be accessed with an instance reference; qualify it with a type name instead [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]

🟢 With fix — 🧪 CommandSourceTests: 🔍 NO MATCH · 53s