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

AngleSharp/Devel breaks the HTMLSanitizer master branch- is there any fork I can use? #117

Closed
vivekjMSFT opened this issue Mar 21, 2017 · 23 comments

Comments

@vivekjMSFT
Copy link

Hi,
I am trying to take AngleSharp/Devel branch but I see HTMLSanitizer breaks since it doesn't find ICssPageRule, ICssKeyframesRule,ICssKeyframeRule etc. Is there any working branch of HTMLSanitzer with latest AngleSharp/Devel code which I can leverage?

@vivekjMSFT
Copy link
Author

vivekjMSFT commented Mar 21, 2017

Since v0.10 is getting delayed more, is there a way I can take these fixes (AngleSharp/AngleSharp#406) and (AngleSharp/AngleSharp#544) and merge devel to HTMLSanitzer? I tried merging devel but its giving massive errors around CCS related stuff.

@tiesont
Copy link

tiesont commented Mar 22, 2017

Well, AngleSharp seems like to keep breaking things, which is why HtmlSanitizer pins the maximum version it can be used with (see #87). I'm sure @mganss is at least tangentially aware of these problems, but it takes time to work around these problems. Is there something specific you need from version 0.9.9 that 0.9.8 doesn't provide?

As far as I've tracked, it seems like a lot of the current batch of problems are because AngleSharp is splitting the CSS-related stuff into a separate library, They really should be doing that on a major version update, but it's obviously their code - they can choose how closely they want to follow semver.

@vivekjMSFT
Copy link
Author

I think @mganss has already released with 0.9.9. Even with angle sharp 0.9.9 , the issue of stack overflow exception exist. I just submitted a PR for angle sharp master branch with a fix. Since it's a catastrophic failure it should be released with minor version. Let's see. There is another big issue around dictionary getting corrupted that should also get fixed. I will give it shot too , since it's taking forever to release v0.10. By the way what's the plan for htmlsanitizer to take v0.1.0 version since it's going to introduce lot of breaking changes and it has huge amount of critical bug fixes.

@tiesont
Copy link

tiesont commented Mar 22, 2017

I would think that not much would change with HtmlSanitizer - it only really has two public methods, which is what the majority of developers are probably using, and the public properties and events have nothing to do with AngleSharp. The only thing that could possibly break is if the IMarkupFormatter interface gets removed or renamed, since that's part of the signature for the public methods. Everything else is internal, so for users of the library any required updates for AngleSharp should be transparent.

@vivekjMSFT
Copy link
Author

is it possible to someone fork HTMLSanitizer and merge with AngleSharp's current devel until they release v0.10? I tried but I am hitting lot of CSS related issues since they moved CSS stuff around. I am still new to the codebase thus it might take me forever to fix these issues. For old timers it might be a breeze to fix these integration issues. Anyway that has to be done for v01.0. make sense?

@mganss
Copy link
Owner

mganss commented Mar 23, 2017

I've made the AngleSharp_0_10 branch. The CSS related stuff is now split into its own NuGet package AngleSharp.Css.

There are around 60 tests failing, mostly 💄 issues, but I haven't investigated further.

@tiesont
Copy link

tiesont commented Mar 23, 2017

@mganss I've looked over most of the failing tests. It looks like most are either empty attributes (style="") or effectively empty attributes (style="background: initial" doesn't really accomplish anything). Is it your preference that the tests pass as-is, or should the tests be updated to reflect the (as far as I can tell) "safe" content that the CSS parser is generating?

@304NotModified
Copy link
Contributor

304NotModified commented Mar 23, 2017

I think the owner of AngleSharp also appreciate any help with the 0.10 release ;)

See AngleSharp/AngleSharp#544 (comment)

@mganss
Copy link
Owner

mganss commented Mar 24, 2017

@tiesont Just update the expected output to match the new actual output.

@tiesont
Copy link

tiesont commented Mar 24, 2017

@mganss I'm not entirely sure that's the way to go. Some of the resulting outputs are odd (IMHO). For instance, why are all colors transformed into RGBA - color: black, for instance, becomes color: rgba(0, 0, 0, 1)? I can understand some of them - background: 0; means the same thing as background: left, but why transform it?

Obviously AngleSharp.Css is more or less alpha at this point, but if it helps you then I'll put some effort into getting the tests updated.

@mganss
Copy link
Owner

mganss commented Mar 24, 2017

@tiesont Don't bother. IMHO we should wait until AngleSharp.Css is stable before we put more effort into adapting to it.

@vivekjMSFT
Copy link
Author

so, looking at the answers in AngleSharp/AngleSharp.Css#9
if we fix the test cases then we are better off with this branch - AngleSharp_0_10 , having all the angleSharp critical fixes?

@mganss
Copy link
Owner

mganss commented Jun 8, 2017

Yes, but you'll have to build your own packages of AngleSharp, AngleSharp.Css, and HtmlSanitizer. Plus, this branch is currently behind master. I'd merge from master back into this branch but currently, I can't even get the devel branch of AngleSharp to build (AppVeyor build is broken, too).

@vivekjMSFT
Copy link
Author

@mganss how do I get the "AngleSharp.Css"? I got the AngleSharp devel branch but it doesn't seems to have this AngleSharp.Css build separately. did you merged the master to "AngleSharp_0_10" already ? Looks like this is best path forward, to fork and take angleSharp devel and use AngleSharp V0.10 avoid these catastrophic errors.

@vivekjMSFT
Copy link
Author

Also, any idea how can i build a non-portable version of anglesharp?

@304NotModified
Copy link
Contributor

@VivekScripts I think you should ask that at Anglesharp?

@vivekjMSFT
Copy link
Author

OK. Finally I was able to build both AngleSharp and AngleSharp.css and build HTMLSanitizer with it. I see some 61 tests are failing.

Ex: - BaseTagXSSTest
it generates exception in AngleSharp :-(

@tiesont
Copy link

tiesont commented Jul 14, 2017

@VivekScripts If you look further up the comments, failing tests is a known issue. The tests won't be corrected until the AngleSharp changes become official (when 0.10.0 is released). You can unload the tests project if you're trying to just build the whole solution instead of individual projects.

@vivekjMSFT
Copy link
Author

Yes, I understand that. However, I mentioned it's just not about the expected output and fixing the tests, this test fails with an exception in anglesharp. did you guys also see this kind of test failed or its something at my end?

@tiesont
Copy link

tiesont commented Jul 14, 2017

@VivekScripts If something @mganss does in the tests is causing an exception, that's a problem with AngleSharp - upgrading that library shouldn't dictate that HtmlSanitizer changes how it's API works.

The specific test you reference is fairly simple, so if AngleSharp is blowing up there, that's definitely a problem, but I wouldn't expect @mganss to have anything to fix - that's an upstream issue.

@vivekjMSFT
Copy link
Author

Agree. Was just asking if @mganss also see this test failed at his end or not. if yes, there is no point running after the devel version of anglesharp since its blowing up this simple test as well.

@mganss
Copy link
Owner

mganss commented Jul 17, 2017

@VivekScripts What's your build setup? I can't even get AngleSharp devel to build (using build.ps1). I get the same error as shown in the AppVeyor build log.

@mganss
Copy link
Owner

mganss commented Jan 7, 2019

Closing this, discussion about AngleSharp 0.10.0 continues at #154.

@mganss mganss closed this as completed Jan 7, 2019
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

No branches or pull requests

4 participants