Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong image src when browser doesn't support srcset (eg. IE11) #496

Open
tschortsch opened this issue Mar 3, 2021 · 6 comments
Open

Wrong image src when browser doesn't support srcset (eg. IE11) #496

tschortsch opened this issue Mar 3, 2021 · 6 comments

Comments

@tschortsch
Copy link

In the library there are several checks if the srcset attribute is supported in the current browser. If the browser doesn't support it it sets the given imagePath to the src attribute which is my opinion shouldn't be done if the useSrcset flag is set to true. If this is the case we can assume that the imagePath is a srcset-string (eg. https://images.unsplash.com/photo-1434725039720-aaad6dd32dfe?fm=jpg 700w, https://images.unsplash.com/photo-1437818628339-19ded67ade8e?fm=jpg 1100w) and not a single image url which is not a valid value for the src attribute and breaks the loading behavior of the image in older browsers.

This is probably related to the following issue: #334. Of course this behavior could be overwritten by using the available hooks. But to do this it there is a lot code which needs to be re-implemented manually which is pretty dangerous if you would like to keep the library updatable.

Versions:

  • Angular: 11.0.8
  • ng-lazyload-image: 9.1.0
@tjoskar
Copy link
Owner

tjoskar commented Mar 3, 2021

I agree.
But what should happen if a srcset-path is provided but the browser does not support srcset?

Lets say we have the following code:

@Component({
  selector: 'image',
  template: ` <img [lazyLoad]="images" [useSrcset]="true" /> `,
})
class ImageComponent {
  images = `https://images.unsplash.com/photo-1434725039720-aaad6dd32dfe?fm=jpg 700w,
            https://images.unsplash.com/photo-1437818628339-19ded67ade8e?fm=jpg 1100w`;
}

What is expected result in IE here? Is it possible for you to use picture instead?

@tschortsch
Copy link
Author

In my opinion the src attribute should anyway always be added to the <img /> tag since this is a required attribute (also in browsers which support srcset). But I think this was already discussed in the mentioned issue. If this is not possible without creating a big breaking change a simpler solution could be to just raise an error or something which could be catched and handled.

@tjoskar
Copy link
Owner

tjoskar commented Mar 3, 2021

In my opinion the src attribute should anyway always be added to the tag since this is a required attribute

Sure but setting a srcset-url (eg. image1.jpg 700w, image2.jpg 1100w) to the src attribute is as much an error as omitting it, don't you agree?

According to the documentation: https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/srcset srcset will always contain a comma-separated list of urls so I guess it should be possible to do something like:

if (useSrcset && !('srcset' in element)) {
  // Take the first image path in 'srcset'
  const src = imagePath.split(',')[0].split(' ')[0];
  element.src = src;
}

@tschortsch
Copy link
Author

Yes, this would definitely make sense. I would even do the same if srcset is supported as in this case the src attribute should be set as well. But as far as I understood from #334 this isn't an option right?

@tjoskar
Copy link
Owner

tjoskar commented Mar 14, 2021

Sorry for my late response.
I think it is doable. I think I did too little research for #334, to give a good answer.

@tschortsch
Copy link
Author

Not sure where your last comment with the proposed srcsetFallback went but this is exactly how I now solved it in a project manually. I introduced a custom attribute (fallbackSrc in my case) and am using this to set the src attribute of the current img. With this approach you would most certainly not introduce a breaking change and the change should be completely backwards compatible. So I would go for such a solution instead of changing the current behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants