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

Remove Fizzler V3 #1092

Merged
merged 33 commits into from
Oct 9, 2023
Merged

Conversation

inforithmics
Copy link
Contributor

@inforithmics inforithmics commented Sep 9, 2023

Reference Issue

#1073

What does this implement/fix? Explain your changes.

remove Fizzler implements a QuerySelectAll that uses the ExCSS Selector Dom

Any other comments?

This pull request is based on this pull request
#1083

TODO:

  • Pass All W3C Images with the same Selector results.
  • Add more Unit Tests
  • Add Benchmarks
  • Implement not Implemented Functions
  • Investigate AngleSharp Selector Integration (nuget or Code)
  • Add more Selector Tests (Selector patterns from AngleSharp Unit Tests)
  • Fix nth-child(2) (needs this pull request for evaluating nth-child(2) make step and offset visible TylerBrinks/ExCSS#162)
  • Implement :not (Is Implemented in Fizzler 1.3.0)
  • Implement nth-type and nth-last-type
  • Investigate :target handling (I think in the context of svg rendering this isn't useful, so this is not implemented, because target only works with the selected url hash)
  • Investigate :root handling

A new Pull Request based on this.
#1086

Main Idea:
The idea is to Implement an IExCssSelectorOps Interface similar interface to IElementOps so that I can regression test to Fizzler Implementation. The ExCss Query tries to map the Selectors to the IExCssSelectorOps Interface, Because Fizzler has not implemented all Css Selectors certain Selectors are not implemented.

List of css selectors which should be supported.
https://stackoverflow.com/questions/42230563/what-is-the-compatibility-of-css3-pseudoclasses-with-svg-renderable-elements
All are handled (:target and :root in need to investigate)

@inforithmics
Copy link
Contributor Author

I have enough Implemented for AngleSharp to run the Benchmark. here are the numbers (Maybe I need some more optimization)

Method Mean Error StdDev Op/s Rank Gen0 Gen1 Allocated
SelectorPerformanceExCss 5.188 us 0.0733 us 0.0685 us 192,739.3 1 2.6550 0.0153 21.7 KB
SelectorPerformanceFizzler 21.200 us 0.2856 us 0.2672 us 47,169.1 2 3.7537 - 30.9 KB
SelectorPerformanceAngleSharp 264.828 us 5.1335 us 5.7059 us 3,776.0 3 49.8047 2.9297 408.43 KB

There is certainly room for improvements in the AngleSharp Implementation. But I don't know if I can find there Factor 50.

@inforithmics inforithmics mentioned this pull request Sep 9, 2023
6 tasks
nth-child(2) does not work yet need to make step and offset visible
@inforithmics
Copy link
Contributor Author

inforithmics commented Sep 16, 2023

Implmented N th Child and N th Last Child with reflection, if the Properties become public in excss than this refelction Code can be changed to Field, Property Access.

TylerBrinks/ExCSS#162

All Css Features that Fizzler supported are now supported. By the Excss Evalutor.

  1. I test css selectors from AngeSharp so that the behave the same in Fizzler and excss.
  2. Parse all svg documents and runt the css Selectors to test if they behave the same.
  3. To reduce the number of dependencies I didn't further investigate the AngleSharp path.

@inforithmics inforithmics marked this pull request as ready for review September 16, 2023 19:32
TypeSelector typeSelector => ops.Type(typeSelector.Name),
UnknownSelector unknownSelector => throw new NotImplementedException(), // TODO:,
IdSelector idSelector => ops.Id(idSelector.Id),
_ => throw new NotImplementedException(), // TODO:,
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the todos and add one more case that is not implemented. For the not typed selectors I have to look into excss what kind of selectors this is.

@@ -421,6 +462,7 @@ private static T Create<T>(XmlReader reader) where T : SvgDocument, new()
{
styles.Add(unknown);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

AttrHyphenSelector attrHyphenSelector => ops.AttributeDashMatch(attrHyphenSelector.Attribute, attrHyphenSelector.Value),
AttrListSelector attrListSelector => ops.AttributeIncludes(attrListSelector.Attribute, attrListSelector.Value),
AttrMatchSelector attrMatchSelector => ops.AttributeExact(attrMatchSelector.Attribute, attrMatchSelector.Value),
AttrNotMatchSelector attrNotMatchSelector => ops.AttributeNotMatch(attrNotMatchSelector.Attribute, attrNotMatchSelector.Value), // TODO:,
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed because it is now implemented

@mrbean-bremen
Copy link
Member

From my perspective this looks quite good, but I'm not really qualified to review this. @wieslawsoltes and @H1Gdev - please check if this can be merged.

@H1Gdev
Copy link
Contributor

H1Gdev commented Sep 30, 2023

@inforithmics
What is AngleSharp used for ?

@inforithmics
Copy link
Contributor Author

Currently nothing, I used it for the Benchmarks. But when I saw that it was not very Performant I didn't continue investigating it. So I remove the AngleSharp Code from this pull request.

@inforithmics
Copy link
Contributor Author

So I reduced the changes and removed AngleSharp Code

@inforithmics
Copy link
Contributor Author

inforithmics commented Oct 1, 2023

I Implemented the :not Css Selector because in newest Fizzler 1.3.0 this was implemented (To keep feature parity).

@@ -36,11 +36,12 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.8.3" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.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.

16.8.3 has a security vulnerability so I upgraded to 16.11.0 to silence the warning.

<PackageReference Include="NUnit" Version="3.13.0" />
<PackageReference Include="NUnit3TestAdapter" Version="3.17.0" />
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" PrivateAssets="All" />
<PackageReference Include="Moq" Version="4.16.1" />
<PackageReference Include="Fizzler" Version="1.3.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.

Upgraded to newest fizzler (for regression tests)

@@ -177,7 +177,7 @@ private IEnumerable<SvgElement> ElementsAfterSelf(SvgElement self)

public Selector<SvgElement> NthLastChild(int a, int b)
{
throw new NotImplementedException();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nlast child was not implemented previously is now implemented.

{
var sel = selector.Class.Substring(PseudoClassNames.Not.Length + 1, selector.Class.Length - 2 - PseudoClassNames.Not.Length);
var parser = new StylesheetParser(true, true, tolerateInvalidValues: true);
var styleSheet = parser.Parse(sel);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

excss does not fully parse the not pseudofunc so I have to parse it myself here.

@@ -27,5 +27,4 @@ This project has dependencies on other open-source projects. These projects are

|Project|Author|Sources|License|
|--------|-----|---|---------|
|Fizzler|Atif Aziz (@atifaziz)|[GitHub](https://github.com/atifaziz/Fizzler)|[LGPL](https://github.com/atifaziz/Fizzler/blob/master/COPYING.txt)|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is now removed from the nuget package as dependency is now only used as unit test dependency.

@mrbean-bremen mrbean-bremen merged commit 1343b56 into svg-net:master Oct 9, 2023
github-actions bot pushed a commit that referenced this pull request Oct 9, 2023
…ators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt initial ExCss Implementation ported from RemoveFizzlerV2 Branch BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt remove Fizzler from Readme BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt updated Fizzler for regression tests to 1.3.0 and implemented :not Pseudofunction BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt implemented nth type and nth-last-type BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Implement :root
github-actions bot pushed a commit to H1Gdev/SVG that referenced this pull request Oct 9, 2023
…d Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt initial ExCss Implementation ported from RemoveFizzlerV2 Branch BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt remove Fizzler from Readme BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt updated Fizzler for regression tests to 1.3.0 and implemented :not Pseudofunction BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt implemented nth type and nth-last-type BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Implement :root
github-actions bot pushed a commit to inforithmics/SVG that referenced this pull request Oct 9, 2023
…d Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt initial ExCss Implementation ported from RemoveFizzlerV2 Branch BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt remove Fizzler from Readme BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt updated Fizzler for regression tests to 1.3.0 and implemented :not Pseudofunction BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt implemented nth type and nth-last-type BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Implement :root
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.

3 participants