Skip to content

Add ViewHandler.SetContainerView so external platform backends can manage ContainerView - #37854

Open
Redth wants to merge 3 commits into
net11.0from
redth-external-container-extensibility
Open

Add ViewHandler.SetContainerView so external platform backends can manage ContainerView#37854
Redth wants to merge 3 commits into
net11.0from
redth-external-container-extensibility

Conversation

@Redth

@Redth Redth commented Aug 26, 2026

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!

Description of Change

ViewHandler.SetupContainer() and ViewHandler.RemoveContainer() are protected extensibility points, but ViewHandler.ContainerView's setter is private protected. A handler that derives from ViewHandler<TVirtualView, TPlatformView> in another assembly — for example a platform backend that ships outside of dotnet/maui — can therefore override SetupContainer(), create a wrapper, and re-parent the platform view into it, but has no way to publish that wrapper as the handler's container view.

The practical consequence (reported from Redth/Maui.Tizen#5, gap G1) is that an external backend has to report NeedsContainer => false and permanently loses gradient/image backgrounds, Clip, and Shadow, since all of those are rendered by the container view on every in-box platform.

The API

A single additive protected method on ViewHandler:

protected void SetContainerView(PlatformView? containerView);
  • Passing a view installs it as ContainerView; passing null clears it.
  • ContainerView's getter is already public, so access was never the gap.
  • The method only records the container view. It never re-parents anything and it does not change HasContainer, so SetupContainer()/RemoveContainer() remain solely responsible for the view tree and .NET MAUI remains solely responsible for invoking them when HasContainer flips.

I deliberately did not widen ContainerView's setter to protected. A method is the narrower contract: it cannot be assigned through a base-typed reference by accident, it gives a single choke point for validation, and it keeps the "the setter is an implementation detail" invariant intact for the built-in handlers.

Preserving platform type safety

On iOS/MacCatalyst and Tizen, ViewHandler<TVirtualView, TPlatformView> shadows ContainerView with a WrapperView-typed property whose getter hard-casts the base value:

public new WrapperView? ContainerView
{
    get => (WrapperView?)base.ContainerView;   // InvalidCastException if the base holds a plain UIView
    protected set => base.ContainerView = value;
}

Any write access to the base value can violate that invariant. So SetContainerView routes through a new private protected virtual void ValidateContainerView(PlatformView) hook, which iOS/MacCatalyst and Tizen override to reject non-WrapperView containers with a clear ArgumentException at the point of the mistake, rather than an InvalidCastException later from an unrelated getter. Android and Windows use is WrapperView pattern matching for their container access, so they accept any platform container type and need no guard.

What is not changed

  • No built-in handler was modified. ButtonHandler, ImageButtonHandler, ImageHandler, LabelHandler, BorderHandler, ShapeViewHandler and the per-platform ViewHandler<,> partials all keep using the existing private protected setter, so setup/remove flow, PlatformView/ContainerView propagation, disposal, and hot reload behavior are byte-for-byte unchanged.
  • IViewHandler.ContainerView, HasContainer, NeedsContainer, and MapContainerView are untouched.
  • No existing public API was changed or removed. The additions are recorded in PublicAPI.Unshipped.txt for all eight API surfaces.

Tests

Existing test assemblies (Microsoft.Maui.UnitTests, Microsoft.Maui.Core.DeviceTests, …) are InternalsVisibleTo friends of Microsoft.Maui, and a friend assembly can reach private protected members from a derived type. A test written there would therefore have proved nothing about external reachability.

So this PR adds src/Core/tests/ExternalBackend/Core.ExternalBackend.csprojMicrosoft.Maui.Core.ExternalBackend.TestSupport, which is deliberately not listed in src/Core/src/Properties/AssemblyInfo.cs. It contains an ExternalBackendViewHandler<TVirtualView, TPlatformView> modelled on the real Tizen handler, written against nothing but public/protected API. The fact that this assembly compiles is the proof that the new API is reachable from a non-friend assembly. (This mirrors the existing Controls.Xaml.UnitTests.ExternalAssembly pattern.)

ExternalBackendContainerViewTests (10 tests in Microsoft.Maui.UnitTests) then covers:

Test What it locks down
ExternalBackendAssemblyIsNotAFriendOfMauiCore Asserts the support assembly is not in Microsoft.Maui's InternalsVisibleTo list, so the compile-time proof cannot silently rot
SetupContainerInstallsContainerViewAndReparentsPlatformView Container installed, platform view re-parented, container takes the exact sibling index the platform view occupied
ContainerViewIsVisibleThroughIViewHandler IViewHandler.ContainerView reflects the externally installed container
RemoveContainerRestoresPlatformViewAndClearsContainerView Platform view restored to its original parent/slot, container cleared and disposed
NeedsContainerTransitionsDriveSetupAndRemoval Full NeedsContainerMapContainerViewHasContainerSetupContainer/RemoveContainer chain, including that an already-present container is not set up twice
SetupAndRemoveCanRoundTripRepeatedly Three full install/remove cycles leave the view tree exactly as it started
SettingHasContainerToTheSameValueIsANoOp No redundant setup/remove
SetContainerViewInstallsAndClearsTheContainerDirectly Raw install/clear semantics, and that SetContainerView does not flip HasContainer
DisconnectingTheHandlerAfterContainerRemovalIsClean Disposal/lifecycle: wrapper disposed, PlatformView nulled, virtual view's handler detached
FullViewMapperStillDrivesTheExternalContainer The real ViewHandler.ViewMapper chain (not just an isolated mapper) drives the external container

Verification run locally

  • Microsoft.Maui.Core.ExternalBackend.TestSupport builds ✅ (the external-access proof)
  • Core.UnitTests961 tests, 958 passed, 3 skipped, 0 failed
  • Controls.Core.UnitTests6237 tests, 6207 passed, 30 skipped, 0 failed
  • PublicAPI analyzer in Validate mode ✅ (verified negatively too: reverting SetContainerView to private protected makes it fail with RS0017)
  • dotnet format whitespace --verify-no-changes reports no findings in any touched file ✅
  • iOS/MacCatalyst/Android/Windows/Tizen TFMs could not be built locally (workloads unavailable in this environment) — relying on CI for those.

Adoption for an external backend (e.g. Maui.Tizen)

public class TizenViewHandler<TVirtualView, TPlatformView> : ViewHandler<TVirtualView, TPlatformView>
    where TVirtualView : class, IView
    where TPlatformView : NView
{
    public override bool NeedsContainer =>
        VirtualView?.Background is not null ||
        VirtualView?.Clip is not null ||
        VirtualView?.Shadow is not null ||
        base.NeedsContainer;

    protected override void SetupContainer()
    {
        if (PlatformView is null || ContainerView is not null)
            return;

        var wrapper = new TizenWrapperView();
        // …detach PlatformView from its parent, put it inside wrapper, attach wrapper in its place…

        SetContainerView(wrapper);
    }

    protected override void RemoveContainer()
    {
        // …detach the wrapper, put PlatformView back in its place, dispose the wrapper…

        SetContainerView(null);
    }
}

NeedsContainer no longer has to be pinned to false, so gradient/image backgrounds, Clip, and Shadow become available to the external backend.

Issues Fixed

Addresses gap G1 from Redth/Maui.Tizen#5.

ViewHandler.SetupContainer() and RemoveContainer() are protected
extensibility points, but ContainerView's setter is `private protected`.
A handler deriving from ViewHandler<TVirtualView, TPlatformView> in
another assembly - for example a platform backend that ships outside of
dotnet/maui - therefore has no way to publish the wrapper it created, so
it has to report NeedsContainer => false and loses gradient/image
backgrounds, clip and shadow.

Add a narrow `protected void SetContainerView(PlatformView?)` to
ViewHandler instead of widening the property setter. It only records the
container view: it never re-parents anything and it does not change
HasContainer, so SetupContainer/RemoveContainer stay in charge of the
lifecycle and all existing built-in handlers are untouched.

Platform type safety is preserved through a `private protected virtual`
ValidateContainerView hook. iOS/MacCatalyst and Tizen shadow
ContainerView with a WrapperView-typed property that hard-casts the base
value, so those overrides reject non-WrapperView containers with a clear
ArgumentException instead of failing later with an InvalidCastException.

Tests live in a new Core.ExternalBackend project whose assembly is
deliberately not an InternalsVisibleTo friend of Microsoft.Maui, so the
fact that it compiles proves the API is reachable externally. The new
unit tests cover setup/remove re-parenting and ordering, NeedsContainer
transitions driven through MapContainerView, repeated round trips,
no-op HasContainer assignments, direct SetContainerView install/clear,
and handler disconnect.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 26, 2026 20:02
@Redth
Redth temporarily deployed to copilot-pat-pool August 26, 2026 20:02 — with GitHub Actions Inactive
@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 -- 37854

Or

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

@Redth
Redth temporarily deployed to copilot-pat-pool August 26, 2026 20:02 — with GitHub Actions Inactive
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

@Redth
Redth temporarily deployed to copilot-pat-pool August 26, 2026 20:03 — with GitHub Actions Inactive
@Redth
Redth temporarily deployed to copilot-pat-pool August 26, 2026 20:05 — with GitHub Actions Inactive
@Redth
Redth temporarily deployed to copilot-pat-pool August 26, 2026 20:06 — with GitHub Actions Inactive
@github-actions github-actions Bot added area-core-platform Integration with platforms platform/ios platform/macos macOS / Mac Catalyst labels Aug 26, 2026
@Redth
Redth temporarily deployed to copilot-pat-pool August 26, 2026 20:07 — with GitHub Actions Inactive

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new protected extensibility point on Microsoft.Maui.Handlers.ViewHandler so handler implementations that live outside the dotnet/maui assembly (external platform backends) can publish a container wrapper into ContainerView, enabling container-driven features like backgrounds, clip, and shadow without requiring friend-assembly access.

Changes:

  • Introduces protected void ViewHandler.SetContainerView(PlatformView? containerView) plus a platform hook to validate container type (ValidateContainerView) where ContainerView is strongly-typed (iOS/MacCatalyst, Tizen).
  • Adds an “external backend” test-support assembly and a new unit test suite validating container lifecycle behavior and proving the API is reachable from a non-friend assembly.
  • Updates PublicAPI.Unshipped.txt across TFMs and wires the new test project into solutions/solution filters.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Core/src/Handlers/View/ViewHandler.cs Adds SetContainerView API, validation hook, and documentation updates around container lifecycle.
src/Core/src/Handlers/View/ViewHandlerOfT.iOS.cs Adds iOS/MacCatalyst override for container validation (WrapperView-only).
src/Core/src/Handlers/View/ViewHandlerOfT.Tizen.cs Adds Tizen override for container validation (WrapperView-only).
src/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt Records new protected API in netstandard2.0 surface.
src/Core/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt Records new protected API in netstandard surface.
src/Core/src/PublicAPI/net/PublicAPI.Unshipped.txt Records new protected API in net surface.
src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt Records new protected API in Android surface.
src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt Records new protected API in iOS surface.
src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt Records new protected API in MacCatalyst surface.
src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt Records new protected API in Windows surface.
src/Core/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt Records new protected API in Tizen surface.
src/Core/tests/ExternalBackend/Core.ExternalBackend.csproj Adds a non-friend “external backend” test-support project referencing Core.
src/Core/tests/ExternalBackend/ExternalPlatformViews.cs Provides minimal external-platform view/container types for tests.
src/Core/tests/ExternalBackend/ExternalBackendViewHandler.cs Implements a model external backend handler using only public/protected APIs.
src/Core/tests/UnitTests/ExternalBackendContainerViewTests.cs Adds unit tests validating external container lifecycle + reachability constraints.
src/Core/tests/UnitTests/Core.UnitTests.csproj References the new external-backend test-support project.
Microsoft.Maui.sln Adds the new test-support project to the main solution.
Microsoft.Maui-dev.sln Adds the new test-support project to the dev solution.
Microsoft.Maui-vscode.sln Adds the new test-support project to the VS Code solution.
Microsoft.Maui-mac.slnf Includes the new test-support project in the mac solution filter.
Microsoft.Maui-windows.slnf Includes the new test-support project in the windows solution filter.
Suppressed comments (1)

src/Core/src/Handlers/View/ViewHandler.cs:156

  • The new remarks for RemoveContainer similarly suggest all overrides should call SetContainerView(null), but in-repo implementations can (and do) clear ContainerView via the existing setter. Clarifying the guidance as applying when the setter isn’t accessible (external assemblies) would make the docs less confusing.
		/// <summary>
		/// Deconstructs the <see cref="ContainerView"/> and removes <see cref="PlatformView"/> from its container. 
		/// </summary>
		/// <remarks>This method is called when <see cref="HasContainer"/> is set to <see langword="false"/>.
		/// Overrides should call <see cref="SetContainerView(PlatformView?)"/> with <see langword="null"/> to clear the container they removed.</remarks>
		protected abstract void RemoveContainer();

Comment on lines 144 to 149
/// <summary>
/// Constructs the <see cref="ContainerView"/> and adds <see cref="PlatformView"/> to a container.
/// </summary>
/// <remarks>This method is called when <see cref="HasContainer"/> is set to <see langword="true"/>.</remarks>
/// <remarks>This method is called when <see cref="HasContainer"/> is set to <see langword="true"/>.
/// Overrides should call <see cref="SetContainerView(PlatformView?)"/> to publish the container they created.</remarks>
protected abstract void SetupContainer();
Comment thread src/Core/src/Handlers/View/ViewHandlerOfT.iOS.cs Outdated
@Redth

Redth commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

CI triage: both failing Windows Helix contexts are exact-base / unrelated infrastructure

Verdict: UNRELATED — reproduces identically on this PR's exact merge base with none of this PR's code. Zero tests failed anywhere in this build.

Root cause: AzDO results-upload outage, not a test failure

Every failed Helix work item exited -4 with a single error ID — DevOpsReportFailure:

Failed to upload results: TF10216: Azure DevOps services are currently unavailable.
Try again later. Activity Id: b785c464-ef2e-48df-975b-c62cd7f7c9c1

(a minority variant is the same upload call failing as ReadTimeout: HTTPSConnectionPool(host='dev.azure.com'...))

The test command itself exited 0 in every case. From the Microsoft.Maui.UnitTests work-item console:

=== TEST EXECUTION SUMMARY ===
   Microsoft.Maui.UnitTests  Total: 961, Errors: 0, Failed: 0, Skipped: 3, Time: 9.933s
C:\...>set _commandExitCode=0
['Microsoft.Maui.UnitTests.dll' END OF WORK ITEM LOG: Command exited with 0]

The -4 is produced after the run, by the Helix reporter failing to POST results to AzDO. Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(99,5) then surfaces it as error : Work item ... has failed, which is what fails the two AzDO contexts.

Exact-base proof

PR #37854 (build 1569121) Exact base bedd1b18b7 (build 1568717)
Branch refs/pull/37854/merge refs/heads/net11.0
Failed legs Windows Helix Unit Tests (Debug) + (Release) — and nothing else Windows Helix Unit Tests (Debug) + (Release) — and nothing else
Work-item exit code -4 -4
Error ID DevOpsReportFailure DevOpsReportFailure
Message TF10216: Azure DevOps services are currently unavailable TF10216: Azure DevOps services are currently unavailable
Test failures 0 0

bedd1b18b7 is this PR's merge base (git merge-base against origin/net11.0), so build 1568717 contains none of this PR's code and fails with a byte-identical fingerprint.

Two further concurrent builds in the same window corroborate a service-wide outage, both with the same DevOpsReportFailure signature and 0 test failures:

  • 1568987refs/heads/main — same two jobs, nothing else
  • 1569014refs/heads/release/11.0.1xx-rc1

Why this can't be caused by this PR

  1. The victim set is non-deterministic across jobs on the same commit. In Debug job e22744d3 Controls.Xaml failed and SourceGen passed; in Debug job 74d58977 — same commit, same binaries — SourceGen failed and Controls.Xaml passed. A code defect is deterministic; a transient upload outage picks random victims.
  2. The victims include assemblies this PR cannot touchMauiBlazorWebView.UnitTests, Controls.SourceGen.UnitTests, Resizetizer.UnitTests, Essentials.UnitTests. This PR changes ViewHandler plus one new test-support project.
  3. Every build leg passed, including Build Windows (Debug) and Build Windows (Release) — so the new Core.ExternalBackend.csproj reference compiles and packages correctly on Windows.
  4. Every affected work item reports Errors: 0, Failed: 0:
Work item Result
Microsoft.Maui.UnitTests Total 961, Failed 0, Skipped 3
Microsoft.Maui.Controls.Core.UnitTests Total 6237, Failed 0, Skipped 30
Microsoft.Maui.Controls.Xaml.UnitTests Total 2123, Failed 0, Skipped 8
Microsoft.Maui.Resizetizer.UnitTests Total 688, Failed 0, Skipped 2
Microsoft.Maui.MauiBlazorWebView.UnitTests Total 46, Failed 0, Skipped 0

Positive signal: the new tests ran and passed on Windows Helix

Microsoft.Maui.UnitTests totals, same Helix queue:

  • exact base bedd1b18b7: 951 tests, 0 failed
  • this PR (Debug): 961 tests, 0 failed
  • this PR (Release): 961 tests, 0 failed

+10 — exactly the 10 [Fact]s in ExternalBackendContainerViewTests. So the new external-assembly container tests were discovered, executed, and passed on Windows in both configurations. That also confirms Microsoft.Maui.Core.ExternalBackend.TestSupport — the deliberately non-friend assembly whose compilation is the external-access proof — builds and loads correctly on Windows.

Recommendation

No code change is warranted. These two contexts need a re-run once the AzDO upload path is healthy. Happy to push a rebase/empty commit to retrigger, or to make any change if the review surfaces something in the API shape itself — just say the word.

Evidence gathered per .github/docs/maui-ci-facts.md (pipeline maui-pr, def 302, dnceng-public/public), enumerating every failed timeline record and reading each Helix work item's /workitems/{name} detail rather than relying on Build Analysis.

…rotected

Two defects found in review of the SetContainerView extensibility point.

1. Disconnecting a handler that still had an active container left both
   ContainerView and HasContainer set. ElementHandler.DisconnectHandler()
   nulls PlatformView and never touched container state, so a later
   reconnect hit the HasContainer equality short-circuit in the setter,
   skipped SetupContainer(), and kept serving the stale wrapper that
   still held the previous platform view.

   The teardown has to run while PlatformView is still reachable: the
   generic PlatformView getter throws when null, so calling
   RemoveContainer() after the null would throw on Android and Tizen.
   Add a private protected virtual ElementHandler.OnDisconnecting() hook
   that runs before PlatformView is cleared, and override it in
   ViewHandler to flip HasContainer off (which unwinds the container
   through the platform's own RemoveContainer()) and clear ContainerView.
   RemoveContainer() is therefore invoked under exactly the precondition
   it already runs under during normal operation, so every existing
   platform override stays on its supported path.

2. ValidateContainerView was private protected, so an external subclass
   that shadows ContainerView with its own wrapper type could not enforce
   that type and the broad setter could corrupt its strongly typed
   getter. Promote it to protected virtual with XML docs, and update the
   iOS/MacCatalyst and Tizen overrides accordingly.

The external backend test handler now overrides ValidateContainerView to
require its own wrapper type. Note the override is declared with `object`
because on the neutral TFM the PlatformView alias is System.Object.

New regression tests (5), all verified to fail without the fixes:
DisconnectingWithAnActiveContainerTearsTheContainerDown,
ReconnectingAfterDisconnectWithAnActiveContainerRebuildsTheContainer,
SetContainerViewRejectsAContainerOfTheWrongType,
SetContainerViewRejectionLeavesAnExistingContainerIntact,
SetContainerViewStillAcceptsNullAfterValidationIsOverridden.

Also applies two reviewer nits: scope the SetupContainer/RemoveContainer
remarks to overrides outside this assembly (in-box handlers still use the
private protected setter), and restore `using System;` first in
ViewHandlerOfT.iOS.cs to match the other platform partials.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 21:42
@Redth

Redth commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

Both required fixes pushed — head 58776f0973

Thanks, both were real defects. Fixed, with regression tests verified to fail without each fix.


1. Disconnect with an active container left stale state

Confirmed. ElementHandler.DisconnectHandler() nulls PlatformView and never touched container state, so ContainerView and HasContainer survived the disconnect. On reconnect, MapContainerView set HasContainer = true — already true — so the setter's equality short-circuit returned early, SetupContainer() never ran, and the handler kept serving the stale wrapper still holding the previous platform view.

The non-obvious constraint you flagged ("before platform view becomes unavailable") is load-bearing: the generic getter throws when null —

public new TPlatformView PlatformView
    => (TPlatformView?)base.PlatformView ?? throw new InvalidOperationException("PlatformView cannot be null here");

— so calling RemoveContainer() after PlatformView = null would throw on Android and Tizen, whose RemoveContainer dereferences it. Teardown therefore has to happen before the null.

Added a private protected virtual ElementHandler.OnDisconnecting() that runs inside the existing guard, before PlatformView is cleared:

if (PlatformView != null && VirtualView != null)
{
    OnDisconnecting();          // <- platform view still reachable
    var oldPlatformView = PlatformView;
    PlatformView = null;
    DisconnectHandler(oldPlatformView);
}

ViewHandler overrides it to flip HasContainer off, which unwinds the container through the platform's own RemoveContainer(), then clears ContainerView.

Why this is safe for in-box handlers: RemoveContainer() is invoked under exactly the precondition it already runs under during normal operation (HasContainer going true → false with a live PlatformView). No platform override sees a novel state — I deliberately avoided inventing a new teardown path.

2. ValidateContainerView was unreachable externally

Confirmedprivate protected meant an external subclass shadowing ContainerView with its own wrapper type couldn't enforce it, leaving the broad setter able to corrupt its strongly typed getter.

Promoted to protected virtual with XML docs and an override example; iOS/MacCatalyst and Tizen overrides updated to protected override and now call base.

⚠️ Worth flagging for the API design review: on the neutral (non-platform) TFM the PlatformView alias is System.Object, so an external backend's override is declared protected override void ValidateContainerView(object containerView) — not its own platform type. The external test handler documents this inline. This is inherent to the existing per-TFM alias pattern (SetupContainer/ContainerView/SetContainerView all share it), not something new here — but it is the shape external backends will actually write against, so it's the right thing to scrutinise.


Tests

5 new tests, and I confirmed each fix is genuinely covered by removing the fix and re-running:

ReconnectingAfterDisconnectWithAnActiveContainerRebuildsTheContainer [FAIL]
DisconnectingWithAnActiveContainerTearsTheContainerDown             [FAIL]
Failed! - Failed: 2, Passed: 13, Total: 15

With the fix restored, all 15 pass.

Test Covers
DisconnectingWithAnActiveContainerTearsTheContainerDown Container unwound + disposed, HasContainer reset, platform view restored to its original parent
ReconnectingAfterDisconnectWithAnActiveContainerRebuildsTheContainer Reconnect runs SetupContainer again (count 1→2), yields a different, non-disposed container wrapping the live platform view
SetContainerViewRejectsAContainerOfTheWrongType ArgumentException with ParamName == "containerView" — from the external, non-friend assembly
SetContainerViewRejectionLeavesAnExistingContainerIntact A rejected install doesn't clobber or dispose the current container
SetContainerViewStillAcceptsNullAfterValidationIsOverridden null still means "clear" and bypasses validation

Full verification (net11.0)

Suite Result
Core.UnitTests 966 total, 0 failed (was 961; +5)
Controls.Core.UnitTests 6237 total, 0 failed
Controls.Xaml.UnitTests 2123 total, 0 failed
PublicAPI analyzer (Validate) clean — entries added for ValidateContainerView across all 8 surfaces, plus the iOS/MacCatalyst/Tizen overrides
GenerateDocumentationFile=true 0 warnings
dotnet format whitespace --verify-no-changes clean in all touched files

Controls.Core.UnitTests and Controls.Xaml.UnitTests are the meaningful blast-radius check, since OnDisconnecting() is on ElementHandler and therefore runs for every handler — both fully green.

Reviewer nits also applied

  • SetupContainer/RemoveContainer remarks now scope the SetContainerView guidance to overrides outside this assembly, since in-box handlers legitimately keep using the private protected setter.
  • ViewHandlerOfT.iOS.cs restored to using System; first, matching Android/Windows/Tizen.

Understood that API design review is still open — happy to reshape the surface (naming, splitting validation off SetContainerView, or a typed setter contract) if you'd prefer a different direction.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Comment on lines 144 to +160
@@ -151,5 +155,10 @@ void IElementHandler.DisconnectHandler()

_handlerState = ElementHandlerState.Disconnected;
}

// Runs while PlatformView is still set, so overrides can use it to unwind platform state.
private protected virtual void OnDisconnecting()
@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Aug 26, 2026

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 5 findings

See inline comments for details.

// reconnect would skip SetupContainer() and keep pointing at the stale container.
if (HasContainer)
{
HasContainer = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Handler Mapper and Property Patterns — Setting HasContainer = false here does not just clear state, it invokes the platform RemoveContainer() override during teardown, which re-parents PlatformView back into the container's live parent (iOS oldParent?.InsertSubview(PlatformView, idx) in ViewHandlerOfT.iOS.cs, oldParentChildren?.Insert(idx, PlatformView) on Windows, WrapperView.RemoveContainer on Android). Concrete scenario: Element.DisconnectHandlers() (page unloaded / Shell tab switch) walks a subtree whose platform views are still attached; for every view with Background/Clip/Shadow the wrapper is removed and the now-disconnected raw platform view is inserted into the parent in its place. Nothing removes it afterwards — the parent-side removal path goes through view.ToPlatform(), which no longer resolves to that instance once the handler is disconnected, so the dead view is left rendered in the parent's children. The new test ExternalBackendContainerViewTests.DisconnectingWithAnActiveContainerTearsTheContainerDown asserts exactly this end state (Assert.Same(parent, platformView.Parent)) but only for the in-memory fake backend, where there is no subsequent removal step to break. If the goal is only to prevent stale ContainerView/HasContainer from surviving into a reconnect, clearing the backing field _hasContainer plus ContainerView (without invoking RemoveContainer) achieves that without mutating the view tree at teardown.

HasContainer = false;
}

ContainerView = null;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Backward Compatibility and Migration — On Tizen this changes the documented dispose order. ViewHandlerOfT.Tizen.cs Dispose(bool) calls (this as IElementHandler)?.DisconnectHandler() and then platformView?.Dispose(); ContainerView?.Dispose();. With OnDisconnecting() now running RemoveContainer(), the Tizen override re-adds PlatformView to parent (containable.Children.Add(PlatformView) / parent?.Add(PlatformView)) and calls PlatformView.UpdateBounds(bounds), and immediately afterwards Dispose() disposes that same platform view — leaving a disposed NUI View parented in the live container. The trailing ContainerView?.Dispose() in Dispose(bool) also becomes dead code because RemoveContainer already disposed and nulled it. No test covers the Tizen dispose path.

{
// Give derived handlers a chance to tear down state that needs the platform view
// while it is still reachable through the PlatformView property.
OnDisconnecting();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Null Safety and Defensive CodingOnDisconnecting() runs before PlatformView is captured/nulled and before DisconnectHandler(oldPlatformView), and it is not exception-guarded. The ViewHandler override now calls into arbitrary RemoveContainer() implementations (including third-party and compatibility renderers) that were previously never invoked on the disconnect path. Concrete failure: ViewHandler<,>.Context on Android throws InvalidOperationException when MauiContext is null, and WrapperView.RemoveContainer(PlatformView, Context, ...) dereferences it; any throw from here aborts the whole disconnect, so VirtualView.Handler is never unlinked, VirtualView/PlatformView are never cleared and _handlerState is never set to Disconnected — the handler is left half-disconnected and the virtual view leaks. Consider wrapping the container unwind so a failure cannot prevent the rest of the disconnect.

ValidateContainerView(containerView);
}

ContainerView = containerView;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Public API Surface DesignSetContainerView stores the container without touching HasContainer, so an external backend can end up with ContainerView != null while HasContainer == false (codified by the new test SetContainerViewInstallsAndClearsTheContainerDirectly, which asserts Assert.False(handler.HasContainer) with a live container). In that state the next MapContainerView pass that evaluates NeedsContainer == false short-circuits in the HasContainer setter (if (_hasContainer == value) return;), so RemoveContainer() is never called and the wrapper stays installed and attached for the life of the handler. Either keep the flag in sync when the value transitions between null and non-null, or document/guard that SetContainerView is only legal from inside SetupContainer/RemoveContainer.

}

[Fact]
public void DisconnectingWithAnActiveContainerTearsTheContainerDown()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — This is the only test covering the new disconnect-time container unwind, and it exercises a netstandard fake backend (ExternalPlatformView/ExternalWrapperView) whose RemoveContainer has none of the semantics of the real platform overrides (no RemoveFromSuperview/InsertSubview index restoration, no WrapperView.Disconnect(), no UpdateTransformation, no Android Context access, no Tizen Dispose). The behavior change in ViewHandler.OnDisconnecting affects iOS/MacCatalyst, Android, Windows and Tizen teardown, but no device test asserts that disconnecting a handler whose container is still attached leaves the real view tree in the expected state (e.g. a Layout child with a Shadow disconnected while the layout survives). Add at least one platform device test before relying on this path.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review Summary

@Redth — new AI review results are available based on commit 58776f0.

Gate Inconclusive Confidence Unknown Platform iOS


🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ INCONCLUSIVE

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

🩺 Base branch does not compile — the without-fix build failed. The gate's "does the test fail without the fix" check is unreliable here; this usually means main is broken or a merge-base file went missing. Investigate before trusting this gate.

/Users/cloudtest/vss/_work/1/s/src/Core/tests/ExternalBackend/ExternalBackendViewHandler.cs(63,27): error CS0115: 'ExternalBackendViewHandler<TVirtualView, TPlatformView>.ValidateContainerView(object)...

Test Without Fix (expect FAIL) With Fix (expect PASS)
🧪 ExternalBackendContainerViewTests ExternalBackendContainerViewTests 🛠️ BUILD ERROR ✅ PASS — 24s
🔴 Without fix — 🧪 ExternalBackendContainerViewTests: 🛠️ BUILD ERROR · 27s

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

/Users/cloudtest/vss/_work/1/s/src/Core/tests/ExternalBackend/ExternalBackendViewHandler.cs(63,27): error CS0115: 'ExternalBackendViewHandler<TVirtualView, TPlatformView>.ValidateContainerView(object)': no suitable method found to override [/Users/cloudtest/vss/_work/1/s/src/Core/tests/ExternalBackend/Core.ExternalBackend.csproj]
🟢 With fix — 🧪 ExternalBackendContainerViewTests: PASS ✅ · 24s

(no coded error found; showing last 1200 chars)

ft.Maui.UnitTests
  Passed FullViewMapperStillDrivesTheExternalContainer [14 ms]
  Passed RemoveContainerRestoresPlatformViewAndClearsContainerView [10 ms]
  Passed SetContainerViewRejectionLeavesAnExistingContainerIntact [< 1 ms]
  Passed SetupContainerInstallsContainerViewAndReparentsPlatformView [< 1 ms]
  Passed SetContainerViewRejectsAContainerOfTheWrongType [< 1 ms]
  Passed ContainerViewIsVisibleThroughIViewHandler [< 1 ms]
  Passed ReconnectingAfterDisconnectWithAnActiveContainerRebuildsTheContainer [< 1 ms]
  Passed ExternalBackendAssemblyIsNotAFriendOfMauiCore [1 ms]
  Passed SetContainerViewInstallsAndClearsTheContainerDirectly [< 1 ms]
[xUnit.net 00:00:00.30]   Finished:    Microsoft.Maui.UnitTests
  Passed SettingHasContainerToTheSameValueIsANoOp [< 1 ms]
  Passed SetupAndRemoveCanRoundTripRepeatedly [< 1 ms]
  Passed NeedsContainerTransitionsDriveSetupAndRemoval [< 1 ms]
  Passed DisconnectingWithAnActiveContainerTearsTheContainerDown [< 1 ms]
  Passed DisconnectingTheHandlerAfterContainerRemovalIsClean [< 1 ms]
  Passed SetContainerViewStillAcceptsNullAfterValidationIsOverridden [< 1 ms]
Test Run Successful.
Total tests: 15
     Passed: 15
 Total time: 0.6827 Seconds

⚠️ Failure Details

  • 🛠️ ExternalBackendContainerViewTests without fix: build failed before tests could run
    • /Users/cloudtest/vss/_work/1/s/src/Core/tests/ExternalBackend/ExternalBackendViewHandler.cs(63,27): error CS0115: 'ExternalBackendViewHandler<TVirtualView, TPlatformView>.ValidateContainerView(object)...
📁 Fix files reverted (17 files)
  • Microsoft.Maui-dev.sln
  • Microsoft.Maui-mac.slnf
  • Microsoft.Maui-vscode.sln
  • Microsoft.Maui-windows.slnf
  • Microsoft.Maui.sln
  • src/Core/src/Handlers/Element/ElementHandler.cs
  • src/Core/src/Handlers/View/ViewHandler.cs
  • src/Core/src/Handlers/View/ViewHandlerOfT.Tizen.cs
  • src/Core/src/Handlers/View/ViewHandlerOfT.iOS.cs
  • src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
  • src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt
  • src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
  • src/Core/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt
  • src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt
  • src/Core/src/PublicAPI/net/PublicAPI.Unshipped.txt
  • src/Core/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt
  • src/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt

📋 Pre-Flight — Context & Validation

PR #37854 Pre-Flight

Problem

External platform backends can override ViewHandler.SetupContainer() and RemoveContainer(), but cannot assign the private protected ContainerView setter from another assembly. They therefore cannot participate in MAUI's container lifecycle for backgrounds, clipping, and shadows.

Current PR approach

The PR targets net11.0 and adds:

  • Protected SetContainerView(PlatformView?) and virtual ValidateContainerView(PlatformView) APIs on ViewHandler.
  • iOS/Mac Catalyst and Tizen validation overrides that preserve their WrapperView-typed shadow properties.
  • A new ElementHandler.OnDisconnecting() hook plus a ViewHandler override that removes an active container and clears stale container state before the platform view is nulled.
  • Public API entries for all Core API surfaces.
  • A deliberately non-friend external-backend test assembly and 15 focused ExternalBackendContainerViewTests.

The direct diff changes 22 files (+892/-2). Seventeen existing files are revertable production/API/solution files; five added test-support files remain available as the compile-time external-access proof.

Alternative-fix constraints

  • A candidate must solve external assignment without repeating the PR's paired SetContainerView plus overridable ValidateContainerView design.
  • Preserve iOS/Mac Catalyst and Tizen type safety where ContainerView hard-casts to WrapperView.
  • Preserve setup/removal, sibling index, reconnect/disconnect, HasContainer, IViewHandler.ContainerView, and mapper behavior covered by the focused tests.
  • Modify only files listed in .github/.baseline-state.json under RevertedFiles; added PR files are read-only.
  • Follow handler lifecycle, public API, and platform threading instructions.

Validation scope

Primary test only:

dotnet test src/Core/tests/UnitTests/Core.UnitTests.csproj --filter "ExternalBackendContainerViewTests"

No mandatory regression tests were produced by the regression cross-reference. Do not run a full suite and do not rerun gate verification.

The prior gate is INCONCLUSIVE because the without-fix state could not compile the added external-backend support assembly after the new APIs were reverted. The with-fix focused run completed with all 15 tests passing, but that does not make the gate conclusive.


🔬 Code Review — Deep Analysis

Expert Evaluation — PR #37854

Verdict

NEEDS_CHANGES with low confidence in overall merge safety because the Gate and current required-check state are undetermined; confidence is high in the concrete disconnect mechanism described below. The new protected container API and platform-specific type validation are directionally sound, but the submitted disconnect cleanup introduces framework-wide teardown behavior that is not required to expose the API safely.

Actionable findings

  1. Major — disconnect mutates the live native view tree. ViewHandler.OnDisconnecting() assigns HasContainer = false, which invokes each platform's RemoveContainer() implementation rather than merely clearing handler state. When a still-attached subtree is disconnected, this can replace the wrapper with a raw platform view that is no longer reachable through the disconnected handler and can remain orphaned in the parent.
  2. Moderate — Tizen can re-parent a view immediately before disposing it. The new unwind runs before ViewHandlerOfT.Tizen.Dispose(bool) disposes the platform view, potentially leaving a disposed NUI view attached to a live parent.
  3. Moderate — teardown can be aborted by a container-removal exception. The new OnDisconnecting() call precedes all existing unlink/clear state transitions. A platform or third-party RemoveContainer() exception can leave the handler half-disconnected and retain the virtual view.
  4. Moderate — SetContainerView permits container/flag desynchronization. It can establish ContainerView != null while HasContainer == false; a later false assignment to HasContainer then short-circuits and never invokes RemoveContainer(). The method should enforce its intended lifecycle context or otherwise preserve the invariant.
  5. Moderate — the disconnect behavior lacks a real-platform test. The netstandard fake backend cannot exercise native re-parenting, context access, transform restoration, or Tizen disposal ordering.

Raw file-and-line findings are persisted in ../inline-findings.json.

Blast radius and failure-mode assessment

The new disconnect hook runs for every ViewHandler on all platforms, including views that use containers for backgrounds, clips, shadows, or input behavior. It adds no static state and does not affect startup, but it changes handler disconnect/reconnect and disposal semantics across the framework. The inconclusive Gate is treated as missing evidence, not a regression failure.

Consolidated refinement

Retain the external-backend API and platform type validation, but remove the new disconnect-time native re-parenting path. A safe consolidated candidate should reset stale handler bookkeeping without assigning HasContainer = false through its behavior-bearing setter, and should make the legal relationship between SetContainerView and the container lifecycle explicit. Validate only with the required focused ExternalBackendContainerViewTests command.


🛠️ Try-Fix — Analysis & Comparison

PR #37854 Alternative Fix Candidates

Candidate 1 — Protected ContainerView setter

Model: claude-opus-5
Result: Blocked
Attempt artifacts: CustomAgentLogsTmp/PRState/37854/PRAgent/try-fix/attempt-1/
Candidate report: CustomAgentLogsTmp/PRState/37854/PRAgent/try-fix-1/content.md

Approach

Widen ViewHandler.ContainerView from private protected set to protected set. External backends could then assign their wrapper directly, while the existing iOS/Mac Catalyst and Tizen WrapperView-typed shadow setters would narrow the accepted type at compile time. Container teardown would use the existing disconnect virtual rather than the PR's new ElementHandler.OnDisconnecting() hook.

Difference from the PR

The PR adds an indirect protected writer plus a virtual runtime-validation hook: inaccessible storage leads to SetContainerView, which calls ValidateContainerView, whose platform overrides reject invalid types at runtime. Candidate 1 instead makes the storage setter externally derivable and relies on the existing strongly typed shadow setters to reject invalid platform wrapper assignments at compile time.

Result and evidence

No source files were changed and no test was run. EstablishBrokenBaseline.ps1 rejected the pre-existing dirty worktree before creating .github/.baseline-state.json; the 44 dirty tracked paths are harness-owned .github/scripts, .github/skills, and eng/scripts files, not PR production files. The only allowed restore command reported No baseline state found / Restored False, which is the permitted no-state blocked path because the attempt made no edits.

The unchanged added external-backend fixture also directly calls SetContainerView, overrides ValidateContainerView, and contains tests for that exact runtime-validation contract. Because those added files are read-only under the baseline boundary, a mechanism-level alternative that removes those APIs cannot compile against the existing focused test fixture without first changing tests outside the allowed file set.

Test command (not run):

dotnet test src/Core/tests/UnitTests/Core.UnitTests.csproj --filter "ExternalBackendContainerViewTests"

Self-review: 0 findings; the candidate diff is empty.

Diff: Empty.

Candidate 2 — External-owned container resolved by a virtual getter

Model: gpt-5.6-sol
Result: Blocked
Attempt artifacts: CustomAgentLogsTmp/PRState/37854/PRAgent/try-fix/attempt-2/
Candidate report: CustomAgentLogsTmp/PRState/37854/PRAgent/try-fix-2/content.md

Approach

Let an external backend own its strongly typed container field and expose that field to MAUI through a virtual getter/resolver. Core would query the backend's container rather than requiring the backend to mutate Core's ContainerView storage.

Difference from prior approaches

This avoids both prior mechanisms. Unlike the PR, it does not add a protected writer followed by runtime validation. Unlike Candidate 1, it does not widen the existing property's setter. Ownership remains in the external handler, where the field type enforces the wrapper contract, while a virtual read path makes the current wrapper visible through MAUI and IViewHandler.

Result and evidence

No source files were changed and no test was run. EstablishBrokenBaseline.ps1 again rejected the same 44 pre-existing dirty harness files before creating .github/.baseline-state.json, leaving no RevertedFiles allow-list. The candidate did not clean, stash, commit, copy, or otherwise work around those harness-owned changes. The mandatory restore command reported No baseline state found / Restored False, the permitted no-state blocked path because no attempt edits were made.

The added external-backend fixture is read-only under the baseline boundary and directly requires the PR's SetContainerView and ValidateContainerView APIs. It therefore cannot compile unchanged against this getter-based alternative.

Test command (not run):

dotnet test src/Core/tests/UnitTests/Core.UnitTests.csproj --filter "ExternalBackendContainerViewTests"

Self-review: 0 findings; the candidate diff is empty.

Diff: Empty.

Aggregate outcome

Two mechanism-level alternatives were bounded and recorded, but neither could be implemented or tested because the required baseline script refused the pre-existing dirty harness worktree before producing an edit allow-list. Both attempts preserved the PR state and all harness-owned changes. These are design candidates only, not verified patches.


📝 PR Finalize — Recommended Title & Description

Assessment: ✏️ Recommend updating — the current metadata understates the submitted disconnect behavior, names ValidateContainerView with stale accessibility, and describes 10 tests although the raw PR HEAD contains 15.

Recommended title

[Core] ViewHandler: Add external ContainerView management and disconnect cleanup

Recommended description

## Description of Change

`ViewHandler.SetupContainer()` and `ViewHandler.RemoveContainer()` are `protected` extensibility points, but `ViewHandler.ContainerView`'s setter is `private protected`. A handler that derives from `ViewHandler<TVirtualView, TPlatformView>` in another assembly—such as a platform backend shipped outside `dotnet/maui`—can create and attach a wrapper but cannot publish it as the handler's container view.

The practical consequence, reported as gap **G1** in [Redth/Maui.Tizen#5](https://github.com/Redth/Maui.Tizen/pull/5), is that an external backend must report `NeedsContainer => false` and loses gradient/image backgrounds, `Clip`, and `Shadow`, because those features are rendered by the container view on the in-box platforms.

### Public API

This PR adds two protected members to `ViewHandler`:

```csharp
protected void SetContainerView(PlatformView? containerView);
protected virtual void ValidateContainerView(PlatformView containerView);
  • SetContainerView records or clears the value exposed by ContainerView; it does not re-parent views or change HasContainer.
  • External handlers call it from SetupContainer() after installing their wrapper and from RemoveContainer() after restoring the platform view.
  • ValidateContainerView is the override point for handlers that expose a narrower container type.
  • The existing ContainerView setter remains private protected, so in-box handlers continue using it directly.

The method is intentionally narrower than widening the setter to protected: it preserves the setter as an implementation detail, prevents assignment through the broader base property from becoming the normal contract, and provides one validation choke point for external implementations.

Platform type safety

iOS/Mac Catalyst and Tizen shadow ContainerView with a WrapperView-typed property whose getter hard-casts the base value. Their ValidateContainerView overrides reject non-WrapperView values with an ArgumentException before an invalid value can reach that getter. Android and Windows use is WrapperView checks and do not require a narrower validator.

Disconnect behavior

The submitted implementation also adds a private protected virtual ElementHandler.OnDisconnecting() hook that runs before PlatformView is cleared. ViewHandler overrides it to set HasContainer = false, invoke the platform RemoveContainer() path for an active container, and clear ContainerView. This is intended to prevent stale HasContainer/ContainerView state from suppressing SetupContainer() if the same handler instance is connected again.

IViewHandler.ContainerView, NeedsContainer, and MapContainerView are otherwise unchanged. The new protected APIs are recorded in PublicAPI.Unshipped.txt for all eight Core API surfaces.

Tests

The existing MAUI test assemblies are InternalsVisibleTo friends and therefore cannot prove that a private protected member is inaccessible to a real external backend. This PR adds src/Core/tests/ExternalBackend/Core.ExternalBackend.csproj, producing Microsoft.Maui.Core.ExternalBackend.TestSupport, which is deliberately not a friend of Microsoft.Maui. This follows the existing Controls.Xaml.UnitTests.ExternalAssembly pattern.

Its ExternalBackendViewHandler<TVirtualView, TPlatformView> uses only public/protected API. ExternalBackendContainerViewTests contains 15 focused tests covering:

  • non-friend assembly reachability;
  • setup/removal, exact sibling-index preservation, mapper visibility, and repeated round trips;
  • NeedsContainer and HasContainer transitions;
  • direct install/clear semantics;
  • disconnect after removal, active-container disconnect, and reconnect;
  • wrong-type rejection, preservation of an existing valid container, and null validation behavior;
  • the full ViewHandler.ViewMapper path.

The focused 15-test run passes. Platform-specific TFMs were not available locally, so iOS, Mac Catalyst, Android, Windows, and Tizen behavior relies on CI.

Adoption for an external backend

public class TizenViewHandler<TVirtualView, TPlatformView> : ViewHandler<TVirtualView, TPlatformView>
    where TVirtualView : class, IView
    where TPlatformView : NView
{
    public override bool NeedsContainer =>
        VirtualView?.Background is not null ||
        VirtualView?.Clip is not null ||
        VirtualView?.Shadow is not null ||
        base.NeedsContainer;

    protected override void SetupContainer()
    {
        if (PlatformView is null || ContainerView is not null)
            return;

        var wrapper = new TizenWrapperView();
        // Detach PlatformView, place it in wrapper, and attach wrapper in the same slot.
        SetContainerView(wrapper);
    }

    protected override void RemoveContainer()
    {
        // Detach and dispose wrapper, then restore PlatformView to its original slot.
        SetContainerView(null);
    }
}

NeedsContainer no longer has to be pinned to false, enabling gradient/image backgrounds, Clip, and Shadow in the external backend.

Issues Fixed

Addresses gap G1 from Redth/Maui.Tizen#5.



</details>

---

<details>
<summary><strong>🏁 Report — Final Recommendation</strong></summary>
<br/>

## ⚠️ Final Recommendation: REQUEST CHANGES

**Winner: `pr-plus-reviewer`.** The submitted PR's API shape is useful and mostly sound, but its new disconnect hook turns every active-container handler disconnect into a platform `RemoveContainer()` operation. That introduces concrete native-tree, disposal-order, and partial-disconnect risks unrelated to the external-backend API. The winning candidate removes that global behavior and enforces the documented `SetContainerView` lifecycle contract.

## Candidate comparison

| Rank | Candidate | Implementation | Focused/regression evidence | Assessment |
| ---: | --- | --- | --- | --- |
| 1 | `pr-plus-reviewer` | Complete patch | **Pass:** 12 passed, 0 failed | Preserves external container extensibility and platform type validation; removes the unsafe disconnect-time re-parenting path; rejects container/`HasContainer` desynchronization. One wrong-type test lost `[Fact]`, so coverage is narrower than intended, but another discovered test still exercises type rejection. |
| 2 | `pr` | Complete submitted patch | With-fix focused run previously passed 15; Gate remains **inconclusive**, not failed | Solves external assignment and preserves iOS/Mac Catalyst/Tizen hard-cast safety, but adds a framework-wide disconnect behavior with concrete risks. `HasContainer = false` invokes native `RemoveContainer()` while the subtree can still be attached; Tizen can then dispose a re-parented view; an exception can abort handler unlinking. |
| 3 | `try-fix-1` | Design only; empty diff | **Blocked:** no test run | Widening the setter is simpler and may provide compile-time wrapper typing, but the attempt produced no patch, could not compile against the immutable PR-specific fixture, and did not validate lifecycle behavior. |
| 4 | `try-fix-2` | Design only; empty diff | **Blocked:** no test run | External-owned state plus a virtual resolver avoids mutation, but creates two sources of container truth and likewise produced no compilable or tested candidate. |

No candidate failed a regression test. The two STEP 5a alternatives rank below the implemented candidates because both were blocked before editing and validation.

## Expert review

The raw PR received five inline findings:

1. **Major:** disconnecting an active container can replace a live wrapper with a now-disconnected raw platform view.
2. **Moderate:** Tizen can re-parent a platform view immediately before disposing it.
3. **Moderate:** an exception from `RemoveContainer()` can leave the handler half-disconnected.
4. **Moderate:** direct `SetContainerView` calls can desynchronize `ContainerView` and `HasContainer`.
5. **Moderate:** the new cross-platform teardown behavior is covered only by a netstandard fake backend.

`pr-plus-reviewer` eliminates findings 1, 2, 3, and 5 by removing the behavioral disconnect change, and addresses finding 4 with a lifecycle-state guard.

## Blast radius and failure modes

- **Raw PR:** shared handler infrastructure on every platform; all views using a container for background, clip, shadow, or related behavior are affected during disconnect.
- **Winning candidate:** additive protected API plus validation on iOS/Mac Catalyst/Tizen; it does not alter startup, static state, existing in-box setup/removal, or disconnect behavior.
- **Null/default behavior:** `null` bypasses container-type validation and is legal during removal/initial empty state; a non-null value is validated before storage.
- **Reconnect behavior:** the candidate deliberately preserves existing MAUI disconnect semantics instead of introducing an unverified global fix for handler-instance reuse.

## Review and CI reconciliation

Earlier Copilot review comments were documentation/accessibility and ordering suggestions, not prior error-level findings. The current raw diff already narrows the setup/remove guidance to external assemblies and fixes the using order; the candidate removes the questioned `OnDisconnecting` hook.

The trusted Gate is **inconclusive** because its without-fix state could not build the external support assembly; this is missing evidence, not a failed fix. A prior exact-base CI analysis found the Windows Helix failures were unrelated Azure DevOps result-upload failures with zero test failures. The current required-check query was unavailable because `gh` was unauthenticated, so CI status is not treated as approval evidence.

**External Output Contract:** Not applicable.  
**Trim/AOT Evidence Chain:** Not applicable.

## Required PR changes

Apply the `pr-plus-reviewer` design before merge: keep the external-backend API/type validation, remove the shared disconnect hook and active-container teardown tests, and enforce the documented setup/remove lifecycle when storing the container. Restore test discovery for `SetContainerViewRejectsAContainerOfTheWrongType` when integrating the patch; the one-pass candidate run could not be repeated.


</details>

---

<details>
<summary><strong>📱 UI Tests — ViewBaseTests</strong></summary>
<br/>

**Detected UI test categories:** `ViewBaseTests`

<!-- DEEP_UITESTS_BEGIN -->
✅ **Deep UI tests** — 112 passed, 0 failed across 1 category on platform-pool agent (replaces in-process counts above).

### 🧪 UI Test Execution Results (deep, platform pool)

| Category | Tests | Snapshot diffs |
|---|---|---|
| `ViewBaseTests` | 112/112 ✓ | — |
📎 [Download `drop-deep-uitests` artifact (TRX + snapshot diffs)](https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=15111001&view=artifacts&pathAsName=false&type=publishedArtifacts)

<!-- DEEP_UITESTS_END -->


</details>

</details>
<!-- SESSION:58776f0 END -->

---

<details>
<summary><strong>🧭 Next Steps</strong> — reviewer changes required</summary>
<br/>

**The reviewer-enhanced candidate identified changes that are not yet in the submitted PR.**

**Why:** pr-plus-reviewer preserves the external-backend API and platform type safety while removing the raw PR&#39;s framework-wide disconnect-time re-parenting risk. Its focused run passed 12 tests with no failures, though one wrong-type test lost its Fact attribute and remains a documented coverage caveat.

Address the actionable findings in this review before merging.

</details>

@MauiBot MauiBot added s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-fix-win AI found a better alternative fix than the PR s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) and removed s/agent-review-in-progress AI review is currently running for this PR labels Aug 26, 2026
The override example showed a backend-specific platform type as the
parameter, but PlatformView is a per-TFM alias that resolves to
System.Object on the non-platform build. An external backend copying the
example verbatim gets CS0115 ("no suitable method found to override"),
which is exactly what happened while writing the external backend test
handler in this PR.

Show the override taking `object` and testing the concrete wrapper type
with a pattern match instead, and state the aliasing rule explicitly so
the constraint is discoverable from the API docs rather than from a
compiler error.

Docs-only; no API or behavior change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 27, 2026 00:19
@Redth

Redth commented Aug 27, 2026

Copy link
Copy Markdown
Member Author

Merge-readiness at head cde2184e97

Every failing check traces to one live Azure DevOps infrastructure outage. Zero tests failed anywhere in this build.

The failures

Build 1569341 had exactly two failing jobs — Windows Helix Unit Tests (Debug) and (Release). I enumerated every failed timeline record and opened each log; there are no other failed legs, and no error lines other than the Helix SDK relaying work-item status.

All 17 failed work items across all 4 Helix jobs share one signature — exit -4, error ID DevOpsReportFailure:

Failed to upload results: TF10216: Azure DevOps services are currently unavailable.

The test command exited 0 in every case. Per-work-item results:

Work item Result
Microsoft.Maui.UnitTests Total 966, Failed 0
Microsoft.Maui.Controls.Core.UnitTests Total 6237, Failed 0
Microsoft.Maui.Controls.Xaml.UnitTests Total 2123, Failed 0
Microsoft.Maui.Controls.SourceGen.UnitTests Total 508, Failed 0
Microsoft.Maui.Essentials.UnitTests Total 513, Failed 0
Microsoft.Maui.MauiBlazorWebView.UnitTests Total 46, Failed 0
Graphics.Tests Total 375, Failed 0

Build Analysis "17 tests failed" — not real tests

Build Analysis reports 17 failures, but every one is a synthetic <Assembly>.dll.WorkItemExecution pseudo-test, which Build Analysis itself annotates "This is a helix work item crash" and links to helix-workitem-deadletter.txt. Not one is an actual xUnit test method. The count matches the 17 upload-failed work items exactly.

Build Analysis also supplies its own historical rates, which show these are long-standing infrastructure flakes independent of this PR:

Pseudo-test Historical failure rate
Microsoft.Maui.Controls.Core.UnitTests.dll.WorkItemExecution 11.00%
Microsoft.Maui.Controls.Xaml.UnitTests.dll.WorkItemExecution 9.68%
Microsoft.Maui.Controls.SourceGen.UnitTests.dll.WorkItemExecution 1.32%
Microsoft.Maui.Essentials.UnitTests.dll.WorkItemExecution 1.04%
Graphics.Tests.dll.WorkItemExecution 0.88%

The maui-pr rollup ("had test failures", 0 errors / 59 warnings) is just the aggregate of those two Helix legs.

The outage is live and repo-wide

I reproduced it directly — querying the AzDO test API myself returns the same error:

GET /_apis/test/ResultSummaryByBuild?buildId=1569341
→ TF10216: Azure DevOps services are currently unavailable.
   CircuitBreakerShortCircuitException

Every completed maui-pr build in the window failed, including branch builds containing none of this code:

Build Branch Failing jobs
1569387 refs/pull/37857/merge Win Helix Debug + Release
1569339 refs/pull/37858/merge Win Helix Debug + Release (+RunOnAndroid)
1569337 refs/pull/37855/merge Win Helix Debug + Release
1568987 refs/heads/main Win Helix Debug + Release
1569014 refs/heads/release/11.0.1xx-rc1 Win Helix Debug + Release
1568717 refs/heads/net11.0 @ bedd1b18b7 — this PR's exact merge base Win Helix Debug + Release

Spot-checked PR #37857's work item: exit -4, DevOpsReportFailure, TF10216 — identical.

Nothing here is caused by this change, and there is nothing in it to fix.

Positive signal: the new tests ran green on Windows

Microsoft.Maui.UnitTests on Windows Helix, all four jobs: Total: 966, Failed: 0.

  • exact base bedd1b18b7: 951
  • previous head: 961 (+10)
  • this head: 966 (+5) — the 5 new disconnect/validation regression tests

966 matches my local run exactly, so all 15 external-container tests pass on Windows in both Debug and Release.


API review (self-audit)

Full public API delta vs net11.0purely additive, no removals or modifications:

Microsoft.Maui.Handlers.ViewHandler.SetContainerView(PlatformView? containerView) -> void
virtual Microsoft.Maui.Handlers.ViewHandler.ValidateContainerView(PlatformView! containerView) -> void
override ViewHandler<TVirtualView, TPlatformView>.ValidateContainerView(...)   // iOS, MacCatalyst, Tizen

Two members per TFM plus three platform overrides. Reviewed against the repo's API rules:

  • Demonstrated use case — unblocks gap G1 from Tizen core backend vertical slice on the public MAUI ViewHandler Redth/Maui.Tizen#5; not speculative.
  • Minimum viable surface — a method rather than widening ContainerView's setter, so it can't be assigned through a base-typed reference and gives one validation choke point.
  • OnDisconnecting() is private protected and correctly does not appear in any PublicAPI.Unshipped.txt — it's an internal extension point, not new public surface.
  • No obsoletions, no behavior change for in-box handlers — every built-in handler still uses the existing private protected setter; RemoveContainer() is invoked only under the precondition it already runs under.
  • Analyzer verified negatively — reverting SetContainerView to private protected fails with RS0017, confirming the recorded entries match the real shape.

One design note for reviewers (unchanged, still worth a decision): PlatformView is a per-TFM alias that is System.Object on the neutral build, so an external backend's override is ValidateContainerView(object), not its own platform type. This is inherent to the existing alias pattern shared by SetupContainer/ContainerView/SetContainerView — not new here — but it is the shape external backends actually write against. I hit CS0115 on it myself while writing the external test handler, so this push corrects the XML doc example to show the object signature and states the aliasing rule explicitly (docs-only, no API or behavior change). Happy to reshape the surface if you'd prefer a different direction.

Re-verification at this head

Check Result
Core.UnitTests 966 total, 0 failed
Controls.Core.UnitTests 6237 total, 0 failed
Controls.Xaml.UnitTests 2123 total, 0 failed
PublicAPI analyzer (Validate) clean, all 8 surfaces
GenerateDocumentationFile=true 0 warnings
dotnet format whitespace --verify-no-changes clean in all touched files

Controls.Core and Controls.Xaml remain the meaningful blast-radius check, since OnDisconnecting() runs for every handler.

Verdict

Ready to merge on the code. The two red Helix contexts need a re-run once AzDO's results-upload path recovers — they are currently red on main, on release/11.0.1xx-rc1, and on this PR's exact merge base. Say the word and I'll retrigger, or address anything the API review turns up.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/Core/tests/UnitTests/ExternalBackendContainerViewTests.cs:45

  • The friend-assembly assertion only checks exact string equality. If an InternalsVisibleTo entry ever includes a strong-name/public key suffix (e.g. "Name, PublicKey=..."), this test would pass even when the external backend assembly is accidentally added as a friend.
				.GetCustomAttributes<InternalsVisibleToAttribute>()
				.Select(a => a.AssemblyName)
				.ToList();

			Assert.DoesNotContain(externalBackend.GetName().Name, friends);

src/Core/src/Handlers/Element/ElementHandler.cs:145

  • The comment says "derived handlers" generally, but OnDisconnecting is private protected, so it’s only overridable by derived handlers in the same assembly. Clarifying this avoids confusing out-of-tree handler authors reading the source.
				// Give derived handlers a chance to tear down state that needs the platform view
				// while it is still reachable through the PlatformView property.

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

Labels

area-core-platform Integration with platforms platform/ios platform/macos macOS / Mac Catalyst s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-fix-win AI found a better alternative fix than the PR s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants