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

Update ex css to 4.2.1 (In preparation for using it to evaluate css selector) #1083

Closed
wants to merge 5 commits into from

Conversation

inforithmics
Copy link
Contributor

Reference Issue

#1073

What does this implement/fix? Explain your changes.

Updates ExCss to the newest Version and fixes Unit Tests for that.

Copy link
Contributor Author

@inforithmics inforithmics left a comment

Choose a reason for hiding this comment

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

I made a self review to explain the changes.

@@ -444,7 +444,7 @@ private static T Create<T>(XmlReader reader) where T : SvgDocument, new()
if (styles.Any())
{
var cssTotal = string.Join(Environment.NewLine, styles.Select(s => s.Content).ToArray());
var stylesheetParser = new StylesheetParser(true, true);
var stylesheetParser = new StylesheetParser(true, true, tolerateInvalidValues: true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TolarateInvalidValues is needed so that Invalid Property Values are still read. Could be Too that ExCss is not correctly annotated yet.

This fixed a Bug: But now we have to set tolarateInvalidValues to still get all values
TylerBrinks/ExCSS@cea80cc

@@ -26,8 +26,8 @@ public void TestOpenWithEntities()

var svgPath = Path.Combine(TestsRootPath, EntitiesSampleSvgPath);
var doc = SvgDocument.Open<SvgDocument>(svgPath, entities);
Assert.That(doc.Children[0].Children[0].Fill, Is.EqualTo(new SvgColourServer(Color.Red)));
Assert.That(doc.Children[0].Children[1].Fill, Is.EqualTo(new SvgColourServer(Color.Yellow)));
Assert.That(doc.Children[0].Children[0].Fill, Is.EqualTo(new SvgColourServer(Color.FromArgb(255, 0 ,0))));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Colors are now in ARGB values so I have to use this to Correctly Compare them

@@ -66,16 +66,16 @@ public void Lexer_FileWithInvalidHex_ColorTagOnlyReverstToDefaultForTheInvalidTa
var doc = GenerateLexerTestFile("fill: #ff00; stroke: #00ff00");
var path = doc.GetElementById<SvgPath>("path1");
// default fill color is Black
Assert.AreEqual(System.Drawing.Color.Black, ((SvgColourServer)path.Fill).Colour);
Assert.AreEqual(System.Drawing.Color.Lime, ((SvgColourServer)path.Stroke).Colour);
Assert.AreEqual(System.Drawing.Color.FromArgb(0,255,255,0), ((SvgColourServer)path.Fill).Colour);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

4Hex values are now valid colors

TylerBrinks/ExCSS@cb19e8f

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Thanks! Can you also an entry in the release notes?

@H1Gdev
Copy link
Contributor

H1Gdev commented Aug 30, 2023

Input values for tests have changed.
Does this mean that the current input does not work ?

@inforithmics
Copy link
Contributor Author

I think only the representation has changed
Red == System.Drawing.Color.FromArgb(255,0,0) Because it is now parsed as a color before it was parsed as Any (which probably parsed it as string) to Red, Now it seems to parse it as color and setting the Colors Directly.

var doc = GenerateLexerTestFile("fill: #ff00; stroke: #00ff00");
Because it can now parse the Fill Color as Color.FromArgb(255,255,0)

@H1Gdev
Copy link
Contributor

H1Gdev commented Aug 30, 2023

I think this change will have a large impact on users.
In tests, the value obtained from the Fill property is changing.

By the way, if you update to 4.2.0, there is no need to change the test. Do I have to update now ?
I am currently in the process of considering the latest support.

@mrbean-bremen
Copy link
Member

I think this change will have a large impact on users.

True, I haven't considered this. I think we still want to have this change, as it is fixing stuff, but communicate the breaking change clearly to the user, and maybe think about a major release because of it (I'm unsure about this).

By the way, if you update to 4.2.0, there is no need to change the test. Do I have to update now ?

It is a bit unfortunate that the color fix in ExCSS was made in a minor patch release and went a bit unnoticed, but as I said - it fixes color handling, so this is something we want to have.

One possibility is to update to 4.2.0, make a patch release, and later update to 4.2.1 in preparation for a major (or minor?) release.

@H1Gdev
Copy link
Contributor

H1Gdev commented Aug 31, 2023

I think purpose is to remove Fizzler dependency from SVG.NET.
(I agree that there will be less dependence.)

I think v4.2.0 can also achieve this purpose.

If we use 4.2.1 as in this PR, I think you need to review handling of colors in SVG.NET (file output, etc.).

@inforithmics
Copy link
Contributor Author

inforithmics commented Aug 31, 2023

@H1Gdev updating to v4.2.0 as an intermediatary step would work, but finally I need to a yet to be realeased Version Greater than 4.2.1 Because of this.

TylerBrinks/ExCSS#160

Because without that I cannot evalute the Selector. (Theoretically I could I would have to Parse the ComplexSelector).

@inforithmics
Copy link
Contributor Author

I think the issue that's need investigating is.

var doc = GenerateLexerTestFile("fill: #ff00; stroke: #00ff00");

The other color parsing changes simply Set the RGB Colors instead of the KnownColor name
ARBG(0,255,0,0) instead of red, which shouldn't cause problems.

I think I will debug through what happens here.
var doc = GenerateLexerTestFile("fill: #ff00; stroke: #00ff00");

@inforithmics
Copy link
Contributor Author

inforithmics commented Aug 31, 2023

I debugged it through and there are really 4 hex Css Colors (8Bit per color) so 4 Hex values is ARGB 8Bit Color, So the behavior is correct.

https://www.quackit.com/css/color/values/css_hex_color_notation_4_digits.cfm

I added a Unit Test to Validate the Default Color Behavior. with an Invalid 2Hex Color

@inforithmics inforithmics changed the title Update ex css to 4.2.1 (In preparation for using it to evaluate css Update ex css to 4.2.1 (In preparation for using it to evaluate css Selector) Aug 31, 2023
@inforithmics inforithmics changed the title Update ex css to 4.2.1 (In preparation for using it to evaluate css Selector) Update ex css to 4.2.1 (In preparation for using it to evaluate css selector) Aug 31, 2023
@inforithmics inforithmics mentioned this pull request Sep 2, 2023
6 tasks
@H1Gdev H1Gdev mentioned this pull request Sep 6, 2023
@inforithmics
Copy link
Contributor Author

Closing this in favor of this
#1091

@inforithmics inforithmics mentioned this pull request Sep 9, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants