defer image loading on search/home (bug 898320)

[ngokevin]

Demo: http://screencast.com/t/Cxr8Dbp3s1A

[cvan]

why are you doing removeClass or doing classList.remove?

[ngokevin]

Thanks, didn't know about classList.

[ngokevin]

@cvan polished the PR + added a demo screencast. it feels pretty good now.

note: except that switching tabs and back breaks

[cvan]

I feel like each thumbnail should be lazy-loaded, not the entire gallery section itself. afaict, we are doing lazy loading when you maximize the gallery as a modal (@potch and/or @spasovski worked on that iirc).

also, it looks like we're deliberately waiting 2 seconds?

are we aborting image requests if a request is requested, but it's not yet finished and we scroll past it and it's no longer needed?

[cvan]

and the benefit of the lazy-loading here does not improve initial page load at all, correct? it's all just to reduce bandwidth, yes?

[cvan]

also, thank you so much for this

[ngokevin]

I feel like each thumbnail should be lazy-loaded, not the entire gallery section itself. afaict, we are doing lazy loading when you maximize the gallery as a modal (@potch and/or @spasovski worked on that iirc).

I think that can be done, I can add a check to make sure the image is within horizontal view as well as vertical.

also, it looks like we're deliberately waiting 2 seconds?

The debounce rate is variable, I have it at 100ms for app icons and at least a second for screenshots. But if I lazy-load by thumbnail, I can reduce that rate. It was just to not queue up too many images as we scroll down

are we aborting image requests if a request is requested, but it's not yet finished and we scroll past it and it's no longer needed?

Thought crossed my mind, I'll give it a shot.

and the benefit of the lazy-loading here does not improve initial page load at all, correct? it's all just to reduce bandwidth, yes?

It sort of improves the initial page load as the UI may be laggy as it tries to load dozens of images at once. I checked the network tab, and even my laptop was taking a long time to load images on the viewport because so many were queued up.

also, thank you so much for this

np, thanks

[potch]

I dig this.

[muffinresearch]

Shiny stuff! One suggestion, maybe you could use a non-animating placeholder image instead of a throbber to reduce the amount of movement. Animation draws the eye and for this it's probably unecessary imho.

[ngokevin]

@muffinresearch Updated to use non-animating placeholders...plus fade-ins as icing.

http://screencast.com/t/z4pn6tZEv

[muffinresearch]

@muffinresearch Updated to use non-animating placeholders...plus fade-ins as icing.

That looks great! 😃

[cvan]

one last thing: can we do this for homepage / cat pages?

[ngokevin]

Updated to work on all homepage/listing pages (exclude detail pages).

[ngokevin]

Don't really like the way my solution looks on the homepage though:

http://screencast.com/t/YhDZxPXAli4k

Because switching between New + Popular tabs triggers a page rebuild, the images are reverted back to the placeholder images, then loaded in again.

[ngokevin]

Homepage issue fixed:

Updated demo

[cvan]

maybe do it on z.page or z.body

[ngokevin]

seems scroll only triggers on doc for me

[ngokevin]

r? i'm sure there are some wrong doings in the code perf-wise. the CSS transitions of multiple images at once are pretty laggy too.

[ngokevin]

I investigated doing deferring image loading horizontally for the screenshots (checking if the image is visible within the main container), but it introduces really specific code. And it'd be ugly code since we don't have JQuery.offset in Commonplace.

[ngokevin]

@spasovski r?

[ngokevin]

@cvan @diox r? this perf gain has been sitting for a while now, and i've seen duplicate work done on this a couple of times.

[dethe]

r+ I like this. I do have a minor concern about this and other of our lazy-loading performance work about what happens when we go back to a page using the back button, only because we've seen a lot of odd (and sometimes hard to reproduce consistently) errors with missing content in those cases. But I don't have a smoking gun pointing to our lazy-loading, just that I suspect it might be tickling some platform bugs that others aren't seeing. I'm happy to have this in, and will keep an eye out for missing content issues.

[cvan]

I'd like to make sure that we're not reinventing the wheel here before landing this.

[ngokevin]

This module allows us to throttle icons but debounce screenshots.

[cvan]

I always get confused by the two; can you explain?

[ngokevin]

As the user scrolls down the page, they will still be able to see the icons since their scroll events are throttled (events are run no matter what in sure intervals).

Screenshots won't be shown until the user stops scrolling because screenshots are debounced (events are grouped and run once). Because we don't want screenshots loading as the user flies down.

[cvan]

awesome - good explanation, thanks