Added support for lazyloading background images defined in stylesheets

[treyrich]

Adds solution for #442 #430 #377 #353 #189 and probably some other issues I haven't found in the project

[tjoskar]

Thanks for your PR! I like the idea but I think there are some parts that we need to take into account.

Async image path

Let's say I want to add an image path async: eg. <div [lazyLoad]="image$ | async">. This will result in first an empty string (or at least a falsy value) which means that the browser will start loading the image path from css (if it is in the viewport) and then when we have the real image path (from image$) we might start loading another image.

Error handling

There is no way of knowing if the image could be loaded or not so we can never show the error image or inform the app that the image could not be loaded.

Showing the default image while loading

The normal case right now is that the default image will be shown until the image is loaded. You can see the difference if you go https://deploy-preview-444--naughty-bose-ec1cfc.netlify.com/bg-image. Open up dev tools and throttle the download speed and scroll down so you see the two images at the bottom. The image with an empty string as the image path will just show a white box while the other image will show the default image.

SSR

Right now the server-side rendering does not do anything and will let the browser load the images. With this change, however, I believe that the image path from the css will be loaded before the directive has started (and can change the image path to the default image)

Possible solutions.

Instead of using an empty string. What do you think of using a magic string like [[use-computed-style-image]] (or something similar). That will solve the issue with asynchronous image paths and I believe it will be more clear in the code:

// ex. 1
- image = '';
+ image = '[[use-computed-style-image]]';

// ex. 2
- if (!imagePath)
+ if (useComputedStyleImage(imagePath))

It might even be possible to use getComputedStyle at the start of the directive, before we do anything (I'm however not sure that it will work).

ngOnChanges() {
  if (useComputedStyleImage(this.lazyImage)) {
    imagePath = getComputedStyle(this.elementRef.nativeElement).backgroundImage;
  }
}

If that works we do not have to make any changes in the rest of the codebase and it will solve the issues above, except for SSR but that will be hard to solve, unless we change the css for the image in ngOnChanges.

What do you think?

[tjoskar]

I'm actually surprised that this works. In previous versions of Angular the template was placed into the dom before the directive was executed so that the browser starts to download background-image before the directive could change it. But I guess this is nice ;)

[tjoskar]

This results in a compilation error (it is weird that CI doesn't report this). loadImage should always return a string. Like return Promise.resolve(''); or similar.

[treyrich]

Interesting. We can definitely change it to '' but it compiled fine for me when working with it. I think that the first thing we need to establish is whether this is the right direction to go here, and if it is then we can change this to an empty string.

[tjoskar]

There is an issue when setting the image path asynchronous and I think it is a bit confusing that we should use the path from css when the string is empty.

[treyrich]

So is the issue you're bringing up here that the default image wouldn't be displayed correctly in this case with the async image? Because other than that, I can't see why it would be a problem to simply remove the inline background definition if it's an empty string...

The trick here is not to look at this like we're 'using the path from css' so much as look at it like we're 'removing the inline background definition', whether someone has defined a background in CSS is their own choice.

[tjoskar]

EDIT: See my comment below instead.

[treyrich]

Async image path

Let's say I want to add an image path async: eg. <div [lazyLoad]="image$ | async">. This will result in first an empty string (or at least a falsy value) which means that the browser will start loading the image path from css (if it is in the viewport) and then when we have the real image path (from image$) we might start loading another image.

This isn't actually an issue in a real-world application, because someone defining their background image via an async image path is extremely unlikely to also define an image path in CSS, so in the case of this user using an async path, the only implication of this change is that the inline background definition would be temporarily removed, but I can't see how that effects their application in real life.

Error handling

There is no way of knowing if the image could be loaded or not so we can never show the error image or inform the app that the image could not be loaded.

This is true, though I'm not sure that anyone would care (at least immediately). In our case for example, our only alternative is to define our background images traditionally in CSS, which also has no error handling, so this feature adds performance by making these images load lazily, and has all the same downsides to traditionally CSS.

I can understand wanting to have error handling and thing, but I see that as additional enhancements on this concept, not a regression in any way because all we're doing in this particular case is adding functionality to people's projects by letting them lazily load background images defined in CSS.

Showing the default image while loading

The normal case right now is that the default image will be shown until the image is loaded. You can see the difference if you go https://deploy-preview-444--naughty-bose-ec1cfc.netlify.com/bg-image. Open up dev tools and throttle the download speed and scroll down so you see the two images at the bottom. The image with an empty string as the image path will just show a white box while the other image will show the default image.

Again, see above, response to "Error handling". Although a default image is nice to have, it isn't necessary for projects that would be forced to maintain traditional CSS if this feature weren't added. It could be added as an enhancement, but I don't see it as required.

SSR

Right now the server-side rendering does not do anything and will let the browser load the images. With this change, however, I believe that the image path from the css will be loaded before the directive has started (and can change the image path to the default image)

I think you're correct on this. If we decide that we'd like to move forward with this approach then I can add a commit to handle this situation.

Possible solutions.

Instead of using an empty string. What do you think of using a magic string like [[use-computed-style-image]] (or something similar). That will solve the issue with asynchronous image paths and I believe it will be more clear in the code:
omitted
If that works we do not have to make any changes in the rest of the codebase and it will solve the issues above, except for SSR but that will be hard to solve, unless we change the css for the image in ngOnChanges.

What do you think?

I love the idea in concept...Though I think that there are still some issues, for example I see this as a worse problem on SSR because you would no longer be capable of showing bots the correct image because support for getComputedStyle doesn't exist on SSR and would mean having to have all of the same code we already have in place just as a condition of a bot accessing the page. The currently proposed solution (though not perfect) handles SSR much better (ignoring the small tweak you suggested up further here).

I do think that this proposal you indicate here handles most of your concerns that you voiced above, with the main exception being the SSR/Bot issues.

Overall I'm open to any direction you want to go, but I definitely think that there is a fairly simple solution to providing a lot more utility on background images than the library currently has. I'm happy to put in a bit more effort here to get us where we need to go if you want to make a decision on the direction you want to take it.

[tjoskar]

I really miss a thread function on github but I will try to summarize my thoughts.

I like your idea that "If the image path is invalid (falsy), use whatever we have" but I still see a few issues with it.

Let's say we have a function to decide what image we should load:

async getImagePath() {
  if (something) {
    return 'https://path/to/image';
  } else {
    return ''; // This means we should remove the inline definition
  }
}

And then: <div [lazyLoad]="getImagePath() | async" defaultImage="someImage.jpg"></div>

In this case, will the CSS image always be loaded since lazyLoad will be falsy at first.

Is this a problem? – Maybe, maybe not but I think we need a way for people to opt-out from the behavior. Example by checking if the imagePath is not null or undefined. I believe the are use case where people have an async function to load the image path and if the path can't be loaded the image should show the provided default image. This can be solved by also adding the default image in the CSS but it will be a breaking change. But if we don't do anything for null/undefined (and maybe false). I think it is fine.

Something like:

- if (!attributes.imagePath && isImageElement(attributes.element)) {
+ if (typeof attributes.imagePath !== 'string' || (attributes.imagePath === '' && isImageElement(attributes.element))) {

Regarding the error handing and showing the default image while loading; I agree with you. I think it's fine to not have it for this use-case.

And regarding SSR, I believe it will be hard to solve but maybe it is fine for know. So if you want to use CSS as your image path provider it will be fine that the image is loaded right away for the first page view when using SSR.

[tjoskar]

I tried out my idea to use getComputedStyle. The problem is that getComputedStyle triggers a reflow so the image is loaded when we call getComputedStyle (in the ngOnChanges-lifecycle so I do not think it is possible to use getComputedStyle).

[treyrich]

Here's another potential solution. What if we were to add an additional attribute (e.g. [useCSS]="true")? Then you could still provide an image to [lazyLoad] if you want it to preload something, but then ultimately rather than setting the background-image style it removes it if the new attribute evaluates to true. This way you could still take advantage of all of the preloading (if you want), but you get the benefit of using CSS with media queries and things for what is ultimately shown to the user.