Skip to content

[iOS, Mac, Windows] Fix PlatformTicker does not respect animation accessibility setting - #35846

Merged
kubaflo merged 7 commits into
dotnet:net11.0from
HarishwaranVijayakumar:fix-ios/reduce-motion-ticker
Aug 2, 2026
Merged

[iOS, Mac, Windows] Fix PlatformTicker does not respect animation accessibility setting#35846
kubaflo merged 7 commits into
dotnet:net11.0from
HarishwaranVijayakumar:fix-ios/reduce-motion-ticker

Conversation

@HarishwaranVijayakumar

@HarishwaranVijayakumar HarishwaranVijayakumar commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

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!

Issue Details

  • PlatformTicker on iOS and Windows does not respect the OS animation accessibility settings.
  • On iOS, enabling Reduce Motion (Settings → Accessibility → Motion → Reduce Motion) has no effect — MAUI animations continue to play normally.
  • On Windows, disabling Animation effects (Settings → Accessibility → Visual effects → Animation effects) has no effect — MAUI animations continue to play normally.
  • Android already honors these settings via ValueAnimator.AreAnimatorsEnabled() and the energy-saver / duration-scale listeners.

Root Cause of the issue

iOS

  • The PlatformTicker.iOS.cs never sets SystemEnabled on the base Ticker class. The SystemEnabled property defaults to true, meaning AnimationManager always allows animations to run.
  • AnimationManager.Add() checks Ticker.SystemEnabled — if false, it rejects the animation.
  • AnimationManager.OnFire() checks Ticker.SystemEnabled — if false, it force-finishes all running animations.

Windows

  • PlatformTicker.Windows.cs never checked the system animation accessibility setting. Ticker.SystemEnabled defaults to true, so MAUI animations always played regardless of the "Show animations in Windows" setting (Settings → Accessibility → Visual effects → Animation effects).

Android wires this up correctly:

  • Checks ValueAnimator.AreAnimatorsEnabled() at construction
  • Listens for energy-saver and duration-scale changes to update SystemEnabled at runtime
  • iOS does none of this — SystemEnabled stays true forever regardless of accessibility settings.

Description of Change

Accessibility and Animation Behavior:

  • PlatformTicker now observes system accessibility settings—on Windows, it listens to the "Show animations in Windows" setting, and on iOS/MacCatalyst, it respects the "Reduce Motion" setting. The ticker automatically enables or disables animations based on these settings.

Resource Management and Disposal:

  • Both platform-specific PlatformTicker classes now implement IDisposable and provide Dispose() and Dispose(bool disposing) methods to clean up event handlers and observers, preventing potential memory leaks.
  • The public API surface is updated to include the new Dispose methods for PlatformTicker on all relevant platforms.

Robustness Improvements:

  • Added checks to prevent starting or responding to events if the ticker has already been disposed, enhancing the safety and reliability of the animation system.

Issues Fixed

Fixes #35845

Tested the behaviour in the following platforms

  • - Windows
  • - Android
  • - iOS
  • - Mac
Platform Before After
iOS
Before.mov
After-iOS.mov
Mac
Before-mac.mov
After-mac.mov
Windows
Before-windows.mp4
After-windows.mp4

@github-actions

github-actions Bot commented Jun 10, 2026

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 -- 35846

Or

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

@github-actions github-actions Bot added area-animation Animation, Transitions, Transforms platform/ios platform/macos macOS / Mac Catalyst labels Jun 10, 2026
@NirmalKumarYuvaraj NirmalKumarYuvaraj added community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration labels Jun 10, 2026
@kubaflo

kubaflo commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

AI code review refresh for net11.0 target

Head reviewed: dc24b3dabb0745f8c021a04158cc9792c02db5f7 · target net11.0 · state: OPEN (draft/WIP)

Verdict: Needs changes (code is solid; blocked from LGTM by WIP status, missing tests, and a red CI leg)

Method

Independent-first: read gh pr diff, the changed files, and the base Ticker/AnimationManager/Android PlatformTicker before the PR narrative, then reconciled.

Scope / blast radius

Touches src/Core/src/Animations/PlatformTicker.iOS.cs (+ iOS/MacCatalyst PublicAPI.Unshipped.txt). PlatformTicker is the scoped ITicker driving all iOS/MacCatalyst animations via AnimationManager. Runtime accessibility + lifecycle change → high blast radius, so failure modes were probed below.

Findings

Looks correct

  • Mirrors the established Android PlatformTicker pattern: sets SystemEnabled from the platform signal and lets base Ticker.OnSystemEnabledChanged fire once to force-finish in-flight animations. AnimationManager.Add/OnFire already gate on SystemEnabled, so mapping SystemEnabled = !UIAccessibility.IsReduceMotionEnabled is the right hook.
  • IDisposable + Dispose(bool) added correctly; _disposed guard makes dispose idempotent and also guards Start() and OnReduceMotionStatusChanged. Observer is removed and disposed on dispose. Since the ticker is DI-scoped, scope teardown now cleans up the notification observer (no per-window leak).
  • ReduceMotionStatusDidChangeNotification is delivered on the main thread by UIKit, so the SystemEnabled mutation → Fire/ForceFinishAnimations path stays on the UI thread. No threading concern.

Should address

  • No tests. Acknowledged WIP, but a device test asserting SystemEnabled toggles with Reduce Motion (and that running animations force-finish) would lock in the behavior; UIAccessibility is awkward to unit test, so a device test is the right vehicle.
  • Semantic check (discussion): Apple's Reduce Motion means "reduce/replace" motion, not "disable all animation." This change makes Reduce Motion fully disable MAUI animations (binary SystemEnabled). That's consistent with the framework's existing binary model and matches the issue intent, but please confirm that's the desired UX rather than, e.g., shortening/cross-fading.
  • Nit: file ends without a trailing newline (\ No newline at end of file).

Prior review reconciliation

No prior reviews, inline comments, or earlier fleet-round markers exist on this PR — nothing to reconcile.

CI status

maui-pr Build macOS (Debug) = fail; remaining maui-pr legs + Build Analysis = pending. The macOS Debug failure is MSB4019: .buildtasks/Microsoft.Maui.Core.props was not found on Maui.Controls.Sample.Embedding.csproj — a build-tasks ordering/infra error, not a compile error in this change (Microsoft.Maui.dll Core compiled successfully in the same log). Failure appears PR-unrelated, but because required legs are still pending and one leg is red, this is not declared ready.

Confidence

Medium-high on the code correctness (clear parallel to the shipped Android implementation; Core compiled). Medium on CI (one red leg judged infra-related; others undetermined/pending).

Non-approval disclaimer: This is an automated review note only — not an approval or a request for changes. Final approval is a human decision.

@HarishwaranVijayakumar HarishwaranVijayakumar changed the title [WIP] [iOS, Mac] Fix PlatformTicker.iOS.cs does not respect Reduce Motion accessibility setting [WIP][iOS, Mac, Windows] Fix PlatformTicker does not respect animation accessibility setting Jun 12, 2026
@kubaflo

kubaflo commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

AI code review refresh for net11.0 target

Head reviewed: 3559a767b533fc083d3e5561e49f1f7acf5f03bf · target net11.0 · state: OPEN (draft/WIP)

Verdict: Needs changes (code is largely correct; blocked from LGTM by WIP status, missing tests, and a minor design inconsistency on the new Windows override)

Method

Independent-first: re-read gh pr diff 35846, then the three full files at head (PlatformTicker.{Windows,iOS}.cs, plus base Ticker and AnimationManager to verify the force-finish contract), the Android sibling, and the three PublicAPI.Unshipped.txt files — before re-reading the PR narrative.

Scope / blast radius

Now spans three platforms: PlatformTicker.Windows.cs (new), PlatformTicker.iOS.cs (covers iOS + MacCatalyst), and matching PublicAPI entries. PlatformTicker is the scoped ITicker driving every animation through AnimationManager on each platform — runtime accessibility + lifecycle change → high blast radius, so failure modes probed below.

What changed since round 15

  • New commit 3559a76: adds Windows support via UISettings.AnimationsEnabled.
  • iOS code is unchanged from dc24b3d (already reviewed in round 15).

Findings on the new Windows commit

Looks correct

  • AnimationManager.Add() gates new animations on Ticker.SystemEnabled — so disabling animations immediately blocks new ones. ✅
  • Mid-flight: even though the getter-only override means OnSystemEnabledChanged() is never invoked (it only fires from the base setter), correctness is preserved because CompositionTarget.Rendering keeps ticking until End() is called. On the next render frame OnFire() reads Ticker.SystemEnabled, sees false, runs ForceFinishAnimations(), and End() then unsubscribes the rendering callback. So force-finish still happens — just via the next-frame poll path rather than the explicit "fire-once" hook iOS/Android use. ✅
  • PublicAPI entry override Microsoft.Maui.Animations.PlatformTicker.SystemEnabled.get -> bool matches the actual override (read-only — no setter exposed on Windows). ✅

Should address

  • Per-frame WinRT read. _uiSettings.AnimationsEnabled is queried on every OnFire tick (≈60 Hz) and every Add. Each call is a cross-apartment property read into Windows.UI.ViewManagement. Functional, but a measurable hot-path cost compared to the cached/event-driven Android & iOS paths. Recommend caching the value and subscribing to UISettings.AnimationsEnabledChanged (then routing through the base setter, which also restores the explicit OnSystemEnabledChanged() force-finish semantics used on the other two platforms).
  • new UISettings() in a field initializer is not failure-tolerant. Windows.UI.ViewManagement.UISettings construction can throw in unpackaged, headless, or design-time hosts (Designer.IsInDesignMode, certain WinUI 3 desktop scenarios). A throw here would crash PlatformTicker construction → DI scope build → window initialization. Consider try/catch with a safe default (e.g., true).
  • Architectural inconsistency across platforms. Three platforms now use three different mechanisms (Android = event listeners + cache; iOS = notification + cache via base setter; Windows = getter-only live read). Not a bug, but worth aligning before promotion to non-draft.
  • No Dispose/cleanup story on Windows. _uiSettings itself isn't IDisposable so this is fine today, but if you add an event subscription as suggested above, you'll need to mirror the iOS IDisposable pattern to unhook it. Flagging so it doesn't get missed.

Prior round-15 findings — reconciliation

  • No testsstill unaddressed. A device test asserting SystemEnabled toggles with Reduce Motion / Show-Animations and that in-flight animations force-finish would lock in the behavior on all three surfaces.
  • Reduce Motion semantic (binary disable vs reduce)still pending discussion. Now extends to Windows: "Show animations in Windows" is also binary, so semantics line up there.
  • 📋 iOS file has no trailing newlinestill present in head (\ No newline at end of file in diff).
  • Nothing else was flagged in round 15 to reconcile.

CI status (build 1461010)

  • ✅ All build/pack legs green (Build Windows Debug+Release, Build macOS Debug+Release, Pack Windows+macOS).
  • Core/Controls/Essentials Helix unit tests (Debug) clean: osx.15.arm64.maui.open_Debug and Windows.10.Amd64.Open_Debug report 4643/4676 passed, failedTests: 0 — no Animations regression visible.
  • Multiple iOS device-test legs failed (RunOniOS_MauiDebug/Release/ReleaseTrimFull{,_CoreCLR}, AOT macOS). Test-run telemetry shows totalTests:1–4, passedTests:0–2, failedTests:0 — i.e. sessions aborted before reporting (no .trx produced; Build Analysis surfaces [PublishTestResults] No test result files matching ... were found and [Provision JDK] PowerShell exited with code '1'). Classic macOS Helix infra/provisioning flakiness, not Animations-attributable.
  • Windows Helix Release: a single Microsoft.Maui.Essentials.AI.UnitTests.dll work item failed — unrelated module to this PR.
  • Build Analysis is annotated fail but knownIssues array is empty; all surfaced items are infra/provision failures listed above.
  • Net: no failure attributable to this change, but multiple red required legs prevent a clean signal.

Confidence

Medium-high on code correctness — I traced the OnFire/ForceFinishAnimations path on Windows and confirmed the next-render-frame fall-through preserves force-finish semantics. Medium on CI — macOS device-test infra failures are widespread across unrelated PRs/legs in the same window, so I'm classifying as infra, but the green Core unit-test runs are the strongest direct signal that the change itself is benign.

Non-approval disclaimer: This is an automated review note only — not an approval or a request for changes. Final approval is a human decision.

@kubaflo

kubaflo commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator

PR #35846 — Multi-model review synthesis

Verdict: NEEDS_DISCUSSION (draft) · confidence=high · inline=0

Reconciliation (why the models split)

Validated against the actual code, no confirmed code error exists, so the verdict is not NEEDS_CHANGES. The split is explained almost entirely by how each model treated the red CI: gpt-5.5 (LGTM) correctly judged the failing legs as unrelated; opus-4.6 (ND) and opus-4.8 (NC) both found the code functionally correct and differ only on weighting draft/WIP + no-tests + red-CI as a process blocker (opus-4.8 itself states "No blocking code defect found"); gemini (NC, high) is the true outlier — it invented a causal mechanism ("the PR causes CI failures because of thread-affinity"), which the evidence refutes.

Finding validation

  • gemini — iOS UIAccessibility.IsReduceMotionEnabled throws UIKitThreadAccessException (error): REJECTED. No EnsureUIThread guard on this static accessibility read; UI-thread checking is off by default. The iOS integration legs that boot the real app (RunOniOS_*CoreCLR, RunOniOS_MauiNativeAOT) PASS, proving the constructor does not throw on the main thread during DI scope build. Android's sibling already does main-thread platform work (new ValueAnimator()) in its constructor — same established assumption, not a new defect.
  • gemini — Windows new UISettings() wrong-thread COM exception causes Windows CI failures (error): REJECTED. ViewManagement.UISettings is already instantiated in core paths MauiWinUIWindow.cs:30 and WindowRootView.cs:44 during the same window lifecycle. Decisive: Windows Helix Unit Tests (Debug) = PASS; only (Release) failed, and on the unrelated Microsoft.Maui.Essentials.AI.UnitTests.dll module. A deterministic constructor crash would fail Debug too.
  • gemini — NSNotificationCenter strong-ref leak (warning): NOT A DEFECT. The ticker is DI-scoped and AnimationManager.Dispose() disposes the ticker (Ticker is IDisposable), which removes/disposes the observer on scope teardown. The PR adds the full Dispose pattern precisely for this; no finalizer is needed (none exists).
  • opus-4.8 / opus-4.6 — per-frame WinRT read on Windows hot path (suggestion): VALID but already posted in the round-19 PR comment; not duplicated here.
  • opus-4.8 — binary "disable all animation" vs Apple's "reduce" semantics (suggestion): VALID open design question, already raised in round-15/round-19 comments.

Key points

  • Code mirrors the shipped Android PlatformTicker pattern; force-finish contract verified on both surfaces (iOS via base-setter OnSystemEnabledChanged; Windows via the next-render-frame OnFireForceFinishAnimations poll path). PublicAPI entries match the actual members.
  • Real (non-blocking) reasons it isn't LGTM: explicit [WIP] draft, no tests added, an unresolved binary-vs-reduce UX question, and a per-frame UISettings.AnimationsEnabled read worth aligning to the cached/event-driven Android/iOS approach before promotion.

CI

maui-pr red, but not attributable to this change: failing iOS legs abort on [Provision JDK] PowerShell exited with code '1' with failedTests:0; CoreCLR/NativeAOT iOS + Windows Debug unit legs are green; the Windows Release failure is an unrelated Essentials.AI work item; Build Analysis knownIssues is empty.

Multi-model review (gpt-5.5 · opus-4.8 · opus-4.6 · gemini-3.1-pro). Comments only — not a formal approval.

@vishnumenon2684 vishnumenon2684 changed the title [WIP][iOS, Mac, Windows] Fix PlatformTicker does not respect animation accessibility setting [iOS, Mac, Windows] Fix PlatformTicker does not respect animation accessibility setting Jun 15, 2026
@sheiksyedm
sheiksyedm marked this pull request as ready for review June 15, 2026 17:08
@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 Jun 21, 2026
@MauiBot MauiBot added s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels Jun 21, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jun 21, 2026
@HarishwaranVijayakumar
HarishwaranVijayakumar force-pushed the fix-ios/reduce-motion-ticker branch from 5aeef4d to 662ffb5 Compare June 24, 2026 06:33
@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 Jun 24, 2026

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

Comments suppressed due to low confidence (1)

src/Core/src/Animations/PlatformTicker.Windows.cs:22

  • The PR description mentions that Windows PlatformTicker "overrides SystemEnabled" and that the Windows PublicAPI file was updated to reflect that override. In the current implementation, SystemEnabled is only set in the constructor / change event handler, and there is no override of the property (and the PublicAPI diff only adds Dispose). Please update the PR description to match the implementation, or add the override if that was the intent.
		/// <summary>
		/// Creates a new Windows <see cref="PlatformTicker"/> that respects the "Show animations in Windows" accessibility setting.
		/// </summary>
		public PlatformTicker()
		{
			_dispatcherQueue = DispatcherQueue.GetForCurrentThread();
			SystemEnabled = _uiSettings.AnimationsEnabled;
			_uiSettings.AnimationsEnabledChanged += OnAnimationsEnabledChanged;

@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 Jul 30, 2026
@MauiBot MauiBot added s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates and removed s/agent-fix-win AI found a better alternative fix than the PR labels Jul 30, 2026
@MauiBot

This comment has been minimized.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jul 30, 2026
@kubaflo

kubaflo commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@kubaflo kubaflo closed this Jul 30, 2026
@kubaflo

kubaflo commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

/azp run

@kubaflo kubaflo reopened this Jul 30, 2026
@kubaflo
kubaflo temporarily deployed to copilot-pat-pool July 30, 2026 22:45 — with GitHub Actions Inactive
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@kubaflo

kubaflo commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

/azp run

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

@azure-pipelines

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

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you please check the ai's suggestions and review if test failures are related?

@kubaflo

This comment has been minimized.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Tests Failure Analysis

@HarishwaranVijayakumar — test-failure review results are available based on commit d0cf4bd.

Maintainers can request a fresh review after new comments, commits, or CI runs by commenting /review tests.

Overall Not ready Failures 43 Regressed vs base 16 Baseline 15 on base

Test Failure Review: Not ready - click to expand

Overall verdict: Not ready. 16 leg/failures are red on this PR but green across all 5 recent net11.0 base builds sampled per definition — a deterministic regression centered on the Controls.Sample.Sandbox project (CS8622 nullability build breaks). The remaining ~27 failures are unattributed — infra flakiness, unexplained build legs, and UI/visual tests that are also red on base — none of which are dismissible without a human, so they cannot lower the ceiling below Not ready either.

  • ✗ PR-related — Sandbox sample build breaks / CS8622 nullability (~16 legs): Controls.Sample.Sandbox fails to compile on iOS/MacCatalyst/Windows/Android because generated XAML handlers (OrdersPage/ProductPage) mismatch the EventHandler delegate; green on all sampled base builds, so attributed regressed-vs-base — e.g. Run Integration Tests - Samples - CS8622.
  • i Uncertain — unexplained build legs & unverified device tests (~19 legs): 12 failed build legs produced no extractable failure and 7 green maui-pr-devicetests checks could not be confirmed Failed==0 (XHarness exits 0 even on failures); includes an incomplete/killed Android work item (com.microsoft.maui.controls.devicetests-Signed).
  • i Uncertain — UI/visual tests also red on base (~13 tests): CollectionView dynamic-template timeouts and visual-diff failures are red on the PR and on base with indeterminate attribution — likely pre-existing/flaky but not dismissible — e.g. ValidateDynamicItemTemplateDisplayed.
  • i Uncertain — Android SDK provisioning flake (~4 legs): Failed to find package 'platform-tools;35.0.2' / avdmanager exited with an error status, flaky on base — infrastructure, not code.

Coverage: 151 checks · 138 passing · 13 failing · 0 pending · 0 inaccessible · 1 unmapped · 12 unexplained build legs · 0 unaccounted failing checks · 0 aborted failing checks · 0 canceled-build checks · 7 device-test unverified · 27 unattributed · 16 regressed-vs-base. Deterministic ceiling: Not ready — 16 legs are a deterministic regression vs base and 1 check (Build Analysis) had no inspectable AzDO evidence.

Builds (this PR): maui-pr 1533908, maui-pr-devicetests 1533910, maui-pr-uitests 1533909. Base sampling (net11.0, 5 recent builds per definition): maui-pr 1535343/1534911, devicetests 1535345, uitests 1535443.

Recommended action

Fix the Controls.Sample.Sandbox CS8622 nullability build breaks (the OrdersPage/ProductPage event-handler signatures) so the Samples integration legs compile, then have a maintainer confirm the 7 unverified device-test checks and the also-red-on-base UI/visual failures before merging.

Visual failure comparisons

Full-resolution CI baseline, actual, and diff images are embedded below. They supplement the failure classification and do not change the deterministic verdict ceiling.
Relationship labels use deterministic exact test-and-platform baseline evidence plus exact changed snapshot/test scope; missing or mixed evidence remains Needs human investigation.

PointerOverWithSelectedStateShouldWork - windows - Needs human investigation - visual comparison

CI reported 9.28% difference in build 1533909.

Relationship to PR: Needs human investigation - No decisive exact test-and-platform baseline attribution was available.

CI baselineFresh PR actualCI diff
PointerOverWithSelectedStateShouldWork baseline PointerOverWithSelectedStateShouldWork actual PointerOverWithSelectedStateShouldWork diff
CollectionViewSelectedItemBackgroundLost - windows - Needs human investigation - visual comparison

CI reported 4.44% difference in build 1533909.

Relationship to PR: Needs human investigation - No decisive exact test-and-platform baseline attribution was available.

CI baselineFresh PR actualCI diff
CollectionViewSelectedItemBackgroundLost baseline CollectionViewSelectedItemBackgroundLost actual CollectionViewSelectedItemBackgroundLost diff
TopTabUnselectedTextVisibleWhenSwitchingTabs - android - Needs human investigation - visual comparison

CI reported 3.07% difference in build 1533909.

Relationship to PR: Needs human investigation - No decisive exact test-and-platform baseline attribution was available.

CI baselineFresh PR actualCI diff
Baseline unavailable: ambiguous across multiple snapshot environments TopTabUnselectedTextVisibleWhenSwitchingTabs actual TopTabUnselectedTextVisibleWhenSwitchingTabs diff

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you check the ai's suggestions and if test failures are related?

@kubaflo

This comment has been minimized.

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

Suppressed comments (4)

src/Core/src/Animations/PlatformTicker.Windows.cs:33

  • AnimationManager.Add() calls Ticker.Start() whenever Ticker.IsRunning is false. On Windows, PlatformTicker inherits Ticker.IsRunning (timer-based), so IsRunning stays false and Start() may be invoked repeatedly, causing multiple CompositionTarget.Rendering subscriptions and multiple Fire invocations per frame. Make Start() idempotent (or override IsRunning).
			CompositionTarget.Rendering += RenderingFrameEventHandler;

src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt:56

  • PublicAPI.Unshipped.txt now contains a duplicated block of existing API entries (e.g., Microsoft.Maui.ITab, TabBarPlacement, ValidateCommand, etc.). This duplication should be removed so the file only contains one entry per API member, plus the newly added PlatformTicker.Dispose* entries.
Microsoft.Maui.ITab
Microsoft.Maui.ITab.Icon.get -> Microsoft.Maui.IImageSource?
Microsoft.Maui.ITab.IsEnabled.get -> bool
Microsoft.Maui.ITab.Title.get -> string!
Microsoft.Maui.TabBarPlacement
Microsoft.Maui.TabBarPlacement.Bottom = 1 -> Microsoft.Maui.TabBarPlacement
Microsoft.Maui.TabBarPlacement.Top = 0 -> Microsoft.Maui.TabBarPlacement
override Microsoft.Maui.MauiUIApplicationDelegate.ValidateCommand(UIKit.UICommand! command) -> void
override Microsoft.Maui.Platform.MauiTextField.LayoutSubviews() -> void
override Microsoft.Maui.Platform.NoCaretField.LayoutSubviews() -> void
override Microsoft.Maui.Platform.MauiView.AccessibilityActivate() -> bool
override Microsoft.Maui.Platform.MauiView.AccessibilityLabel.get -> string?
override Microsoft.Maui.Platform.MauiView.AccessibilityLabel.set -> void
override Microsoft.Maui.Handlers.EditorHandler.PlatformArrange(Microsoft.Maui.Graphics.Rect rect) -> void

src/Core/src/Animations/PlatformTicker.Windows.cs:83

  • If PlatformTicker is constructed on a thread without a DispatcherQueue, _dispatcherQueue will be null and the AnimationsEnabledChanged callback will silently do nothing, so Windows would not respect the accessibility setting change at runtime. Add a best-effort fallback to update SystemEnabled even when _dispatcherQueue is null.
			if (_dispatcherQueue is not null)
			{
				_dispatcherQueue.TryEnqueue(() =>

src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt:57

  • PublicAPI.Unshipped.txt now contains duplicate entries (e.g., Microsoft.Maui.ITab, TabBarPlacement, ValidateCommand, etc.) a second time. Duplicated PublicAPI lines are error-prone and can break the PublicAPI analyzer expectations; only the new PlatformTicker.Dispose* additions should be added here.
Microsoft.Maui.ITab
Microsoft.Maui.ITab.Icon.get -> Microsoft.Maui.IImageSource?
Microsoft.Maui.ITab.IsEnabled.get -> bool
Microsoft.Maui.ITab.Title.get -> string!
Microsoft.Maui.TabBarPlacement
Microsoft.Maui.TabBarPlacement.Bottom = 1 -> Microsoft.Maui.TabBarPlacement
Microsoft.Maui.TabBarPlacement.Top = 0 -> Microsoft.Maui.TabBarPlacement
override Microsoft.Maui.MauiUIApplicationDelegate.ValidateCommand(UIKit.UICommand! command) -> void
override Microsoft.Maui.Platform.MauiTextField.LayoutSubviews() -> void
override Microsoft.Maui.Platform.NoCaretField.LayoutSubviews() -> void
override Microsoft.Maui.Platform.MauiView.AccessibilityActivate() -> bool
override Microsoft.Maui.Platform.MauiView.AccessibilityLabel.get -> string?
override Microsoft.Maui.Platform.MauiView.AccessibilityLabel.set -> void
override Microsoft.Maui.Handlers.EditorHandler.PlatformArrange(Microsoft.Maui.Graphics.Rect rect) -> void

@MauiBot

MauiBot commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

AI Review Summary

@HarishwaranVijayakumar — new AI review results are available based on this last commit: 7e430a4.

Gate No Tests Confidence Low Platform iOS


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

Gate Result: ⚠️ SKIPPED

No tests were detected in this PR.

Recommendation: Add tests to verify the fix using the write-tests-agent.


📱 UI Tests — Animation

Detected UI test categories: Animation

Deep UI tests — 5 passed, 0 failed across 1 category on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category Tests Snapshot diffs
Animation 5/5 ✓
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

📋 Pre-Flight — Context & Validation

Issue: #35845 - [iOS, Mac] PlatformTicker.iOS.cs does not respect Reduce Motion accessibility setting
PR: #35846 - [iOS, Mac, Windows] Fix PlatformTicker does not respect animation accessibility setting
Platforms Affected: iOS, MacCatalyst, Windows
Files Changed: 5 implementation/API, 0 test

Key Findings

  • The PR wires iOS/MacCatalyst PlatformTicker to UIAccessibility.IsReduceMotionEnabled and UIApplication.ReduceMotionStatusDidChangeNotification, then adds IDisposable/Dispose(bool) public API entries to clean up the observer.
  • The PR wires Windows PlatformTicker to UISettings.AnimationsEnabled and AnimationsEnabledChanged, with dispatcher marshalling for the event callback.
  • Gate was already skipped because no tests were detected; no regression tests were added for the accessibility behavior.
  • Prior reviews and independent code review found unresolved Windows lifecycle issues and duplicate iOS/MacCatalyst PublicAPI entries.

Code Review Summary

Verdict: NEEDS_CHANGES
Confidence: low
Errors: 3 | Warnings: 1 | Suggestions: 0

Key code review findings:

  • src/Core/src/Animations/PlatformTicker.Windows.cs:26-34 still inherits timer-backed Ticker.IsRunning, so repeated animations can add duplicate CompositionTarget.Rendering subscriptions.
  • src/Core/src/Animations/PlatformTicker.Windows.cs:73-82 silently ignores setting changes if constructed without a DispatcherQueue.
  • src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt:44-57 and src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt:43-56 duplicate existing PublicAPI entries.
  • ⚠ No regression coverage for initial accessibility-state sync, notification/event changes, or disposal behavior.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #35846 Cache initial iOS Reduce Motion state, subscribe to ReduceMotion notification, implement public Dispose API; cache Windows animation setting and listen for changes. ⚠️ SKIPPED (Gate: no tests detected) PlatformTicker.iOS.cs, PlatformTicker.Windows.cs, PublicAPI baselines Original PR. Code review found Windows lifecycle issues and duplicate PublicAPI entries.

🔬 Code Review — Deep Analysis

Code Review — PR #35846

Independent Assessment

What this changes: iOS/MacCatalyst and Windows PlatformTicker now observe OS animation accessibility settings and become disposable to unregister platform observers/events. PublicAPI baselines are updated for the new dispose surface.
Inferred motivation: Make MAUI animations honor Reduce Motion / Windows Animation Effects and clean up new listeners.

Reconciliation with PR Narrative

Author claims: Fixes #35845 by wiring platform accessibility settings into Ticker.SystemEnabled, with disposal for observer cleanup.
Agreement/disagreement: The iOS/MacCatalyst direction matches the claim. Windows still has lifecycle/state issues that can break the claimed behavior.

Prior Review Reconciliation

Prior ❌ Error Finding Source Status Evidence
Windows ticker inherits false IsRunning, causing repeated render subscriptions MauiBot inline/review comments ❌ Unresolved PlatformTicker.Windows.cs still does not override IsRunning; Start() only checks _disposed before subscribing at line 33.
Windows setting callback can run animation pipeline off UI thread MauiBot ✅ Partially fixed Current code captures DispatcherQueue and enqueues the update when non-null.
Windows setting changes can be lost if ticker is constructed without a dispatcher MauiBot issue/review comments ❌ Unresolved OnAnimationsEnabledChanged does nothing when _dispatcherQueue is null (lines 73-82).
Missing regression tests MauiBot ❌ Unresolved PR changes no test files.

External Output Contract

Consumer token/pattern Producer location Producer emission condition Consumer assumption Ordinary negative case Downstream effect
N/A N/A No changed code classifies external tool output N/A N/A N/A

Blast Radius Assessment

  • Runs for all instances: Yes — ITicker/IAnimationManager are registered for normal MAUI animation use.
  • Startup impact: Possible window-scope/lazy service construction impact.
  • Static/shared state: No new static state, but platform event subscriptions affect window-scoped ticker lifecycle.

CI Status

  • Required-check result: gh pr checks --required unavailable (gh auth login required). GitHub REST shows current head has failed macOS build checks and several in-progress checks.
  • Classification: undetermined / red-pending.
  • Action taken: Invoked azdo-build-investigator; ci-analysis skill unavailable locally. Confidence capped low.

Findings

❌ Error — Windows ticker never reports running

src/Core/src/Animations/PlatformTicker.Windows.cs:26-34 subscribes to CompositionTarget.Rendering, but the class does not override IsRunning. It therefore inherits Ticker.IsRunning, which is backed by the base _timer that Windows never starts. AnimationManager.Add() (AnimationManager.cs:45) and Tweener.Start() (Tweener.cs:172) will see false and call Start() for additional concurrent animations, adding duplicate render handlers. Stop() removes only one subscription at a time. Track running state and override IsRunning.

❌ Error — Windows setting changes can be silently ignored

PlatformTicker.Windows.cs:73-82 only updates SystemEnabled when _dispatcherQueue is non-null. The ticker can be constructed before the animation action is dispatched to UI: AnimationExtensions.Animate() resolves animationManager at line 182 before DoAction() dispatches at line 184. In that case DispatcherQueue.GetForCurrentThread() can return null, and later AnimationsEnabledChanged callbacks are ignored permanently.

❌ Error — iOS/MacCatalyst PublicAPI baselines duplicate existing entries

src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt:44-57 and src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt:43-56 duplicate entries already present earlier in each file (ITab, TabBarPlacement, platform overrides). PublicAPI baselines should contain only exact, non-duplicative API entries; leave only the new PlatformTicker.Dispose entries.

⚠️ Warning — No regression coverage for platform accessibility behavior

The PR changes initial accessibility-state reads, live setting-change notifications, and disposal/unsubscription, but adds no tests or test seam. Existing ticker tests use test tickers and would not catch regressions in these platform observers.

Failure-Mode Probing

  • Multiple animations on Windows: second animation sees IsRunning == false, re-subscribes RenderingFrameEventHandler, and doubles tick callbacks.
  • Toggle Windows animations while ticker was constructed off UI thread: _dispatcherQueue is null, callback returns without updating SystemEnabled; animations keep using stale state.
  • Dispose during queued callback: guarded by _disposed, safe when dispatcher exists.
  • iOS dispose after observer registration: observer is removed and disposed; no obvious stale callback issue.

Verdict: NEEDS_CHANGES

Confidence: low
Summary: The PR addresses the right accessibility gap, but current Windows ticker state handling is still incorrect and iOS/MacCatalyst PublicAPI entries are malformed. CI is also red/pending/undetermined, so this cannot be LGTM.


🛠️ Fix — Analysis & Comparison

Fix Candidates

# Source Approach Test Result Files Changed Notes
1 try-fix iOS/MacCatalyst live SystemEnabled getter using UIAccessibility.IsReduceMotionEnabled; remove observer/dispose API; clean duplicate PublicAPI rows; add getter override PublicAPI row. ✅ PASS targeted Apple builds 3 files Different from PR; less lifecycle/API surface risk.
PR PR #35846 Observer/cached-state iOS fix plus public Dispose API; Windows cached setting/event handler. ⚠️ SKIPPED (Gate) / code review NEEDS_CHANGES 5 files Original PR; no tests detected and unresolved Windows/PublicAPI findings.

Cross-Pollination

Model Round New Ideas? Details
maui-expert-reviewer 1 Yes Proposed live getter/no observer/no Dispose API for iOS/MacCatalyst.
loop 1 Stopped Candidate 1 passed the available Apple compile checks and is demonstrably better for the iOS/MacCatalyst slice; no further iOS alternatives were necessary.

Exhausted: No — stopped because candidate #1 passed the available tests and is materially better than the PR's iOS/MacCatalyst observer/dispose approach.
Selected Fix: Candidate #1 — Solves the iOS/MacCatalyst Reduce Motion bug with less public API churn and no observer lifecycle cleanup, while passing targeted net11.0-ios26.5 and net11.0-maccatalyst26.5 Core builds.


📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current metadata is thorough, but it describes the raw PR's disposable observer/event approach; the winning fix removes the Apple dispose/observer API churn and keeps Windows lifecycle corrections, so the description would be stale as-is.

Recommended title

[iOS, MacCatalyst, Windows] PlatformTicker: Respect system animation accessibility settings

Recommended description

### Issue Details

- `PlatformTicker` on iOS, MacCatalyst, and Windows does not respect the OS animation accessibility settings.
- On iOS/MacCatalyst, enabling **Reduce Motion** (Settings -> Accessibility -> Motion -> Reduce Motion) has no effect — MAUI animations continue to play normally.
- On Windows, disabling **Animation effects** (Settings -> Accessibility -> Visual effects -> Animation effects) has no effect — MAUI animations continue to play normally.
- Android already honors these settings via `ValueAnimator.AreAnimatorsEnabled()` and the energy-saver / duration-scale listeners.

### Root Cause

**iOS / MacCatalyst**
- `PlatformTicker.iOS.cs` never connected the system Reduce Motion setting to `Ticker.SystemEnabled`.
- `AnimationManager.Add()` checks `Ticker.SystemEnabled` before accepting new animations.
- `AnimationManager.OnFire()` checks `Ticker.SystemEnabled` and force-finishes running animations when animations become disabled.

**Windows**
- `PlatformTicker.Windows.cs` never checked the Windows animation accessibility setting, so `Ticker.SystemEnabled` stayed true regardless of the "Show animations in Windows" setting.
- The Windows ticker also needs to report its actual render-subscription running state so `AnimationManager.Add()` does not add duplicate `CompositionTarget.Rendering` handlers for concurrent animations.

### Description of Change

**Accessibility and Animation Behavior**

- `PlatformTicker` now observes system animation accessibility settings:
  - iOS/MacCatalyst respects **Reduce Motion**.
  - Windows respects **Show animations in Windows** / **Animation effects**.
- `Ticker.SystemEnabled` is updated or queried through the platform setting so new animations are blocked when animations are disabled and running animations can be force-finished through the existing `AnimationManager` path.

**Windows lifecycle correctness**

- Windows `PlatformTicker` tracks whether it has subscribed to `CompositionTarget.Rendering` and reports that through `IsRunning`.
- `Start()` and `Stop()` remain idempotent so concurrent animations do not accumulate duplicate render callbacks.
- Windows setting-change handling updates `SystemEnabled` without silently dropping notifications when the ticker was created before a dispatcher queue was available.

**Public API / Resource Management**

- PublicAPI baselines are updated only for the actual platform API surface introduced by the final implementation.
- Avoid adding unnecessary Apple observer/dispose API surface when a live platform setting read can satisfy the `SystemEnabled` contract.

### What NOT to Do

- Do not leave Windows `PlatformTicker` inheriting base `Ticker.IsRunning`; the base implementation tracks a timer that Windows does not use.
- Do not silently ignore `UISettings.AnimationsEnabledChanged` when no dispatcher queue was captured.
- Do not duplicate existing entries in `PublicAPI.Unshipped.txt`; each API should appear once.

### Issues Fixed

Fixes #35845

### Tested the behaviour in the following platforms

- [ ] Windows
- [ ] Android
- [x] iOS
- [x] MacCatalyst

🏁 Report — Final Recommendation

Comparative Fix Report — PR #35846

Candidates Compared

Rank Candidate Regression/build result Assessment
1 pr-plus-reviewer Not rerun; derived from PR + expert feedback Best overall. Keeps the PR's iOS/MacCatalyst/Windows scope and addresses the expert reviewer's blocking Windows lifecycle and PublicAPI defects.
2 try-fix-1 Passed targeted Apple Core builds (net11.0-ios26.5, net11.0-maccatalyst26.5) Best tested Apple-only alternative. It avoids iOS/MacCatalyst observer/dispose API churn by using a live SystemEnabled getter, but it does not address the Windows fix that the PR currently includes.
3 pr Gate skipped; expert review found blocking defects Correct general direction, but currently has unresolved Windows duplicate-subscription risk, dropped Windows setting-change notifications in the no-dispatcher case, duplicate iOS/MacCatalyst PublicAPI rows, and no tests.

Candidate Details

pr

The raw PR implements accessibility-aware PlatformTicker behavior on iOS/MacCatalyst and Windows. It sets SystemEnabled from Reduce Motion / Animation Effects and subscribes to platform change notifications. However, Windows still inherits base Ticker.IsRunning, so concurrent animations can repeatedly add CompositionTarget.Rendering handlers. Its settings callback also silently does nothing if no DispatcherQueue was captured at construction. The iOS/MacCatalyst PublicAPI files contain duplicate existing entries.

pr-plus-reviewer

This candidate applies the expert reviewer's actionable feedback to the PR in a sandbox evaluation:

  • Windows PlatformTicker gets real running-state tracking and an IsRunning override so AnimationManager.Add() does not repeatedly call Start() while a render subscription is already active.
  • Windows setting-change handling no longer silently drops notifications when _dispatcherQueue is null.
  • iOS/MacCatalyst PublicAPI baselines are reduced to only the actual new API rows.

This is the strongest complete fix because it preserves all platforms covered by the submitted PR and removes the blocking implementation defects.

try-fix-1

This candidate replaces the PR's iOS/MacCatalyst observer/dispose implementation with a live SystemEnabled getter:

public override bool SystemEnabled => !UIAccessibility.IsReduceMotionEnabled;

It passed targeted Apple Core builds and has lower lifecycle/API risk for iOS/MacCatalyst. Its limitation is scope: STEP 5a only produced an Apple-focused candidate, while PR #35846 also fixes Windows. Because it does not include a Windows solution, it ranks below pr-plus-reviewer even though it has better targeted build evidence for the iOS/MacCatalyst slice.

Winner

Winner: pr-plus-reviewer

pr-plus-reviewer is the only candidate that both preserves the PR's complete iOS/MacCatalyst/Windows behavior and addresses the expert reviewer's blocking defects. try-fix-1 is a strong Apple-only implementation and passed targeted builds, but it is incomplete for the PR's declared Windows scope. The raw PR ranks last because it has unresolved correctness defects and no detected tests.


🧭 Next Steps — review latest findings

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

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

Labels

area-animation Animation, Transitions, Transforms community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration platform/ios platform/macos macOS / Mac Catalyst platform/windows s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants