[Bug] CSS styles applied incorrectly when adding tree of views

[cabal95]

Description

If you have a tree of Views and then add the top-most view into the real view hierarchy, the CSS styles do not get applied correctly if any styles exist on the views you are adding.

Example:

You have a ContentPage with a StackLayout as it's content. The ContentPage has a StyleSheet object in it's Resources. Now you create a new "off-screen" StackLayout and set a StyleSheet on this new layout that overrides some of the ContentPage styles. Next you add a Label to this new layout. Finally, you add the new StackLayout to the already existing StackLayout to put in on-screen.

The system calculates the list of StyleSheets that should be applied to the new StackLayout (i.e. just the ContentPage StyleSheet) and applies it to the StackLayout. Up to now we are good. The problem is that when a StyleSheet is applied to a view, it also applies itself to all descendant views as well. So what ends up happening to the Label is that it only gets the ContentPage stylesheet applied and not the stylesheet that was declared on the StackLayout as well.

Now as a final test, if we create a second Label and add that label to the new StackLayout (which is already on-screen), it correctly gets the ContentPage stylesheet applied AND also the StackLayout stylesheet applied.

Steps to Reproduce

1. Open attached project
2. Run

-or-

1. Create a view with a stylesheet and place it on screen (say ^label { background-color: red }.
2. Create a StackLayout with a stylesheet of ^label { background-color: blue } but do not add it to the parent view yet.
3. Create a Label and add it to the StackLayout created in step 2.
4. Add the StackLayout created in step 2 to the view created in step 1.
5. Notice that the label background color is red instead of blue.
6. Create a second Label and add it to the StackLayout created in step 2.
7. Notice that the second label correctly has a background color of blue.

Expected Behavior

When a view with children is added to the visual tree, each descendant view should calculate it's own list of stylesheets to be applied.

Actual Behavior

Only the view that is actually added performs the stylesheet calculation/gathering and then those styles are applied to all descendant views, regardless of the existence of any stylesheets on child views.

Suggested Fix

It seems like the fix would be to move the "apply to all descendants" out of the stylesheet and into the method that gathers and then applies all the stylesheets. Said another way, currently the recursion happens inside the StyleSheet.Apply(Element styleable) method. Instead I believe it should be happening inside the ApplyStyleSheetsOnParentSet() method so that each child view properly calculates it's list of stylesheets to be applied.

An alternative, that might be more performant, is to still remove the recursion from the StyleSheet.Apply(Element styleable) method. Then also remove the "apply these style sheets to the view" into a new method that applies the entire List of stylesheets and handles recursion. The new method would apply the stylesheets to this, then check if this declares any stylesheets and append them to the list and then apply that new list of stylesheets to any children by calling itself. Something like:

internal void CoreApplyStyleSheetsOnParentSet(List<StyleSheet> sheets)
{
    // Apply current stylesheets to this view.
    for (var i = sheets.Count - 1; i >= 0; i--)
        ((IStyle)sheets[i]).Apply(this);

    // Append any new stylesheets this view declares.
    var resourceProvider = this as IResourcesProvider;
    var vpSheets = resourceProvider?.GetStyleSheets();
    if (vpSheets != null)
    {
        sheets = new List<StyleSheet>(sheets);
        sheets.AddRange(vpSheets);
    }

    // Apply new stylesheet set to all children.
    foreach (var child in styleable.AllChildren)
        child.CoreApplyStyleSheetsOnParentSet(sheets);
}

Basic Information

• Version with issue: 4.3.0.908675
• Last known good version:
• IDE: Visual Studio 2019 16.4.5
• Platform Target Frameworks: iOS, Android for sure, probably all of them though.
  • iOS: 13.2
  • Android: 9.0
  • UWP:
• Android Support Library Version:
• Nuget Packages:
• Affected Devices:

Screenshots

Reproduction Link

App1.zip

Workaround

Currently, I have lifted the code from the Xamarin.Forms.Stylesheet namespace (since it's all private) and use Reflection to manually call the method that applies stylesheets to a view after I add my "created off-screen view" to the visual tree. This works, but is a significant performance hit because stylesheets are being applied potentially hundreds of times due to the recursive applying that happens.

[hartez]

Only the view that is actually added performs the stylesheet calculation/gathering and then those styles are applied to all descendant views, regardless of the existence of any stylesheets on child views.

I think you might be misunderstanding how the CSS is applied.

Set a breakpoint between stackView.Children.Add( shouldBeBlueLabel ); and slViews.Children.Add( stackView ); - the BackGroundColor of shouldBeBlueLabel will be Blue. Set a breakpoint after slViews.Children.Add( stackView ); - the BackGroundColor of shouldBeBlueLabel will now be Red.

The CSS styles are applied as soon as the Label is parented. When the Label is added to the StackLayout stackView, the blue background color is applied. When the the stackView is added to slViews, the CSS from slViews is applied, and the color is changed to red.

In other words, given a conflict, the last CSS rule applied wins.

@StephaneDelcroix knows more about the internals of CSS in Forms, so maybe he can weigh in here - I might be wrong about how this works. But I'm guessing that having to recompute the entire list of potential applicable CSS rules every time the tree is modified would be a non-starter from a performance standpoint. And if your design relies on the order in which the CSS styles are applied, I would suggest giving your controls specific classes to avoid the ambiguity.

[cabal95]

In other words, given a conflict, the last CSS rule applied wins.

Yes, absolutely correct. The problem is that the CSS rules are not being computed correctly so the "last CSS rule applied" is not the correct rule. I should not be able to add two identical views at the same spot in the view hierarchy and have them end up with two different styles applied. That would be a bug in expected behavior IMO.

But I'm guessing that having to recompute the entire list of potential applicable CSS rules every time the tree is modified would be a non-starter from a performance standpoint.

This already happens. If I build my tree from the top down then every time I add a single view it's doing the exact same calculation for every single view. In fact it is doing more calculations because it has to re-calculate everything all the way up the entire tree for every single view added. If the functionality is changed as I suggested in my Alternative method above, it should actually improve performance slightly because it's only having to partially calculate the new CSS tree since it's moving down and simply adding things to the StyleSheets list as it moves down rather than re-building the entire list for every single view.

[StephaneDelcroix]

well, you're both right ¯\_(ツ)_/¯: @hartez explanation:

In other words, given a conflict, the last CSS rule applied wins.

is right, and @cabal95 expectation:

I should not be able to add two identical views at the same spot in the view hierarchy and have them end up with two different styles applied

is valid.

That being said, I haven't looked at the original use case in details yet.

Unrelated to this... you should build your UI bottom-up, for this and other (e.g. bindings) performance reasons. That's how the XAML parser builds a view for you.

[StephaneDelcroix]

looks like the bug report is valid, nested stylesheets should be applied last, and have precedence

[cabal95]

you should build your UI bottom-up

Just because I want to be 100% I'm on the same page. "bottom-up" meaning if I have a Page that contains a ScrollView that contains a StackLayout that contains a Label; I should be creating the Label, then create the StackLayout and add the Label to it, then create a ScrollView and add the StackLayout to it, and then add the ScrollView to the Page? Or by "bottom-up" do you mean the inverse of what I just described?

(assuming I'm having to do this programmatically that is)

[StephaneDelcroix]

Just because I want to be 100% I'm on the same page. "bottom-up" meaning if I have a Page that contains a ScrollView that contains a StackLayout that contains a Label; I should be creating the Label, then create the StackLayout and add the Label to it, then create a ScrollView and add the StackLayout to it, and then add the ScrollView to the Page? Or by "bottom-up" do you mean the inverse of what I just described?

the creation order has no importance, but it's better to start parenting from the bottom, like you just described

[cabal95]

@samhouts @StephaneDelcroix This seems to be fixed. I noticed this commit https://github.com/xamarin/Xamarin.Forms/commit/a3e408a388f5cc3460488180859aaa1968d8d52e and re-tested the reproduction application in the original post and it now works as correctly.

So not sure if that commit specifically fixed it, but it is now working correctly on 4.8.0.1687 at least. So this can be closed unless there is some other followup work that needs to be done internally.