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

[4.0-pre] parsing invalid input #847

Closed
alexlamsl opened this issue Dec 20, 2016 · 11 comments
Closed

[4.0-pre] parsing invalid input #847

alexlamsl opened this issue Dec 20, 2016 · 11 comments
Labels
Milestone

Comments

@alexlamsl
Copy link
Contributor

With the latest version, <![CDATA[p.b{background:red}]]> get parsed and gives <![CDATA[p.b{background:red} whereas 3.x will leave it as is.

Ideally it should throw an error, but in any case html-minifier has already dropped support for CDATA, so this isn't the end of the world but I thought I'll let you know of the behavioural difference.

@jakubpawlowicz
Copy link
Collaborator

Thanks @alexlamsl - it should definitely throw an error about invalid input.

@jakubpawlowicz jakubpawlowicz added this to the 4.0 milestone Dec 20, 2016
@alexlamsl
Copy link
Contributor Author

alexlamsl commented Dec 20, 2016

There is also *{font: &quot;monospace&#34;} which transforms into *{font:&quot monospace&#34} without any errors.

(The HTML entity characters are intentionally left not decoded.)

@alexlamsl
Copy link
Contributor Author

As of a239f68, *{font: &quot;monospace&#34;} now works (fails) the same way as 3.4.x

One down, one to go 👍

@jakubpawlowicz
Copy link
Collaborator

Thanks @alexlamsl for keeping a close eye on this!

jakubpawlowicz added a commit that referenced this issue Dec 23, 2016
Why:

* Browsers won't apply such rules so neither should we.
@jakubpawlowicz
Copy link
Collaborator

I've added more rules to ignoring invalid selectors but that list may not be exhaustive.

@alexlamsl
Copy link
Contributor Author

If I got this right, the current master <![CDATA[\np.a{background:red}\n]]> minifies to an empty string. In previous versions it would only replace those \n to whitespaces.

Also, this test now fails:

p.h{background:red}<!--
p.i{background:red}
-->p.j{background:red}

Expects

p.h{background:red}<!-- p.i{background:red}-->p.j{background:red}

Actual

p.h{background:red}-->p.j{background:red}

@alexlamsl
Copy link
Contributor Author

There isn't anything in the errors array either, so I can't really workaround these cases easily. 😕

@alexlamsl
Copy link
Contributor Author

Ah it's being reported as a warning instead. Skipping minification on sighting on any warnings seem a little bit of an overkill though?

@alexlamsl
Copy link
Contributor Author

I've put up a branch for illustrating the failures: https://github.com/kangax/html-minifier/tree/clean-css-4

After checking out and switching to the branch, just npm install then npm test to see the failures.

@jakubpawlowicz
Copy link
Collaborator

@alexlamsl yup, these are warnings for the reason you mentioned. If browsers safely ignore such rules so should we.

Thanks for the clean-css-4 branch. Will check out those failures.

@alexlamsl
Copy link
Contributor Author

@jakubpawlowicz so if I understand you correctly, with those chunks chopped off there won't be any loss of (CSS) functionality (read: broken the same way) in modern web browsers?

If so, I guess html-minifier can just work with that - after all, appearing the same way after minification in web browsers is the end goal. 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants