Page MenuHomePhabricator

Updates to lazy loaded images UI
Closed, ResolvedPublic3 Story Points

Description

Images should load progressively and be less intrusive on the user. It should be less obvious to the end user that any lazy loading is happening.

Acceptance Criteria

  • Spinner should be removed
  • Animation should stay. It's important but it should not depend on the loaded class that is added via JavaScript. We should schedule the animation via CSS at the same point that we start loading the image itself. In other words: Insert the image element into the DOM and start the animation (for example: a 0.3 second fade-in animation with a 0.5s delay). Which means after 0.5s the image canvas will fade in whether completed or not. If not completed, the rest of the image will load progressively to the user directly. All which can happen concurrently with scroll events.
    • Moved delayed appearance of image to T137874 in a better explained self contained task/bug. This AC mixes different concerns.
  • Remove the border (and radius) and make the background color @colorGrayLightest

Original Bug Report(s) from @Krinkle :

Bug 1:
At the moment the lazy-load images implementation is relatively complex. Part of that complexity is various a border, border-box modifications, and background-color/opacity animations.

I assume that this complexity is the reason the implementation also adds the image to the page layout after it has completely finished loading.

Normally in browsers, without lazy-loading, an <img> is interpreted by the browser as a target canvas on which to progressively render the image data as it comes in.

The browser does this in a way that is friendly with render performance (e.g. no forced re-rendering for every bit of data that arrives, but it doesn't wait for all data to arrive, either).

This means the user sees (parts of) the image even if the download takes a while and/or if the connection is spotty.

I strongly recommend that we either:

  • Reduce layout complexity and make the is-visible handler simply replace the placeholder with the target <img> (like we do in the Grade C fallback).
  • Or; Figure a way to do it in the current layout.

I'm recommending the former and re-evaluate the added value of the CSS animations for a later iteration.

Bug 2:
There are a couple of things we can do to make the lazy loading of images on mobile less intrusive. Meaning that for the user, she/he will not even know that the images are lazy loaded, only in the case of a really slow connection the user will see that, but that's the case even today.

I've been discussing this with @Krinkle and I think a couple of things can make the experience for the user more seamless.

Skip the spinner

You get the spinner every time (I think) and the camera on my phone couldn't catch it because the spinner is so fast on fast connection but check this desktop recording. Do you see the spinner?

I don't see how the spinner adds any value for the user. I think there should be a wait time before we use the spinner or rather don't use it at all (to mimic the normal browser behavior) and I think @Nirzar can help us here about best practice.

Do not highlight the image area before it arrives

The grey background and the border doesn't make any sense if we all agree that we want to make the lazy loading as seamless as possible. Then I think we need to make sure we reserve the space needed for making images fit, and do not highlight where it will be, so it looks the same as default browser behavior.

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
DeclinedNone
OpenNone
Resolveddr0ptp4kt
DuplicateJhernandez
Duplicatedr0ptp4kt
OpenNone
Resolved Jdlrobson
Resolved Jdlrobson
ResolvedJhernandez
ResolvedJhernandez
DuplicateNirzar
Resolveddr0ptp4kt

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dr0ptp4kt added a subscriber: Nirzar.

As in T135476#2318155, I'm re-tagging with sprint 74 in acknowledgment of @Nirzar's comment (in T135476#2318155) and to accommodate for more time to analyze design, while we wait for T133565: Images do not load until scroll event completes (not during scroll) on iOS to be built and settle in.

@Krinkle I'm having difficultly groking the description of this task.
Could you take a pass at explaining it better and separating the concerns captured in T135476 ?

Krinkle added a comment.EditedMay 23 2016, 10:12 PM

I assume that this complexity is the reason the implementation also adds the image to the page layout after it has completely finished loading.

Can you elaborate what you mean by this statement?

I meant that I assume the intent to have animations is what led to the choice to load the image in a detached state and thus only insert it only once the image finished loading. Doing so understandably makes it considerably easier to have a fade-in animation. However, this comes at the cost of:

  1. Having no visual progress while loading.
  2. Having no image at all unless the loading successfully completes. If loading is disrupted, the user will see no image. In theory this could be worked around by inserting the image from an "error" handler, but this may not happen in the case of a connection that appears merely slow but is really absent ("LieFi"). It also feels like a futile attempt at trying to be better than the browser at rendering an image in a performant way that balances speed, user experience and power usage.
  3. Having no image render during scroll (T133565). Due to the visibility depending on a CSS animation, which in turn is triggered from JavaScript adding a class, which in turn depends on a next animation frame, which is triggered from an image "load" event handler in JavaScript - no visual progress can happen while scrolling because image load JavaScript events (and other event loop stuff) tends to be at lower priority while scrolling. A scroll event handler can and will fire, but not an image load event. Those will be deferred.

This last point is the main one here. If the image is inserted into the DOM directly it will render naturally - even while scrolling.

This does mean however that there is no animation.

In the opening post I recommended removing the animation for that reason.

The other option I mentioned, if you want an animation, requires it to be a CSS transition or animation that is triggered independently (it must not depend on a JavaScript starting the animation manually via a timer or other callback) . For example, you can apply the animation to a plain img selector in CSS (without waiting for the loaded class). And perhaps delay the animation in CSS by 1 second. This means it will fade in after 1 second (counting from when the image load starts, as triggered by lazyload scroll handlers inserting the image in the DOM). If the image is loaded by then, it'll fade in naturally (even if the user is still scrolling). If the image is not loaded, the empty or partial image canvas will appear and it'll render progressively from there, just like it would without lazy loading. The animation has no added value in that case.

Thankyou. Much clearer now about what you are trying to achieve. Once T133565 is merged I can demo your suggestions to @Nirzar and see what he thinks

Jhernandez triaged this task as Normal priority.May 25 2016, 4:38 PM
Jhernandez added a project: Design.

I talked to Nirzar. These were the conclusions:

  • Spinner can be removed
  • Animation should stay. It's important but it should not depend on the loaded class that is added via JavaScript
  • Remove the border (and radius) and make the background color #f5f5f5 the one we use for base colors. which is much lighter
Nirzar added a comment.EditedMay 25 2016, 10:07 PM

I talked to Nirzar. These were the conclusions:

+1

Jdlrobson renamed this task from Lazy-load images should load progressively to Updates to lazy loaded images UI.May 27 2016, 3:51 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Jhernandez.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 289014 abandoned by Krinkle:
Lazy-load: Allow progressive image loading

https://gerrit.wikimedia.org/r/289014

Jdlrobson set the point value for this task to 3.May 31 2016, 4:19 PM

Change 293305 had a related patch set uploaded (by Jhernandez):
Make lazy loading images UX less intrusive

https://gerrit.wikimedia.org/r/293305

293305 deals with:

  • Spinner should be removed
  • Remove the border (and radius) and make the background color @colorGrayLightest

Gif showing the loading now. Personally seems faster than with the spinner (even if it is the same loading time), which is cool.

Regarding

Animation should stay. It's important but it should not depend on the loaded class that is added via JavaScript. We should schedule the animation via CSS at the same point that we start loading the image itself. In other words: Insert the image element into the DOM and start the animation (for example: a 0.3 second fade-in animation with a 0.5s delay). Which means after 0.5s the image canvas will fade in whether completed or not. If not completed, the rest of the image will load progressively to the user directly. All which can happen concurrently with scroll events.

If we set a delay, the animation will trigger when image may be already visible, making a flash from image to white, then animate to image. Not good. If we set the image's opacity to 0 during that delay, cached images that load in 0 will appear white during the delay when they shouldn't necessarily.

  • Because so, I'm not going to implement a forced delay on the animation (unless someone has any objections with good reasoning).

Regarding triggering the animation before the image loads, I want to clarify myself in written form with @Nirzar that that is the wanted behavior:

  • Currently
    • Lazy images look like a light gray square before loading (see gif in previous comment)
    • When image is close to viewport, it'll start loading, but the square will still be the same (previously there was the spinner)
    • When the image is fully loaded, the gray rectangle cross-fades to the image quickly (0.3s)
  • With the proposed change of inserting the image immediately while it loads:
    • The animation will trigger instantly the moment the image starts loading (instead of when fully loaded)
      • If the image is cached animation will show up almost instantly, and the gray rectangle will cross-fade to the image
      • If the image is not cached (most frequent case), then the rectangle will start crossfading to nothing (white background) and the image will start downloading. If during those 0.3s there's some image data coming in, the browser will render part of the image in between the fade.
      • If the image takes longer to respond than 0.3s, then the rectangle will have cross-faded to white, and there will be an empty white space there.

It may be hard to visualize these ^ interactions, I can work on showing them if necessary. Can you confirm the change @Nirzar?

Personally all animated image loading I've seen doesn't show the image until loaded to show a full consistent animation. But there's also a good point on making lazy loading images more invisible by simulating browser behavior (the progressive loading). But then if we want it to be transparent, the placeholder with the background doesn't make sense since the browsers don't do that by default.

Thoughts?

I'd appreciate review on the current patch for now, but don't move to signoff if merged, move back to doing or -1 please.

Personally all animated image loading I've seen doesn't show the image until loaded to show a full consistent animation. But there's also a good point on making lazy loading images more invisible by simulating browser behavior (the progressive loading). But then if we want it to be transparent, the placeholder with the background doesn't make sense since the browsers don't do that by default.

I agree, if we're trying to mimick the default loading behavior, we should get rid of the background and the animation.

Moving back to -1 as there is more work to be done and as suggested by @Jhernandez above.

Change 293305 merged by jenkins-bot:
Make lazy loading images UX less intrusive

https://gerrit.wikimedia.org/r/293305

phuedx added a subscriber: phuedx.Jun 9 2016, 4:24 PM

I'd appreciate review on the current patch for now, but don't move to signoff if merged, move back to doing or -1 please.

Mibad.

Peter added a comment.Jun 9 2016, 7:00 PM

I think it would be good with an example with and without the animation so @Nirzar can check, so we can agree of what it will look like.

I personally think 300 ms fading time can make it feel slow. When I use the Hovercards I get a TTFB 150-200 ms most of the time and I feel that.

@Peter I've collected some rationale on transitions and animations over at T77949.

@Peter I'm going to make a WIP patch that removes the animations and favors image insertion pre-loading (browser-like progressive image) and will record a few gifs to show and clarify understanding for everyone :D

Change 293708 had a related patch set uploaded (by Jhernandez):
Remove outdated comment

https://gerrit.wikimedia.org/r/293708

Change 293709 had a related patch set uploaded (by Jhernandez):
WIP: Insert lazy images in the DOM immediately

https://gerrit.wikimedia.org/r/293709

Mkay, here are two gifs I captured before and after 293709 (which makes the change specified on this task). Note that they take +10s to show the effect, so wait after you open them.

a) Currently: Delayed insertion (after image has loaded)

b) Proposed in this task: Immediate insertion (before image has loaded)

Gifs have been recorded on a Good 2G chrome dev-tools profile to make the progressive image effect obvious (otherwise what's the point to even discuss).


Notes:

  • On a) the placeholder stays longer, but the image appears all at once with the cross-fade
  • On b) the placeholder get's swapped earlier, fading to white (barely noticeable, since from f5f5f5 to ffffff in 300ms is really subtle), while the image gets inserted and starts loading. Then the browser progressively renders the image as it get's the data.

Hopefully this will clear what I'm asking about. This is a change in UX and just wanted to clear with Design that this is what we're going for.

If so, I'd recommend considering removing the gray placeholder, and fully imitating the browser (empty space while image is waiting to be loaded).

Change 293708 merged by jenkins-bot:
Remove outdated comment

https://gerrit.wikimedia.org/r/293708

If so, I'd recommend considering removing the gray placeholder, and fully imitating the browser (empty space while image is waiting to be loaded).

+1

@Jhernandez

Last where I left the conversation i remember talking about this issue - while I'm scrolling through the article and images being loaded only when you stop loading due to the fade in animation. If that's still the case then version B will create half loaded images while you are scrolling the article. It is also more jarring than the whole picture showing up.

We started from placeholders with spinners in them and ending up with browser style image loading. the only difference is, images will load lazily. It is not good experience but something we could live with.

The placeholer background is a good to have since as the name suggests, it holds the place otherwise you will see empty white spaces for no reason.

I'm guessing there is another task to show the blurry version of the image and even for that we will *have to* do the fade in. I carefully saw medium's experience and they fade in the image on touchmove events even without touchend happening. I think solving that problem is future proof if we go for blurry placeholders in the future.

The placeholer background is a good to have since as the name suggests, it holds the place otherwise you will see empty white spaces for no reason.

Without the spinner the user won't know whether something is still loading inside the placeholder (Users who block ads may think of the placeholder as a place where ad is supposed to appear - I know we don't serve ads). Coupled with showing the image once it's ready, this is a bad UI decision imo. The users are already familiar with empty spaces as browsers have been operating that way, so I'd say remove the background and let the browser load the image partially.

It's worth noting the original issue (dating back to Jon Katz's original bug report T133565) was around scrolling so it would be only fair to redo these gifs with a page on a 2g connection on an ios tablet.

You can see how medium did it here:
https://jmperezperez.com/medium-image-progressive-loading-placeholder/
(feel free to play around with a test article https://medium.freecodecamp.com/salary-negotiation-how-not-to-set-a-bunch-of-money-on-fire-605aabbaf84b#.exmxqyt5v)

The key thing they do differently to use is use a css transition and when I tried that out it was much smoother. I haven't had much time to research but I'm guessing the CSS keyframe animation is blocked on a scroll but a CSS transition is not since it can be triggered as soon as it's in the DOM. Was there a reason we used keyframes?

FWIW medium.com on a 2G connection:

Was there a reason we used keyframes?

See 103614e. Now that we're not using setTimeout, we can revert to using CSS transitions if we want.

we can revert to using CSS transitions if we want.

can we please try and figure this out. i would like to get the smooth transition in and not go back to 90s browser style image load. as i said before, in future when we do blurry placeholder all of this will pay off :D

Change 293709 abandoned by Jhernandez:
WIP: Insert lazy images in the DOM immediately

Reason:
Not the wanted behavior for now. See https://phabricator.wikimedia.org/T135430#2381123

https://gerrit.wikimedia.org/r/293709

Per design's comments (T135430#2381123), I've abandoned the progressive image loading in favor of the current version given the UX concerns.

I've extracted what I've been able to gather about the images not animating in while scrolling into a followup task better specified and self contained (T137874)

If needed so, discussion about if images should load browser-like or app-like will have to happen to reach clear consensus and common understanding.

There are valid concerns and there are heuristics we can apply to mix the two modes if needed be. For example, if an image is taking more than 3s (or other amount) to load, in order to not show an empty placeholder with no indication of loading or progress, we could switch to progressive loading and that image wouldn't have the full consistent animation, but would progressively show loading thus avoiding the empty placeholder for a long time without signs of activity (while keeping other images that loaded faster with the better UX). Of course magic-numbers are always troublesome, but there's things we can do.


I'm going to consider this task worked on to the fullest of it's extent and move it to signoff.

If there are more concerns about behavior I'd suggest having a meeting of Readers-Web-Backlog, Performance and Design to discuss and reach clear common consensus of the work to be done.

Was there a reason we used keyframes?

See 103614e. Now that we're not using setTimeout, we can revert to using CSS transitions if we want.

Ignore the second sentence. Of course we might have to revert to using setTimeout to reliably trigger the CSS transition :/

This is working on the browsers below on the beta cluster in beta mode. But it seems to be pushing the content around upon lazy load, making things perceptibly jumpy:

Contrast this with the existing lazy loading on the prod cluster in beta mode, which doesn't have the jumpiness.

We should get rid of the jumpiness. I assumed incorrectly that implicit in the Description was the notion that we would continue to have the non-reflowing behavior of the existing lazy loaded images. So, trying to make that more explicit, this is what I think in the short run we need to do until we reconvene on T137874: Lazy loaded images animation doesn't trigger until scrolling stops in mobile browsers -

  • Spinner should be removed
  • Make the background color @colorGrayLightest
  • (not sure if this already the case?) The placeholder should be the same size as the image
  • The border, if any, should not outline the placeholder
  • There should be no additional user perceived reflowing introduced by these changes

@Jhernandez, would this be too much trouble for this task? If I understand correctly it's probably reinstating and maybe modifying some of the lines deleted from images.less. My naive read of the the tip of MASTER is that our latest enhancements aren't made any less performant by reinstating some of those lines.

Browsers tested:

  • iPhone 5c iOS 9.3 Safari
  • Nexus 7 Android 5 Chrome
  • LG Nexus 4 Android 5 Chrome
  • HTC Trophy 7 Windows 7.5 IE
  • Opera Mini (via iPhone 5c iOS 9.3 with transcoder enabled)

Change 294689 had a related patch set uploaded (by Jhernandez):
Fix reflow from lazy loaded images improvements

https://gerrit.wikimedia.org/r/294689

Change 294689 merged by jenkins-bot:
Fix reflow from lazy loaded images improvements

https://gerrit.wikimedia.org/r/294689

Jhernandez closed this task as Resolved.Jun 17 2016, 10:28 AM

I've verified in beta cluster there's no more reflows (http://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama).

@dr0ptp4kt feel free to have a look if you have some time

Agreed, I didn't see the reflow on iOS Safari or Android Chrome.