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

collapse_whitespace missing whitespace removal in one location #463

Closed
GoogleCodeExporter opened this Issue Apr 6, 2015 · 10 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter

GoogleCodeExporter commented Apr 6, 2015

trivial cosmetics ...

Running MPS r1686, enabling

    ModPagespeedEnableFilters  collapse_whitespace

works as expected except in one location (where the "!!"'s are ...),

    <!DOCTYPE html>
    <html lang=en dir=ltr>
    <head><script type='text/javascript'>window.mod_pagespeed_start = Number(new Date());</script>
    ...
    <link rel=stylesheet href="https://testxxxxx.net/sites/default/files/css/I.css_kSOiWCweo-F0v_YzsJEz8V4ZSYq1XRltxQdPIhpL6_w.css+css_EbwNfzJmadWvyfeNELiHJR6ebp0JXvNzdW-tKa2IC-w.css+css_TsVRTbLFUpEZAfw-_bWPJu840QT523CPjUVJ5MRWfyk.css+css_AAn076ZJGmT0gQpUuyKCeurBl7OwD1v9AoKwdA_eJpA.css,Mcc.9w4pcvTDkO.css.pagespeed.cf.IjF13lbwP3.css" media=all />
!!  
!!  
!!  
    <link rel=stylesheet href="https://testxxxxx.net/sites/default/files/css/css_Z-pdTCNASqqfCb0rEY9nK8-wC2rF3fBJgAm-HMT2XNY.css" media=all />
    <!--[if (lt IE 9)&(!IEMobile)]>
    <link rel="stylesheet" href="https://testxxxxx.net/sites/default/files/css/css_ocSxct__X363UErQwlAf_FAoNLh2eUzxqGX5SUvyfbI.css" media="all" />
    ...

Original issue reported on code.google.com by pgnet.dev on 12 Jul 2012 at 5:11

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

I think this is working as expected.  We are not going to change

...>
!!
!!
!!
<...

into

...>!!!!!!<...

What did you expect to occur?

Original comment by jmara...@google.com on 12 Jul 2012 at 5:29

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

I haven't requested that change.  "!!" is quite obviously NOT whitespace.  
Those "!!", as called out, are markers there to point to the lines in question 
...

What I'd 'expect' is that the filter consistely resomves all whitespace.

Instead of

    --------------------------------------------
    <link rel=stylesheet href="https://testxxxxx.net/sites/default/files/css/I.css_kSOiWCweo-F0v_YzsJEz8V4ZSYq1XRltxQdPIhpL6_w.css+css_EbwNfzJmadWvyfeNELiHJR6ebp0JXvNzdW-tKa2IC-w.css+css_TsVRTbLFUpEZAfw-_bWPJu840QT523CPjUVJ5MRWfyk.css+css_AAn076ZJGmT0gQpUuyKCeurBl7OwD1v9AoKwdA_eJpA.css,Mcc.9w4pcvTDkO.css.pagespeed.cf.IjF13lbwP3.css" media=all />



    <link rel=stylesheet href="https://testxxxxx.net/sites/default/files/css/css_Z-pdTCNASqqfCb0rEY9nK8-wC2rF3fBJgAm-HMT2XNY.css" media=all />
    --------------------------------------------

the result should be

    --------------------------------------------
    <link rel=stylesheet href="https://testxxxxx.net/sites/default/files/css/I.css_kSOiWCweo-F0v_YzsJEz8V4ZSYq1XRltxQdPIhpL6_w.css+css_EbwNfzJmadWvyfeNELiHJR6ebp0JXvNzdW-tKa2IC-w.css+css_TsVRTbLFUpEZAfw-_bWPJu840QT523CPjUVJ5MRWfyk.css+css_AAn076ZJGmT0gQpUuyKCeurBl7OwD1v9AoKwdA_eJpA.css,Mcc.9w4pcvTDkO.css.pagespeed.cf.IjF13lbwP3.css" media=all />
    <link rel=stylesheet href="https://testxxxxx.net/sites/default/files/css/css_Z-pdTCNASqqfCb0rEY9nK8-wC2rF3fBJgAm-HMT2XNY.css" media=all />
    --------------------------------------------

Original comment by pgnet.dev on 12 Jul 2012 at 5:34

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

> Instead of
> 
>         --------------------------------------------
> 
>         <link rel=stylesheet 
href="https://testxxxxx.net/sites/default/files/css/I.css_kSOiWCweo-F0v_YzsJEz8V
4ZSYq1XRltxQdPIhpL6_w.css+css_EbwNfzJmadWvyfeNELiHJR6ebp0JXvNzdW-tKa2IC-w.css+cs
s_TsVRTbLFUpEZAfw-_bWPJu840QT523CPjUVJ5MRWfyk.css+css_AAn076ZJGmT0gQpUuyKCeurBl7
OwD1v9AoKwdA_eJpA.css,Mcc.9w4pcvTDkO.css.pagespeed.cf.IjF13lbwP3.css" media=all 
/>
> 
> 
> 
>         <link rel=stylesheet 
href="https://testxxxxx.net/sites/default/files/css/css_Z-pdTCNASqqfCb0rEY9nK8-w
C2rF3fBJgAm-HMT2XNY.css" media=all />

I'll point out that I am fairly sure those blank lines are there because we've 
removed the combined CSS link elements. IIUC, previously the source was 
something like:

>         <link rel=stylesheet 
href="https://testxxxxx.net/sites/default/files/css/css_kSOiWCweo-F0v_YzsJEz8V4Z
SYq1XRltxQdPIhpL6_w.css" media=all />
>         <link rel=stylesheet 
href="https://testxxxxx.net/sites/default/files/css/css_EbwNfzJmadWvyfeNELiHJR6e
bp0JXvNzdW-tKa2IC-w.css" media=all />
>         <link rel=stylesheet 
href="https://testxxxxx.net/sites/default/files/css/css_TsVRTbLFUpEZAfw-_bWPJu84
0QT523CPjUVJ5MRWfyk.css" media=all />
>         <link rel=stylesheet 
href="https://testxxxxx.net/sites/default/files/css/css_AAn076ZJGmT0gQpUuyKCeurB
l7OwD1v9AoKwdA_eJpA.css" media=all />
>         <link rel=stylesheet 
href="https://testxxxxx.net/sites/default/files/css/css_Z-pdTCNASqqfCb0rEY9nK8-w
C2rF3fBJgAm-HMT2XNY.css" media=all />

And we've replace the removed <link> lines with blank lines.

IDK how easy it would be to remove the entire line but I don't think filters 
are particularly newline-aware - they just remove the entire element, leaving 
the rest behind.

Perhaps collapsing of multiple newlines to one would suffice, if that doesn't 
break HTML?

Original comment by matterb...@google.com on 12 Jul 2012 at 6:16

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Oh, we should run collapse whitespace post-render. I'm on it.

Original comment by sligocki@google.com on 12 Jul 2012 at 6:36

  • Changed state: Started
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

It'd be worth considering collapsing whitespace in head unconditionally (or at 
least by default); as I understand it, whitespace in body has caused spacing 
problems for some pages in some browsers, but that shouldn't be true for head 
at all.

Original comment by jmaes...@google.com on 12 Jul 2012 at 9:58

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

That'd be only true if we actually modeled the head/body split inside browsers; 
the syntactic head/body are largely noise, after all. Also <title> is special, 
we may have to be careful with that.

Original comment by morlov...@google.com on 13 Jul 2012 at 1:34

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Interesting point.  Could we erase whitespace except in title, up until we 
reached a "forbidden" (contentful) tag like div, span, or p, and/or running 
text in <head> and call that good enough?  I assume text in head gets rendered 
by browsers as though it were in body, and that's where browsers can differ 
from the markup, but perhaps I've misunderstood you?

Original comment by jmaes...@google.com on 13 Jul 2012 at 2:30

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

This issue was closed by revision r1694.

Original comment by sligocki@google.com on 16 Jul 2012 at 10:43

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Original comment by sligocki@google.com on 16 Jul 2012 at 10:44

  • Added labels: release-note
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 6, 2015

Original comment by j...@google.com on 26 Oct 2012 at 6:04

  • Added labels: Milestone-v23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment