Fix TreeView missing children issue

[Felix-Dev]

Description

With data bound TreeView items some recycled item containers (TreeViewItem) are losing their bound children collection. Futher debugging the code shows that the TreeView nodes of such TreeViewItems also have lost their children (TreeViewNode.m_itemsDataSource is null). As such, calling TreeViewNode.SyncChildrenNodesWithItemsSource() is not fixing this issue.

The current fix is a first attempt at fixing the issue. It forces a binding re-evaluation of the TreeViewItem.ItemsSource dependency property. As such, TreeViewItem.OnPropertyChanged is called which in return sets the items source of the corresponding TreeViewNode (TreeViewNode.ItemsSource(itemsSource)).

As we are basically doing the steps after TreeView creation again, there might be a better way to fix the issue. I would like to ask a team help aid me in search for a better fix in case there is one 🙂

Motivation and Context

Fixes #1701 and Fixes #523.

How Has This Been Tested?

Added test to the MUXControlsTestApp TreeView test page. Test can be modified if requested.

[japf]

I was looking into this exact issue yesterday, fun timing :)

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[kaiguo]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[kaiguo]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

@Felix-Dev the test failure looks like its a real issue to me, are you able to look into it?

[Felix-Dev]

@StephenLPeters I'm seeing the test failures locally too. I will look at them this weekend and see if there is something I can do.

Edit:
@StephenLPeters Reporting some first test results. I will edit this issue with new results from further testing.

Here's the setup before the crash:

Observations:

• Root.0 has to be expanded
• Root.2 has no children (this is correct)

If we now collapse and then expand "Root" the app crashes. It doesn't matter if "Root.1" is expanded or not. As long as "Root.0" is expanded when we collapse "Root" the crash will occur when subsequently epxanding "Root" again.

Edit2 @kaiguo:
I tried to compare the treeview logic executed when a) expanding "Root" with "Root.0" collapsed (no-crash scenario) and b) expanding "Root" with "Root.0" expanded (crash scenario). Here are two images showing a difference:
Scenario a (no crash):

Scenario b (crash);

Note that in scenario a) the underlying treenode of "Root.2" is not expanded. In scenario b) however, the underlying treenode is expanded (why is that?). Thus, the logic executed in both cases might differ.

I added additional test code which you can see in the pics above:

targetNode.IsExpanded(isItemExpanded);

This sets the underlying treenode for "Root.2" to not expanded and scenario b) now seems to follow the code path of scenario a) as a result.

However, with or without this line, I still get the same crash which happens sometimes after TreeViewItemSource.HasUnrealizedChildren for "Root.0" is requested (using the debugger, the code crashes as the next step after TreeViewPage.g.cs::Update_HasUnrealizedChildren is called). The next step in code execution would be TreeViewList::OnContainerContentChanging() for "Root.1" (based on scenario a). Unfortunately, something is causing the crash prior to that code execution in scenario b) and all I'm seeing for now is a bunch of Windows.Ui.Xaml.dll calls I cannot inspect.

It looks like the issue is releated to "Root.0" and its children and not something related to "Root.2". I'm not sure if I can debug this code further and find the root cause on my own.

[StephenLPeters]

@kaiguo thoughts on @Felix-Dev s comment?

[kaiguo]

@Felix-Dev I think It's because you are trying to update ListView ItemsSource during layout pass. The node expansion operation is basically adding all the children into Listview ItemsSource which triggers a layout pass and calls ContainerContantChanging, and inside ContainerContantChanging ItemsSource is being set again, it'll trigger another layout pass. @ranjeshj are there other places we can safely put the ItemsSource updating code?

[ranjeshj]

@Felix-Dev I think It's because you are trying to update ListView ItemsSource during layout pass. The node expansion operation is basically adding all the children into Listview ItemsSource which triggers a layout pass and calls ContainerContantChanging, and inside ContainerContantChanging ItemsSource is being set again, it'll trigger another layout pass. @ranjeshj are there other places we can safely put the ItemsSource updating code?

Please avoid updating ItemsSource (or changing the collection in any way) within layout. Doing so will likely end up causing issues down the line. You can set it through a dispatcher call though which will run after layout completes.

[Felix-Dev]

@ranjeshj @kaiguo So I added a dispatcher call (looked at TreeViewList::PrepareContainerForItemOverride) and it added a bunch of new problems:

if (auto itemsSource = targetItem.ItemsSource())
{
    const DispatcherHelper dispatcher{ *this };
        dispatcher.RunAsync(
            [treeViewItem, targetNode, itemsSource]()
            {
                treeViewItem->SetItemsSource(targetNode, itemsSource);
            });
}   

See the following image for the failing test case for example (collapse Root with both Root.0 and Root.1 collapsed, then expand):

with expected look:

If I now expand Root.2 then collapse Root and expand Root again, the app crashes as in the previous cases (only Root.0 substituted with Root.2).

Ignore the dispatcher code for now, I added another check to the code which seems to fix the crash and failing tests just fine:

if (auto itemsSource = targetItem.ItemsSource())
{
    if (treeViewNode->ItemsSource() == nullptr)
    {
        treeViewItem->SetItemsSource(targetNode, itemsSource);
    }
}  

This is based on the observation that the treeview issues this PR attempts to fix really only happened because TreeViewNode::m_itemDataSource was null for the failing treeview elements. Now this check makes sure we only re-set the item source where there's a mismatch between the container and the underlying tree node (mismatch: container has an item source whereas treenode does not).

Since this check fixes the issues I then wrapped the ItemsSource update inside a dispatcher call, like so:

if (auto itemsSource = targetItem.ItemsSource())
{
    if (treeViewNode->ItemsSource() == nullptr)
    {
        const DispatcherHelper dispatcher{ *this };
        dispatcher.RunAsync(
            [treeViewItem, targetNode, itemsSource]()
            {
                treeViewItem->SetItemsSource(targetNode, itemsSource);
            });
    }
}   

That, however, caused three new tests to fail: ItemsSourceResycTest, TreeViewRootNodeBindingTest and TreeViewWithMultiLevelChildrenExpandCollapseTest.

I decided to push the new check for now as that makes all my local tests run fine. If the tests run fine here too, we can see how to put that in a dispatcher call if still required so that no new test failures will occur.

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Felix-Dev]

@kaiguo @ranjeshj Now that all tests have succeeded and the call is not yet wrapped inside a dispatcher call (which would cause new test failures based on the attempt above), how do we want to continue here?

[kaiguo]

@kaiguo Kai Guo FTE @ranjeshj Ranjesh Jaganathan FTE Now that all tests have succeeded and the call is not yet wrapped inside a dispatcher call (which would cause new test failures based on the attempt above), how do we want to continue here?

If null check fixes the problem I think we should go with the null check approach since dispatcher call makes the code hard to debug and understand, but I'll defer to @ranjeshj to make the call : )

[ranjeshj]

@kaiguo Kai Guo FTE @ranjeshj Ranjesh Jaganathan FTE Now that all tests have succeeded and the call is not yet wrapped inside a dispatcher call (which would cause new test failures based on the attempt above), how do we want to continue here?

If null check fixes the problem I think we should go with the null check approach since dispatcher call makes the code hard to debug and understand, but I'll defer to @ranjeshj to make the call : )

Ok. The null check sounds reasonable and we can revisit if this becomes an issue.

[StephenLPeters]