[Windows, MAC] - Fix Selected State Not Being Retained in CollectionView Items When PointerOver Is Applied#29815
Conversation
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| bool isSelected = this.IsElementInSelectedState(); | ||
| if (IsPointerOver) | ||
| { | ||
| VisualStateManager.GoToState(this, isSelected ? VisualStateManager.CommonStates.Selected : VisualStateManager.CommonStates.PointerOver); |
There was a problem hiding this comment.
Can simplify it reduced duplicate calls to VisualStateManager.GoToState:
string targetState = isSelected
? VisualStateManager.CommonStates.Selected
: (IsPointerOver ? VisualStateManager.CommonStates.PointerOver : VisualStateManager.CommonStates.Normal);
VisualStateManager.GoToState(this, targetState);
There was a problem hiding this comment.
@jsuarezruiz, I have updated the changes as suggested.
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #29815 | Preserve Selected visual state by detecting current selected state before resolving PointerOver vs Normal in ChangeVisualState() |
PENDING (Gate) | src/Controls/src/Core/VisualElement/VisualElement.cs, src/Controls/src/Core/VisualStateManager.cs |
Original PR |
Issue: #29484 - CollectionView Selected state does not work on the selected item when combined with PointerOver.
PR: #29815 - [Windows, MAC] - Fix Selected State Not Being Retained in CollectionView Items When PointerOver Is Applied
Platforms Affected: Windows repro confirmed in issue; PR targets Windows and macOS with UI snapshots
Files Changed: 2 implementation, 2 test source, 2 snapshot assets
Key Findings
- The linked issue reports a Windows CollectionView repro where an item template defining both
PointerOverandSelectedvisual states honors hover styling but drops selected styling. - Issue comment adds an edge case: grouped CollectionView items exhibit the same selected-vs-pointer-over problem.
- The PR changes
VisualElement.ChangeVisualState()and addsVisualStateManager.IsElementInSelectedState()to preserveSelectedoverPointerOverandNormal. - Test coverage is a UI test via
Issue29484with Windows and Mac snapshot baselines under theCollectionViewcategory. - Prior automated review output exists on the PR and indicates Gate previously passed, but this run is performing a fresh review and not relying on that result.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #29815 | Preserve Selected visual state by detecting current selected state before resolving PointerOver vs Normal in ChangeVisualState() |
PENDING (Gate) | src/Controls/src/Core/VisualElement/VisualElement.cs, src/Controls/src/Core/VisualStateManager.cs |
Original PR |
🚦 Gate — Test Verification
Gate Result: PASSED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes: Verified via verify-tests-fail-without-fix for Issue29484. The UI test PointerOverWithSelectedStateShouldWork failed without the PR fix due to snapshot regression and passed with the PR fix applied.
Gate Result: PASSED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes: Verified via verify-tests-fail-without-fix for Issue29484. The UI test PointerOverWithSelectedStateShouldWork failed without the PR fix due to snapshot regression and passed with the PR fix applied. The generated verification log listed eng/pipelines/ci-copilot.yml as an additional reverted file, but the implementation verdict for the actual fix files remained valid.
🔧 Fix — Analysis & Comparison
Gate Result: PASSED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes: Verified via verify-tests-fail-without-fix for Issue29484. The UI test PointerOverWithSelectedStateShouldWork failed without the PR fix due to snapshot regression and passed with the PR fix applied.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Add an internal GoToState overload that protects Selected from automatic transitions to PointerOver/Normal when state priority should be respected |
PASS | 2 files | Effective, but adds an extra VSM path and priority flag plumbing |
| 2 | try-fix | Track selection via a _isVisualStateSelected field updated by VisualStateManager.GoToState() and read directly by ChangeVisualState() |
PASS | 2 files | Works, but introduces extra cached state on VisualElement |
| 3 | try-fix | Skip automatic PointerOver/Normal transitions when the active visual-state group is already Selected and also defines PointerOver |
PASS | 2 files | Narrow and workable, but still hinges on a VSM-side deferral concept |
| 4 | try-fix | Refactor ChangeVisualState() to calculate a target state first, then preserve Selected before applying the automatic common-state transition |
PASS | 1 file | Smallest alternative, but behavior is very close to the PR with less reuse outside VisualElement |
| 5 | try-fix | Have Windows ItemContentControl.UpdateIsSelected() set an internal VisualElement selection flag that ChangeVisualState() uses directly |
PASS | 2 files | Uses authoritative handler state, but is Windows-specific and less portable |
| 6 | try-fix | Add GoToInteractionState() so automatic pointer-state updates skip groups whose active state has selection semantics |
PASS | 2 files | Distinct, but significantly larger VSM change footprint |
| PR | PR #29815 | Preserve Selected visual state by detecting current selected state before resolving PointerOver vs Normal in ChangeVisualState() |
PASSED (Gate) | 2 files | Original PR |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | n/a | Initial alternative fix attempt completed |
| claude-sonnet-4.6 | 1 | n/a | Initial alternative fix attempt completed |
| gpt-5.3-codex | 1 | n/a | Initial alternative fix attempt completed |
| gemini-3-pro-preview | 1 | n/a | Initial alternative fix attempt completed |
| claude-opus-4.6 | 2 | Yes | Control-level separation of selection visuals from transient pointer state |
| claude-sonnet-4.6 | 2 | Yes | Split selection into a separate visual-state group |
| gpt-5.3-codex | 2 | Yes | Limit automatic updates to interaction-state groups rather than selection groups |
| gemini-3-pro-preview | 2 | Yes | Drive selection truth from platform selection handlers via explicit element state |
| claude-opus-4.6 | 3 | No | NO NEW IDEAS |
| claude-sonnet-4.6 | 3 | No | NO NEW IDEAS |
| gpt-5.3-codex | 3 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 3 | No | NO NEW IDEAS |
Exhausted: Yes
Selected Fix: PR #29815 It passes Gate, stays cross-platform instead of Windows-only, keeps the change small and readable, and matches existing VisualStateManager/VisualElement patterns better than the heavier alternatives.
📋 Report — Final Recommendation
Gate Result: PASSED
Platform: windows
Mode: Full Verification
- Tests FAIL without fix:
- Tests PASS with fix:
Notes: Verified via verify-tests-fail-without-fix for Issue29484. The UI test PointerOverWithSelectedStateShouldWork failed without the PR fix due to snapshot regression and passed with the PR fix applied.
Final Recommendation: APPROVE
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | COMPLETE | Issue, PR, comments, prior bot review, and changed-file classification captured |
| Gate | PASSED | Windows full verification: Issue29484 failed without fix and passed with fix |
| Try-Fix | COMPLETE | 6 passing alternatives explored; PR fix remained the best overall tradeoff |
| Report | COMPLETE |
Summary
PR #29815 fixes the CollectionView visual-state conflict where automatic pointer-state updates override the selected-state styling. The added UI repro test on Windows correctly fails without the fix and passes with it. I also explored six independent alternatives; while several passed, the PR's approach remained the strongest balance of simplicity, portability, and alignment with the existing codebase.
Root Cause
VisualElement.ChangeVisualState() automatically resolved to PointerOver or Normal without considering that the element might already be in the Selected visual state. In CollectionView item templates where Selected and PointerOver coexist, that automatic transition overwrote the selected styling.
Fix Quality
The PR addresses the problem with a narrow framework change: it reads the current visual-state groups to determine whether Selected is already active, then preserves that state instead of blindly switching to PointerOver or Normal. Compared with the alternatives, this keeps the logic centralized, avoids platform-specific state flags, and avoids introducing larger new VSM APIs.
Notes
The gate helper's verification log included eng/pipelines/ci-copilot.yml in its temporary revert list, but the actual fail/pass outcome for the targeted fix remained valid. I left an unrelated dirty worktree file (src/Core/src/Handlers/HybridWebView/HybridWebView.js) untouched throughout the review.
…iew Items When PointerOver Is Applied (#29815) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause : The `ChangeVisualState` method does not account for the `Selected` state in its state resolution logic. It prioritizes PointerOver over Selected, and defaults to Normal when no pointer interaction exists—even if the item is selected. This results in inconsistent visual feedback where the Selected state styling is ignored or overridden. ### Description of Change Enhanced `IsElementInSelectedState`() extension to VisualElement that properly detects if an element is currently in the Selected visual state by inspecting the CurrentState property of all visual state groups. Updated `ChangeVisualState`() to prioritize Selected over PointerOver and Normal: When an element is enabled, it first checks if the element is in the Selected state - If the elements with the pointer over them, it applies either the `Selected` or `PointerOver` state, depending on selection status - If the elements are without the pointer over them, it applies either the `Selected` or `Normal` state, depending on the selection status This change ensures a consistent visual representation of `selected` items while preserving pointer and focus behavior. ### Issues Fixed Fixes #29484 **Tested the behaviour in the following platforms** - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Output | Before| After| |--|--| | <video src="https://github.com/user-attachments/assets/f2f82f55-7501-4560-9b80-162b9f923f0a"> | <video src="https://github.com/user-attachments/assets/88b33563-f165-4271-bf04-97a57305d6ab"> | <video src="https://github.com/user-attachments/assets/2649e2bc-69bc-48a4-b825-0d7169a62c0d"> | <video src="https://github.com/user-attachments/assets/d531d7ac-efee-4e8a-b1f9-7235a303c487"> |
…iew Items When PointerOver Is Applied (dotnet#29815) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause : The `ChangeVisualState` method does not account for the `Selected` state in its state resolution logic. It prioritizes PointerOver over Selected, and defaults to Normal when no pointer interaction exists—even if the item is selected. This results in inconsistent visual feedback where the Selected state styling is ignored or overridden. ### Description of Change Enhanced `IsElementInSelectedState`() extension to VisualElement that properly detects if an element is currently in the Selected visual state by inspecting the CurrentState property of all visual state groups. Updated `ChangeVisualState`() to prioritize Selected over PointerOver and Normal: When an element is enabled, it first checks if the element is in the Selected state - If the elements with the pointer over them, it applies either the `Selected` or `PointerOver` state, depending on selection status - If the elements are without the pointer over them, it applies either the `Selected` or `Normal` state, depending on the selection status This change ensures a consistent visual representation of `selected` items while preserving pointer and focus behavior. ### Issues Fixed Fixes dotnet#29484 **Tested the behaviour in the following platforms** - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Output | Before| After| |--|--| | <video src="https://github.com/user-attachments/assets/f2f82f55-7501-4560-9b80-162b9f923f0a"> | <video src="https://github.com/user-attachments/assets/88b33563-f165-4271-bf04-97a57305d6ab"> | <video src="https://github.com/user-attachments/assets/2649e2bc-69bc-48a4-b825-0d7169a62c0d"> | <video src="https://github.com/user-attachments/assets/d531d7ac-efee-4e8a-b1f9-7235a303c487"> |
…iew Items When PointerOver Is Applied (#29815) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause : The `ChangeVisualState` method does not account for the `Selected` state in its state resolution logic. It prioritizes PointerOver over Selected, and defaults to Normal when no pointer interaction exists—even if the item is selected. This results in inconsistent visual feedback where the Selected state styling is ignored or overridden. ### Description of Change Enhanced `IsElementInSelectedState`() extension to VisualElement that properly detects if an element is currently in the Selected visual state by inspecting the CurrentState property of all visual state groups. Updated `ChangeVisualState`() to prioritize Selected over PointerOver and Normal: When an element is enabled, it first checks if the element is in the Selected state - If the elements with the pointer over them, it applies either the `Selected` or `PointerOver` state, depending on selection status - If the elements are without the pointer over them, it applies either the `Selected` or `Normal` state, depending on the selection status This change ensures a consistent visual representation of `selected` items while preserving pointer and focus behavior. ### Issues Fixed Fixes #29484 **Tested the behaviour in the following platforms** - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Output | Before| After| |--|--| | <video src="https://github.com/user-attachments/assets/f2f82f55-7501-4560-9b80-162b9f923f0a"> | <video src="https://github.com/user-attachments/assets/88b33563-f165-4271-bf04-97a57305d6ab"> | <video src="https://github.com/user-attachments/assets/2649e2bc-69bc-48a4-b825-0d7169a62c0d"> | <video src="https://github.com/user-attachments/assets/d531d7ac-efee-4e8a-b1f9-7235a303c487"> |
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!
Root Cause :
The
ChangeVisualStatemethod does not account for theSelectedstate in its state resolution logic. It prioritizes PointerOver over Selected, and defaults to Normal when no pointer interaction exists—even if the item is selected. This results in inconsistent visual feedback where the Selected state styling is ignored or overridden.Description of Change
Enhanced
IsElementInSelectedState() extension to VisualElement that properly detects if an element is currently in the Selected visual state by inspecting the CurrentState property of all visual state groups.Updated
ChangeVisualState() to prioritize Selected over PointerOver and Normal:When an element is enabled, it first checks if the element is in the Selected state
SelectedorPointerOverstate, depending on selection statusSelectedorNormalstate, depending on the selection statusThis change ensures a consistent visual representation of
selecteditems while preserving pointer and focus behavior.Issues Fixed
Fixes #29484
Tested the behaviour in the following platforms
Output
BeforeFix.mp4
AfterFix.mp4
BeforeFix.mov
AfterFix.mov