-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Windows] Fix CollectionView selected item visibility #31126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hey there @@Vignesh-SF3580! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
| var margin = WinUIHelpers.CreateThickness(0, v, 0, v); | ||
|
|
||
| var style = new WStyle(typeof(ListViewItem)); | ||
| var defaultStyle = GetDefaultStyle("DefaultListViewItemStyle"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we store the resourceKey in a field and use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the code to store the resourceKey in a field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where CollectionView selected item states were not visible on Windows. The root cause was that when creating custom container styles for spacing and layout, the default styles containing selection visual states were being overridden completely instead of being extended.
Key changes:
- Modified style creation to use
BasedOnproperty to inherit from default styles - Added UI test to verify selection visibility behavior
- Applied fix to both GridView and ListView item container styles
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs |
Updated style creation logic to inherit from default styles using BasedOn property, ensuring selection states remain visible |
src/Controls/tests/TestCases.HostApp/Issues/Issue30868.cs |
Added test page with CollectionView configured for multiple selection mode to demonstrate the fix |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue30868.cs |
Added UI test that taps an item and verifies selection state through screenshot comparison |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| VerticalTextAlignment = TextAlignment.Center, | ||
| Padding = new Thickness(10, 0, 10, 10) | ||
| }; | ||
| label.SetBinding(Label.TextProperty, "."); |
Copilot
AI
Aug 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Label element lacks an AutomationId property. According to the testing guidelines, all interactive UI elements should have unique AutomationIds to ensure proper test automation. Consider adding label.AutomationId = (string)label.BindingContext; or a similar unique identifier.
| [Category(UITestCategories.CollectionView)] | ||
| public void CollectionViewSelectionMode() | ||
| { | ||
| App.WaitForElement("Item 2"); |
Copilot
AI
Aug 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is waiting for an element with text 'Item 2', but the corresponding HostApp page doesn't assign AutomationIds to the Label elements within the CollectionView items. This could cause the test to fail because WaitForElement may not be able to locate the element reliably without proper AutomationIds.
| var margin = WinUIHelpers.CreateThickness(h, v, h, v); | ||
|
|
||
| var style = new WStyle(typeof(GridViewItem)); | ||
| var defaultStyle = GetDefaultStyle(GridViewItemStyleKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GetDefaultStyle() method could return null if the resource key doesn't exist. Must not happen except mistake, but, can protect it?
if (defaultStyle is not null)
{
style.BasedOn = defaultStyle;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz Added a null check to avoid unexpected failures.
src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs
Outdated
Show resolved
Hide resolved
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { | ||
| App.WaitForElement("Item 2"); | ||
| App.Tap("Item 2"); | ||
| VerifyScreenshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve committed the images for Windows and macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we modify it a little bit? Use
| public static void SetLightTheme(this IApp app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz I have updated the test cases based on your suggestion. Let me know if any further changes are needed.
@jsuarezruiz Previously, the default style for SelectedItems was not updating properly. It is now updated along with the default styles. I have updated these with the newly generated images. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs
Show resolved
Hide resolved
| PropertyChangedEventHandler _layoutPropertyChanged; | ||
| const string ListViewItemStyleKey = "DefaultListViewItemStyle"; | ||
| const string GridViewItemStyleKey = "DefaultGridViewItemStyle"; | ||
| static WStyle _listViewItemStyle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could validate if user switches between Light/Dark themes works as expected or static cached styles won't update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz I have validated switching between Light and Dark themes, and it works as expected. The checkbox colors are updated correctly based on the theme changes defined in the default style. I have attached a video for reference.
31126Theme.mp4
src/Controls/src/Core/Handlers/Items/StructuredItemsViewHandler.Windows.cs
Show resolved
Hide resolved
| { | ||
| App.WaitForElement("Item 2"); | ||
| App.Tap("Item 2"); | ||
| VerifyScreenshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we modify it a little bit? Use
| public static void SetLightTheme(this IApp app) |
d1b4700 to
dd0f21f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-running the Windows CollectionView UITests. The process was canceled after some time running.
@jsuarezruiz Changing themes continuously fails on CI for Windows. It seems that theme changes do not work on Windows, and this was already noted in the implemented PR. |
f8eeb1f to
059dbe0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |



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
On Windows, the CollectionView selection state was not visible.
Root Cause
When adding new container styles, the default styles was not added.
Description of Change
Used the BasedOn property to include the default style along with the new style, so selection states remain visible.
Tested the behavior in the following platforms
Issues Fixed
Fixes #30868
Screenshots
SelectionMode: Multiple
SelectionMode: Single