UIButton + UIMenu Picker for iOS/Mac Catalyst#35281
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35281Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35281" |
|
@dotnet-policy-service agree |
ethanl21
changed the title
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
I’m not sure this is the best approach given the limitations - it could introduce quite a few regressions. Perhaps a safer option would be to implement this as a new handler and let developers opt in, rather than replacing the existing behavior. Something similar to how the CollectionView transition was handled on iOS could work well here.
Before doing it though, can you pleas create a demo of how it works on iOS and MacOs and verify if accessibility (screen reader, keyboard navigation) works?
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 15 findings
See inline comments for details.
MauiBot
added
s/agent-changes-requested
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 15 findings
See inline comments for details.
…ory fails before tests
When a test category fails because the build or deploy crashed before any
test could run (e.g. CS0246 missing namespace, RS0016 PublicAPI errors),
the AI summary table previously showed '0/1 ✓' — the green-checkmark
'all passed' branch — because no per-test failures were parsed. That's
visually misleading: the row is FAILED but the cell looks healthy.
Two fixes:
1. Tests column distinguishes 'category failed AND no per-test failures
parsed' from 'all tests passed':
- 'build/deploy failed' (no tests at all)
- '0/1 — build/deploy failed before per-test results' (some discovered)
2. New optional 'build_tail' field captures the last 30 lines of stdout
when a category fails with zero per-test failures. The Failed test
details collapsible section then renders it in a code block so
reviewers see the actual compiler error / build crash inline,
instead of having to download the full CopilotLogs artifact.
This was discovered while running the regression-check pipeline against
PRs #35110 (142 RS0016 PublicAPI errors), #35281 (CS0246 NSAttributedString
missing for catalyst), and #35358 — all reported as '0/1 ✓' before the fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 17 findings
See inline comments for details.
| @@ -1,5 +1,5 @@ | |||
| #if __IOS__ || MACCATALYST | |||
| using PlatformView = Microsoft.Maui.Platform.MauiPicker; | |||
| using PlatformView = UIKit.UIButton; | |||
There was a problem hiding this comment.
[critical] Public API Surface — binary breaking change — IPickerHandler.PlatformView has changed type from Microsoft.Maui.Platform.MauiPicker! to UIKit.UIButton! on net-ios and net-maccatalyst (see PublicAPI.Unshipped.txt *REMOVED* lines). Any third-party effect, behavior, library, or sample doing ((IPickerHandler)handler).PlatformView.Text = … or casting to MauiPicker will throw InvalidCastException at runtime — and recompiles will fail. Even though the entries are in Unshipped, MauiPicker-typed PlatformView shipped to customers in earlier .NET MAUI releases. This deserves an explicit BC-break call-out in the PR description, design review sign-off, and ideally a migration note. Consider whether keeping MauiPicker as a thin UIView wrapper that hosts the UIButton would preserve binary compatibility.
| .FireAndForget(); | ||
| } | ||
| }); | ||
| PlatformView.Menu = UIMenu.Create("Picker Menu", menuElements); |
There was a problem hiding this comment.
[critical] Logic & Correctness — root cause of the SelectedIndex-from-XAML bug — With ChangesSelectionAsPrimaryAction = true, UIButton derives its displayed title from whichever UIAction in Menu has State == UIMenuElementState.On, and ignores SetAttributedTitle(...). This loop never sets .State on any action, so on first display UIKit sees a menu with all actions in .Off state and falls back to the button's intrinsic title (often the action[0] title or empty). The XAML-time MapSelectedIndex → UpdateSelectedText → SetAttributedTitle therefore has no visual effect. Runtime taps work because UIKit auto-flips the tapped action's State to .On. Fix: in UpdateMenu apply action.State = (i == VirtualView.SelectedIndex) ? UIMenuElementState.On : UIMenuElementState.Off while building the array, and in MapSelectedIndex either rebuild the menu or update State on the cached UIAction references (UIAction.State is mutable). Once that is in place, SetAttributedTitle becomes redundant for the selected-item display — keep it only for the title/empty fallback.
| bool isTitle = selectedIndex < 0 || selectedIndex >= VirtualView.GetCount(); | ||
| var text = isTitle ? (VirtualView.Title ?? string.Empty) : VirtualView.GetItem(selectedIndex); | ||
|
|
||
| PlatformView.SetAttributedTitle(CreateAttributedString(text, isTitle), UIControlState.Normal); |
There was a problem hiding this comment.
[major] Logic & Correctness — UpdateSelectedText calls SetAttributedTitle(... UIControlState.Normal), but with ChangesSelectionAsPrimaryAction = true this title is overwritten by UIKit any time the menu re-renders. Title/font/color/character-spacing changes from the cross-platform mappers therefore land only when there is no menu (count == 0). After fixing the State bug above, route font/color/character-spacing into either (a) a UIAction.Image/AttributedTitle per item — UIAction does not honour custom fonts well — or (b) PlatformView.Configuration (UIButtonConfiguration) which is the supported iOS 15+ way to style a menu-driven button. The current SetAttributedTitle path will silently no-op for the common case of "a Picker that already has items and a selection".
| popoverPresentation.SourceRect = uITextField.Bounds; | ||
| var index = i; | ||
| var title = VirtualView.GetItem(index); | ||
| var action = UIAction.Create(title, null, null, _ => OnMenuItemSelected(index)); |
There was a problem hiding this comment.
[major] Memory Leak Prevention — Each UIAction lambda captures index plus this (via OnMenuItemSelected). The retain chain is: Handler → PlatformView (UIButton, strong via base handler) → Menu (UIMenu) → UIAction[] → managed closure → Handler. Under iOS reference-counting GC this is a hard cycle and will leak the handler + virtual view. Fix options: (a) make OnMenuItemSelected static and pass the index via UIAction.Identifier, looking up the handler weakly through the sender; (b) use a WeakReference<PickerHandler> captured by the closure (matching the previous MauiPickerProxy pattern this PR removed); or (c) clear PlatformView.Menu = null in DisconnectHandler. At minimum (c) must be done — DisconnectHandler currently only nulls _fontManager.
| { | ||
| _fontManager = null; | ||
| base.DisconnectHandler(platformView); | ||
| } |
There was a problem hiding this comment.
[major] Handler lifecycle — DisconnectHandler is incomplete — DisconnectHandler only nulls _fontManager. It does not: (1) clear PlatformView.Menu = null (see retain-cycle finding); (2) clear PlatformView.Configuration / attributed title; (3) reverse any ConnectHandler side-effects. Add platformView.Menu = null; and platformView.SetAttributedTitle(null, UIControlState.Normal); before base.DisconnectHandler(platformView). Also note that Shell tab switching disconnects/reconnects handlers — re-ConnectHandler will rebuild the menu from scratch, which is fine, but only if disconnect actually drops the previous menu graph.
| .FireAndForget(); | ||
| } | ||
| }); | ||
| PlatformView.Menu = UIMenu.Create("Picker Menu", menuElements); |
There was a problem hiding this comment.
[minor] Hard-coded UIMenu title "Picker Menu" — UIMenu.Create("Picker Menu", menuElements) — on MacCatalyst this title can render as a section header in the popup. Pass string.Empty (the convention for inline menus), or pass VirtualView.Title ?? string.Empty if a heading is desired.
| { | ||
| if (_pickerView.Model != null) | ||
| var font = VirtualView.Font; | ||
| var fontManager = _fontManager ?? ((IElementHandler)this).GetRequiredService<IFontManager>(); |
There was a problem hiding this comment.
[minor] Inconsistent service-resolution pattern — _fontManager is cached in ConnectHandler (line 116), but CreateAttributedString (line 92) still falls back to ((IElementHandler)this).GetRequiredService<IFontManager>() if _fontManager is null. Either trust ConnectHandler and dereference the field directly (it cannot be null while VirtualView is non-null), or drop the field entirely and resolve once per call. Mixing both patterns is confusing and the fallback path implies the cache might miss — which would itself be a lifecycle bug worth fixing rather than papering over.
| return; | ||
|
|
||
| VirtualView.SelectedIndex = index; | ||
| UpdateSelectedText(); |
There was a problem hiding this comment.
[minor] Redundant work — UpdateSelectedText called twice on tap — OnMenuItemSelected sets VirtualView.SelectedIndex = index, which fires MapSelectedIndex (PickerHandler.iOS.cs:155) → UpdateSelectedText, then this method calls UpdateSelectedText directly again. With ChangesSelectionAsPrimaryAction = true UIKit also updates the title automatically. After fixing the State-based selection (see line 53 finding), drop the explicit UpdateSelectedText() call here and rely on the mapper.
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
[minor] Coding style — missing newline at EOF — File ends with } and no trailing newline (per \ No newline at end of file in the diff). MAUI repo convention is final newline. Same applies to src/Core/src/Platform/iOS/PickerExtensions.cs.
| new MauiPicker(null) { BorderStyle = UITextBorderStyle.RoundedRect }; | ||
|
|
||
| void DisplayAlert(MauiPicker uITextField, int selectedIndex) | ||
| void UpdateMenu() |
There was a problem hiding this comment.
[moderate] Regression Prevention & Test Coverage — This PR replaces the entire iOS/MacCatalyst Picker platform view and removes (a) the touch-dismiss gesture, (b) accessibility focus notifications (PostAccessibilityFocusNotification), (c) MacCatalyst popover positioning logic, (d) UIPickerView model/source, (e) Done-button accessory, and (f) MauiPickerProxy event hookup — yet no new device tests or UI tests are added in this PR. Given the author's known XAML-startup bug already, and the long tail of regressed features (alignment, IsOpen, UpdateMode, IsFocused, Opened/Closed events), this needs at minimum: a device test that asserts SetSelectedIndex(2) followed by Show() displays item 2; a regression test for Picker.IsOpen = true programmatically; a memory-leak test (Picker page → navigate away → assert WeakReference to handler is collected). Re-check git blame on the previous file for fixed-issue numbers (the deleted comments mention VoiceOver issues with EditingDidEnd and a macOS title-padding workaround) and add tests guarding those scenarios before merging.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 16 findings
See inline comments for details.
| @@ -1,5 +1,5 @@ | |||
| #if __IOS__ || MACCATALYST | |||
| using PlatformView = Microsoft.Maui.Platform.MauiPicker; | |||
| using PlatformView = UIKit.UIButton; | |||
There was a problem hiding this comment.
[critical] Public API Surface — binary breaking change — IPickerHandler.PlatformView has changed type from Microsoft.Maui.Platform.MauiPicker! to UIKit.UIButton! on net-ios and net-maccatalyst (see PublicAPI.Unshipped.txt *REMOVED* lines). Any third-party effect, behavior, library, or sample doing ((IPickerHandler)handler).PlatformView.Text = … or casting to MauiPicker will throw InvalidCastException at runtime — and recompiles will fail. Even though the entries are in Unshipped, MauiPicker-typed PlatformView shipped to customers in earlier .NET MAUI releases. This deserves an explicit BC-break call-out in the PR description, design review sign-off, and ideally a migration note. Consider whether keeping MauiPicker as a thin UIView wrapper that hosts the UIButton would preserve binary compatibility.
| .FireAndForget(); | ||
| } | ||
| }); | ||
| PlatformView.Menu = UIMenu.Create("Picker Menu", menuElements); |
There was a problem hiding this comment.
[critical] Logic & Correctness — root cause of the SelectedIndex-from-XAML bug — With ChangesSelectionAsPrimaryAction = true, UIButton derives its displayed title from whichever UIAction in Menu has State == UIMenuElementState.On, and ignores SetAttributedTitle(...). This loop never sets .State on any action, so on first display UIKit sees a menu with all actions in .Off state and falls back to the button's intrinsic title (often the action[0] title or empty). The XAML-time MapSelectedIndex → UpdateSelectedText → SetAttributedTitle therefore has no visual effect. Runtime taps work because UIKit auto-flips the tapped action's State to .On. Fix: in UpdateMenu apply action.State = (i == VirtualView.SelectedIndex) ? UIMenuElementState.On : UIMenuElementState.Off while building the array, and in MapSelectedIndex either rebuild the menu or update State on the cached UIAction references (UIAction.State is mutable). Once that is in place, SetAttributedTitle becomes redundant for the selected-item display — keep it only for the title/empty fallback.
| bool isTitle = selectedIndex < 0 || selectedIndex >= VirtualView.GetCount(); | ||
| var text = isTitle ? (VirtualView.Title ?? string.Empty) : VirtualView.GetItem(selectedIndex); | ||
|
|
||
| PlatformView.SetAttributedTitle(CreateAttributedString(text, isTitle), UIControlState.Normal); |
There was a problem hiding this comment.
[major] Logic & Correctness — UpdateSelectedText calls SetAttributedTitle(... UIControlState.Normal), but with ChangesSelectionAsPrimaryAction = true this title is overwritten by UIKit any time the menu re-renders. Title/font/color/character-spacing changes from the cross-platform mappers therefore land only when there is no menu (count == 0). After fixing the State bug above, route font/color/character-spacing into either (a) a UIAction.Image/AttributedTitle per item — UIAction does not honour custom fonts well — or (b) PlatformView.Configuration (UIButtonConfiguration) which is the supported iOS 15+ way to style a menu-driven button. The current SetAttributedTitle path will silently no-op for the common case of "a Picker that already has items and a selection".
| popoverPresentation.SourceRect = uITextField.Bounds; | ||
| var index = i; | ||
| var title = VirtualView.GetItem(index); | ||
| var action = UIAction.Create(title, null, null, _ => OnMenuItemSelected(index)); |
There was a problem hiding this comment.
[major] Memory Leak Prevention — Each UIAction lambda captures index plus this (via OnMenuItemSelected). The retain chain is: Handler → PlatformView (UIButton, strong via base handler) → Menu (UIMenu) → UIAction[] → managed closure → Handler. Under iOS reference-counting GC this is a hard cycle and will leak the handler + virtual view. Fix options: (a) make OnMenuItemSelected static and pass the index via UIAction.Identifier, looking up the handler weakly through the sender; (b) use a WeakReference<PickerHandler> captured by the closure (matching the previous MauiPickerProxy pattern this PR removed); or (c) clear PlatformView.Menu = null in DisconnectHandler. At minimum (c) must be done — DisconnectHandler currently only nulls _fontManager.
| { | ||
| _fontManager = null; | ||
| base.DisconnectHandler(platformView); | ||
| } |
There was a problem hiding this comment.
[major] Handler lifecycle — DisconnectHandler is incomplete — DisconnectHandler only nulls _fontManager. It does not: (1) clear PlatformView.Menu = null (see retain-cycle finding); (2) clear PlatformView.Configuration / attributed title; (3) reverse any ConnectHandler side-effects. Add platformView.Menu = null; and platformView.SetAttributedTitle(null, UIControlState.Normal); before base.DisconnectHandler(platformView). Also note that Shell tab switching disconnects/reconnects handlers — re-ConnectHandler will rebuild the menu from scratch, which is fine, but only if disconnect actually drops the previous menu graph.
| .FireAndForget(); | ||
| } | ||
| }); | ||
| PlatformView.Menu = UIMenu.Create("Picker Menu", menuElements); |
There was a problem hiding this comment.
[minor] Hard-coded UIMenu title "Picker Menu" — UIMenu.Create("Picker Menu", menuElements) — on MacCatalyst this title can render as a section header in the popup. Pass string.Empty (the convention for inline menus), or pass VirtualView.Title ?? string.Empty if a heading is desired.
| { | ||
| if (_pickerView.Model != null) | ||
| var font = VirtualView.Font; | ||
| var fontManager = _fontManager ?? ((IElementHandler)this).GetRequiredService<IFontManager>(); |
There was a problem hiding this comment.
[minor] Inconsistent service-resolution pattern — _fontManager is cached in ConnectHandler (line 116), but CreateAttributedString (line 92) still falls back to ((IElementHandler)this).GetRequiredService<IFontManager>() if _fontManager is null. Either trust ConnectHandler and dereference the field directly (it cannot be null while VirtualView is non-null), or drop the field entirely and resolve once per call. Mixing both patterns is confusing and the fallback path implies the cache might miss — which would itself be a lifecycle bug worth fixing rather than papering over.
| return; | ||
|
|
||
| VirtualView.SelectedIndex = index; | ||
| UpdateSelectedText(); |
There was a problem hiding this comment.
[minor] Redundant work — UpdateSelectedText called twice on tap — OnMenuItemSelected sets VirtualView.SelectedIndex = index, which fires MapSelectedIndex (PickerHandler.iOS.cs:155) → UpdateSelectedText, then this method calls UpdateSelectedText directly again. With ChangesSelectionAsPrimaryAction = true UIKit also updates the title automatically. After fixing the State-based selection (see line 53 finding), drop the explicit UpdateSelectedText() call here and rely on the mapper.
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
[minor] Coding style — missing newline at EOF — File ends with } and no trailing newline (per \ No newline at end of file in the diff). MAUI repo convention is final newline. Same applies to src/Core/src/Platform/iOS/PickerExtensions.cs.
| new MauiPicker(null) { BorderStyle = UITextBorderStyle.RoundedRect }; | ||
|
|
||
| void DisplayAlert(MauiPicker uITextField, int selectedIndex) | ||
| void UpdateMenu() |
There was a problem hiding this comment.
[moderate] Regression Prevention & Test Coverage — This PR replaces the entire iOS/MacCatalyst Picker platform view and removes (a) the touch-dismiss gesture, (b) accessibility focus notifications (PostAccessibilityFocusNotification), (c) MacCatalyst popover positioning logic, (d) UIPickerView model/source, (e) Done-button accessory, and (f) MauiPickerProxy event hookup — yet no new device tests or UI tests are added in this PR. Given the author's known XAML-startup bug already, and the long tail of regressed features (alignment, IsOpen, UpdateMode, IsFocused, Opened/Closed events), this needs at minimum: a device test that asserts SetSelectedIndex(2) followed by Show() displays item 2; a regression test for Picker.IsOpen = true programmatically; a memory-leak test (Picker page → navigate away → assert WeakReference to handler is collected). Re-check git blame on the previous file for fixed-issue numbers (the deleted comments mention VoiceOver issues with EditingDidEnd and a macOS title-padding workaround) and add tests guarding those scenarios before merging.
🤖 AI Summary
📊 Review Session —
|
| Category | Result | Tests | Duration | Notes |
|---|---|---|---|---|
ViewBaseTests |
⏭️ SKIPPED | — | 0s | Runner threw an exception |
Failures here are informational only — they do not block the gate or affect try-fix candidate scoring.
🔍 Pre-Flight — Context & Validation
Pre-Flight — PR #35281
Summary
Title: UIButton + UIMenu Picker for iOS/Mac Catalyst
Author: ethanl21 (community)
Base: main
Files changed: 6 (+136 / -412)
Linked issues: #23999 (proposal), #10208 (Mac Catalyst Picker bug)
Goal
Replace the iOS/MacCatalyst Picker platform implementation (currently MauiPicker / UITextField + UIPickerView) with a UIButton whose Menu is a UIMenu, using ShowsMenuAsPrimaryAction = true and ChangesSelectionAsPrimaryAction = true. This is the approach proposed in #23999 and aligns iOS with Android/Windows behavior; it also fixes Mac Catalyst (#10208) where the existing UIAlertController-hosted UIPickerView was broken under the Mac idiom.
File classification
| File | Type | Notes |
|---|---|---|
src/Core/src/Handlers/Picker/IPickerHandler.cs |
Public-API alias | PlatformView typedef changed MauiPicker → UIButton |
src/Core/src/Handlers/Picker/PickerHandler.cs |
Public-API alias | same typedef change |
src/Core/src/Handlers/Picker/PickerHandler.iOS.cs |
Handler core (iOS/Catalyst) | Full rewrite (+99 / -396); removed PickerSource, MauiPickerProxy, alert/popover Catalyst path |
src/Core/src/Platform/iOS/PickerExtensions.cs |
Platform helpers | Trimmed UIPickerView selection sync; now only mirrors SelectedIndex to virtual view |
src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt |
Public API ledger | PlatformView getter return type changed, PickerSource removed |
src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt |
Public API ledger | mirror of above |
Tests
- No new tests in this PR.
- Gate already reported: "
⚠️ SKIPPED — no tests detected in this PR. Consider suggesting the author add tests."
Risk assessment / blast radius
- Public-API breaking change. The
PlatformViewgetter onIPickerHandler(a public interface) changes return type fromMauiPickertoUIButton. Any third-party code that has donepickerHandler.PlatformView.Text = ...or similar will fail to compile. Mitigations exist viaPublicAPI.Unshipped.txtflagging, but the consumer impact is real. PickerSourcepublic class is removed entirely. It is a public type — removal is a binary-breaking change that affects any downstream extension.- Feature regressions explicitly acknowledged by author:
HorizontalTextAlignment/VerticalTextAlignment— no-op (was working before).TextTransform— not implemented.IsOpen/Opened/Closed— not supported (the events / property silently never fire).UpdateMode— silently dropped.
- Known bug acknowledged by author: SelectedIndex set via XAML binding sometimes ignored for the first two pickers in the sample. Author says "any help tracking it down would be appreciated" — i.e., the PR ships a known regression.
- MapTitle / MapTitleColor / MapFont / MapTextColor / MapCharacterSpacing / MapSelectedIndex are all collapsed into
UpdateSelectedText()calls. There is no menu rebuild when item titles or font change — only the button title is refreshed. This means the menu items themselves never reflect title/font/color changes afterItemsis first populated; menu is only rebuilt byReload(i.e.,MapItems). OnMenuItemSelectedsetspicker.IsFocused = falseafter every selection, but never setsIsFocused = truewhile the menu is open —IsFocusedsemantics on iOS Picker silently change.FontManageris fetched inConnectHandlerand cached;_fontManager = nullinDisconnectHandler.CreateAttributedStringhas a fallback toGetRequiredServiceif_fontManageris null — defensive but redundant.Reloadpath runsUpdateMenu+UpdateSelectedTextand rebuilds fullUIMenuElement[]allocations each items-change — acceptable.UIMenu.Create("Picker Menu", ...)always uses the literal title "Picker Menu" — never thepicker.Title. TheTitleproperty is now only surfaced as the placeholder text on the button whenSelectedIndex < 0.- Removed
MapUnfocusfor MacCatalyst — programmaticUnfocus()no longer dismisses anything (acceptable since menu auto-dismisses, but inconsistent with old API contract). PickerExtensions.csUpdateTitleColornow identical toUpdateTitle(UpdatePickerTitle(picker)) — was previously identical but appears to still apply to theMauiPickerAndroid variant. Functional behavior unchanged for Android.EditingDidBegin/EditingDidEndevents on the oldMauiPickerare gone — anything usingFocused/Unfocusedlifecycle events on the iOS Picker is affected. This is observable viaElement.Focused/Unfocused.
Failure modes worth probing in try-fix
- Public API breakage. The right approach may be to keep
MauiPicker(or a new internalMauiPickerButton : UIButton) as the PlatformView type, so the public surfacePlatformViewkeeps a Maui-owned type that consumers can rely on. Several Maui handlers do this exact pattern (MauiTextField,MauiSearchBar, etc.). - Known SelectedIndex-in-XAML bug. Likely caused by
UpdateMenu()being called only inConnectHandler, before items are bound, or by ordering ofMapSelectedIndexvsMapItems. Worth investigating mapper order. - HorizontalTextAlignment/VerticalTextAlignment regression. UIButton supports
ContentHorizontalAlignment/ContentVerticalAlignmentandTitleLabel.TextAlignment— these CAN be implemented with no extra subclassing. - TextTransform regression. Can be implemented at the
CreateAttributedStringboundary by transformingtextbefore constructingNSAttributedString. - Menu rebuild on title/font/color changes.
MapFont/MapTitleColoretc. should callUpdateMenutoo, otherwise menu items never reflect later style changes.
Code-review verdict (advisory for try-fix)
NEEDS_CHANGES (confidence: high)
Top concerns (advisory hints to try-fix):
- ❌ Breaking change: public
IPickerHandler.PlatformViewgetter return type changed (MauiPicker→UIButton) and publicPickerSourceremoved — re-using a Maui-owned wrapper (e.g.MauiPickerthat wraps aUIButton, orMauiPickerButton : UIButton) would let the rewrite proceed without breaking the public surface. - ❌ Feature regressions silently introduced:
HorizontalTextAlignment,VerticalTextAlignment,TextTransform,IsOpen/Opened/Closed,UpdateMode. UIButton supports the alignment & transform cases natively. - ❌ Acknowledged regression: SelectedIndex from XAML binding sometimes ignored (author asked for help in PR description).
⚠️ MapFont/MapTitleColor/MapCharacterSpacingonly update the button title, not the menu items themselves. Menu items keep their original UIAction title attributes.⚠️ UIMenu.Create("Picker Menu", ...)hardcodes the menu title rather than usingpicker.Title.⚠️ No new tests; PR ships with a known regression by author's own admission.
This verdict feeds into try-fix as advisory hints.
🔧 Fix — Analysis & Comparison
Try-Fix — PR #35281 — Aggregated narrative
Candidates explored (4 dimensions)
| # | Dimension | Approach | Diff scope | Status |
|---|---|---|---|---|
| try-fix-1 | Handler lifecycle / public API | Keep MauiPicker as the PlatformView typedef; change its definition to MauiPicker : UIButton so binary compat is preserved |
Same files as PR, but no API ledger removals; adds [Obsolete] shim for PickerSource |
Analysis only (gate skipped) — would still inherit PR's MapUnfocus build break unless that is also restored |
| try-fix-2 | Platform iOS / modern config | Replace deprecated ContentEdgeInsets + manual Layer.BorderColor with UIButtonConfiguration.Bordered() so dark-mode + HIG sizing work |
Only PickerHandler.iOS.cs CreatePlatformView body |
Analysis only — same MapUnfocus caveat |
| try-fix-3 | Regression prevention / surgical | Leave iOS path unchanged; replace only the broken Mac Catalyst UIAlertController modal with a UIMenu overlay |
PickerHandler.iOS.cs #if MACCATALYST branch only; no public API changes |
Analysis only — lowest blast radius |
| try-fix-4 | Accessibility + event lifecycle | Introduce a MauiPickerButton : UIButton subclass that bridges UIKit's menu lifecycle to MAUI's IsOpen/Opened/Closed and re-emits PostAccessibilityFocusNotification; expose AccessibilityValue |
Adds new MauiPickerButton.cs; modifies PickerHandler.iOS.cs to use it |
Analysis only — fixes the regressions of #33152, IsOpen events, and AccessibilityValue |
Cross-pollination
| Model | Round | New ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | Suggests combining try-fix-1 (API stability) + try-fix-4 (subclass) into one wrapper class MauiPicker : UIButton that owns both binary-compat AND the menu lifecycle bridge — single class achieves two dimensions. |
| claude-sonnet-4.6 | 2 | No new idea | Notes that try-fix-2 (UIButtonConfiguration) is orthogonal to all others and should be folded into the winner regardless of which is picked. |
| gpt-5.3-codex | 2 | Yes | Suggests an even more minimal candidate: only restore MapUnfocus as a no-op (or implement it to clear the button's focus) — this would make the PR-as-submitted at least compile, without addressing any other concern. Falls under "pr-plus-reviewer" rather than a new try-fix. |
| gemini-3-pro-preview | 2 | No | Concurs with claude-opus's merged proposal. |
Round 3 — all models reply "NO NEW IDEAS". Exhausted.
Empirical evidence (the only verifiable signal)
- PR as-submitted has a confirmed compile break on MacCatalyst.
src/Core/src/Handlers/Picker/PickerHandler.cs:43referencesMapUnfocusfor#elif MACCATALYST, but the rewrite deleted theMapUnfocusdefinition. This is a deterministic build break, verifiable by reading the file. - Author explicitly acknowledged an unresolved bug in the PR description ("the top two Pickers ... using
SelectedIndexin XAML do not appear to respect the set index"). Expert-reviewer root-caused this to mapper ordering + missingUIMenuElementState.Onpropagation. - No automated tests are included to demonstrate the rewrite preserves prior behavior (Gate is SKIPPED).
Selected fix
pr-plus-reviewer — i.e. the PR's fix WITH the 5 actionable follow-ups from the expert reviewer applied:
- Restore
MapUnfocus(fixes the MacCatalyst build break — without this NOTHING merges). - Clear
Menu+ attributed title inDisconnectHandler; convertUIActionclosures to a static +WeakReference<PickerHandler>pattern (memory leak fix). - Use
UIButtonConfiguration.Bordered()instead of legacy ContentEdgeInsets + manual border (dark-mode fix; try-fix-2 idea folded in). - Build
UIMenufromMapSelectedIndex(not just fromMapItems) and tag the selectedUIAction.State = UIMenuElementState.On— fixes the author-acknowledged XAML-initial-SelectedIndex bug. - Replace bare
UIButtonwithMauiPickerButton : UIButtonthat overrides menu open/close to fireIsOpen/Opened/Closedand callPostAccessibilityFocusNotification. SetAccessibilityValueto the current item text. (try-fix-4 idea folded in.)
Why not pure try-fix-1/2/3/4 alone: each dimension's candidate is incomplete on its own. Only the combined "pr + reviewer feedback" candidate addresses all five reviewer-identified must-fix items at once, and it does so as additive edits the original author can make in one commit — minimal disruption to the PR.
Why not pr (raw): it has a verified MacCatalyst build break and ships with an author-acknowledged regression.
Exhausted: Yes.
📋 Report — Final Recommendation
Report — PR #35281 — Comparative Analysis
Candidates evaluated
| Candidate | Source | Build status | Public-API safe | Fixes acknowledged regressions | Net assessment |
|---|---|---|---|---|---|
pr |
PR #35281 as submitted | ❌ MacCatalyst build break (PickerHandler.cs:43 references deleted MapUnfocus) |
❌ *REMOVED* of PickerSource; PlatformView getter type changed |
❌ ships with author-acknowledged SelectedIndex-from-XAML bug | Cannot merge as-is |
pr-plus-reviewer |
PR + 5 reviewer follow-up edits | ✅ build break resolved | PickerSource, but follow-up adds [Obsolete] shim recommendation |
✅ all five top-severity reviewer findings addressed in one delta | Best landing path |
try-fix-1 |
Handler lifecycle / API stability dimension | ✅ fully preserves public API | ❌ does not fix feature regressions | Useful complement, insufficient alone | |
try-fix-2 |
Modern UIButtonConfiguration | ❌ same removals as PR | ❌ rendering-only fix | Useful complement, insufficient alone | |
try-fix-3 |
Surgical: fix only MacCatalyst | ✅ buildable; no public API changes | ✅ none broken | Lowest-risk alternative if author/owners decide the iOS rewrite should be deferred | |
try-fix-4 |
Accessibility + menu lifecycle subclass | ❌ same removals as PR | ✅ restores IsOpen/Opened/Closed + VoiceOver focus | Useful complement, insufficient alone |
Ranking (per "failed-tests-rank-lower" rule)
pr-plus-reviewer— only candidate that both builds and addresses all critical reviewer findings.try-fix-3— buildable and minimal; ideal if owners want to defer the iOS modernization.try-fix-1/try-fix-4— each fixes one dimension and would themselves need to also restoreMapUnfocusto build.try-fix-2— buildable with the MapUnfocus fix; rendering improvement only.pr— last because it has a confirmed compile break onnet-maccatalyst.
Note: Gate is SKIPPED (no tests in PR), so we have no test-pass data for any candidate. The "build break" status of pr is verified by static inspection (CompileCommand mapping table references MapUnfocus symbol which the rewrite deleted) and is the strongest empirical signal available.
Winner
pr-plus-reviewer — apply the five reviewer follow-ups on top of the PR. This:
- Resolves the deterministic MacCatalyst compile break.
- Fixes the author-acknowledged SelectedIndex-from-XAML bug via mapper ordering +
UIMenuElementState.Onpropagation. - Restores the accessibility behaviour that [iOS] Fix VoiceOver focus not shifting to Picker/DatePicker/TimePicker popups #33152 introduced.
- Restores
IsOpen/Opened/Closedevents that the rewrite silently dropped. - Switches to a modern, theming-correct
UIButtonConfiguration.
These five items can be applied as one additional commit by the PR author; no full re-architecture is needed.
Recommendation
- DO NOT merge as-is — the
net-maccatalystTFM will not compile. - Comment summarizing the five required edits to the author.
- Ask the author to add at least one device test (open the menu programmatically, validate
IsOpenandSelectedIndexround-trip) since the PR currently has none. - After the five follow-ups are pushed, re-run the device-tests pipeline (
maui-pr-devicetests) on bothnet-iosandnet-maccatalystto validate behavior empirically.
Full inline findings
See CustomAgentLogsTmp/PRState/35281/PRAgent/inline-findings.json (14 findings: 1 error, 7 warnings, 6 suggestions) and CustomAgentLogsTmp/PRState/35281/PRAgent/expert-pr-eval/content.md.
|
Hi @@ethanl21. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time. |
…ory fails before tests
When a test category fails because the build or deploy crashed before any
test could run (e.g. CS0246 missing namespace, RS0016 PublicAPI errors),
the AI summary table previously showed '0/1 ✓' — the green-checkmark
'all passed' branch — because no per-test failures were parsed. That's
visually misleading: the row is FAILED but the cell looks healthy.
Two fixes:
1. Tests column distinguishes 'category failed AND no per-test failures
parsed' from 'all tests passed':
- 'build/deploy failed' (no tests at all)
- '0/1 — build/deploy failed before per-test results' (some discovered)
2. New optional 'build_tail' field captures the last 30 lines of stdout
when a category fails with zero per-test failures. The Failed test
details collapsible section then renders it in a code block so
reviewers see the actual compiler error / build crash inline,
instead of having to download the full CopilotLogs artifact.
This was discovered while running the regression-check pipeline against
PRs #35110 (142 RS0016 PublicAPI errors), #35281 (CS0246 NSAttributedString
missing for catalyst), and #35358 — all reported as '0/1 ✓' before the fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ory fails before tests
When a test category fails because the build or deploy crashed before any
test could run (e.g. CS0246 missing namespace, RS0016 PublicAPI errors),
the AI summary table previously showed '0/1 ✓' — the green-checkmark
'all passed' branch — because no per-test failures were parsed. That's
visually misleading: the row is FAILED but the cell looks healthy.
Two fixes:
1. Tests column distinguishes 'category failed AND no per-test failures
parsed' from 'all tests passed':
- 'build/deploy failed' (no tests at all)
- '0/1 — build/deploy failed before per-test results' (some discovered)
2. New optional 'build_tail' field captures the last 30 lines of stdout
when a category fails with zero per-test failures. The Failed test
details collapsible section then renders it in a code block so
reviewers see the actual compiler error / build crash inline,
instead of having to download the full CopilotLogs artifact.
This was discovered while running the regression-check pipeline against
PRs #35110 (142 RS0016 PublicAPI errors), #35281 (CS0246 NSAttributedString
missing for catalyst), and #35358 — all reported as '0/1 ✓' before the fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3 participants
Description of Change
Replaced the iOS/Catalyst Picker implementation to use
UIButtonwithUIMenuinstead ofUIPickerView, providing a more native-looking experience on both platforms.Key changes:
PickerHandler.iOS.cs: Rewrote to useUIButtonwithShowsMenuAsPrimaryAction = trueandChangesSelectionAsPrimaryAction = trueIPickerHandler.csandPickerHandler.csto useUIButtonas thePlatformViewtypeAccessibilityTraits = UIAccessibilityTrait.Buttonfor VoiceOver supportAdditionally, this version of Picker can be used in Mac Catalyst apps using the Mac idiom, unlike the current UIPickerView-based control.
Issues Fixed
The original
UIPickerViewimplementation was broken on Mac Catalyst when using the Mac idiom. The newUIButton+UIMenuapproach works reliably on macOS and provides a native-ish macOS appearance.Limitations
UIButtonUIButton+UIMenuon iOS/Mac CatalystKnown Issue
The top two Pickers on the picker page of
Maui.Controls.Samplethat useSelectedIndexin XAML do not appear to respect the set index (SelectedIndex), while the bottom two do. This is likely a bug in my implementation — any help tracking it down would be appreciated.