`ContentPresenter` overwrites child control's inherited `DataContext`

TomEdwardsEnscape commented 1 year ago

To Reproduce

Paste this XAML into a window:

<ContentControl DataContext="HELLO WORLD">
    <TextBlock>
        DataContext is <Run Text="{Binding}"/>
    </TextBlock>
    <ContentControl.ContentTemplate>
        <DataTemplate>
            <Border Padding="10">
                <ContentPresenter Content="{Binding}"/>
            </Border>
        </DataTemplate>
    </ContentControl.ContentTemplate>
</ContentControl>

The text will say "DataContext is Avalonia.Controls.TextBlock".

Expected behavior

The text should say "DataContext is HELLO WORLD".

This is because the TextBlock is defined in the logical tree and should therefore inherit its DataContext from its parent, regardless of what happens in the visual tree. It certainly shouldn't be its own DataContext!

If the DataContext is defined directly on the TextBlock then it will be preserved, which is correct. Only inherited values are overwritten.

Environment

OS: NlA
Avalonia-Version: 11.0.4

rabbitism commented 1 year ago

I doubt if this is by design in ContentControl, please see this legacy documentation issue my colleague raised...

https://github.com/AvaloniaUI/Documentation/issues/391

TomEdwardsEnscape commented 1 year ago

I don't entirely understand what you mean. The example shows that in order for content to appear, ContentControl.Content must be set. That's correct. In my repro sample above I am setting ContentControl.Content to a TextBlock, and this bug is about TextBlock.DataContext being incorrect.

I looked into the code yesterday evening and found that ContentPresenter is misbehaving. It's inserting its content into the logical tree, which is incorrect. In the repro case, you can use the dev tools to inspect the logical tree and you will see the TextBlock appearing in two locations!

timunie commented 1 year ago

We need @Gillibald to double-check what's wrong here. I also think that Run should have parent DataContext in this case.

TomEdwardsEnscape commented 1 year ago

The issue isn't specific to inlines, I just used them to make the issue easier to identify. Control.DataContext is wrong no matter what specific control is used.

kekekeks commented 1 year ago

ContentPresenter supposed to be used as a root element of a ContentControl's control Template (not data template). You aren't supposed to use it directly.

Use ContentControl instead

TomEdwardsEnscape commented 1 year ago

Switching to ContentControl doesn't fix the issue, because there is still a ContentPresenter within it and the same bug is triggered. Please re-open the issue.

Using a ContentControl within a DataTemplate is in any case a very bad idea, because it can lead to stack overflows when the template is selected via x:DataType (or any other inherited means). The ContentControl selects the template, then generates an inner ContentControl, which selects the same template, which generates another ContentControl, and so on until you crash.

The same thing happens in WPF, which is why they tell you to use ContentPresenter inside DataTemplates.

timunie commented 1 year ago

As discussed on telegram today, we should throw an exception in XAML or at least a warning for this and other use cases. Some other areas:

ItemsPresenter
TemplateBinding
...

TomEdwardsEnscape commented 1 year ago

Are you talking about the stack overflow I mentioned in my previous comment? That's a separate issue.

timunie commented 1 year ago

I will open another issue the other day but wanted to have it noted at least.

TomEdwardsEnscape commented 1 year ago

I've dug deeper into the behaviour of the visual and logical trees in Avalonia and WPF. Avalonia has drifted from the WPF spec in some significant ways. I'm sure there's more to discover but this comment is long enough already, so I'll go with just the below for now.

Tree search algorithm

When searching for inherited properties and resources, WPF has a two-pass system. First it searches the logical tree. If no value is found, then it searches the visual tree.

When routing a bubbling/tunnelling RoutedEvent, WPF uses only the visual tree.

Avalonia searches only the logical tree for inherited properties and resources, but it hacks the hell out of it:

When applying a template, TemplatedControl calls ISetLogicalParent.SetParent on the root template control. But it does not add that control to its collection of logical children. This creates a one-way link which allows template control to find resources in the logical tree when searching up the tree, without the control being detectable when searching down the tree (e.g. the Logical Tree tab of the developer tools).
ContentPresenter uses a different system: InheritanceParent. This is normally synchronised with the logical parent, but is reconfigured by ContentPresenter so that the presenter's tree is searched for styles by the child. ContentPresenter then also adds the child to its logical children, creating a one-way link in the opposite direction from TemplatedControl, and potentially causing Content to appear twice in the logical tree.

It looks like the above behaviours were added by different people at different times to fix different bugs, rather than being part of a reasoned architectural decision.

Multiple logical trees

In WPF, there can be (and often are) multiple logical trees within one visual tree. Each template has its own logical tree, the root of which is the root element of the template. The template's target control is not part of this tree; if you want to navigate to it from within the template, you have to use the TemplatedParent property or the visual tree.

In Avalonia, as we saw above, it's kind of the same, but all logical trees within a visual tree are actually bound together with questionable one-way links.

Avalonia also has ILogical.IsAttachedToLogicalTree, which means "is this control in a logical tree with a TopLevel at its root". This is used in a handful of places as an optimisation. For example, Button won't subscribe to ICommand.CanExecuteChanged unless it's in a rooted logical tree, and HeaderedContentControl won't search for a HeaderTemplate.

The value of ILogical.IsAttachedToLogicalTree is determined by StyledElement, which searches for a root by recursively checking the value of IStyleHost.StylingParent, which normally returns the InheritanceParent, but can be overridden. This is very confusing.

Test case

This test case can be used in both Avalonia and WPF. It has a ContentPresenter which steals the content of a ContentControl, and in doing so allows us to separate the visual and logical trees.

<StackPanel Orientation="Vertical">
    <HeaderedContentControl Header="ContentControl" Foreground="Red">
        <ContentControl Name="MyContentControl" Template="{x:Null}">
            <TextBlock Text="Logical tree element"/>
        </ContentControl>
    </HeaderedContentControl>

    <HeaderedContentControl Header="ContentPresenter" Foreground="Green">
        <ContentPresenter Content="{Binding Content, ElementName=MyContentControl}">
            <ContentPresenter.ContentTemplate>
                <DataTemplate>
                    <StackPanel Orientation="Horizontal">
                        <TextBlock Text="Visual tree element"/>
                        <ContentPresenter Content="{Binding}"/>
                    </StackPanel>
                </DataTemplate>
            </ContentPresenter.ContentTemplate>
        </ContentPresenter>
    </HeaderedContentControl>
</StackPanel>

WPF:

Avalonia:

In WPF, the "Logical tree element" text is red, which is correct. In Avalonia, even though the logical tree is searched for inherited values, the manipulations to the tree made by ContentPresenter mean that the red foreground is not found.

Solution

My proposed solution is "do what WPF does". A two-pass search and no more hacky manipulation of the logical tree. I would also deprecate ILogical.IsAttachedToLogicalTree and the various "Set" interfaces, and ensure that one-way links are not possible.

In the large majority of cases this won't change the outcome, but it will change the process. This would be a breaking change for users who are manually searching the logical tree in their own code.

TomEdwardsEnscape commented 1 year ago

I've discovered another wrinkle. This bug's repro case exhibits a second bug: post-load changes to DataContext are not reflected. This is because ContentPresenter has set a local value, and Content is a binding which reads from that local value. So it never changes.

WPF has a very complicated internal system to avoid this problem. It creates a backdoor for all ContentPresenters created in a template, which allows Content="{Binding}" etc. to continue receiving values from the template target even though ContentPresenter.DataContext has a local value.

This second bug can also be reproduced in the Control Gallery:

Switch to the TabControl page
Enable "Set TabItem.ContentTemplate"
Change tab on the right-hand TabControl

The tab's displayed content doesn't change.

Workaround: give the parent Border and name and then bind like this: {Binding #Border.DataContext}.

StefanKoell commented 1 year ago

I think I'm fighting the same issue right now. I derived from a ContentControl and I need to pass on the DataContext to all childs in the ContentPresenter used in the ControlTemplate. How can I do this in a generic way?

timunie commented 1 year ago

@StefanKoell you could make use of the VisualTree-Extension methods I guess.

AvaloniaUI / Avalonia