Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Stylesheets on page level now always override parent stylesheets #6772

Merged
merged 10 commits into from Apr 5, 2020

Conversation

csteeg
Copy link
Contributor

@csteeg csteeg commented Jul 3, 2019

Description of Change

To fix issue #5812 I moved the responsibility for appyling a stylesheet to child elements to the Element class.
It would be weird for a stylesheet to get all parent stylesheets of an element and call Apply on them too. I think it's a good thing that StyleSheet.Apply(Element) really only applies it on the given Element? The main issue was that when the ApplyParentStyleSheets was called, the stylesheet of the element itself wasn't applied, so you could never override parent style elements unless you set the stylesheet after the parent was set.

Issues Resolved

API Changes

None

Platforms Affected

  • Core/XAML (all platforms)

Behavioral/Visual Changes

Stylesheets on page level now always override parent stylesheets if they set the same properties. Before this fix this was only happening when the stylesheet was added to the page after the parent was set

Before/After Screenshots

Not applicable

Testing Procedure

Included two new unit test scenario's

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

chris added 2 commits July 2, 2019 12:01
…esheet to child elements to the Element class.

It would be weird for a stylesheet to get all parent stylesheets of an element and call Apply on them too. I think it's a good thing that StyleSheet.Apply(Element) really only applies it on the given Element? The main issue was that when the ApplyParentStyleSheets was called, the stylesheet of the element itself wasn't applied, so you could never override parent style elements
@csteeg csteeg changed the base branch from 4.0.0 to 4.1.0 July 3, 2019 15:20
@csteeg
Copy link
Contributor Author

csteeg commented Jul 3, 2019

Hmm, rebase to 4.1 tag seems to include some extra commits .. is that correct or do I need to change something?

@StephaneDelcroix StephaneDelcroix self-assigned this Jul 3, 2019
@samhouts samhouts changed the base branch from 4.1.0 to master July 3, 2019 23:43
@samhouts
Copy link
Member

samhouts commented Jul 4, 2019

@csteeg Try rebasing again, this time on master. Thanks!

@jfversluis
Copy link
Member

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

Assert.That((page.Content as Label).BackgroundColor, Is.EqualTo(Color.Blue));
}

[Test]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the original issue I see two cases defined:

  • Styles in the page resources should precede the style in the app
  • Styles in a subview should precede the style in the superview

I see cases for the page resource and app scenarios, but not the other one. Would it be useful to add those?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an extra test

@@ -1,5 +1,6 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a couple of LINQ queries happening here. Can we do without? It might have impact on the performance here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the .concat, but left the .Any(), hope that's ok

@jfversluis
Copy link
Member

Build failure is an unrelated (unit) test

@samhouts samhouts added this to In Review in v4.3.0 Jul 8, 2019
@jsuarezruiz
Copy link
Contributor

@csteeg Can you review the conflict?. Thanks!

# Conflicts:
#	Xamarin.Forms.Core/StyleSheets/StyleSheet.cs
@csteeg
Copy link
Contributor Author

csteeg commented Aug 14, 2019

@csteeg Can you review the conflict?. Thanks!

Fixed

@samhouts samhouts removed the p/UWP label Aug 16, 2019
@csteeg
Copy link
Contributor Author

csteeg commented Aug 22, 2019

Are there actions left that I need to take care off @samhouts and @StephaneDelcroix ?

@samhouts
Copy link
Member

@csteeg No, nothing needed from you just yet! Thanks!!

@samhouts samhouts added this to In Review in v4.4.0 Aug 30, 2019
@samhouts samhouts removed this from In Review in v4.3.0 Sep 3, 2019
@samhouts samhouts added Core and removed Core labels Sep 5, 2019
@samhouts samhouts added in-progress This issue has an associated pull request that may resolve it! and removed in-progress This issue has an associated pull request that may resolve it! labels Oct 2, 2019
@samhouts samhouts removed this from In Review in v4.4.0 Nov 2, 2019
@samhouts samhouts added this to In Review in v4.5.0 Nov 8, 2019
@samhouts samhouts added this to In Review in v4.6.0 Feb 7, 2020
@samhouts samhouts removed this from In Review in v4.5.0 Feb 13, 2020
@samhouts samhouts removed this from In Review in v4.6.0 Mar 2, 2020
@samhouts samhouts added this to In Review in vCurrent (4.8.0) Mar 27, 2020
@samhouts samhouts added the approved Has two approvals, no pending reviews, and no changes requested label Mar 28, 2020
@samhouts
Copy link
Member

samhouts commented Apr 3, 2020

Build is failing:

"D:\agent\1\s\Xamarin.Forms.sln" (Build target) (1) ->
"D:\agent\1\s\Xamarin.Forms.ControlGallery.Android\Xamarin.Forms.ControlGallery.Android.csproj" (default target) (10) ->
"D:\agent\1\s\Xamarin.Forms.Controls\Xamarin.Forms.Controls.csproj" (default target) (11:2) ->
"D:\agent\1\s\Xamarin.Forms.Build.Tasks\Xamarin.Forms.Build.Tasks.csproj" (default target) (19) ->
"D:\agent\1\s\Xamarin.Forms.Build.Tasks\Xamarin.Forms.Build.Tasks.csproj" (Build target) (19:2) ->
"D:\agent\1\s\Xamarin.Forms.Core\Xamarin.Forms.Core.csproj" (default target) (3:16) ->
  MergedStyle.cs(73,21): error CS1061: 'Element' does not contain a definition for 'ApplyStyleSheetsOnParentSet' and no accessible extension method 'ApplyStyleSheetsOnParentSet' accepting a first argument of type 'Element' could be found (are you missing a using directive or an assembly reference?) [D:\agent\1\s\Xamarin.Forms.Core\Xamarin.Forms.Core.csproj]


@rmarinho rmarinho merged commit a3e408a into xamarin:master Apr 5, 2020
vCurrent (4.8.0) automation moved this from In Review to Done Apr 5, 2020
@samhouts samhouts added this to Done in Sprint 168 Apr 30, 2020
@samhouts samhouts added this to the 4.7.0 milestone May 12, 2020
@samhouts samhouts removed this from Done in vCurrent (4.8.0) May 12, 2020
@samhouts samhouts added this to Done in 4.7.0 Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a/CSS a/style approved Has two approvals, no pending reviews, and no changes requested Core in-progress This issue has an associated pull request that may resolve it! proposal-open t/enhancement ➕
Projects
No open projects
4.7.0
  
Done
Sprint 168
  
Done
Development

Successfully merging this pull request may close these issues.

Can't overwrite styles when using CSS
6 participants