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

Use native CSS parsing #9

Merged
merged 1 commit into from
Mar 20, 2016
Merged

Use native CSS parsing #9

merged 1 commit into from
Mar 20, 2016

Conversation

AkeemMcLennon
Copy link
Contributor

Hello @bitinn

I recently created a library similar to vdom-parser at https://github.com/AkeemMcLennon/dom2hscript

I noticed your version had bug where it failed to parse certain CSS properties that contained semicolons such a base64 encoded strings. This pull request uses the browser's native css parser to improve accuracy.

@AkeemMcLennon
Copy link
Contributor Author

It looks like the Zuul build is complaining about a lack of sauce labs credentials, but the tests seem to pass otherwise

@bitinn
Copy link
Owner

bitinn commented Sep 24, 2015

I believe I have avoided using native style object due to some reasons (likely virtual-dom related), but can't recall it yet. Still, thx for providing a test case :)

@AkeemMcLennon
Copy link
Contributor Author

@bitinn The unit test "'should parse bracket style attribute on node'" initially failed when I made the changes. However, changing el = doc.body.firstChild; to el = doc.getElementsByTagName('body')[0].firstChild; in the parser resolved the issue.

I suspect this may have been part of your initial rationale since that test seemed oddly specific.

@niksy
Copy link
Contributor

niksy commented Nov 22, 2015

This would be useful to have since it would solve the issue of not having any inline styles in IE8 (#7)

@bitinn
Copy link
Owner

bitinn commented Nov 23, 2015

@niksy yep I would like to work on this soon. though I am not sure we will ever get full support for IE8.

@niksy
Copy link
Contributor

niksy commented Nov 23, 2015

@bitinn I have some commits on my fork which normalize feature set for IE8 and for the project I’m currently working on vdom-parser works really great. I can submit PRs when you get time to work on this.

bitinn added a commit that referenced this pull request Mar 20, 2016
@bitinn bitinn merged commit e348db4 into bitinn:master Mar 20, 2016
@bitinn
Copy link
Owner

bitinn commented Mar 20, 2016

I figure out why I didn't use CSS parser after merging this: phantomjs actually output differently from chrome. especially on inline input like background: url(path). Chrome will change it to url("path") while phantomjs, without proper DOMParser support, return url(path).

Since this is intended for browser anyway, I will workaround the phantomjs issue...

@bitinn
Copy link
Owner

bitinn commented Mar 20, 2016

https://travis-ci.org/bitinn/vdom-parser/builds/117241478

Things are a lot worse than I remembered, Safari doesn't agree with Chrome. So we either generate additional patches for Safari (including iOS) or for Chrome (including android) using CSS parser.

Maybe I have to back out of this...

@bitinn
Copy link
Owner

bitinn commented Mar 20, 2016

landed in v1.3.0

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