handle whitespace within and around inline tags #519

alexlamsl · 2016-03-01T10:35:33Z

inspired by the tests in PHPTAL:

<div>a <a href=#> b </a> c</div>

should minify to:

<div>a <a href=#>b </a>c</div>

before this PR:

<div>a <a href=#>b</a> c</div>

note the whitespace placement before c.

One case we cannot handle optimally is:

<div> <span> a </span> </div>

with this PR this turns to:

<div><span>a </span></div>

even though we should be able to remove that final whitespace safely.

alexlamsl · 2016-03-01T11:41:35Z

Did I say we have a sub-optimal case? 😏

(As it turns out, we don't need to flush buffer to results at all, so that helps.)

kangax · 2016-03-01T19:21:28Z

dist/htmlminifier.js

            text = prevTag || nextTag ? collapseWhitespaceSmart(text, prevTag, nextTag, options) : trimWhitespace(text);
+            if (!text && / $/.test(currentChars) && prevTag && prevTag.charAt(0) === '/') {


Are you sure we don't want \s$ instead of $?

I'm thinking of collapsed whitespaces always getting turned into - but I guess no harm making them\s

kangax · 2016-03-01T19:22:29Z

Do we know how this affects minifier performance?

alexlamsl · 2016-03-01T19:40:30Z

I've run benchmark.js and no noticeable performance difference is observed.

In terms of minified sizes, they stayed almost the same - eloquentjavascript got <1KB bigger due to lots of:

<a>P </a>

es6 got <1KB smaller due to consecutive trailing spaces being optimised.

alexlamsl · 2016-03-01T19:56:47Z

@kangax comments addressed 😉

kangax · 2016-03-01T20:50:26Z

Does it mean that "eloquentjavascript" was rendering incorrectly before? With some spaces removed?

kangax · 2016-03-01T20:51:33Z

tests/minifier.js

@@ -658,7 +672,7 @@
    equal(minify(input, { collapseWhitespace: true }), output);

    input = '<p> foo    <span>  blah     <i>   22</i>    </span> bar <img src=""></p>';
-    output = '<p>foo <span>blah <i>22</i></span> bar <img src=""></p>';
+    output = '<p>foo <span>blah <i>22</i> </span>bar <img src=""></p>';


Hm, why is this space necessary here?

It's removing the space after </span> rather than the one before.

If there isn't any space before </span> then this wouldn't have happened.

handle whitespace within and around inline tags

alexlamsl · 2016-03-01T20:57:21Z

Yes - those spaces were significant, so removing them packed those links too close to normal text after them.

XhmikosR · 2016-03-21T10:49:22Z

@alexlamsl: are you sure this change doesn't break things? I notice differences in a project of mine, namely https://github.com/mpc-hc/mpc-hc.org (run it with grunt server). I had to turn on conservativeCollapse to get the same output as with the older version. Look for the spaces around the font awesome icons.

XhmikosR · 2016-03-21T10:57:30Z

I also notice spaces before the end closing bracket, like

<a href=/acknowledgments/ >

I don't remember this one happening with the old version.

EDIT: happens with 1.2.0 too.

alexlamsl · 2016-03-21T11:38:07Z

<a href="/acknowledgements/"> should transform to <a href=/acknowledgements/ >, as <a href=/acknowledgements/> would cause the final slash to be confused as a closing slash for XML.

And I don't think this PR is the one that changes this IIRC...

XhmikosR · 2016-03-21T11:39:10Z

But it's not XML.

alexlamsl · 2016-03-21T11:42:06Z

Found it: f8db419

The old behaviour is to retain the quotes as it'd cause some browsers to consume that as />. The new behaviour saves one byte by removing the quotes but add a trailing whitespace to avoid parsing errors.

alexlamsl · 2016-03-21T11:44:11Z

As in, <a href=/acknowledgements/> will get confused as <a href="/acknowledgements"/> in some browsers, hence <a href=/acknowledgements/ > is the shortest representation that is also safe.

I'll have a go at your project now - is there a specific link you'd want me to visit after grunt serve?

XhmikosR · 2016-03-21T11:45:00Z

It's non standard behavior if "some" browsers behave like that. And HTML is not XML so the specs are clear. Anyway, that issue is a separate one. I'm more concerned about the whitespace issue I mention #519 (comment).

XhmikosR · 2016-03-21T11:45:56Z

Just any page, but it's clearer in the homepage in the news post icons.

Note that I have already reverted to the old version in master so in order to see what I mean you will need to set grunt-contrib-htmlmin to ^1.0.0 or 1.1.0.

alexlamsl · 2016-03-21T11:48:27Z

Thanks, I'll investigate now - I guess I can roll back and forth between the versions to see the difference as well...

alexlamsl · 2016-03-21T12:02:44Z

Feels like an idiot, but... 😓

> grunt serve

Warning: Task "serve" not found. Use --force to continue.

Aborted due to warnings.


Execution Time (2016-03-21 12:01:01 UTC)
loading tasks  3ms  ████████████75%
Total 4ms

Edit: I guess it's grunt server...

alexlamsl · 2016-03-21T12:08:43Z

@XhmikosR I know I'm probably being annoying now, but is there a way I can test without installing ruby and jekyll and possibly more which I don't have on my PC? 😰

alexlamsl · 2016-03-21T12:25:57Z

Okay, so I visit https://mpc-hc.org/ on Chrome 49:

grab document.body.innerHTML
run it on http://kangax.github.io/html-minifier/ with most (safer) options and without conservativeCollapse
paste the results back into document.body.innerHTML

And I compare it with another tab of the original site. The only thing I've noticed so far are the disappearing whitespace between fa icons and the following text.

Are there other discrepancies I should be paying attention to as well?

XhmikosR · 2016-03-21T12:27:16Z

It's not possible to build without Ruby, sorry.

But what you see is the issue I described, yes. There might be more, but the font awesome ones were the obvious.

alexlamsl · 2016-03-21T12:30:07Z

The whitespace here is being removed </span> Downloads - I'll track it down and try it and report back 😉

alexlamsl added 3 commits March 1, 2016 23:08

handle whitespace within and around inline tags

b0dc2cb

use single array to store minification fragments

ac3aded

improve handling of trailing inline whitespace

fdbc328

kangax reviewed Mar 1, 2016
View reviewed changes

alexlamsl added 2 commits March 2, 2016 03:52

more permissive trailing whitespace detection

7ff2229

use String.slice() instead of substr()

3c17af6

kangax reviewed Mar 1, 2016
View reviewed changes

kangax added a commit that referenced this pull request Mar 1, 2016

Merge pull request #519 from alexlamsl/inline-whitespace

2b530b3

handle whitespace within and around inline tags

kangax merged commit 2b530b3 into kangax:gh-pages Mar 1, 2016

alexlamsl mentioned this pull request Mar 8, 2016

Changelog for 1.3.0 #547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle whitespace within and around inline tags #519

handle whitespace within and around inline tags #519

alexlamsl commented Mar 1, 2016

alexlamsl commented Mar 1, 2016

kangax Mar 1, 2016

alexlamsl Mar 1, 2016

kangax commented Mar 1, 2016

alexlamsl commented Mar 1, 2016

alexlamsl commented Mar 1, 2016

kangax commented Mar 1, 2016

kangax Mar 1, 2016

alexlamsl Mar 1, 2016

alexlamsl commented Mar 1, 2016

XhmikosR commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

		text = prevTag \|\| nextTag ? collapseWhitespaceSmart(text, prevTag, nextTag, options) : trimWhitespace(text);
		if (!text && / $/.test(currentChars) && prevTag && prevTag.charAt(0) === '/') {

handle whitespace within and around inline tags #519

handle whitespace within and around inline tags #519

Conversation

alexlamsl commented Mar 1, 2016

alexlamsl commented Mar 1, 2016

kangax Mar 1, 2016

Choose a reason for hiding this comment

alexlamsl Mar 1, 2016

Choose a reason for hiding this comment

kangax commented Mar 1, 2016

alexlamsl commented Mar 1, 2016

alexlamsl commented Mar 1, 2016

kangax commented Mar 1, 2016

kangax Mar 1, 2016

Choose a reason for hiding this comment

alexlamsl Mar 1, 2016

Choose a reason for hiding this comment

alexlamsl commented Mar 1, 2016

XhmikosR commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

alexlamsl commented Mar 21, 2016

XhmikosR commented Mar 21, 2016

alexlamsl commented Mar 21, 2016