microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.31k stars 676 forks source link

TreeView: scrambled contextflyouts when dynamically assigned #8280

Open jschwizer99 opened 1 year ago

jschwizer99 commented 1 year ago

Describe the bug

Our application uses contextmenu flyouts that are dynamically configured depending on the tree view item data. When tree nodes are removed and added the dynamic context menu does not match with the datacontext of the related tree view item.

As we still have some XML-defined static contextmenu flyouts and our treeview uses template selectors the ContextRequested event will be swallowed. Instead, we tried with Loaded, Opeing, and DataContextChanged events. They all show the same issue that context menus get scrambled. For the moment we get the best behavior when defining Opening events in the menuflyout.

The issue strongly correlates with the memory-leaking behavior reported in #8275. When using Clear() which leaks all nodes the scrambling is very frequent (but not always). With proper node removal, the issue is triggered very seldom. Unfortunately, occasionally it is still observed.

Steps to reproduce the bug

Unpack TreeStability_scrambling.zip. Restore Nuget packages. Close and reopen the solution (to avoid a weird deployment error). Compile and run (in Debug x64).

Frequent Occurrence

Rare Occurrence

Example of failure cases

Scrambling1 Scrambling2

Be aware that Populate Methods Mixed 1st Expanded and Mixed 2nd Expanded suffer from the IsExpanded() crash, see #8275.

Expected behavior

The number shown in the sub-context menu of 'Dynamic' should match the number of the related tree view item.

ContextFlayout_pass_case

Screenshots

No response

NuGet package version

WinUI 3 - Windows App SDK 1.2.4: 1.2.230217.4

Windows version

Windows 10 (21H2): Build 19044, Windows 10 (20H2): Build 19042

Additional context

Some More Background

would be the obvious way. But for unknown reasons, the Items Property in MenuFlyoutSubItem is a read-only property from Xaml side. As we have the UI element inside a template we also cannot use the approach using `x:Name` (that is documented on every documentation page of MS). The event ContextRequested isn't of help either. As soon as an initial context menu is defined in Xaml it gets swallowed. Removing and re-attaching the events in code didn't help. It seems that the datacontext of the flyout is different than the underlying data.
jschwizer99 commented 1 year ago

Issue with all element properties updated directly in code

Actually, not only do context menu entries get scrambled but any element property that gets updated directly by code. This can be e.g. a background color of a text field.

void MainWindow::OnDataContextChanged(
    winrt::Windows::Foundation::IInspectable const& sender,
    winrt::Windows::Foundation::IInspectable const& args)
{
    if (const auto border = sender.try_as<winrt::Microsoft::UI::Xaml::Controls::Border>())
    {
         border.Background(brush);  // <- Will fail as border background is set directly
         // Iterate through elements to get the property
         // ...
         textblock.Text(text);  // <- Will fail as textblock property text is set directly
         if (const auto item = border.DataContext().try_as<TreeStability::FirstItem>())
         {
             item.Name(text);  // <- Will work as textblock entry is updated over the data context with x:bind
         }
    }
}

In the following example (see attached solution TreeStability_FailureCaseOpeningContextMenu.zip) we modify the background color, one of the textbox texts and the context menu by code. Once the failure is triggered all these directly modified properties show the wrong value. The ones that are changed over the data context with x:bind behave correctly.

image

Once we hit the failure mode all future updates will be scrambled. When the failure mode is triggered is not exactly clear in the example I managed to trigger it by opening a context menu by right click or just during normal Clear() operations. It also seems system dependent. We could see the initially described context scrambling only on specific systems.

The scrambling manifests itself in this case as a recycling of an older visual style that does not match the current data context. The changes are updated on a background object but do not get used for display. This looks like virtualization is causing the issue as it doesn't seem to care about element property updates.

Workaround using x:bind

From the above observations a possible solution for the context menu scrambling becomes obvious.

Only perform treeview and listview item updated by using x:bind!

For the context menu it is unfortunate that we cannot use x:bind on Items() [See https://github.com/microsoft/microsoft-ui-xaml/issues/1087]. Thus, we need to use x:bind on ContextFlyout. This is quite unfortunate as the entire context menu needs to be recreated with every update.

<DataTemplate x:Key="oneTreeViewItemTemplate" x:DataType="local:FirstItem">
    <Border
        ContextFlyout="{x:Bind ContextMenu, Mode=OneWay}"
        DataContextChanged="{x:Bind OnDataContextChanged}">
        <StackPanel
            Orientation="Horizontal"
            Background="#50ff0000"
            CornerRadius="3"
            Padding="4"
            Spacing="16">
            <TextBlock
                Text="First"/>
            <TextBlock
                Text="{x:Bind Name, Mode=OneWay}"/>
            <TextBlock
                Text="{x:Bind Uid, Mode=OneWay}"/>
            <TextBlock
                Text="{x:Bind FirstValue, Mode=OneWay}"/>
        </StackPanel>
    </Border>
</DataTemplate>

winrt::Microsoft::UI::Xaml::Controls::Primitives::FlyoutBase FirstItem::ContextMenu() const
{
    return contextMenu_;
}

void FirstItem::OnDataContextChanged(
    winrt::Windows::Foundation::IInspectable const& sender,
    winrt::Microsoft::UI::Xaml::DataContextChangedEventArgs const& args)
{
    if (const auto data = args.NewValue().try_as<TreeStability::FirstItem>())
    {
        contextMenu_ = winrt::Microsoft::UI::Xaml::Controls::MenuFlyout();
        // Replace MenuFlyoutSubItem as just replacing the Items() will not work
        auto dynamicFlyout = winrt::Microsoft::UI::Xaml::Controls::MenuFlyoutSubItem();
        dynamicFlyout.Text(L"Dynamic");

        // Fill Items()
        auto flyout = winrt::Microsoft::UI::Xaml::Controls::MenuFlyoutItem();
        flyout.Text(data.Uid());
        flyout.Tag(winrt::box_value(winrt::hstring(data.Name())));
        dynamicFlyout.Items().Append(flyout);

        auto staticFlyout = winrt::Microsoft::UI::Xaml::Controls::MenuFlyoutItem();
        staticFlyout.Text(L"Static");

        contextMenu_.Items().Append(dynamicFlyout);
        contextMenu_.Items().Append(winrt::Microsoft::UI::Xaml::Controls::MenuFlyoutSeparator());
        contextMenu_.Items().Append(staticFlyout);

        RaisePropertyChanged(L"ContextMenu");
    }
    else
    {
        OutputDebugStringW(L"Error: Expected a FirstItem type, but got something else.\n");
        assert(!"Expected a FirstItem type, but got something else.");
    }
}

winrt::Microsoft::UI::Xaml::DependencyProperty FirstItem::firstValueProperty_ =
    winrt::Microsoft::UI::Xaml::DependencyProperty::Register(
        L"FirstValue",
        winrt::xaml_typename<winrt::hstring>(),
        winrt::xaml_typename<TreeStability::FirstItem>(),
        winrt::Microsoft::UI::Xaml::PropertyMetadata{ winrt::box_value(winrt::hstring()) }
);

See TreeStability_workaround.zip for the full solution that is working expected.

Note: The ticket https://github.com/microsoft/microsoft-ui-xaml/issues/1652 is closely related but the proposed approach will not solve the scrambling. It is not enough to replace the entire flyout. Only updating the full context menu by x:bind will behave correctly.