apache / incubator-pagespeed-mod

Apache module for rewriting web pages to reduce latency and bandwidth.
http://modpagespeed.com
Apache License 2.0
696 stars 158 forks source link

Deferring CSS does not change its priority in Chrome with HTTP/2 #1228

Open GuillaumeRossolini opened 8 years ago

GuillaumeRossolini commented 8 years ago

Hi,

We got the CSS priorization filter working and now the CSS is deferred to just before the closing tag, and without renaming the asset which is what we need in our use case: <noscript class=psa_add_styles><link href="https://www.example.com/static/www.3079.css" media=screen rel=stylesheet type="text/css"></noscript> However, while inspecting network traffic with Chrome, this asset is still perceived as "high priority" when on an HTTP/2 connection.

cf. https://drive.google.com/file/d/0Bz1P7lhSviUUMVVFeXJ1UVpMa3c/view?usp=sharing

Here's how the page was optimized:

mod_pagespeed on
Filters:
ai  Add Instrumentation
cw  Collapse Whitespace
mc  Convert Meta Tags
di  Delay Images
ea  Elide Attributes
hw  Flushes html
gf  Inline Google Font CSS
io  In-place optimize for browser
ll  Lazyload Images
pr  Prioritize Critical Css
rc  Remove Comments
rq  Remove Quotes
cf  Rewrite Css
jm  Rewrite External Javascript
jj  Rewrite Inline Javascript
cs  Rewrite Style Attributes

Am I doing anything wrong or is it something in development? Thanks,

morlovich commented 8 years ago

Hi, thanks for the report, and apologies for the slow response --- was away for the holidays. Anyway, we are only really kicking off H2 stuff, so I guess "something in development except I wasn't aware of this potential problem until reading your report" would be accurate. I am a bit confused here, though, since the snippet you cited should be only used when the browser has JavaScript off, what with being within noscript tags. Were you testing like that? If not, then the style sheet should be loaded from a later-running script --- I think one of the views in dev tools shows that information. Perhaps the script is insufficiently deferred itself, though your screenshot suggests your page might be far lighter than what's usually out there, too.

Hmm, in theory it may be possible to change priority without renaming, in cases where it goes through the same connection (e.g. same domain/certset)... if mod_h2 supports that, anyway.

GuillaumeRossolini commented 8 years ago

Well, that was the only obvious reference I found, but there could have been another. It seems this is the standard PageSpeed way of deferring CSS:

<script pagespeed_no_defer="">//<![CDATA[
(function(){var f=this,g=function(a,d){var b=a.split("."),c=f;b[0]in c||!c.execScript||c.execScript("var "+b[0]);for(var e;b.length&&(e=b.shift());)b.length||void 0===d?c[e]?c=c[e]:c=c[e]={}:c[e]=d};var k=function(){var a=window,d=h;if(a.addEventListener)a.addEventListener("load",d,!1);else if(a.attachEvent)a.attachEvent("onload",d);else{var b=a.onload;a.onload=function(){d.call(this);b&&b.call(this)}}};var l=!1,h=function(){if(!l){l=!0;for(var a=document.getElementsByClassName("psa_add_styles"),d=0,b;b=a[d];++d)if("NOSCRIPT"==b.nodeName){var c=document.createElement("div");c.innerHTML=b.textContent;document.body.appendChild(c)}}};g("pagespeed.CriticalCssLoader.addAllStyles",h);
g("pagespeed.CriticalCssLoader.Run",function(){var a=window.requestAnimationFrame||window.webkitRequestAnimationFrame||window.mozRequestAnimationFrame||window.oRequestAnimationFrame||window.msRequestAnimationFrame||null;a?a(function(){window.setTimeout(h,0)}):k()});})();
pagespeed.CriticalCssLoader.Run();
//]]></script>

Which is to say, onLoad get the "noscript" element which has a "psa_add_styles" class, and write its contents into a new div. In this light, providing you with the <noscript> tag is the same as the <script>, but it is the only one of the two that has the actual URLs to download, and it is much shorter. Unless I am completely mistaken?

About the page weight, please keep in mind I did not grab all the assets with that screenshot. In fact, I deliberately sorted them to make the screenshot as easy to read as possible. There are a few megabytes worth of resources in that page. The resources shown in the screenshot are sorted by download start time, so the screnshot shows the 4 first resources that were downloaded.

In my use case, there is 1 connection for the HTML (and the beacon), and another connection for the assets. In this example, the CSS asset was deferred by PageSpeed, but the H2 priority effectively meant it was downloaded first anyway, which I am sure was not the intent.

GuillaumeRossolini commented 8 years ago

I was hoping a simple solution would be to add a timer to the script part, or change the event to listen to?

jmarantz commented 8 years ago

Note that there is a setTimeout in the flow already. That JS in cleartext is here: https://github.com/pagespeed/mod_pagespeed/blob/master/net/instaweb/rewriter/critical_css_loader.js

/* * Loads deferred CSS in noscript tags by copying the text of the noscript into * a new div element. /pagespeed.CriticalCssLoader. addAllStyles = function() { if (pagespeed.CriticalCssLoader.stylesAdded) { return; } pagespeed.CriticalCssLoader.stylesAdded_ = true; var elements = document.getElementsByClassName('psa_addstyles'); for (var i = 0, e; e = elements[i]; ++i) { if (e.nodeName != 'NOSCRIPT') { continue; } var div = document.createElement('div'); div.innerHTML = e.textContent; document.body. appendChild(div); }}; /* * Sets up the CSS style lazyloader to run at the appropriate event. Runs at * requestAnimationFrame if it is available, since that will ensure the page has * rendered before loading the CSS. Otherwise, wait until onload. * @export /pagespeed.CriticalCssLoader.Run = function() { var raf = pagespeedutils.getRequestAnimationFrame(); if (raf) { raf(function() { window.setTimeout(pagespeed.CriticalCssLoader.addAllStyles, 0); }); } else { pagespeedutils.addHandler( window, 'load', pagespeed. CriticalCssLoader.addAllStyles); }};

asdf

On Wed, Jan 6, 2016 at 8:03 AM, Guillaume Rossolini < notifications@github.com> wrote:

I was hoping a simple solution would be to add a timer to the script part, or change the event to listen to?

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/mod_pagespeed/issues/1228#issuecomment-169318694 .

GuillaumeRossolini commented 8 years ago

I'm just saying there are cases when the CSS is not deferred at all, although PageSpeed has done some work.

GuillaumeRossolini commented 8 years ago

Hi, Do you think that might have been caused by my use of Chrome Canary? What would be the right test-case? Do you kow if the Chrome team needs to know this? What would be the best way to have their input? Thanks,