Add an option to discard null ItemClickEventArgs.ClickedItem before executing ListViewExtensions.Command

[vgromfeld]

Describe the bug

The system ListView and GridView controls exhibit an unexpected behavior when their items are reordered while a ListViewBase.ItemClick event is being processed. It seems that there is some internal race inside the control since we can receive a null ItemClickEventArgs.ClickedItem in the event even if there is no null item within the list.

This issue can easily by hit in our application but I didn't find a way to reproduce this issue in a sample app 😥. It seems to depend on the complexity on the template, the load on the UI thread and the click speed.

Regression

No response

Reproducible in sample app?

• [ ] This bug can be reproduced in the sample app.

Steps to reproduce

In our application, clicking several times very fast on the item while the ListView/ GridView items are being reordered almost always trigger the issue.

Expected behavior

There is two ways to work around this issue:

1. Add a null check in all our ICommands
2. Prevent the unexpected null event from being triggered.

Option 1 is not viable on my side, so I'd like to add an option to ListViewExtensions to add the ability to discard the null value at the ListViewExtensions.Command level.

Screenshots

No response

Windows Build Number

• [ ] Windows 10 1809 (Build 17763)
• [ ] Windows 10 1903 (Build 18362)
• [ ] Windows 10 1909 (Build 18363)
• [ ] Windows 10 2004 (Build 19041)
• [ ] Windows 10 20H2 (Build 19042)
• [ ] Windows 10 21H1 (Build 19043)
• [ ] Windows 11 21H2 (Build 22000)
• [ ] Other (specify)

Other Windows Build number

No response

App minimum and target SDK version

• [ ] Windows 10, version 1809 (Build 17763)
• [ ] Windows 10, version 1903 (Build 18362)
• [ ] Windows 10, version 1909 (Build 18363)
• [ ] Windows 10, version 2004 (Build 19041)
• [ ] Other (specify)

Other SDK version

No response

Visual Studio Version

No response

Visual Studio Build Number

No response

Device form factor

No response

Nuget packages

No response

Additional context

No response

Help us help you

Yes, I'd like to be assigned to work on this item.

[ghost]

Hello vgromfeld, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[Arlodotexe]

Option 1 is not viable on my side, so I'd like to add an option to ListViewExtensions to add the ability to discard the null value at the ListViewExtensions.Command level.

This ListViewExtension directly passes the ClickedItem provided by your ListView. If that value is nullable, you should be able to handle that on your side. Is this feature request because it's not possible, or to provide convenience?

[vgromfeld]

This ListViewExtension directly passes the ClickedItem provided by your ListView.

True but in some (rare) cases, we can receive a null clicked item from the ListView/GridView whereas there is no null item in the list/grid.

Our code is designed to not accept any null value since we consider them as invalid. I can still go through all the code and add a null check but I don't want to modify everything on my side because of a system control bug. For now, I'm using a copy/pasted variant of the toolkit code which is always discarding the null clicked items.

[michael-hawker]

@vgromfeld is there an issue for this filed with the WinUI team? Would like to know their input on this type of issue.

Is the idea that if you click and hold and drag an item to re-order it, that it's both moving it and firing an ItemClick? I wouldn't of thought the ItemClick should fire in that case?

We can sync offline if it's more sensitive, but could be handy to get more info or a video or something of the repro to see more details.

I'm also wondering if we call ItemFromContainer on the attached listview with the originalsource of the click event what it returns still?

I mean this seems like a platform issue and we're usually fairly hesitant to ship specific workarounds for those in the toolkit.

[vgromfeld]

Is the idea that if you click and hold and drag an item to re-order it, that it's both moving it and firing an ItemClick? I wouldn't of thought the ItemClick should fire in that case?

No, this seems to work fine. We can repro the issue if we click several times on a list item and the first click triggered a reorder of the list.

1. click on list view item
2. ItemClick event is raised
3. list is reordered
4. 2nd click
5. ItemClick event is raised with a null item

When this issue occur, the source for the click event is the ListView/GridView.

[michael-hawker]

@vgromfeld is there a reason putting the null check in the command is a problem? Or is it that there's a lot of commands attached to the item in the template?

public void CommandHandlerFunction(object item)
{
    if(item == null) return;
}

[vgromfeld]

There would be a lot of commands to update (+ all the associated unit tests) since none of our commands are expecting a null input. It is easier to fix the issue in the attached property. For now we are using the following forked attached property code:

var command = GetCommand(listViewBase);
if (command == null || e.ClickedItem == null)
{
    return;
}

if (command.CanExecute(e.ClickedItem))
{
    command.Execute(e.ClickedItem);
}

[michael-hawker]

Do we know if we've gotten any traction on the underlying platform issue? I just feel adding a whole other attached property to control an attached property is always a bit much (vs. then moving to a behavior). Or also just wondering if we can come up with a better name...

[vgromfeld]

I realized that I never opened the WinUI bug 😓 so here it is: ListViewBase.ItemClick can be triggered with a null ItemClickEventArgs.ClickedItem even if it does not contain any null item..

I initially added a second attached property to avoid a breaking change. A (configurable) behavior may be more convenient here. I don't have any strong opinion on which option would be the best :).

We can reverse the property intent and name it AllowNullCommandParameters ?

[Arlodotexe]

Since this is an underlying platform issue and not a problem with the toolkit, we're closing this issue as "not planned".

Please continue in the issue filed in the WinUI repo

[michael-hawker]

@Arlodotexe was just going through the issue list and saw this one. I agree we don't want to patch over platform issues usually. This is a pretty annoying case though.

I wasn't a fan of adding a property here to change the behavior. But I'm wondering if we just ignore if the clicked item is null in all cases outright? Like if the platform isn't providing the item, what is a command going to do? Usually, a command on an item needs the context of the item to do anything, right?

@Sergio0694 @vgromfeld what are your thoughts in this space? Could there be a case where getting the null is ever useful?

[vgromfeld]

Most of the time, the commands relying on a parameter are expecting something not null. But I can see the case of a "hybrid" command expecting any kind of parameter. For example, a "play" command can either receive the item to play or null as its parameter. If it receives something, it plays it otherwise, it plays everything.