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
papandreou
force-pushed
the
feature/preserveOddballWhitespace
branch
2 times, most recently
from
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.
preserveLineBreaks doesn't seem to be required here?
There was a problem hiding this comment.
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! |
papandreou
force-pushed
the
feature/preserveOddballWhitespace
branch
from
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 |
2 participants
There are some "precious" space characters matched by /\s/ that should never be collapsed or trimmed away.
See assetgraph/assetgraph#778