apache / incubator-pagespeed-ngx

Automatic PageSpeed optimization module for Nginx
http://ngxpagespeed.com/
Apache License 2.0
4.37k stars 365 forks source link

Lazyload preloads images with display:none, contrary to JQuery's Lazyload #395

Closed veitnachtmann closed 10 years ago

veitnachtmann commented 11 years ago

we have a lot of content that is never displayed on the site because the entire block has css: display:none.

with jquery lazyload (http://www.appelsiini.net/projects/lazyload) this works perfectly, I have about HALF the requests that ngx_pagespeed produces, since most pictures aren't shown anyways.

any way to fix this?

jeffkaufman commented 11 years ago

Does the lazyload_images filter do what you want here? https://developers.google.com/speed/pagespeed/module/filter-lazyload-images

veitnachtmann commented 11 years ago

that's what I meant; I activated that pagespeed-filter and deactivated my own jquery lazyload, now the requests went up.

jeffkaufman commented 11 years ago

webpagetest waterfalls: pagespeed on, pagespeed off.

jeffkaufman commented 11 years ago

Looking at the source I see:

<img
  pagespeed_lazy_src="uploads/pics/casserole24/55x68xtelefonfrau.png.pagespeed.ic.J8CisLNjLq.png"
  width="55" height="68" border="0" alt="" src="/ngx_pagespeed_static/1.Hy2LQaukh5.gif"
  onload="pagespeed.lazyLoadImages.loadIfVisible(this);"/>

Which I think means that pagespeed is correctly lazyloading the image but incorrectly determining that the image is visible (loadIfVisible is returning true when it shouldn't).

veitnachtmann commented 11 years ago

maybe because of this ;)

pagespeed.LazyloadImages.prototype.isVisible_ = function(element) {
  var element_position = this.getStyle_(element, 'position');
  if (element_position == 'relative') {
    // TODO(ksimbili): Check if this code is still needed. Find out if any other
    // alterntive will solve this.
    // If the element contains a "position: relative" style attribute, assume
    // it is visible since getBoundingClientRect() doesn't seem to work
    // correctly here.
    return true;
  }

  if (!this.onload_done_ &&
      (element.offsetHeight == 0 || element.offsetWidth == 0)) {
    // The element is most likely hidden.
    // Since we don't know when the element will become visible, we'll try to
    // load the image after onload, so that we can improve PLT.
    return false;
  }
 [...]
};

I believe the order of those two checks should be reversed, or the first one even removed.

I'm not sure if I can overwrite that prototype, otherwise I'll have to compile PSOL from source, I guess...

JQuery does this in the same way as it turns out:

jquery: "In jQuery 1.3.2 an element is visible if its browser-reported offsetWidth or offsetHeight is greater than 0."

jeffkaufman commented 11 years ago

I think you're right that the position == relative check could go later, but I don't think that's the whole problem here. With pagespeed if we fix that we're still going to load the element, just after onload, while I think with your current jquery based solution the image isn't loaded until (if) it becomes visible.

But how does your jquery solution know when that happens? How is it notified of visibility changes?

veitnachtmann commented 11 years ago

it binds to scroll and resize events:

https://raw.github.com/tuupola/jquery_lazyload/master/jquery.lazyload.js (look for update())

jeffkaufman commented 11 years ago

Imagine you have other javascript that responds to mouse clicks (or timers, or network events) by changing the div to no longer be display:none. There won't be a scroll or resize event but the lazyloader needs to know to run.

jlporter commented 11 years ago

That plugin does have a 'skip_invisible' option default to true which skips loading of display: none images, but that seems unsafe to me unless you are binding other events to show the images (which that plugin does let you do). As Jeff mentioned, it's probably not the scroll or resize events that cause the display property to change. The ngx_pagespeed lazyloader is designed to not require any changes to your input html to work. But for your particular use case, if you know which events are changing display and are willing to modify your input source, you might get more mileage from something like that jquery plugin.

veitnachtmann commented 11 years ago

i tried that, but then pagespeed doesn't optimize the pictures... can I somehow get pagespeed to go for the data-original attribute instead of src?

jeffkaufman commented 11 years ago

can I somehow get pagespeed to go for the data-original attribute instead of src?

Actually, yes! I think https://developers.google.com/speed/pagespeed/module/domains#url-valued-attributes might do what you need.

jeffkaufman commented 10 years ago

Using UrlValuedAttribute for the data-original worked, right? So this is resolved? Reopen if you want me to look at this more.

samaraiza commented 9 years ago

@jlporter You just saved my life (or at least a few days of it) with that skip_invisible setting that I didn't know existed. ARGH. Thanks!