[Android] Implement material3 support for TimePicker

[HarishwaranVijayakumar]

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

This pull request introduces a new Material3-based time picker for Android and updates the handler registration and platform-specific extensions to support it. The main goal is to enable the use of TimePickerHandler2 and MauiMaterialTimePicker when Material3 is enabled, while maintaining compatibility with the existing time picker otherwise.

Material3 Time Picker Integration

• Added new TimePickerHandler2 class for Android, implementing a Material3-style time picker dialog, with custom mapping and event handling for properties such as time, format, background, and open/close state. (src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs)
• Introduced MauiMaterialTimePicker control for Android, which wraps the Material3 time picker UI and exposes methods for showing/hiding the picker and updating its appearance. (src/Core/src/Platform/Android/MauiMaterialTimePicker.cs)

Handler Registration Updates

• Modified AddControlsHandlers method to register TimePickerHandler2 for Android when Material3 is enabled, and fall back to the original handler otherwise. (src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs) [1] [2]

Platform Extension Methods

• Added extension methods for updating format, time, and text color for MauiMaterialTimePicker, mirroring existing methods for the legacy time picker, and ensuring correct formatting and appearance. (src/Core/src/Platform/Android/TimePickerExtensions.cs)

Material Design Spec - TimePicker

Issues Fixed

Fixes #33645

Output

Material 2	Material 3

[Copilot]

Pull request overview

Implements an Android Material3-based TimePicker pipeline, introducing a new material time picker control and handler, and wiring them into the handler registration when Material3 is enabled.

Changes:

• Added MauiMaterialTimePicker, a Material3-themed TextInputEditText wrapper that exposes ShowPicker/HidePicker actions and uses MauiMaterialContextThemeWrapper for control-level theming.
• Introduced TimePickerHandler2 on Android, which uses MaterialTimePicker (M3 dialog) and maps ITimePicker properties (Time, Format, TextColor, IsOpen, etc.) to the new material platform view.
• Updated handler registration and Android TimePickerExtensions so that TimePickerHandler2 and MauiMaterialTimePicker are used when RuntimeFeature.IsMaterial3Enabled, while preserving the existing handler and extension behavior otherwise.

Reviewed changes

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

File	Description
src/Core/src/Platform/Android/TimePickerExtensions.cs	Adds UpdateFormat, UpdateTime, SetTime, and UpdateTextColor overloads for MauiMaterialTimePicker, mirroring the existing MauiTimePicker behavior for formatting and text color.
src/Core/src/Platform/Android/MauiMaterialTimePicker.cs	Introduces the internal MauiMaterialTimePicker control based on TextInputEditText, wired to open the picker via ShowPicker and configured with Material3/themed background and picker initialization.
src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs	Defines TimePickerHandler2, a Material3-specific Android handler that creates and manages a MaterialTimePicker dialog, maps ITimePicker properties, and synchronizes IsOpen with dialog visibility via listeners.
src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs	Adjusts AddControlsHandlers to register TimePickerHandler2 for TimePicker on Android when RuntimeFeature.IsMaterial3Enabled, and otherwise keep using the existing TimePickerHandler.

[Copilot]

This new Material3-specific handler (TimePickerHandler2) introduces a separate code path for Android time pickers, but there are no corresponding device/unit tests exercising its behavior (e.g., IsOpen mapping, time formatting, 24-hour vs 12-hour modes, and text color updates), while the existing TimePickerHandler is covered by tests in src/Core/tests/DeviceTests/Handlers/TimePicker/TimePickerHandlerTests*.cs. To prevent regressions and ensure the Material3 path matches the legacy handler’s behavior, consider adding Android device tests that force RuntimeFeature.IsMaterial3Enabled and verify TimePickerHandler2 correctly handles these scenarios.

[rmarinho]

🤖 AI Summary

📊 Expand Full Review

---

🔍 Pre-Flight — Context & Validation

---

📝 Review Session — Remove dispose call · 6d1e724

---

Issue: #33645 - Implement Material3 support for TimePicker
PR: #33646 - [Android] Implement material3 support for TimePicker
Author: HarishwaranVijayakumar (Syncfusion partner)
Platforms Affected: Android only
Files Changed: 4 files (2 new, 2 0 test filesmodified)

Issue Summary

Issue #33645 requests that the TimePicker control adopt the Material 3 visual design system when UseMaterial3 MSBuild property is enabled, including updated clock dial design, enhanced time input fields, AM/PM selector styling, and Material 3 theme resources.

Files Changed

Implementation files (fix):

• src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs (+280) - NEW: Material3-based time picker handler using MaterialTimePicker dialog
• src/Core/src/Platform/Android/MauiMaterialTimePicker.cs (+58) - NEW: Platform control wrapping TextInputEditText for Material3 appearance
• src/Core/src/Platform/Android/TimePickerExtensions.cs (+40) - Extension methods for MauiMaterialTimePicker (UpdateFormat, UpdateTime, UpdateTextColor)
• src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs (+12/-1) - Handler registration: uses TimePickerHandler2 when RuntimeFeature.IsMaterial3Enabled, else TimePickerHandler

Test files: NONE

PR Discussion

• dotnet-policy-service[bot]: Standard welcome message
• copilot-pull-request-reviewer (2 unresolved inline comments):

File:Line	Reviewer Says	Status
TimePickerHandler2.Android.cs:63	_positiveButtonClickListener and _dismissListener (both Java.Lang.Object) are not disposed before nulling - potential native memory UNRESOLVED	leak
TimePickerHandler2.Android.cs:34	No device/unit tests for TimePickerHandler2 - existing TimePickerHandler tests not extended to cover Material3 code UNRESOLVED	path

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #33646	New TimePickerHandler2 + MauiMaterialTimePicker with Material3 dialog; conditional handler PENDING (Gate)	4 files (+390/-1)	Original PR	registration

---

🚦 Gate — Test Verification

---

📝 Review Session — Remove dispose call · 6d1e724

---

** FAILEDResult:**
Platform: android
Mode: No-tests check

Findings

• No test files found for issue Implement Material3 support for TimePicker #33645 or TimePickerHandler2
  • Checked src/Controls/tests/TestCases.HostApp/ no Issue33645* filesIssues/
  • Checked src/Core/tests/DeviceTests/Handlers/ no tests for TimePickerHandler2TimePicker/
  • PR added 390 lines of new implementation code with 0 test lines

Existing test coverage for comparison

The existing (non-Material3) TimePickerHandler is covered by:

• src/Core/tests/DeviceTests/Handlers/TimePicker/TimePickerHandlerTests.cs
• src/Core/tests/DeviceTests/Handlers/TimePicker/TimePickerHandlerTests.Android.cs

No equivalent tests exist for TimePickerHandler2 (Material3 path).

tests must be added before this PR can proceed.FAILED Gate

---

🔧 Fix — Analysis & Comparison

---

📝 Review Session — Remove dispose call · 6d1e724

---

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #33646	New TimePickerHandler2 + MauiMaterialTimePicker with Material3 MaterialTimePicker dialog; conditional registration via `RuntimeFeature. NOT RUN (Gate failed)	4 files (+390/-1)	Original not validated	PR IsMaterial3Enabled`

Exhausted: N/ Gate failed, try-fix phase skipped per workflow rulesA
Selected Fix: N/ cannot select until tests exist and Gate passesA

---

📋 Report — Final Recommendation

---

📝 Review Session — Remove dispose call · 6d1e724

---

Final Recommendation: REQUEST CHANGES##

Summary

PR #33646 implements Material3 support for the Android TimePicker by adding a new TimePickerHandler2 class backed by Google's MaterialTimePicker dialog and a MauiMaterialTimePicker platform control. The implementation approach is well-structured and follows existing MAUI patterns (MauiMaterialButton precedent), and the conditional handler registration via RuntimeFeature.IsMaterial3Enabled is architecturally sound.

However, the PR cannot be approved in its current state due to missing tests and a native resource leak in DisconnectHandler.

Gate Result

** No tests exist for TimePickerHandler2. The PR adds ~390 lines of new handler and platform control code with zero corresponding test files. The existing TimePickerHandler has device tests in src/Core/tests/DeviceTests/Handlers/TimePicker/ that should be mirrored for the Material3 path.FAILED**

Issues Found

TimePickerHandler2 has no device tests. The following scenarios from the existing TimePickerHandlerTests.Android.cs should be covered:

• IsOpen mapping (show/hide dialog via property)
• Time initialization and format (12h vs 24h)
• UpdateTextColor / null text color handling
• UpdateFont and UpdateCharacterSpacing

Suggested approach: Add TimePickerHandler2Tests.Android.cs in src/Core/tests/DeviceTests/Handlers/TimePicker/ that forces RuntimeFeature.IsMaterial3Enabled for the test scope and validates the same scenarios covered for TimePickerHandler.

Additionally, a UI test (Issue33645) in TestCases.HostApp with a corresponding TestCases.Shared.Tests NUnit test would demonstrate the end-to-end Material3 appearance on Android.

In DisconnectHandler, _positiveButtonClickListener and _dismissListener are set to null after RemoveListeners(), but their Dispose() method is never called. Since both implement Java.Lang.Object, failing to dispose them keeps the underlying Java heap objects alive until the GC finalizes them, which can cause native memory pressure.

Fix:

// In DisconnectHandler, after RemoveListeners():
_positiveButtonClickListener?.Dispose();
_positiveButtonClickListener = null;
_dismissListener?.Dispose();
_dismissListener = null;

// Current (redundant):
if (handler.IsConnected() && handler is TimePickerHandler2 timePickerHandler && !timePickerHandler._isUpdatingIsOpen)

// Simplified (handler is already TimePickerHandler2):
if (handler.IsConnected() && !handler._isUpdatingIsOpen)

What's Good

• The MaterialTimePicker dialog integration (using FragmentManager, AddOnPositiveButtonClickListener, AddOnDismissListener) is solid and follows Android best practices
• WeakReference<TimePickerHandler2> in listener classes correctly prevents reference cycles
• Guard against duplicate dialogs (_dialog.IsVisible || _dialog.IsAdded) is correct
• The Use24HourView logic correctly handles both device locale and explicit format string
• _isUpdatingIsOpen flag prevents feedback loops when updating VirtualView.IsOpen
• The conditional handler registration in AppHostBuilderExtensions.cs is clean and correct

Requested Changes

1. [Required] Add device tests for TimePickerHandler2 in src/Core/tests/DeviceTests/Handlers/TimePicker/
2. [Required] Add Dispose() calls on _positiveButtonClickListener and _dismissListener in DisconnectHandler
3. [Optional] Add a UI test (Issue33645) in TestCases.HostApp demonstrating Material3 TimePicker appearance
4. [Optional] Simplify redundant handler is TimePickerHandler2 timePickerHandler cast in MapIsOpen

---

📋 Expand PR Finalization Review

---

Title: ✅ Good

Current: [Android] Implement material3 support for TimePicker

Description: ✅ Good

Description needs updates. See details below.
Missing Elements:

**

1. ❌ Missing NOTE block - The required PR testing note is absent. Must be prepended.
2. ❌ Missing // TODO: Material3: Make it public in .NET 11 context - The description doesn't mention that TimePickerHandler2 and MauiMaterialTimePicker are intentionally internal and will be made public in .NET 11 (as noted by the TODO comments in code).
3. ❌ No "Breaking Changes" section - Should explicitly state "None".
4. ❌ No mention of MapIsOpen / _isUpdatingIsOpen re-entrancy guard - This is a non-obvious design decision worth documenting.

Verdict: Good description with accurate content. Add NOTE block and minor clarifications; do not replace wholesale.

Actions needed:

• Prepend NOTE block
• Add "Breaking Changes: None" section
• Clarify the internal/TODO-public-in-.NET-11 intent

---

Phase 2: Code Review

See code-review.md for detailed findings.

Summary:

• 🔴 1 critical issue (redundant pattern match, functionally harmless but misleading)
• 🟡 3 suggestions (listener disposal, missing newlines at EOF, state inconsistency edge case)
• ❌ 0 blocking issues, but existing review comments (listener disposal, no tests) remain unaddressed

---

Action Summary

Item	Priority	Action
Add NOTE block to description	High	Prepend to description
Dispose listeners in DisconnectHandler	Medium	Address existing reviewer comment
Fix redundant is check in MapIsOpen	Low	Cosmetic fix
Add tests for TimePickerHandler2	Medium	Address existing reviewer comment
Add newlines at EOF	Low	TimePickerHandler2.Android.cs and TimePickerExtensions.cs
Add "Breaking Changes: None"	Low	Add to description

✨ Suggested PR Description

[!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

This pull request introduces a new Material3-based time picker for Android and updates the handler registration and platform-specific extensions to support it. The main goal is to enable the use of TimePickerHandler2 and MauiMaterialTimePicker when Material3 is enabled, while maintaining compatibility with the existing time picker otherwise.

Note: TimePickerHandler2 and MauiMaterialTimePicker are intentionally internal in this PR. They will be made public in .NET 11 once the Material3 migration is complete. See TODO: Material3: Make it public in .NET 11 comments throughout the code.

Material3 Time Picker Integration

• Added new TimePickerHandler2 class for Android, implementing a Material3-style time picker dialog using Google.Android.Material.TimePicker.MaterialTimePicker (dial/clock face mode). Handles property mappings for time, format, background, text color, font, and open/close state. Uses _isUpdatingIsOpen guard to prevent re-entrant IsOpen updates. (src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs)
• Introduced MauiMaterialTimePicker control for Android, extending TextInputEditText with Material3 context theming via MauiMaterialContextThemeWrapper. Exposes ShowPicker/HidePicker action delegates invoked from the handler. (src/Core/src/Platform/Android/MauiMaterialTimePicker.cs)

Handler Registration Updates

• Modified AddControlsHandlers to register TimePickerHandler2 for Android when RuntimeFeature.IsMaterial3Enabled is true, falling back to TimePickerHandler otherwise. On all other platforms, TimePickerHandler continues to be used unconditionally. (src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs)

Platform Extension Methods

• Added UpdateFormat, UpdateTime, and UpdateTextColor extension methods for MauiMaterialTimePicker, mirroring the existing methods for MauiTimePicker and ensuring correct formatting and appearance with Material3 theming. (src/Core/src/Platform/Android/TimePickerExtensions.cs)

Material Design Spec - TimePicker

Breaking Changes

None

Issues Fixed

Fixes #33645

Output

Material 2	Material 3

Code Review: ⚠️ Issues Found

Code Review: PR #33646 — [Android] Implement Material3 support for TimePicker

🔴 Critical Issues

None that block merge.

---

🟡 Suggestions

1. Redundant type check in MapIsOpen

File: src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs

Problem: handler is TimePickerHandler2 timePickerHandler is always true because the method signature already types the parameter as TimePickerHandler2. This creates a misleading pattern suggesting the cast could fail.

// Current (line ~170)
public static void MapIsOpen(TimePickerHandler2 handler, ITimePicker picker)
{
    if (handler.IsConnected() && handler is TimePickerHandler2 timePickerHandler && !timePickerHandler._isUpdatingIsOpen)
    {
        if (picker.IsOpen)
            timePickerHandler.ShowPickerDialog();
        else
            timePickerHandler.HidePickerDialog();
    }
}

Recommended fix:

public static void MapIsOpen(TimePickerHandler2 handler, ITimePicker picker)
{
    if (handler.IsConnected() && !handler._isUpdatingIsOpen)
    {
        if (picker.IsOpen)
            handler.ShowPickerDialog();
        else
            handler.HidePickerDialog();
    }
}

Note: This contrasts with the existing TimePickerHandler.MapIsOpen which does the is check because it uses ITimePickerHandler as the parameter type.

---

2. Listener instances not disposed in DisconnectHandler

File: src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs

Problem: MaterialTimePickerPositiveButtonClickListener and MaterialTimePickerDismissListener both derive from Java.Lang.Object. In DisconnectHandler, after RemoveListeners(), the listener instances are set to null but never Dispose()d. Other Android handler code in this repo explicitly disposes Java.Lang.Object derivatives to release native references.

(This was also flagged by the existing copilot-pull-request-reviewer comment.)

Current code (lines ~57–65):

_positiveButtonClickListener = null;
_dismissListener = null;

Recommended fix:

_positiveButtonClickListener?.Dispose();
_positiveButtonClickListener = null;
_dismissListener?.Dispose();
_dismissListener = null;

---

3. UpdateIsOpenState(true) called even when dialog creation returns null

File: src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs

Problem: CreateTimePickerDialog returns MaterialTimePicker? (nullable). In ShowPickerDialog(TimeSpan? time), the null-conditional call _dialog?.Show(...) means that if _dialog is null, nothing is actually shown — but UpdateIsOpenState(true) is still called unconditionally, leaving VirtualView.IsOpen = true even though no dialog appeared.

// Current
_dialog = CreateTimePickerDialog(hour, minute);
_dialog?.Show(fragmentManager, "MaterialTimePicker");

UpdateIsOpenState(true);  // ← called even if _dialog is null

Recommended fix:

_dialog = CreateTimePickerDialog(hour, minute);
if (_dialog is null)
    return;

_dialog.Show(fragmentManager, "MaterialTimePicker");
UpdateIsOpenState(true);

---

4. Missing newline at end of files

Files:

• src/Core/src/Handlers/TimePicker/TimePickerHandler2.Android.cs — \ No newline at end of file
• src/Core/src/Platform/Android/TimePickerExtensions.cs — \ No newline at end of file

Both files are missing a trailing newline. Most files in this repo end with a newline; the diff shows \ No newline at end of file for both. Add a newline to match repo conventions.

---

5. Secondary constructors in MauiMaterialTimePicker don't apply theme wrapper

File: src/Core/src/Platform/Android/MauiMaterialTimePicker.cs

Problem: The primary constructor wraps the context with MauiMaterialContextThemeWrapper.Create(context), but the secondary constructors ((context, attrs) and (context, attrs, defStyleAttr)) pass context directly without wrapping. Compare with MauiMaterialButton, which applies the wrapper in all constructors.

// Current
public MauiMaterialTimePicker(Context context) : base(MauiMaterialContextThemeWrapper.Create(context))
public MauiMaterialTimePicker(Context context, IAttributeSet? attrs) : base(context, attrs)  // no wrap
public MauiMaterialTimePicker(Context context, IAttributeSet? attrs, int defStyleAttr) : base(context, attrs, defStyleAttr)  // no wrap

Recommended: If the control can be inflated from XML (unlikely but possible), apply the wrapper consistently. If the secondary constructors are purely for JNI/runtime use, this is lower priority but should be documented.

---

❌ Unaddressed Existing Review Comments

Two review comments from copilot-pull-request-reviewer posted 2026-01-27 are not yet resolved:

1. [PRRC_kwDOD6PVWM6iw2g-] — Dispose _positiveButtonClickListener and _dismissListener in DisconnectHandler (see Suggestion Update README.md #2 above)
2. [PRRC_kwDOD6PVWM6iw2hP] — No device tests for TimePickerHandler2; the existing handler is covered by tests in src/Core/tests/DeviceTests/Handlers/TimePicker/. Material3 path should also be tested (e.g., IsOpen mapping, time formatting, 24-hour vs 12-hour modes, text color updates).

---

✅ Looks Good

• Handler registration logic is correct: #if ANDROID block registers TimePickerHandler2 when RuntimeFeature.IsMaterial3Enabled, else falls back to TimePickerHandler. Non-Android platforms unconditionally use TimePickerHandler. The removal of the unconditional AddHandler<TimePicker, TimePickerHandler>() at line 99 (old) correctly avoids double-registration.
• Dialog lifecycle management is solid: RemoveListeners() is called before DismissAllowingStateLoss(), preventing the OnDismiss callback from firing when HidePickerDialog is called explicitly. The OnDismiss path (user dismisses via back/outside tap) correctly sets _dialog = null before UpdateIsOpenState.
• _isUpdatingIsOpen re-entrancy guard correctly prevents feedback loops between MapIsOpen and UpdateIsOpenState.
• Use24HourView logic matches the original TimePickerHandler exactly.
• MauiMaterialContextThemeWrapper.Create(context) usage in the primary constructor is consistent with MauiMaterialButton.
• WeakReference<TimePickerHandler2> in listener classes prevents the listeners from keeping the handler alive beyond its useful lifetime.
• DismissAllowingStateLoss() is used instead of Dismiss(), which is correct for scenarios where the fragment state may have already been saved (e.g., backgrounded activity).

---

[sheiksyedm]

/azp run maui-pr-uitests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[HarishwaranVijayakumar]

Added UI tests and addressed all valid concerns.

[jfversluis]

PR #33646 Code Review: [Android] Implement material3 support for TimePicker

Executive Summary

This PR implements Material3 TimePicker support for Android using conditional handler registration. While the implementation follows good patterns for lifecycle management and memory safety, there are critical bugs and design issues that must be addressed before merge.

Verdict: 🚫 CHANGES REQUESTED

---

Critical Issues (MUST FIX)

1. CRITICAL BUG: Operator Precedence Error in Use24HourView ⚠️

Location: TimePickerHandler2.Android.cs:237-238

// WRONG - Missing parentheses causes incorrect evaluation
bool Use24HourView => VirtualView is not null && (DateFormat.Is24HourFormat(PlatformView?.Context)
        && VirtualView.Format == "t" || VirtualView.Format == "HH:mm");

Problem: Due to C# operator precedence (&& before ||), this evaluates as:

(DateFormat.Is24HourFormat(context) && Format == "t") || (Format == "HH:mm")

Bug scenario: If device is in 12-hour mode but user explicitly sets Format="HH:mm", the expression incorrectly returns true and shows a 24-hour dialog.

Fix:

bool Use24HourView => VirtualView is not null && DateFormat.Is24HourFormat(PlatformView?.Context)
        && (VirtualView.Format == "t" || VirtualView.Format == "HH:mm");

Note: The existing TimePickerHandler.Android.cs has the SAME bug (lines 133-134). This should be fixed in BOTH handlers.

---

2. Code Duplication in TimePickerExtensions.cs

Location: TimePickerExtensions.cs - Lines 8-83

Problem: Four extension methods are duplicated verbatim for MauiTimePicker and MauiMaterialTimePicker:

• UpdateFormat (lines 8-17 vs 14-17)
• UpdateTime (lines 19-28 vs 25-28)
• SetTime (lines 30-36 vs 38-44)
• UpdateTextColor (lines 46-63 vs 66-83)

Why it matters:

• 41 lines of duplicated code (49% of the file)
• Bug fixes must be applied twice (maintenance burden)
• Violates DRY principle

Recommended fix: Use generic extension methods or shared helper:

// Option 1: Generic constraint (preferred if both inherit from EditText)
public static void UpdateTextColor<T>(this T platformTimePicker, ITimePicker timePicker) 
    where T : EditText
{
    // Single implementation
}

// Option 2: Shared helper method
internal static void UpdateTextColorImpl(EditText platformView, ITimePicker timePicker)
{
    // Shared implementation
}

public static void UpdateTextColor(this MauiTimePicker picker, ITimePicker timePicker) 
    => UpdateTextColorImpl(picker, timePicker);

internal static void UpdateTextColor(this MauiMaterialTimePicker picker, ITimePicker timePicker)
    => UpdateTextColorImpl(picker, timePicker);

---

Major Issues (Should Fix)

3. Test Coverage: Limited Negative Testing

Location: Material3TimePickerFeatureTests.cs

Strengths:
✅ 21 comprehensive tests covering font, color, format, culture
✅ Good use of screenshot verification with tolerance
✅ Platform-guarded with #if ANDROID
✅ Proper test ordering

Gaps:

• ❌ No tests for picker dialog dismissal (cancel button, back button)
• ❌ No tests for IsOpen property toggle
• ❌ No tests for rapid open/close scenarios (race conditions)
• ❌ No tests for handler lifecycle (DisconnectHandler while dialog open)
• ❌ Limited keyboard/24-hour format tests

Recommendation: Add edge case tests:

[Test]
public void Material3TimePicker_CancelButton_DoesNotUpdateTime()
{
    // Open picker, tap cancel, verify Time unchanged
}

[Test]  
public void Material3TimePicker_RapidOpenClose_HandlesGracefully()
{
    // Toggle IsOpen rapidly, verify no crashes
}

[Test]
public void Material3TimePicker_DisconnectWhileOpen_CleansUpProperly()
{
    // Open picker, navigate away, verify no leaks
}

---

4. Missing Tests for Use24HourView Logic

Given the operator precedence bug, add explicit tests:

[Test]
public void Material3TimePicker_Format_HHmm_12HourDevice_Shows24HourDialog()
{
    // Set device to 12h mode, Format="HH:mm"
    // Open picker, verify 24h clock shown
}

[Test]
public void Material3TimePicker_Format_t_12HourDevice_Shows12HourDialog()
{
    // Set device to 12h mode, Format="t"
    // Open picker, verify 12h clock shown
}

---

Minor Issues (Good to Fix)

5. Documentation: TODO Comments Need Context

Location: Multiple files

// TODO: Material3: Make it public in .NET 11

Recommendation: Add context about why these are internal:

// TODO: Material3: Make it public in .NET 11
// Currently internal because Material3 feature flag is experimental (defaults to false).
// Once Material3 is stable in .NET 11, promote to public API.

---

6. Consistency: RemapForControls Not Updated

Location: AppHostBuilderExtensions.cs:251

The PR adds conditional handler registration but doesn't update TimePicker.RemapForControls(). Verify if TimePickerHandler2 needs a separate remap or if the existing one applies to both.

---

7. Null Safety: Minor Improvement Opportunity

Location: TimePickerHandler2.Android.cs:157

protected virtual MaterialTimePicker? CreateTimePickerDialog(int hour, int minute)
{
    var dialog = new MaterialTimePicker.Builder()
        .SetHour(hour)
        .SetMinute(minute)
        .SetTimeFormat(Use24HourView ? TimeFormat.Clock24h : TimeFormat.Clock12h)
        .SetInputMode(MaterialTimePicker.InputModeClock)
        .Build();

    if (_positiveButtonClickListener is not null && _dismissListener is not null)
    {
        dialog?.AddOnPositiveButtonClickListener(_positiveButtonClickListener);
        dialog?.AddOnDismissListener(_dismissListener);
    }

    return dialog;
}

Issue: dialog can only be null if Build() throws, not returns null. The dialog?. checks are unnecessary.

Recommendation:

if (_positiveButtonClickListener is not null && _dismissListener is not null && dialog is not null)
{
    dialog.AddOnPositiveButtonClickListener(_positiveButtonClickListener);
    dialog.AddOnDismissListener(_dismissListener);
}

Or better yet, assert non-null if Build() is guaranteed to return non-null:

var dialog = builder.Build() ?? throw new InvalidOperationException("MaterialTimePicker.Builder.Build() returned null");

---

What's Good (Praise) ✅

1. Excellent Handler Lifecycle Management

protected override void DisconnectHandler(MauiMaterialTimePicker platformView)
{
    if (_dialog is not null)
    {
        RemoveListeners();
        if (_dialog.IsAdded) { _dialog.DismissAllowingStateLoss(); }
        _dialog = null;
    }
    
    _positiveButtonClickListener?.Dispose();
    _positiveButtonClickListener = null;
    _dismissListener?.Dispose();
    _dismissListener = null;
    
    platformView.ShowPicker = null;
    platformView.HidePicker = null;
    
    base.DisconnectHandler(platformView);
}

✅ Proper disposal of Java objects
✅ Null-checks before Dispose
✅ Dialog dismissal before cleanup
✅ Clear action delegates

---

2. Proper Use of WeakReference Pattern

internal class MaterialTimePickerPositiveButtonClickListener : Java.Lang.Object, View.IOnClickListener
{
    readonly WeakReference<TimePickerHandler2> _handler;
    
    public void OnClick(View? v)
    {
        if (!_handler.TryGetTarget(out var handler) || handler.VirtualView is null)
            return;
        // ...
    }
}

✅ Prevents memory leaks from Java→C# references
✅ Consistent with MAUI patterns
✅ Proper null-checks after TryGetTarget

---

3. Smart Dialog State Management

void ShowPickerDialog(TimeSpan? time)
{
    // Prevent duplicate dialogs
    if (_dialog is not null && (_dialog.IsVisible || _dialog.IsAdded))
        return;
    
    // FragmentActivity validation
    if (Context?.GetActivity() is not FragmentActivity fragmentActivity ||
        fragmentActivity.IsDestroyed || fragmentActivity.IsFinishing)
        return;
    
    // ...
}

✅ Duplicate dialog prevention
✅ Activity lifecycle checks
✅ Defensive programming

---

4. First Production Use of Material3 Feature Flag

This PR pioneers the RuntimeFeature.IsMaterial3Enabled pattern in handler registration, setting precedent for future Material3 controls. The pattern is clean and well-structured.

---

Architectural Notes

Handler Registration Pattern (First Material3 Usage)

#if ANDROID
if (RuntimeFeature.IsMaterial3Enabled)
    handlersCollection.AddHandler<TimePicker, TimePickerHandler2>();
else
    handlersCollection.AddHandler<TimePicker, TimePickerHandler>();
#else
handlersCollection.AddHandler<TimePicker, TimePickerHandler>();
#endif

Assessment: ✅ Correct approach. Keeps Material3 Android-only without affecting other platforms.

Future concern: As more Material3 handlers are added, consider extracting to a helper method to avoid repetitive #if blocks.

---

Testing Notes

CI Status: ✅ ALL PASSING

• Integration tests: ✅ Pass (Windows, macOS, iOS, Android)
• Build tests: ✅ Pass
• Screenshot tests: ✅ 19 snapshots committed for android-notch-36

Recommendation: Before merge, run UI tests with Material3 enabled to verify the handler is actually being used.

---

Summary of Required Changes

Must Fix (Blocking)

1. ✅ Fix operator precedence in Use24HourView (both handlers)
2. ✅ Eliminate code duplication in TimePickerExtensions.cs

Should Fix (Strongly Recommended)

3. ✅ Add negative testing (cancel, rapid toggle, disconnect while open)
4. ✅ Add explicit tests for Use24HourView logic with different formats

Nice to Fix (Optional)

5. Improve TODO comment context
6. Verify RemapForControls applicability
7. Simplify null-checks in CreateTimePickerDialog

---

Final Recommendation

Status: 🚫 CHANGES REQUESTED

The implementation demonstrates solid understanding of Android handler patterns, lifecycle management, and memory safety. However, the operator precedence bug is critical and will cause incorrect behavior in production. The code duplication significantly impacts maintainability.

Action items:

1. Fix the Use24HourView operator precedence bug (CRITICAL)
2. Refactor TimePickerExtensions to eliminate duplication (HIGH)
3. Add edge case tests for dialog dismissal and lifecycle (MEDIUM)
4. Add Use24HourView format tests (MEDIUM)

Once these are addressed, this will be a strong contribution to Material3 support.

---

For the Team

This is the first Material3 handler using the feature flag. Consider:

• Documenting this pattern in contributor guidelines
• Creating a Material3 handler template for future controls
• Planning Material3 migration strategy for other pickers (DatePicker, etc.)

Great work by @HarishwaranVijayakumar (Syncfusion) on this foundational Material3 implementation! 🎉

The core architecture is excellent - just needs these bug fixes and improvements before shipping.

[jfversluis]

AI Multi-Model Review — PR #33646

Reviewed by: Claude Sonnet 4.5, Claude Opus 4.6, GPT 5.1

Overall

Solid Material3 TimePicker implementation — good handler lifecycle, memory safety (WeakReference listeners), and consistent with existing Material3 patterns. 21 comprehensive screenshot tests. This sets a good precedent as the first RuntimeFeature.IsMaterial3Enabled handler registration.

However, there are a few issues that should be addressed before merge.

---

🔴 Operator Precedence Ambiguity in Use24HourView

File: TimePickerHandler2.Android.cs line 237-238

bool Use24HourView => VirtualView is not null && (DateFormat.Is24HourFormat(PlatformView?.Context)
    && VirtualView.Format == "t" || VirtualView.Format == "HH:mm");

Because && binds tighter than ||, this evaluates as:

VirtualView is not null && ((Is24HourFormat && Format == "t") || Format == "HH:mm")

This means Format == "HH:mm" always results in 24h mode regardless of system setting. That's arguably correct behavior (HH:mm is explicitly 24h format), but the parenthesization is misleading — the opening ( after the first && makes it look like Is24HourFormat should govern both branches.

Note: This is copied from the original TimePickerHandler.Android.cs (same exact pattern), so it's pre-existing. But since this is new code, please add explicit parentheses to clarify intent:

// Option A: If HH:mm should always be 24h (likely intended):
bool Use24HourView => VirtualView is not null
    && ((DateFormat.Is24HourFormat(PlatformView?.Context) && VirtualView.Format == "t")
        || VirtualView.Format == "HH:mm");

// Option B: If Is24HourFormat should gate both:
bool Use24HourView => VirtualView is not null
    && DateFormat.Is24HourFormat(PlatformView?.Context)
    && (VirtualView.Format == "t" || VirtualView.Format == "HH:mm");

---

🟡 Unused Variable in en-US Culture Test

File: Material3TimePickerFeatureTests.cs — Material3TimePicker_SetCulture_enUS_VerifyTimeFormat

var cultureFormatText = App.WaitForElement("CultureFormatLabel").GetText();
VerifyScreenshot(...);

cultureFormatText is assigned but never asserted on. The ar-EG and ja-JP tests both assert on it. Either add an assertion (e.g., Assert.That(cultureFormatText, Is.EqualTo("Culture: en-US, Time: 5:30 AM"))) or remove the unused variable.

---

🟡 Missing Newline at End of File

Both TimePickerHandler2.Android.cs and TimePickerExtensions.cs are missing a trailing newline (diff shows \ No newline at end of file).

---

🟢 Extension Method Duplication (Non-blocking)

SetTime, UpdateFormat, UpdateTime, and UpdateTextColor are character-for-character identical between MauiTimePicker and MauiMaterialTimePicker overloads (~40 lines). Both types inherit from EditText, so a shared internal helper targeting EditText could reduce this. Not blocking since the TODO comments indicate these will be consolidated when made public in .NET 11.

---

✅ What's Good

• Handler lifecycle: Thorough DisconnectHandler — removes listeners, DismissAllowingStateLoss(), disposes Java objects, nulls delegates
• Memory safety: WeakReference<TimePickerHandler2> in both listener classes
• Dialog management: Duplicate prevention (IsVisible || IsAdded), FragmentActivity IsDestroyed/IsFinishing checks
• _isUpdatingIsOpen guard: Prevents re-entrant feedback loops between MapIsOpen and UpdateIsOpenState
• MauiMaterialContextThemeWrapper: Follows established pattern from MauiMaterialButton
• Tests: 21 screenshot tests covering fonts, formats, cultures (en-US, ar-EG, ja-JP), states

---

Reviewed using multiple AI models for thoroughness.

[HarishwaranVijayakumar]

@jfversluis, Addressed the valid concerns.