-
Notifications
You must be signed in to change notification settings - Fork 572
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
Preserve en/em/non-breaking/hair space etc. while minifying #849
Preserve en/em/non-breaking/hair space etc. while minifying #849
Conversation
There are some "precious" space characters matched by /\s/ that should never be collapsed or trimmed away. See assetgraph/assetgraph#778
According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#special-white-space Maybe the The White Space Processing Rules says:
... so at least form feed ( |
I prefer only changing the For instance, https://github.com/kangax/html-minifier/pull/849/files#diff-bd35077e6c438d3f6866b57bb2481260L33 isn't necessary. (I have tested on a few web browsers and agree that the issue illustrated by your test cases need to be fixed.) |
Just to be sure that I understand your POV correctly -- isn't that actually an argument for adding test cases that validate that and the other replacement? |
@papandreou I see - if you can have the new test cases cover all the replacements of |
…cifically handled in the replacer
dab9ad2
to
1d600cc
Compare
Okay, I added some more tests for the cases I could find, then rolled back to However, I'm fairly sure that some of those tests can be written, so I probably wouldn't include 1d600cc -- those other kinds of spaces should basically be treated as regular glyphs, so I think |
I prefer changing them only if/when we hit an issue, and would therefore has test coverage for it. The new tests & rollbacks LGTM - if there isn't anything else I shall go ahead & merge. |
tests/minifier.js
Outdated
|
||
// Preserve hair space in attributes: | ||
input = '<p class="foo\u200abar"></p>'; | ||
assert.equal(minify(input, { collapseWhitespace: true, preserveLineBreaks: true }), input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preserveLineBreaks
doesn't seem to be required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, fixed + rebased.
…ttributes Unfortunately this means we'll have to abandon String#trim as it seems to be implemented as str.replace(/^\s+|\s+$/g, '') Also, test preservation of oddball whitespace in class names when deduplicating and reordering
Nothing further for now. I might pick it up again when I get some time :) |
@papandreou I shall merge after your rebase is pushed out then (GitHub having a slow day?) Thanks a lot for the patch! |
1d600cc
to
c3d239d
Compare
@alexlamsl Hah, I don't know where I tried to push to before, but here it is :) |
@alexlamsl Thanks! Can I persuade you to do a new release with this fix? |
That's the plan - I went straight to bed after merging your PR 😅 Give me a few hours while I sort out my $day_job, then I'll make |
There are some "precious" space characters matched by /\s/ that should never be collapsed or trimmed away.
See assetgraph/assetgraph#778