Skip to content

Add public modal navigation extensibility seam for external platform backends - #37853

Open
Redth wants to merge 4 commits into
net11.0from
redth-modal-navigation-extensibility
Open

Add public modal navigation extensibility seam for external platform backends#37853
Redth wants to merge 4 commits into
net11.0from
redth-modal-navigation-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

Adds a minimal, additive public seam so an external platform backend can render modal push/pop without forking Microsoft.Maui.Controls.

Today Window constructs the internal ModalNavigationManager directly, NavigationImpl routes PushModalAsync/PopModalAsync through it, and the only way to supply platform presentation is an internal partial-class completion (ModalNavigationManager.Tizen.cs, .Android.cs, …) compiled into the framework assembly. The neutral Standard partial only updates logical state, so a third-party backend gets no rendering at all and has no DI/factory/provider hook.

This is part of the external-backend extensibility work tracked by #34099, and follows the shape already established by IAlertManager and IGesturePlatformManagerFactory.

API shape

Three new public interfaces in Microsoft.Maui.Controls.Platform:

public interface IModalNavigationPlatform : IDisposable
{
    bool IsReady { get; }
    Task PushModalAsync(Page modal, bool animated);
    Task PopModalAsync(Page modal, bool animated);
    void PageAttached();
}

public interface IModalNavigationPlatformFactory
{
    IModalNavigationPlatform? CreateModalNavigationPlatform(IModalNavigationHost host);
}

public interface IModalNavigationHost
{
    Window Window { get; }
    IMauiContext MauiContext { get; }
    IReadOnlyList<Page> PlatformModalStack { get; }
    Page? CurrentPage { get; }
    Page CurrentPlatformPage { get; }
    bool IsWindowReady { get; }
    bool IsBatchPopping { get; }
    bool IsBatchPushing { get; }
    void RequestSync();
}

Registration is a single line, no reflection, no partial types, no fork:

builder.Services.AddSingleton<IModalNavigationPlatformFactory, MyModalNavigationPlatformFactory>();

How it works

  • ModalNavigationManager implements IModalNavigationHost explicitly (no internals widened) and lazily resolves IModalNavigationPlatformFactory from Window.Handler.MauiContext.Services.
  • That provider is the per-window service scope, and the factory is invoked once per window, so per-window isolation is inherent.
  • The framework keeps ownership of everything shared and hard: the cross-platform modal stack, Appearing/Disappearing/NavigatedTo/NavigatedFrom, Window.ModalPushing/ModalPopped, Shell batch semantics, and the reconciliation loop that replays queued modals once the platform is ready. Only the visual presentation of a single push/pop is delegated.

Readiness — deliberately recursion-free

IsWindowReady is framework readiness only (window handler + page handler). It intentionally excludes IModalNavigationPlatform.IsReady, because the framework folds IsReady into its own overall readiness separately. The natural backend implementation is:

public bool IsReady => _host.IsWindowReady && _nativeWindowIsRealized;

A host property that already included IsReady would turn that into an uncatchable StackOverflowException. There is a regression test asserting this exact shape works.

Failure semantics

One rule, applied symmetrically:

The requested stack (Navigation.ModalStack) is intent. PlatformModalStack is reality. A faulted platform operation restores reality to "the operation did not take effect"; the next reconciliation pass drives reality back toward intent.

On fault
PushModalAsync modal removed from PlatformModalStack (it is not on screen); stays in the requested stack, so reconciliation retries the presentation
PopModalAsync modal restored to PlatformModalStack (it is presumed still on screen); already gone from the requested stack, so reconciliation retries the dismissal

Restoring on pop failure is the important half: without it a visible native modal is absent from both stacks and can never be reached again.

Where a fault surfaces

Navigation.PushModalAsync/PopModalAsync complete as soon as the framework has updated its own state; they do not wait on a deferred presentation and therefore cannot report a failure in one. The contract now states this explicitly instead of promising a rethrow it cannot deliver:

  • applied inline (platform was ready at request time) → fault rethrown to the navigation caller
  • deferred (platform was not ready) → fault logged via the window's ILogger; the stack-restoration rule still applies

Threading

RequestSync() is safe from any thread. When a dispatch is required the entire reconciliation entry — readiness checks, page lifecycle events and the platform push/pop calls — is marshalled through the window scope's IDispatcher. On the UI thread it runs inline, and the resulting synchronous reentrancy (IsReady, and possibly PushModalAsync/PopModalAsync, can be re-entered before RequestSync returns) is documented so backends don't call it under a lock.

Lifecycle

  • Teardown is terminal: after Window.Destroying the override cannot be lazily recreated. Only attaching a new handler — which brings a new service scope, e.g. an Android activity recreation — re-enables resolution.
  • Teardown does not call PopModalAsync for still-presented modals. Dispose owns dismissing them, and must not assume MauiContext / the window handler / platform views are still usable.
  • A throwing factory is logged and falls back to the built-in platform permanently, rather than escaping from whatever arbitrary call site triggered resolution or retrying on every access.
  • PageAttached is keyed on the page handler instance, so it is delivered exactly once per attachment even when the page-change sync resolves the override before PageAttachedHandler runs (this was a real doubling bug caught by a test).

Native dismissals

If the user dismisses a modal natively (swipe-to-dismiss, back, native close button) the framework cannot see it. Backends must route it back through host.Window.Navigation.PopModalAsync(); the framework then calls PopModalAsync for that page, so implementations must be idempotent for an already-dismissed modal. This is documented on the interface.

Batch hints

IsBatchPopping/IsBatchPushing are documented as optional Shell-only optimization hints (suppress animation so intermediate modals don't flash), not contracts — an implementation that ignores them stays correct.

Dogfooding: the in-box Tizen backend uses the seam

The framework is now the seam's first consumer. TizenModalNavigationPlatform is an
IModalNavigationPlatform written entirely against IModalNavigationHost, with no access to
ModalNavigationManager internals — if the contract were insufficient for a real backend, it
could not have been written. The Tizen partial delegates to it, and its stack bookkeeping now
mirrors PushModalWithOverrideAsync / PopModalWithOverrideAsync exactly, so the fallback and
seam paths apply identical ordering.

It is a mechanical translation — every call and its order is preserved. The single reordering is
moving _platformModalPages.Add ahead of the platform call to satisfy the seam contract, which is
equivalent because CurrentPage is computed from the requested stack, not _platformModalPages.

No DI registration was added. With no factory registered the built-in path runs; a registered
IModalNavigationPlatformFactory still short-circuits before this type is constructed. So
resolution behavior is unchanged and no precedence rules are needed.

Note

Verification caveat, stated plainly. Tizen has IncludeTizenTargetFrameworks=false in CI
("Disabled until net10.0-tizen is available") and the Tizen SDK pack is not installable locally,
so this code compiles nowhere in the normal build. It was validated by compiling both files
against the real seam interfaces with stubs standing in only for the Tizen-native calls —
which checks interface conformance, signatures, nullability and name resolution (0 errors,
0 warnings). The Tizen-native calls themselves are copied verbatim from the shipped file. A
reviewer with a Tizen SDK should still give it a compile pass.

Behavior preservation

When no factory is registered nothing changes. The existing Android / iOS / MacCatalyst / Windows / Tizen / Standard partials are kept as the compatible fallback and are only renamed (PushModalPlatformAsyncPushModalPlatformCoreAsync, IsModalPlatformReadyIsModalPlatformReadyCore, etc.) so the shared file can route between them and an override. The platform diffs are pure symbol renames.

One deliberate behavior fix that also affects the built-in platforms: a modal popped while the platform wasn't ready previously reconciled with animated: false unconditionally, because the pop request leaves the logical stack before the deferred reconciliation runs. Pending pop animation metadata is now retained until the platform actually dismisses the modal, so PopModalAsync(animated: true) animates as requested. Covered in both directions by tests.

Issues Fixed

Contributes to #34099

Tests

src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs (39 tests), including regressions for every review finding:

  • recursion: a platform whose IsReady => host.IsWindowReady resolves without stack overflow, and tracks framework state
  • threading: RequestSync from a "background" thread queues the whole entry on the dispatcher and runs nothing inline; RequestSync on the UI thread runs inline with no dispatch
  • fault/rollback: push fault removes from the platform stack and keeps the requested stack; pop fault restores the platform stack; a recovered backend converges on the next RequestSync
  • deferred animation: deferred pop preserves animated: true, and does not inherit an animated push when the caller asked for animated: false
  • lifecycle: no resurrection after destroy; recreation only when a new handler arrives; teardown with presented modals only disposes and never pops; late resolution delivers PageAttached exactly once
  • factory throw: logged, not rethrown, not retried, and modal navigation still works via the built-in fallback
  • plus the original coverage: DI selection, one-factory-call-per-window, per-window isolation, push/pop ordering, platform-stack visibility during presentation, animation flags, deferred readiness replay, pop cancellation, factory-returns-null fallback, no-registration path, deferred resolution, public-visibility guard

Each new regression was verified to fail against a mutation of its corresponding fix (rollback removed, animation metadata dropped, dispatch removed, terminal destroy relaxed, factory try/catch removed) and pass with it.

src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs exercises the seam with real handlers: push/pop routing, suppression of the built-in presentation, animation flag, host stack state while presenting, pop-fault restoration and recovery, deferred pop animation, and teardown-only-disposes.

PublicAPI.Unshipped.txt updated additively for all seven baselines (net, net-android, net-ios, net-maccatalyst, net-tizen, net-windows, netstandard).

External platform backends (for example a community Tizen or GTK backend)
could not render modal push/pop without forking. `Window` constructed the
internal `ModalNavigationManager` directly, `NavigationImpl` routed
`PushModalAsync`/`PopModalAsync` through it, and the only way to supply
platform presentation was an internal partial class completion compiled
into `Microsoft.Maui.Controls` itself.

This adds a minimal, additive seam in `Microsoft.Maui.Controls.Platform`
that follows the existing `IAlertManager` and `IGesturePlatformManagerFactory`
extensibility patterns:

- `IModalNavigationPlatform` — presents and dismisses a single modal
  (`IsReady`, `PushModalAsync`, `PopModalAsync`, `PageAttached`, `Dispose`).
- `IModalNavigationPlatformFactory` — application-wide DI factory that
  creates one platform per `Window`. Returning `null` keeps the built-in
  implementation.
- `IModalNavigationHost` — the framework state a platform needs
  (`Window`, `MauiContext`, `PlatformModalStack`, `CurrentPage`,
  `CurrentPlatformPage`, `IsModalReady`, `IsBatchPopping`, `RequestSync`).

`ModalNavigationManager` now implements `IModalNavigationHost` and resolves
the factory lazily from the window handler's `IMauiContext.Services`, which
is the per-window service scope, so each window gets its own isolated
platform instance. The framework keeps ownership of the cross-platform modal
stack, page lifecycle events, `Window` modal events and the push/pop
reconciliation loop; only visual presentation is delegated.

When no factory is registered, behavior is unchanged: the existing
Android/iOS/MacCatalyst/Windows/Tizen/Standard partials are kept as the
fallback implementation and are only renamed to `*Core` so the shared file
can route between them and an override.

Adds unit tests covering DI selection, per-window isolation, push/pop
ordering, platform stack visibility during presentation, animation flag
propagation, deferred readiness via `RequestSync`, pop cancellation,
factory-returns-null fallback, push failure rollback, disposal on window
destroy and handler change, and the no-registration path; plus device tests
that exercise the seam with real handlers.

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

Or

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

@Redth
Redth temporarily deployed to copilot-pat-pool August 26, 2026 19:17 — 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.

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 introduces a new public extensibility seam for modal navigation presentation so external/third-party platform backends can implement modal push/pop without forking Microsoft.Maui.Controls. It does this by adding public IModalNavigation* interfaces and routing ModalNavigationManager through an optional DI-resolved platform override, while preserving existing built-in platform behavior as the fallback.

Changes:

  • Add public modal navigation contracts (IModalNavigationPlatform, IModalNavigationPlatformFactory, IModalNavigationHost) under Microsoft.Maui.Controls.Platform.
  • Update ModalNavigationManager to implement IModalNavigationHost and optionally route modal presentation through a DI-resolved override (fallback remains the existing platform partials).
  • Add unit + device tests validating routing, ordering, stack visibility, readiness/deferred sync, and disposal; update PublicAPI baselines.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.cs Adds DI-resolved override routing and implements IModalNavigationHost.
src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationPlatform.cs New public interface contract for platform modal presentation.
src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationPlatformFactory.cs New public factory interface + docs for DI registration.
src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs New public host interface exposing modal navigation state to platforms.
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Standard.cs Renames core platform methods to *Core* to support override routing.
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Android.cs Renames core platform methods to *Core* to support override routing.
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.iOS.cs Renames core platform methods to *Core* to support override routing.
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Windows.cs Renames core platform methods to *Core* to support override routing.
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Tizen.cs Renames readiness/sync/core methods to *Core* to support override routing.
src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs Adds unit coverage for DI selection, ordering, readiness sync, and disposal.
src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs Adds device coverage validating routing and suppression of built-in presentation.
src/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txt Adds new public API entries for modal navigation seam.
src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt Adds new public API entries for modal navigation seam.
src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt Adds new public API entries for modal navigation seam.
src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt Adds new public API entries for modal navigation seam.
src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt Adds new public API entries for modal navigation seam.
src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt Adds new public API entries for modal navigation seam.
src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt Adds new public API entries for modal navigation seam.
Suppressed comments (1)

src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs:22

  • XML doc cref uses Controls.Window.ModalPushing/Controls.Window.ModalPopped, which doesn't resolve. Use Window.ModalPushing and Window.ModalPopped so documentation links are valid.
	/// The framework owns the cross-platform modal stack, the page lifecycle events
	/// (<c>Appearing</c>/<c>Disappearing</c>, <c>NavigatedTo</c>/<c>NavigatedFrom</c>), the
	/// <see cref="Controls.Window.ModalPushing"/>/<see cref="Controls.Window.ModalPopped"/> events and the
	/// reconciliation loop that keeps the platform stack in sync with the requested stack. A platform

bool IModalNavigationHost.IsBatchPopping =>
_window.Page is Shell shell && shell.CurrentItem?.CurrentItem?.IsPoppingModalStack == true;

void IModalNavigationHost.RequestSync() => SyncModalStackWhenPlatformIsReady();
Comment on lines +12 to +17
/// <para>
/// The framework implements this interface and supplies an instance to
/// <see cref="IModalNavigationPlatformFactory.CreateModalNavigationPlatform(IModalNavigationHost)"/>.
/// One host exists per <see cref="Controls.Window"/>, so an implementation must never be shared
/// between windows.
/// </para>
Comment on lines +5 to +10
/// <summary>
/// Application-wide factory for creating <see cref="IModalNavigationPlatform"/> instances.
/// Register an implementation in the application's
/// <see cref="Microsoft.Extensions.DependencyInjection.IServiceCollection"/> to replace the built-in
/// modal presentation for every window.
/// </summary>
Redth added a commit to Redth/Maui.Tizen that referenced this pull request Aug 26, 2026
Modal page navigation was the one area of this slice that could not be built on
the shipped .NET MAUI 11 surface. ModalNavigationManager.Tizen.cs upstream is an
internal partial-class completion compiled into Microsoft.Maui.Controls, and the
neutral Standard partial only updates logical state, so an out-of-tree backend
gets no rendering and has no DI, factory or provider hook.

dotnet/maui#37853 adds that seam, following the shape already used by
IAlertManager (#36633) and IGesturePlatformManagerFactory (#36655). It is still
OPEN, so its interfaces are not in the 11.0.0-preview.7 package this repository
builds against.

Provisional alignment
---------------------
Core/Platform/Modal/ProvisionalModalNavigationContracts.cs carries copies of
IModalNavigationPlatform, IModalNavigationPlatformFactory and
IModalNavigationHost with member shapes taken verbatim from the PR. Adopting the
real interfaces is then a namespace change on two types plus deleting that file.

The copies live in Microsoft.Maui.Platforms.Tizen, NOT
Microsoft.Maui.Controls.Platform: re-declaring a MAUI type name in a MAUI
namespace would collide (CS0433) for consumers that also reference MAUI's own
build once the PR lands.

ProvisionalModalNavigationContractTests keeps the copies honest. It asserts the
member shape of each interface, asserts the namespace rule, and fails outright
once Microsoft.Maui.Controls.Platform.IModalNavigationPlatform appears in the
referenced assembly, with instructions to delete the provisional file.

Implementation
--------------
TizenModalNavigationPlatform ports ModalNavigationManager.Tizen.cs onto the seam.
SendDisappearing/SendAppearing and the manual _platformModalPages bookkeeping are
deliberately absent: under the seam the framework owns the platform stack and
raises the page lifecycle events, so keeping them would fire those events twice.

Batch pops suppress animation so the intermediate modals of a Shell pop-to-root
do not flash, and the back-button handler resolves the current page on every
press rather than capturing it.

TizenModalPageRealizer replaces modal.ToPlatform(context), which is compiled per
platform and has no Tizen build now that Tizen left the MAUI repository. It does
the same work through public, platform-neutral handler APIs, which also makes
page realization testable on the host.

Dialog coordination is now neutral too
--------------------------------------
NuiModalHost is replaced by TizenModalHost, which drives the new Tizen-owned
ITizenNavigationStack instead of NUI directly. Placeholder balance - the failure
mode that wedges every subsequent modal in the app - is therefore verified by
host-side tests rather than only on device. NuiNavigationStack is now the single
NUI-aware piece of modal coordination.

Window-scoped services
----------------------
ITizenNavigationStack and ITizenWindowBackButton wrap objects the window owns,
but registration happens before any window exists, so both are registered scoped
as holders that the window handler fills in via AttachTizenWindow.

They fail differently on purpose: an unattached navigation stack throws, because
a modal that reports success without appearing is worse than a clear failure,
while an unattached back button records and replays the handler, because
PageAttached can run before the window handler does and a missing back button is
not fatal.

No back-button implementation is supplied here. Upstream that registry lives in
Microsoft.Maui.Platform.WindowExtensions and is consumed by MauiApplication, both
of which belong to the Tizen Core layer; duplicating it would create a second,
competing source of truth for back-button routing. AttachTizenWindow takes the
Core layer's implementation as an optional argument instead.

Testing
-------
150 host-side tests, up from 101. New coverage: modal push/pop ordering,
animation-flag propagation, batch-pop suppression, back-button routing and
disposal, factory per-window isolation and null-return fallback, dialog
placeholder balance including the fault and buried-placeholder paths,
window-scoped holder semantics, and the provisional contract guards.

eng/verify-nui-sources.sh now also type-checks the modal sources. It caught the
back-button boundary problem: NuiWindowBackButton was calling
SetBackButtonPressedHandler, which is a MAUI Core Tizen extension rather than a
NUI or Tizen.UIExtensions API and does not exist here.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
API review and code review found blocking issues in the seam. This applies
all of them.

Recursion (blocking): IModalNavigationHost.IsModalReady folded
IModalNavigationPlatform.IsReady back in, so the natural implementation
`IsReady => host.IsModalReady && ...` recursed into an uncatchable
StackOverflowException. Replaced with IsWindowReady, which is framework
readiness only (window handler + page handler) and is documented as safe to
consult from IsReady.

Threading (blocking): IModalNavigationHost.RequestSync documented itself as
callable from any thread but ran readiness checks, page lifecycle events and
platform presentation on the calling thread. It now marshals the whole
reconciliation entry through the window scope's IDispatcher when a dispatch
is required, and the remaining synchronous reentrancy on the UI thread is
documented.

Failure semantics: a faulting custom PopModalAsync removed the page from
PlatformModalStack and never restored it, so a still-visible native modal
became absent from both stacks and unreachable. Push and pop now follow one
rule: the requested stack is intent, PlatformModalStack is reality, and a
faulted operation restores reality to "did not take effect" so the next
reconciliation pass retries. Push fault removes, pop fault restores.

Deferred pop animation: the pop request leaves the logical stack before the
deferred reconciliation runs, so the reconciled pop always got animated:false
and the caller's flag was silently dropped. Pending pop animation metadata is
now retained until the platform actually dismisses the modal.

Deferred completion: the docs promised faults are rethrown to the navigation
caller, but a deferred operation completes that caller before the platform
runs and the later sync is fire-and-forget. Rather than change when
navigation completes, the contract now states exactly when a fault reaches
the caller (inline application) and when it is logged instead (deferred).

Lifecycle: teardown is now terminal for the override so it cannot be lazily
recreated or leaked after Window.Destroying; only attaching a new handler,
which brings a new service scope, re-enables resolution. A throwing factory
is logged and falls back to the built-in platform permanently instead of
escaping from an arbitrary call site or retrying on every access.
PageAttached is now keyed on the page handler instance, which fixes a real
doubling: the page-change sync can resolve the override before
PageAttachedHandler runs.

Also documents that teardown does not pop still-presented modals and Dispose
must not assume MauiContext is usable, that native dismissals must be routed
back through Navigation.PopModalAsync and PopModalAsync must be idempotent,
and that IsBatchPopping is a Shell-only optimization hint. Adds IsBatchPushing
for symmetry. The factory example no longer uses IPlatformViewHandler, which
external backends cannot compile against, and uses the public TFM-neutral
DisconnectHandlers instead.

Adds regressions for recursion avoidance, background-thread RequestSync
marshalling, inline RequestSync, push and pop fault rollback, fault recovery
by reconciliation, deferred pop animation in both directions, platform
dismissal round-trip, throwing factory, no resurrection after destroy,
recreation on new handler, teardown with presented modals, and late
resolution delivering PageAttached exactly once. Each was verified to fail
against a mutation of the corresponding fix.

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

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 18 out of 18 changed files in this pull request and generated no new comments.

@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
Second review pass found four issues in the seam's lifecycle and threading,
all on paths that only open up during teardown or a handler swap.

RequestSync resolved IDispatcher through _window.Handler.MauiContext. During
handler teardown that context — and the IDispatcher registered in its scope —
is already gone, so the lookup returned null and the documented any-thread
guarantee silently degraded to inline execution at exactly the moment a
backend is most likely to be finishing work off the UI thread. It now uses
Window.Dispatcher, which is public, non-null and handler-independent.

A marshalled RequestSync could also be overtaken by teardown: the callback
ran after Window.Destroying and repopulated modal state on a torn-down
window, driving presentation through disconnected resources. Requests are now
tagged with a scope generation and dropped when the window has been destroyed
or its handler replaced in the meantime.

HandlerChanging cleared the resolution latch while _window.Handler still
pointed at the OUTGOING handler, so a reentrant sync during the transition
could build and latch an override from the DI scope that was going away.
Resolution now stays blocked through the transition and is re-enabled from
HandlerChanged, once the new handler is actually installed.

A failed pop lost the caller's animation intent. The metadata was cleared
before the dismissal was attempted (and, on the inline path, only recorded
when the pop was deferred at all), so the retry after a failure always fell
back to unanimated. It is now recorded unconditionally and cleared only once
the dismissal succeeds, covering the inline and deferred failure paths.

Also narrows PageAttached delivery. It previously fell back to the built-in
platform whenever the override was not yet resolvable, which for a window
without a service scope meant an external backend could end up with the
built-in hook installed (on Tizen, the back-button handler) in addition to
its own. The notification is now withheld until resolution has completed and
routed to exactly one of the two, still at most once per page handler.

Adds regressions for background-thread marshalling with a null handler,
destroy-queued and handler-swap-queued sync being dropped, no resolution from
the outgoing scope during a handler swap, and failed-pop retry preserving the
animation flag. Each was verified to fail against a mutation of its fix.

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

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 18 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

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

src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs:301

  • This factory method implements IModalNavigationPlatformFactory.CreateModalNavigationPlatform, whose contract allows returning null for fallback. Using a non-nullable return type here can produce nullability-mismatch warnings (and in other tests you do return null). Consider matching the interface signature with a nullable return type.
			public IModalNavigationPlatform CreateModalNavigationPlatform(IModalNavigationHost host)
			{
				var platform = new RecordingModalNavigationPlatform(host);
				Created.Add(platform);
				return platform;
			}

src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs:1025

  • IModalNavigationPlatformFactory.CreateModalNavigationPlatform returns a nullable platform (factory may return null to fall back). This stub currently uses a non-nullable return type even though _create(host) is allowed to (and in tests does) return null, which can trigger nullability warnings and obscures the intended contract in the test helper.
		sealed class StubModalNavigationPlatformFactory : IModalNavigationPlatformFactory
		{
			readonly Func<IModalNavigationHost, IModalNavigationPlatform> _create;

			public StubModalNavigationPlatformFactory(Func<IModalNavigationHost, IModalNavigationPlatform> create)

Comment on lines +137 to +139
/// This is safe to call from any thread: when the caller is not already on the window's UI
/// thread the work is marshalled there and this method returns immediately. When called
/// <b>on</b> the UI thread the reconciliation starts synchronously, which means
Redth added a commit to Redth/Maui.Tizen that referenced this pull request Aug 26, 2026
1. Position contract. MAUI documents GetPosition(relativeTo) as "the element to use
   as the coordinate reference, or null for SCREEN coordinates". The dispatcher was
   answering null with the view-local position, which is silently wrong. Gesture
   events now carry both spaces (TizenGesturePosition) and resolve null -> screen,
   same view -> local, other element -> null. The NUI detectors populate screen
   coordinates from TapGesture.ScreenPoint, LongPressGesture.ScreenPoint,
   PanGesture.ScreenPosition, PinchGesture.ScreenCenterPoint and
   Touch/Hover.GetScreenPosition. A missing screen coordinate stays null rather than
   being faked from the local one.

2. Real pixel scaler. AddTizenNuiControlsPlatform now registers a scaler backed by
   DeviceInfo.ScalingFactor. Identity scaling is only correct on a 1x display; Tizen
   wearables and TVs are not, so every pan, swipe, pinch, tap and pointer coordinate
   was wrong by the display factor. The registration is exposed as AddTizenPixelScaler
   taking a Func<double>, which is what makes it executable on the host: only reading
   the factor needs a device, and that atom is parameterized rather than baked in. The
   factor is read lazily because DeviceInfo is unusable until the app has initialised,
   and a non-positive or non-finite value degrades to 1 rather than throwing during
   window creation.

3. Unsubscribe is detach-only. MAUI calls Unsubscribe on ordinary page churn, not only
   at teardown, so dismissing dialogs there cancelled a DisplayAlertAsync the app was
   legitimately awaiting across a page swap. Dialogs are now dismissed only in Dispose,
   which the container calls at window-scope teardown.

4. Late-bound window. The subscription captured PlatformWindow at construction. MAUI
   can create the page handler - and therefore call Subscribe - before the window
   handler attaches the native window, so that snapshot could be null forever and
   silently drop every alert for the window's lifetime. The window is now resolved per
   request, and an unattached window services the request rather than discarding it.

5. Button masks. Tap and pointer dispatch now carry the originating button and filter
   against recognizer.Buttons, so a recognizer configured for Primary never fires on a
   right-click. Buttons come from Touch.GetMouseButton; Tizen.NUI.Hover exposes no
   equivalent, so hovers report none. Touch input reports MouseButton.Invalid, which
   maps to Primary - as does anything unclassified, so a stray value can never
   fabricate a secondary click.

6. Awaited stack operations. TizenModalHost discarded the PushAsync/PopAsync tasks,
   swallowing faults and letting a dialog open over a stack that had not taken the
   placeholder. Both are awaited, and ShownBehindPage is unwound even when the push
   faults.

7. Cross-window page reuse. A page popped from one window and pushed modally on another
   kept a handler bound to the originating window's IMauiContext, realizing it into the
   wrong view tree. Such handlers are now disconnected and rebuilt from the target
   window's factory, and the target context is applied unconditionally.

Testing
-------
193 tests, up from 158. Each fix was verified load-bearing by reverting it and
confirming the new tests fail: 9 failures across all seven areas, no overlap.

Two API constraints found by the ref-pack lane rather than at runtime:
TapGestureRecognizer.SendTapped takes no button argument, so the mask is enforced by
filtering; and Tizen.NUI.Hover has no GetMouseButton.

Long press remains the one gesture that cannot be dispatched, and the provisional
dotnet/maui#37853 modal contracts and their expiry guard are unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Redth added a commit to Redth/Maui.Tizen that referenced this pull request Aug 27, 2026
Conflict was the workload-free lane's project list again, purely additive:
core's foundation-owned probes plus this branch's two Controls projects.

Also closes the last outstanding review item. TizenModalHost set
ShownBehindPage to open the dialog placeholder and then forced it back to
false, rather than restoring what it had been. ShownBehindPage is stack-wide
state belonging to whatever is already presented, so forcing false silently
reconfigured how every later push rendered for the lifetime of the window.
It is now saved and restored, including when the placeholder push faults.

Verified load-bearing: reverting the restore fails
ShownBehindPageIsRestoredRatherThanForcedFalse and
ShownBehindPageIsRestoredEvenWhenThePlaceholderPushFails.

196 tests. Long press remains internal and dotnet/maui#37853 remains open at
the pinned 11.0.0-preview.7.26426.4, both re-checked against the resolved
assembly this turn, so the provisional modal contracts and the long-press
lane stay exactly as they are.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The seam was so far only exercised by tests and by external backends. This
makes the framework its own first consumer: the Tizen presentation now lives
in TizenModalNavigationPlatform, an IModalNavigationPlatform written entirely
against IModalNavigationHost with no access to ModalNavigationManager
internals. If the contract were insufficient for a real backend, this
refactor could not have been written — which is the point.

The Tizen partial keeps the platform hooks but delegates to that type, and
its stack bookkeeping now mirrors PushModalWithOverrideAsync /
PopModalWithOverrideAsync exactly, so the fallback path and the seam path
apply identical ordering.

This is a mechanical translation. Every call and its order is preserved: pop
still sends Disappearing, no-ops when the modal has no platform handler, pops
the native stack, sends Appearing on the revealed page and disposes the
handler; push still sends Disappearing, realizes the view, pushes, and sends
Appearing only if the modal is still current. The one reordering is moving
_platformModalPages.Add ahead of the platform call to satisfy the seam
contract, which is equivalent because CurrentPage is computed from the
requested stack, not from _platformModalPages.

Deliberately no DI registration: with no factory registered the built-in path
runs, and a registered IModalNavigationPlatformFactory still takes precedence
and short-circuits before this type is ever constructed. So this changes no
resolution behavior and needs no precedence rules.

Note on verification. Tizen has IncludeTizenTargetFrameworks=false in CI
("Disabled until net10.0-tizen is available") and the Tizen SDK pack is not
installable locally, so this code compiles nowhere in the normal build. It
was instead validated by compiling both files against the real seam
interfaces with stubs standing in only for the Tizen-native calls, which
checks interface conformance, signatures, nullability and name resolution;
the Tizen-native calls themselves are copied verbatim from the shipped file.

Adds a test asserting the no-factory path maintains PlatformModalStack with
the same ordering the seam promises, since the Tizen delegation now depends
on that invariant.

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

Redth commented Aug 27, 2026

Copy link
Copy Markdown
Member Author

Ready for final code + API review

Head is now 74b35c774c. All previously raised blockers are applied and individually mutation-verified; this comment is the summary for the final pass.

What changed since the last review round

Lifecycle / threading hardening (2f809371bf)

  • RequestSync marshals through Window.Dispatcher (public, non-null, handler-independent) instead of the handler's MauiContext scope — the old lookup returned null during teardown and silently degraded the documented any-thread guarantee to inline execution.
  • Queued syncs carry a scope generation and are dropped if the window was destroyed or its handler replaced before the callback ran.
  • Resolution stays blocked through the handler transition (HandlerChanging still observes the outgoing handler) until HandlerChanged, so a reentrant sync cannot latch an override from the dying DI scope.
  • Failed pops retain the caller's animation intent, so a retry does not silently downgrade to unanimated.
  • PageAttached is delivered to exactly one target, at most once per page handler.

Seam dogfooding (74b35c774c) — the in-box Tizen backend is now itself an IModalNavigationPlatform, written only against IModalNavigationHost. No DI registration added, so resolution behavior is unchanged and an external factory still takes precedence.

Review focus

  1. API shape is unchanged from what API review approved — no signature changes in either commit. The seam is still 3 interfaces.
  2. The Tizen change compiles nowhere in CI (IncludeTizenTargetFrameworks=false, "Disabled until net10.0-tizen is available") and the SDK pack isn't installable locally. I validated it by compiling both files against the real seam interfaces with stubs only for Tizen-native calls (0 errors / 0 warnings), and the native calls are verbatim from the shipped file — but it would be worth a compile pass from anyone with a Tizen SDK. If the team would rather not carry an unverifiable platform change in this PR, I'm happy to split that commit out; the seam stands on its own without it.
  3. One deliberate behavior fix reaches the built-in platforms: deferred pops previously reconciled with animated: false unconditionally, dropping the caller's flag. Covered in both directions by tests.

Verification

  • Controls.Core.UnitTests: 6246 passed / 0 failed (39 in ModalNavigationPlatformTests).
  • Controls.Xaml.UnitTests: 2115 passed / 0 failed.
  • Clean build for net11.0, netstandard2.0, netstandard2.1 with PublicApiType=Validate.
  • Every regression added across the review rounds was verified to fail against a mutation of its corresponding fix, so none are vacuous.
  • CI on the previous head was fully green (build 1569397, 0 failed records) after the two failures there were shown to be an infra flake and a pre-existing base-branch condition.

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 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs:15

  • XML doc cref uses Controls.Window, which doesn't resolve and can produce broken documentation/CS1574 warnings. Use Window (or fully-qualified Microsoft.Maui.Controls.Window) in cref instead.
	/// One host exists per <see cref="Controls.Window"/>, so an implementation must never be shared

src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs:21

  • XML doc cref uses Controls.Window.ModalPushing/Controls.Window.ModalPopped, which doesn't resolve and can produce broken documentation/CS1574 warnings. Reference Window directly instead.
	/// <see cref="Controls.Window.ModalPushing"/>/<see cref="Controls.Window.ModalPopped"/> events and the

src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs:108

  • XML doc cref uses Controls.Window.Page, which doesn't resolve and can produce broken documentation/CS1574 warnings. Use Window.Page (or fully-qualified type) instead.
		/// <see langword="true"/> while <see cref="Controls.Window.Page"/> is a <see cref="Shell"/> that

@MauiBot

MauiBot commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

AI Review Summary

@Redth — new AI review results are available based on commit 2f80937.

Gate Inconclusive Confidence Unknown Platform Android


🗂️ Review Sessions — click to expand

[!WARNING]
This run reviewed commit 2f80937, but the PR advanced to 74b35c7 while it was running. These results are informational; re-run /review for the current head.


🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ INCONCLUSIVE

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

🩺 Could not verify — environment/infrastructure error. The gate ran the tests but hit an environment error (an emulator/simulator/Appium/XHarness flake, a device that would not boot, or an empty/invalid result file), so it could not record a real pass/fail. The ⚠️ ENV ERROR marks below are infrastructure, not test failures — this is not a problem with your PR. Comment /review to retry on a fresh agent.

XHarness did not produce the expected fresh result 'testResults-abe5c1e4f8254478bde84f12662f8dec.xml' for requested class(es) 'Microsoft.Maui.DeviceTests.ModalNavigationPlatformTests' (the target tests did not run).

Test Without Fix (expect FAIL) With Fix (expect PASS)
🧪 ModalNavigationPlatformTests ModalNavigationPlatformTests 🛠️ BUILD ERROR ✅ PASS — 62s
📱 ModalNavigationPlatformTests (RegisteredPlatformReceivesPushAndPop, RegisteredPlatformSuppressesTheBuiltInPresentation, AnimationFlagReachesTheRegisteredPlatform, RegisteredPlatformSeesTheHostStackWhilePresenting, FailedPopLeavesTheModalOnThePlatformStack, DeferredPopPreservesTheRequestedAnimationFlag, TeardownWithPresentedModalsOnlyDisposes, FailedPopRetryPreservesTheRequestedAnimationFlag) Category=Modal 🛠️ BUILD ERROR ⚠️ ENV ERROR
🔴 Without fix — 🧪 ModalNavigationPlatformTests: 🛠️ BUILD ERROR · 54s

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

/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1005,40): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1007,51): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1007,73): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1014,16): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1016,66): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1016,11): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1030,56): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1032,13): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1034,49): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1054,51): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1056,44): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(1061,11): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
🟢 With fix — 🧪 ModalNavigationPlatformTests: PASS ✅ · 62s

(no coded error found; showing last 1200 chars)

indowGetsItsOwnPlatformInstance [2 ms]
  Passed RequestSyncFromABackgroundThreadIsMarshalledToTheUIThread [2 ms]
  Passed PlatformIsDisposedAndRecreatedWhenTheWindowHandlerChanges [1 ms]
  Passed TeardownWithPresentedModalsDoesNotCallPopAndOnlyDisposes [1 ms]
  Passed PlatformIsDisposedWhenTheWindowIsDestroyed [< 1 ms]
  Passed CustomPlatformResolvedFromDependencyInjection [1 ms]
  Passed DestroyedWindowDoesNotResurrectThePlatform [1 ms]
  Passed ResolutionIsDeferredUntilTheWindowHasAServiceScope [1 ms]
  Passed ModalNavigationSeamInterfacesArePublic [< 1 ms]
  Passed PageAttachedIsCalledWhenTheWindowPageGetsAHandler [2 ms]
  Passed RequestSyncStillMarshalsWhenTheWindowHandlerIsNull [1 ms]
  Passed ThrowingFactoryFallsBackToTheBuiltInPlatformWithoutRetrying [< 1 ms]
  Passed PlatformCanConsultWindowReadinessFromIsReadyWithoutRecursing [1 ms]
  Passed LateResolutionDeliversPageAttachedExactlyOnce [1 ms]
[xUnit.net 00:00:02.02]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed RequestSyncQueuedBeforeAHandlerSwapIsDropped [1 ms]
  Passed FailedPopRetryPreservesTheRequestedAnimationFlag [2 ms]
Test Run Successful.
Total tests: 38
     Passed: 38
 Total time: 2.4939 Seconds
🔴 Without fix — 📱 ModalNavigationPlatformTests (RegisteredPlatformReceivesPushAndPop, RegisteredPlatformSuppressesTheBuiltInPresentation, AnimationFlagReachesTheRegisteredPlatform, RegisteredPlatformSeesTheHostStackWhilePresenting, FailedPopLeavesTheModalOnThePlatformStack, DeferredPopPreservesTheRequestedAnimationFlag, TeardownWithPresentedModalsOnlyDisposes, FailedPopRetryPreservesTheRequestedAnimationFlag): 🛠️ BUILD ERROR · 130s

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

/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(21,21): error CS0246: The type or namespace name 'IModalNavigationPlatformFactory' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(283,58): error CS0246: The type or namespace name 'IModalNavigationPlatformFactory' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(296,66): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(296,11): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(304,51): error CS0246: The type or namespace name 'IModalNavigationPlatform' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(306,44): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(311,11): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/DeviceTests/Controls.DeviceTests.csproj::TargetFramework=net11.0-android]
Build FAILED.
🟢 With fix — 📱 ModalNavigationPlatformTests (RegisteredPlatformReceivesPushAndPop, RegisteredPlatformSuppressesTheBuiltInPresentation, AnimationFlagReachesTheRegisteredPlatform, RegisteredPlatformSeesTheHostStackWhilePresenting, FailedPopLeavesTheModalOnThePlatformStack, DeferredPopPreservesTheRequestedAnimationFlag, TeardownWithPresentedModalsOnlyDisposes, FailedPopRetryPreservesTheRequestedAnimationFlag): ⚠️ ENV ERROR · 401s

No log file found

⚠️ Failure Details

  • 🛠️ ModalNavigationPlatformTests without fix: build failed before tests could run
    • /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs(37,9): error CS0246: The type or namespace name 'IModalNavigationHost' could not be found (are you missing a using...
  • 🛠️ ModalNavigationPlatformTests (RegisteredPlatformReceivesPushAndPop, RegisteredPlatformSuppressesTheBuiltInPresentation, AnimationFlagReachesTheRegisteredPlatform, RegisteredPlatformSeesTheHostStackWhilePresenting, FailedPopLeavesTheModalOnThePlatformStack, DeferredPopPreservesTheRequestedAnimationFlag, TeardownWithPresentedModalsOnlyDisposes, FailedPopRetryPreservesTheRequestedAnimationFlag) without fix: build failed before tests could run
    • /home/vsts/work/1/s/src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs(21,21): error CS0246: The type or namespace name 'IModalNavigationPlatformFactory' could not be found ...
  • ⚠️ ModalNavigationPlatformTests (RegisteredPlatformReceivesPushAndPop, RegisteredPlatformSuppressesTheBuiltInPresentation, AnimationFlagReachesTheRegisteredPlatform, RegisteredPlatformSeesTheHostStackWhilePresenting, FailedPopLeavesTheModalOnThePlatformStack, DeferredPopPreservesTheRequestedAnimationFlag, TeardownWithPresentedModalsOnlyDisposes, FailedPopRetryPreservesTheRequestedAnimationFlag) with fix: XHarness did not produce the expected fresh result 'testResults-abe5c1e4f8254478bde84f12662f8dec.xml' for requested class(es) 'Microsoft.Maui.DeviceTests.ModalNavigationPlatformTests' (the target tests did not run).
📁 Fix files reverted (13 files)
  • src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Android.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Standard.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Tizen.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Windows.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.iOS.cs
  • src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txt
  • src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt

New files (not reverted):

  • src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationPlatform.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationPlatformFactory.cs

📋 Pre-Flight — Context & Validation

PR #37853 Pre-Flight

Context

  • Title: Add public modal navigation extensibility seam for external platform backends
  • Base: net11.0 at bedd1b18b7682193e05b47267509cec8c49c6853
  • PR head: 2f809371bf3f57e9fea9567d9a1adfc3a500e131
  • Local review commit: 5bf1eea287db399244e7633c7954c749862b9dd2 (squashed PR commit whose parent is the merge base)
  • Platform: Android
  • Goal: Let an external backend replace native modal push/pop presentation through public DI-resolved contracts without forking Controls, while the framework retains requested/platform stack reconciliation, lifecycle events, Shell batching, animation intent, failure recovery, and built-in platform fallback.

Direct Diff Inspection

The squashed PR changes 18 files (+2382/-31):

  • Adds public IModalNavigationHost, IModalNavigationPlatform, and IModalNavigationPlatformFactory contracts.
  • Makes ModalNavigationManager the host and lazily resolves one platform override per window handler scope.
  • Routes readiness and push/pop presentation through the override or the existing built-in partial methods.
  • Adds scope-generation guards, terminal teardown behavior, UI-thread dispatch for RequestSync, exact-once PageAttached, factory-failure fallback, push/pop rollback, and deferred pop-animation tracking.
  • Renames built-in platform partial methods on Android, iOS, Windows, Tizen, and Standard so they remain the fallback.
  • Adds all seven required PublicAPI.Unshipped.txt surfaces.
  • Adds focused Core unit coverage and Android-capable Controls device coverage.

The existing implementation's central mechanism is a nullable, lazily resolved _platformOverride embedded in ModalNavigationManager, with _platformOverrideResolved, _destroyed, and _scopeGeneration coordinating lifecycle. The same manager owns _platformModalPages, _modalPages, and _pendingPopAnimations. Any alternative must be mechanism-level different rather than relocating equivalent checks.

Required Behavioral Invariants

  1. No registration preserves the built-in platform path.
  2. A registered factory is resolved from the current window scope once per scope and returns a per-window disposable backend.
  3. IModalNavigationHost.IsWindowReady excludes backend readiness to avoid recursion.
  4. RequestSync is callable from any thread; the complete reconciliation entry runs on the UI thread, and stale queued callbacks are dropped after teardown or handler replacement.
  5. Push failure removes the page from platform reality; pop failure restores it so later reconciliation can retry.
  6. Deferred and failed-retry pops preserve the caller's animation flag.
  7. Destroy/handler replacement disposes without issuing platform pops; destroy is terminal until a new handler scope arrives.
  8. PageAttached is delivered exactly once per page-handler attachment to either the override or built-in path.
  9. Factory exceptions are logged once and permanently fall back for that scope.
  10. Existing lifecycle, navigation-stack, Shell batch, and public API behavior remains intact.

Detected Tests and Gate Evidence

Do not rerun the completed gate or overwrite gate/content.md.

  • Primary unit test: ModalNavigationPlatformTests in Controls.Core.UnitTests.
    • Allowed command: dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~ModalNavigationPlatformTests"
    • Gate result with the PR fix: 38/38 passed in 62 seconds.
  • Mandatory Android regression test: the eight device cases in Microsoft.Maui.DeviceTests.ModalNavigationPlatformTests, detected through Category=Modal.
    • Allowed command: pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform android -TestFilter "Category=Modal"
    • Gate result with the PR fix: inconclusive environment error because XHarness did not produce a fresh result file; this is not a product-test failure.

No full suite is permitted. One candidate test pass may run only the two commands above; after at most one focused code correction, it may rerun only the affected command(s).

Try-Fix Boundary

EstablishBrokenBaseline.ps1 previously identified 13 revertable tracked fix files and three PR-added production files:

  • src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationHost.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationPlatform.cs
  • src/Controls/src/Core/Platform/ModalNavigationManager/IModalNavigationPlatformFactory.cs

The loaded try-fix skill requires reporting Blocked before editing if .github/.baseline-state.json has a non-empty NewFiles array. Follow that rule exactly; do not bypass it or use manual Git restoration. The only permitted restoration command is:

pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore

The worktree also contains pre-existing harness changes under .github/ and eng/. Preserve them exactly and modify only baseline-allowed files. Candidate 2 must consume candidate 1's recorded result and choose a different root-cause mechanism without reopening or rerunning candidate 1.


🔬 Code Review — Deep Analysis

Expert Evaluation — Raw PR Fix

Verdict: NEEDS_CHANGES
Confidence: medium-high

The public seam is placed at the right abstraction boundary: ModalNavigationManager retains requested/platform stack reconciliation, lifecycle events, Shell batching, and built-in platform fallback, while IModalNavigationPlatform delegates only native presentation. The split between framework-only IsWindowReady and backend IsReady correctly avoids recursive readiness checks, and the public API surfaces are consistent across target frameworks.

The submitted implementation nevertheless has blocking lifecycle and state-management defects:

  1. Major — _pendingPopAnimations can retain pages indefinitely. A push followed by a pop while the platform is not ready leaves both modal stacks empty but records the page in _pendingPopAnimations; reconciliation then exits as already synchronized and never removes the entry. This roots the page tree for the window lifetime and can later replay a stale animation value.
  2. Major — override resolution invokes backend code inside a readiness getter. PlatformOverride calls DeliverPageAttached(). If PageAttached() calls the documented RequestSync() hook during PopModalAsync, nested reconciliation can pop the platform page before the outer pop resumes, causing an empty-stack exception or double pop.
  3. Major — third-party Dispose() can break teardown invariants. An exception from DisposePlatformOverride() can skip scope invalidation during window destruction or handler replacement, allowing stale queued sync work and permanently preventing resolution in the replacement scope.
  4. Moderate — deferred animation replay changes built-in platform behavior. _pendingPopAnimations is consulted for built-in Android/iOS/Windows/Tizen paths as well as the external override. Catch-up pops that were previously unanimated may now animate, including intermediate pages in multi-pop reconciliation.
  5. Moderate — an early page attachment can be permanently dropped. If the page handler attaches before the window handler, DeliverPageAttached() cannot resolve a route and no reliable handler-scope hook retries delivery. This can omit Tizen back-button setup.
  6. Moderate — stale load watcher and unguarded IsReady. Switching from an early built-in route to an external override can leave a platform-page loaded subscription connected. Backend IsReady exceptions also escape from arbitrary navigation and reconciliation call sites.
  7. Moderate — IModalNavigationHost.MauiContext has a throwing non-nullable contract. During handler teardown the getter throws despite teardown being a documented backend lifecycle phase. Because the API is unshipped, a nullable context would give callers an explicit, safe contract.
  8. Moderate — handler-changed can drop an undisposed override and retain a page handler. OnWindowHandlerChanged assigns _platformOverride = null rather than disposing it, and _pageAttachedNotifiedForHandler can strongly retain a disconnected page handler after Window.Page is cleared.

The trusted gate is inconclusive rather than failing: the focused unit gate previously passed 38/38, while Android device validation was blocked because XHarness produced no fresh result file. The findings above are based on source-level state-transition traces.

Consolidated refinement direction: resolve the backend at the handler-scope boundary rather than while evaluating readiness; keep the getter side-effect free; retry page attachment after scope installation; make disposal non-throwing and idempotent; disconnect any built-in load watcher when routing externally; remove deferred animation entries for pages never presented; preserve legacy built-in animation behavior; and make MauiContext nullable while the API remains unshipped.


🛠️ Try-Fix — Analysis & Comparison

Try-Fix Candidates

Candidate 1

Result: Blocked (baseline safety; no files edited, no tests run)

Approach

Always-Present Scope-Bound Strategy — a null-object platform binding, a scope-identity
lifecycle object, and pop requests that retain their animation intent until completion.

  1. Null-object platform instead of a nullable override. Add an internal
    BuiltInModalNavigationPlatform : IModalNavigationPlatform that forwards to the existing
    built-in partial methods (IsModalPlatformReadyCore, PushModalPlatformCoreAsync,
    PopModalPlatformCoreAsync, OnPageAttachedHandler). ModalNavigationManager holds a
    non-nullable IModalNavigationPlatform that is never null, so no routing site has a
    PlatformOverride?.X ?? XCore branch and no "resolved" latch exists — "not yet resolved" is
    simply not a representable state. Invariant 1 (no registration ⇒ built-in path) holds
    structurally because the default binding is the built-in platform.

  2. Scope-identity object instead of an integer generation counter plus _destroyed. Extract
    lifecycle into an internal ModalPlatformScope owning the bound platform instance, the
    page-attach latch, and its own liveness. The scope is bound eagerly at the
    Window.HandlerChanged edge
    — the exact moment a per-window IMauiContext service scope
    exists — rather than lazily on first property touch. Window.Destroying and HandlerChanging
    swap _scope for a terminal detached scope bound to the built-in platform that reports
    IsAlive == false. RequestSync captures the scope reference and its UI-thread continuation
    runs only when ReferenceEquals(captured, _scope) && captured.IsAlive (invariant 4).
    Staleness becomes object identity rather than counter arithmetic; "terminal until a new handler
    scope arrives" becomes a state of the scope object rather than a boolean cleared in two places
    (invariant 7). Eager binding also removes the reentrancy hazard the PR defends against, where
    the resolution getter calls DeliverPageAttached which re-enters the getter and must re-check
    the latch afterwards; PageAttached becomes one latch flip on an already-bound scope
    (invariant 8). Factory exceptions are caught once at the bind edge, logged once, and the scope
    binds to the built-in platform permanently for its lifetime (invariant 9). IsWindowReady
    continues to exclude backend readiness (invariant 3), and per-window disposable backends are
    created from the window handler's service scope exactly once per scope (invariant 2).

  3. Retain the pop request until completion instead of reconstructing lost intent. The removal
    of the pop request from _modalPages moves from request time to successful completion time,
    so the caller's animated flag is never destroyed and needs no side table (invariant 6). A
    failed push leaves the platform stack unchanged and a failed pop leaves the entry in place, with
    the retained request driving the retry on the next reconciliation pass (invariant 5). Existing
    lifecycle, navigation-stack, Shell batch, and public API behavior are untouched (invariant 10).

Prior approach avoided and mechanism-level difference

Prior approach avoided: PR #37853's shipped implementation — a nullable _platformOverride
lazily resolved inside a PlatformOverride property getter, coordinated by
_platformOverrideResolved, _destroyed, _scopeGeneration, and
_pageAttachedNotifiedForHandler, with ?? Core fallbacks at every routing site and a
Dictionary<Page, bool> _pendingPopAnimations side table. Its shared failure mechanism is
deferred binding: because the override is materialised at an arbitrary first-touch call site, the
type must simulate scope lifetime with out-of-band flags, must block resolution during the handler
swap by setting the "resolved" latch to true while holding a null value, must guard reentrancy
between the getter and DeliverPageAttached with a double re-check, and must reconstruct the
animation flag that an earlier step already discarded.

Mechanism-level difference: Because binding happens at the handler-scope edge rather than at
first touch, the manager never observes a half-resolved state, so the flags that exist only to
describe that state disappear entirely. Because the bound platform is a null object rather than a
nullable reference, no call site carries a fallback branch — "which path handles this call" is
decided once at bind time instead of re-derived at every routing site. Because liveness is carried
by the scope object's own identity, a queued RequestSync compares against the object it was issued
for instead of an integer that must be bumped correctly in three separate places. And because the
pop request is retained until the dismissal actually succeeds, the animation intent is never lost,
so the compensating dictionary is unnecessary. This is a different root-cause hypothesis (bind
eagerly, keep intent) rather than the same checks relocated.

Known tradeoffs the blocked test pass would have measured: eager binding invokes a registered
factory for windows that never present a modal (the PR's lazy resolution avoids that cost), and
retaining the pop request past request time changes _modalPages counts observed by Shell batch
reconciliation, which is the highest-risk part of the design.

Files changed

None. Editing was blocked before any change was made. The candidate would have touched
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.cs, the per-platform
ModalNavigationManager.*.cs partials, the three new contract files, and the seven
src/Controls/src/Core/PublicAPI/*/PublicAPI.Unshipped.txt files.

Result

Blocked

Complete git diff for the candidate

Explicitly empty. git diff -- src/Controls/src/Core/Platform/ModalNavigationManager src/Controls/src/Core/PublicAPI returns zero bytes, and git status --short -- src/ returns no
entries. fix.diff and reviewer-findings.diff in the artifact directory are both 0 bytes. No
production file was created, modified, or deleted by this attempt.

Test commands and results

Neither permitted command was run.

  1. dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~ModalNavigationPlatformTests"not run
  2. pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform android -TestFilter "Category=Modal"not run

There was no candidate code to exercise, and the invoking constraints state that tests must not be
run merely to consume time when baseline safety blocks editing. The completed gate was not rerun and
gate/content.md was not modified. No full suite or any other test was run.

Failure / block analysis

Two independent skill boundaries force Blocked.

  1. No baseline state. pwsh .github/scripts/EstablishBrokenBaseline.ps1 aborted with
    "ERROR: DIRTY WORKING DIRECTORY - Cannot establish baseline" and threw "Working directory is not
    clean", listing 44 modified/deleted paths — all of them under .github/scripts,
    .github/skills, and eng/scripts, i.e. exactly the pre-existing harness changes that must be
    preserved. .github/.baseline-state.json was therefore never created, so there is no
    RevertedFiles allow-list and no tracked file may be edited. The script's own remediation
    (git checkout -- .) is forbidden by the skill's script-only restoration principle and by the
    invoking constraints, and would destroy the harness state, so no permitted route to a clean
    worktree existed.
  2. Non-empty NewFiles. Independently, pre-flight records that this PR adds three new
    production files — IModalNavigationHost.cs, IModalNavigationPlatform.cs, and
    IModalNavigationPlatformFactory.cs under
    src/Controls/src/Core/Platform/ModalNavigationManager/. The skill requires reporting Blocked
    before editing whenever NewFiles is non-empty, because the restore script cannot safely restore
    added production files. The candidate's public seam necessarily spans that same contract surface,
    so it would have been blocked even with a clean worktree.

Actionable observations: the baseline script gates on a globally clean worktree rather than on the
PR's production paths, so any environment legitimately carrying .github//eng/ harness edits will
block every try-fix attempt at Step 2; and because this PR delivers its seam largely through new
public contract files, it is structurally un-attemptable under the current NewFiles rule without
either a restore path that can delete attempt-created production files or a branch where the
contracts are already committed and only ModalNavigationManager is in play.

Inline expert self-review

0 findings (reviewer-findings.json is []). The self-review was performed inline against
.github/agents/maui-expert-reviewer.md — no sub-agent, reviewer, or rubber-duck was invoked — over
an empty candidate diff, so no Overarching Principle, always-active dimension, or routed dimension
had any code to apply to. [] is the correct output for a no-diff attempt.

Artifact directory

CustomAgentLogsTmp/PRState/37853/PRAgent/try-fix/attempt-1

Contains: baseline.log, approach.md, reviewer-findings.json, reviewer-findings.diff,
result.txt, fix.diff, test-output.log, analysis.md, restore.log.

Restore confirmation

pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore was run exactly as specified and
completed with:

No baseline state found. Nothing to restore.
Restored   False
Message    No baseline state found

This is the expected verified no-state completion: Step 2 reported Blocked before any file was
changed and .github/.baseline-state.json was never created, so there was nothing for the script to
restore. Confirmed afterwards that git status --short -- src/ is empty — no attempt-created
changes to any target file. All pre-existing dirty .github/ and eng/ worktree changes are
untouched, no pre-existing untracked path was modified or deleted, no git checkout/restore/
reset/clean/stash was used at any point, and nothing was committed or pushed.

Candidate 2

Try-Fix Candidate 2 — PR #37853

Result: Blocked (baseline safety; no files edited, no tests run)

Approach

Cancellation-Driven Reconciliation Journal — make modal transitions durable records and process them through one cancellable, UI-thread reconciliation pump per handler scope.

  1. Durable transition journal instead of split request/animation state. Each requested push or pop creates an immutable record containing operation, page, and animation intent. The pump is the sole writer of _platformModalPages; it removes a record only after the corresponding backend call succeeds. Push failure rolls platform reality back, pop failure restores it, and the same record remains available for retry with its original animation flag. _modalPages continues to represent only the requested navigation stack and is removed from at normal request time, so this neither reconstructs animation through _pendingPopAnimations (current PR) nor retains pop requests in _modalPages (candidate 1). Existing lifecycle events and Shell batch semantics stay tied to requested-stack mutation, while the journal governs only presentation reconciliation.

  2. Explicit route transition inside a serialized pump. The first attachment or reconciliation signal adds a BindRoute transition. On the UI thread it resolves the factory from the current handler's service scope exactly once and transitions a closed route state from Unbound to BuiltIn, External, or FaultedBuiltIn. No registration/null return selects BuiltIn; a factory exception logs once and selects FaultedBuiltIn; an external result is owned and disposed by that pump. This is not the PR's nullable property getter and resolved latch, and it is not candidate 1's always-present null-object or eager binding at HandlerChanged. Built-in/external dispatch is centralized at the pump route switch. IModalNavigationHost.IsWindowReady remains framework-only and the three public interfaces plus all seven PublicAPI surfaces remain unchanged.

  3. Cancellation closes a handler scope instead of flags, generations, or scope identity. RequestSync is a thread-safe enqueue. If already on the UI thread the pump drains synchronously; otherwise its dispatcher callback drains the complete reconciliation entry on the UI thread. Handler replacement/destroy cancels and closes that pump, so stale queued callbacks observe cancellation and cannot touch a replacement context; there is no _destroyed, _scopeGeneration, integer comparison, or liveness-bearing scope object/reference comparison. Closing disposes an external route and never synthesizes platform pops. A later handler creates a fresh pump; no handler means no live pump. Attachment signals are coalesced by page-handler identity inside the pump and delivered only after route binding, exactly once to the selected external or built-in path.

Prior approaches avoided and mechanism-level difference

Current PR avoided: PR #37853 stores a nullable, lazily resolved _platformOverride; coordinates it with _platformOverrideResolved, _destroyed, _scopeGeneration, and _pageAttachedNotifiedForHandler; branches between override and *Core at routing sites; and reconstructs discarded pop intent using _pendingPopAnimations. Its failure-prone mechanism is state split across an arbitrary first-touch getter, lifecycle latches/generation arithmetic, requested/platform stacks, and an animation side table.

Candidate 1 avoided: Candidate 1 replaces null with an always-present built-in null-object adapter, binds eagerly at HandlerChanged, makes a ModalPlatformScope reference carry identity and liveness, and keeps pop requests in _modalPages until platform completion. Its mechanism makes binding and liveness scope-object concerns and changes requested-stack retention to preserve intent. This candidate did not reopen, alter, regenerate, or use candidate 1 artifacts as active work.

Mechanism-level difference: Candidate 2 changes the source of truth for outstanding platform work: an append-only-until-success transition record contains all retry data, while a serialized pump alone mutates platform reality. Because intent is durable at request creation, neither an animation side table nor prolonged _modalPages membership is needed. Backend selection is an explicit state-machine transition preceding pump work, rather than a nullable getter side effect or eager handler-edge binding. Because scope replacement closes a cancellable work stream, queued work becomes inert by cancellation, not by generation checks or object-reference liveness. Built-in fallback is a route-state case, not a null object. The causal chain is: request records intent → one UI-thread consumer attempts it under one selected route → failure rolls back platform reality but retains that same record → later sync retries identical intent; closing the consumer prevents all old-scope work after teardown.

Files changed

None. Baseline safety blocked editing before any production change. The proposed implementation would have been limited to:

  • src/Controls/src/Core/Platform/ModalNavigationManager/
  • src/Controls/src/Core/PublicAPI/*/PublicAPI.Unshipped.txt

The three contract files and seven PublicAPI files would preserve their documented public API shapes. No .github/, eng/, test, gate, candidate-1, or production path was modified by this candidate.

Result

Blocked

Complete candidate git diff

Explicitly empty. The candidate-scoped production diff is zero bytes:

Both fix.diff and reviewer-findings.diff are empty. git status --short -- src/Controls/src/Core/Platform/ModalNavigationManager src/Controls/src/Core/PublicAPI returned no entries before and after exact restore. No production file was created, modified, or deleted.

Exact test commands and results

Neither permitted command was run:

  1. dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~ModalNavigationPlatformTests"not run
  2. pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Controls -Platform android -TestFilter "Category=Modal"not run

Baseline safety produced no candidate implementation to exercise, and the invocation forbids tests merely to consume time when editing is blocked. Gate verification was not rerun, gate/content.md was not overwritten, no full suite or other test ran, and no correction/retest allowance was consumed.

Failure / block analysis

Two independent safety conditions prevent empirical implementation/testing:

  1. pwsh .github/scripts/EstablishBrokenBaseline.ps1 failed with exit code 1 and ERROR: DIRTY WORKING DIRECTORY - Cannot establish baseline. It listed 44 modified/deleted pre-existing harness files, all under .github/ and eng/. These must remain untouched. The script did not create .github/.baseline-state.json; without that file there is no RevertedFiles allow-list, so the skill requires Blocked before editing. Cleaning, stashing, reverting, deleting, or bypassing those paths is forbidden.
  2. Independently, pre-flight documents three PR-added production contract files (IModalNavigationHost.cs, IModalNavigationPlatform.cs, and IModalNavigationPlatformFactory.cs). A successful baseline would therefore have a non-empty NewFiles array, which also mandates Blocked before editing because script-only restoration cannot safely restore added production files.

The mechanism was designed but could not be empirically tested within the mandatory restoration boundary. This is Blocked, not Pass or Fail.

Inline expert self-review

0 findings (reviewer-findings.json is []). Review was performed inline only; no task, child agent, reviewer, rubber-duck, or maui-expert-reviewer was invoked. The empty candidate diff was checked against all eight Overarching Principles and the always-active Logic and Correctness, Regression Prevention, and Complexity Reduction dimensions. Public API Surface was considered because of the target surface, but no public hunk exists. With no changed line or concrete candidate-code failure, zero findings is correct.

Artifact directory

CustomAgentLogsTmp/PRState/37853/PRAgent/try-fix/attempt-2

Contains: baseline.log, approach.md, reviewer-findings.json, reviewer-findings.diff, result.txt, fix.diff, test-output.log, analysis.md, restore.log.

Exact restore confirmation

After mandatory attempt artifacts were captured, the exact required command was run once:

pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore

It exited successfully with:

No baseline state found. Nothing to restore.

Name                           Value
----                           -----
Message                        No baseline state found
Restored                       False

This is the accepted verified no-state outcome: baseline creation failed before edits, .github/.baseline-state.json never existed, and no candidate change required restoration. Target production status is empty after restore. All pre-existing dirty .github/ and eng/ changes and untracked paths remain untouched. No checkout, restore, reset, clean, stash, manual deletion, commit, or push was performed.


📝 PR Finalize — Recommended Title & Description

Assessment: ✏️ Recommend updating — the description is strong but incorrectly says the factory runs once per window, names a handler-scope IDispatcher instead of Window.Dispatcher, and reports 33 rather than 38 focused unit tests.

Recommended title

Add public modal navigation extensibility seam for external platform backends

Recommended description

### Description of Change

Adds a minimal, additive public seam so an external platform backend can render modal push/pop without forking `Microsoft.Maui.Controls`.

Today `Window` constructs the internal `ModalNavigationManager` directly, `NavigationImpl` routes `PushModalAsync`/`PopModalAsync` through it, and the only way to supply platform presentation is an internal partial-class completion (`ModalNavigationManager.Tizen.cs`, `.Android.cs`, …) compiled into the framework assembly. The neutral `Standard` partial only updates logical state, so a third-party backend gets no rendering at all and has no DI/factory/provider hook.

This is part of the external-backend extensibility work tracked by #34099, and follows the shape already established by `IAlertManager` and `IGesturePlatformManagerFactory`.

### API shape

Three new public interfaces in `Microsoft.Maui.Controls.Platform`:

```csharp
public interface IModalNavigationPlatform : IDisposable
{
    bool IsReady { get; }
    Task PushModalAsync(Page modal, bool animated);
    Task PopModalAsync(Page modal, bool animated);
    void PageAttached();
}

public interface IModalNavigationPlatformFactory
{
    IModalNavigationPlatform? CreateModalNavigationPlatform(IModalNavigationHost host);
}

public interface IModalNavigationHost
{
    Window Window { get; }
    IMauiContext MauiContext { get; }
    IReadOnlyList<Page> PlatformModalStack { get; }
    Page? CurrentPage { get; }
    Page CurrentPlatformPage { get; }
    bool IsWindowReady { get; }
    bool IsBatchPopping { get; }
    bool IsBatchPushing { get; }
    void RequestSync();
}
```

Registration is a single line, with no reflection, partial types, or framework fork:

```csharp
builder.Services.AddSingleton<IModalNavigationPlatformFactory, MyModalNavigationPlatformFactory>();
```

### How it works

- `ModalNavigationManager` implements `IModalNavigationHost` explicitly, without widening existing internals, and lazily resolves `IModalNavigationPlatformFactory` from `Window.Handler.MauiContext.Services`.
- That provider is the current window-handler service scope. The factory is invoked once per handler scope and again if the window receives a replacement handler, preserving per-window isolation across scope recreation.
- The framework retains ownership of the cross-platform modal stack, `Appearing`/`Disappearing`/`NavigatedTo`/`NavigatedFrom`, `Window.ModalPushing`/`ModalPopped`, `Shell` batch semantics, and reconciliation of queued modals. Only visual presentation of one push or pop is delegated.

#### Readiness — deliberately recursion-free

`IsWindowReady` is framework readiness only (window handler + page handler). It intentionally excludes `IModalNavigationPlatform.IsReady`, because the framework folds `IsReady` into overall readiness separately. The natural backend implementation is:

```csharp
public bool IsReady => _host.IsWindowReady && _nativeWindowIsRealized;
```

Including backend readiness in the host property would recurse into an uncatchable `StackOverflowException`. A regression test covers this shape.

#### Failure semantics

The requested stack (`Navigation.ModalStack`) is intent; `PlatformModalStack` is reality. A fault restores platform reality to “the operation did not take effect,” and a later reconciliation pass drives reality back toward intent.

| Operation | On fault |
|---|---|
| `PushModalAsync` | Removes the modal from `PlatformModalStack`; it remains requested so reconciliation can retry presentation |
| `PopModalAsync` | Restores the modal to `PlatformModalStack`; it remains absent from the requested stack so reconciliation can retry dismissal |

Restoring on pop failure is essential: otherwise a still-visible native modal would be absent from both stacks and unreachable.

#### Where a fault surfaces

`Navigation.PushModalAsync`/`PopModalAsync` complete after the framework updates its own state; they do not wait for a deferred presentation:

- applied inline because the platform was ready at request time → the fault is rethrown to the navigation caller;
- deferred because the platform was not ready → the fault is logged through the window's `ILogger`, while stack restoration still enables a retry.

#### Threading

`RequestSync()` is safe from any thread. When dispatch is required, the complete reconciliation entry — readiness checks, page lifecycle events, and platform push/pop calls — is marshalled through `Window.Dispatcher`, which remains available independently of the current handler scope. On the UI thread reconciliation starts inline; the resulting synchronous reentrancy is documented so backends do not call it while holding a lock.

Queued requests capture the current handler-scope generation and are dropped if window destruction or handler replacement overtakes them.

#### Lifecycle

- Teardown is terminal: after `Window.Destroying`, lazy access cannot recreate the override. A later handler, with a new service scope such as an Android activity recreation, enables a new factory invocation.
- Teardown does not call `PopModalAsync` for still-presented modals. `Dispose` owns dismissing them and must not depend on `MauiContext`, the window handler, or platform views still being usable.
- A throwing factory is logged and permanently selects built-in fallback for the current handler scope instead of escaping from an arbitrary resolution call site or being retried repeatedly.
- Page attachment is keyed by page-handler identity to avoid duplicate delivery, including the late-resolution path where the page handler exists before a service scope is available.

#### Native dismissals

If the user dismisses a modal natively (swipe-to-dismiss, back, native close button), the backend must route it through `host.Window.Navigation.PopModalAsync()`. The framework then calls backend `PopModalAsync` for that page, so implementations must treat an already-dismissed modal idempotently.

#### Batch hints

`IsBatchPopping`/`IsBatchPushing` are optional, `Shell`-only optimization hints for suppressing animation so intermediate modals do not flash. Ignoring them remains correct.

### Behavior preservation

When no factory is registered, the existing Android, iOS, MacCatalyst, Windows, Tizen, and Standard partials remain the fallback. Their platform changes are symbol renames (`PushModalPlatformAsync` → `PushModalPlatformCoreAsync`, `IsModalPlatformReady` → `IsModalPlatformReadyCore`, and counterparts) so shared code can route between built-in presentation and an override.

One deliberate behavior fix also affects built-in platforms: a pop requested while the platform was not ready previously reconciled with `animated: false`, because the request left the logical stack before deferred reconciliation. Pending pop animation metadata now survives until dismissal so the requested animation value is retained. Tests cover both animated and unanimated deferred override paths.

### Issues Fixed

Contributes to #34099

### Tests

`src/Controls/tests/Core.UnitTests/ModalNavigationPlatformTests.cs` contains 38 focused tests covering:

- recursion-free readiness;
- background-thread dispatch and inline UI-thread reconciliation;
- push/pop fault rollback and retry;
- animated and unanimated deferred pops;
- destroy/handler-replacement lifecycle, stale queued callbacks, teardown-only disposal, and late `PageAttached`;
- throwing and null-returning factories with built-in fallback;
- DI selection, per-window instances, per-handler-scope factory invocation, stack ordering/state, animation forwarding, pop cancellation, no-registration fallback, deferred resolution, and public interface visibility.

The regression cases were mutation-checked against their corresponding safeguards.

`src/Controls/tests/DeviceTests/Elements/Modal/ModalNavigationPlatformTests.cs` adds eight real-handler cases for routing, suppression of built-in presentation, animation forwarding, host stack state, pop-fault restoration/recovery, deferred pop animation, and teardown-only disposal.

`PublicAPI.Unshipped.txt` is updated additively for all seven baselines (`net`, `net-android`, `net-ios`, `net-maccatalyst`, `net-tizen`, `net-windows`, and `netstandard`).

🏁 Report — Final Recommendation

⚠️ Final Recommendation: REQUEST CHANGES

Winner: pr

The submitted PR is the strongest available candidate because it is the only implemented candidate that passed its focused regression suite (38/38 unit tests). It is not merge-ready: the expert review identified concrete lifecycle, reentrancy, teardown, and page-retention defects, while the trusted Android gate remains inconclusive.

Comparative ranking

Rank Candidate Implementation Regression evidence Assessment
1 pr Complete submitted fix Focused unit gate passed 38/38; Android gate inconclusive because XHarness produced no fresh result Best available baseline, but expert findings require another revision
2 try-fix-1 Blocked before editing; empty diff No tests run The eager scope-bound/null-object design directly targets lazy-resolution complexity, but its proposed delayed _modalPages removal risks changing Shell semantics and was never implemented
3 try-fix-2 Blocked before editing; empty diff No tests run The cancellable transition journal preserves operation intent cleanly, but introduces a substantially larger state machine and has no executable evidence
4 pr-plus-reviewer One consolidated reviewer patch Failed focused unit regression: 42 passed, 1 failed; Android ended APP_CRASH before useful modal results Addresses the expert findings, but cannot outrank the passing PR because PushIsAppliedOnceTheWindowPageGetsAHandler regressed

Expert assessment of the submitted PR

The API boundary is sound: the framework retains requested/platform stack reconciliation, lifecycle events, Shell batching, and fallback behavior while delegating native presentation. However, the raw implementation has three major defects:

  • _pendingPopAnimations can retain a page indefinitely after push/pop-before-ready and later replay stale animation intent.
  • PlatformOverride invokes factory and PageAttached backend code while evaluating readiness, allowing a documented RequestSync() call to re-enter an outer push/pop and double-apply or empty the platform stack.
  • An exception from third-party Dispose() can abort teardown before scope invalidation, leaving stale queued callbacks or a permanently blocked replacement scope.

Additional concerns include changed built-in deferred-animation behavior, missed early page-attachment delivery, an orphaned Android/Windows load watcher, unguarded IsReady, a throwing non-nullable MauiContext API, and an undisposed override/page-handler retention edge.

pr-plus-reviewer result

The frozen refinement moved resolution to page/handler lifecycle edges, made readiness access side-effect free, guarded backend readiness and disposal, cleaned deferred animation state, preserved legacy built-in animation behavior, disconnected stale load watchers, made MauiContext nullable, and added focused regressions.

git diff --check 0ee11ba5d4e02b46ac96d5c0e77cf20cc7906507 passed. The focused unit command then produced 42 passed / 1 failed: PushIsAppliedOnceTheWindowPageGetsAHandler observed no external push. The likely transition is that root-page setup reconciles through the built-in route before the external factory is resolved, leaving the stacks synchronized when PageAttached later binds the override. Per the execution contract, the candidate was not repaired or retested.

The Android command built and launched artifacts under /home/vsts/work/_temp/pr-37853-pr-plus-reviewer, then XHarness returned exit code 80 (APP_CRASH) after beginning unrelated test execution and before useful modal results. This is recorded as inconclusive for modal behavior, not as evidence that the submitted PR fails.

Decision

Keep pr as the comparative winner, but request a new revision that resolves the expert findings without regressing page-handler readiness. Neither blocked try-fix design can replace executable code, and the reviewer refinement must rank below the passing submitted candidate because it failed a focused regression test.


📱 UI Tests — Button,Label,Layout

Detected UI test categories: Button,Label,Layout

Deep UI tests — 360 passed, 0 failed, 7 skipped across 3 categories on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category Tests Snapshot diffs
Button 71/73 (2 skipped) ✓
Label 97/99 (2 skipped) ✓
Layout 192/195 (3 skipped) ✓
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

🧭 Next Steps — review latest findings

No alternative fix was selected for this run. Review the session findings and CI results before merging.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants