Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

collapse_whitespace missing whitespace removal in one location #463

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

collapse_whitespace missing whitespace removal in one location #463

GoogleCodeExporter opened this issue Apr 6, 2015 · 10 comments
Labels
auto-migrated Milestone-v23 Priority-Medium release-note Type-Defect

Comments

@GoogleCodeExporter
Copy link 

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
Copy link
Author 

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
Copy link
Author 

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
Copy link
Author 

> 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
Copy link
Author 

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
Copy link
Author 

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
Copy link
Author 

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
Copy link
Author 

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
Copy link
Author 

This issue was closed by revision r1694.

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

  • Changed state: Fixed

@GoogleCodeExporter
Copy link
Author

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

  • Added labels: release-note

@GoogleCodeExporter
Copy link
Author

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

  • Added labels: Milestone-v23

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
auto-migrated Milestone-v23 Priority-Medium release-note Type-Defect
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@GoogleCodeExporter