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

Cannot lazy load images in Firefox or Chrome #1828

Open
rschristian opened this issue Jul 27, 2023 · 31 comments
Open

Cannot lazy load images in Firefox or Chrome #1828

rschristian opened this issue Jul 27, 2023 · 31 comments
Labels
bug Something isn't working

Comments

@rschristian
Copy link

rschristian commented Jul 27, 2023

Describe the bug

(I'm putting this first as I think it's important pretext, sincere apologies if this throws anyone off)

FireFox and Safari have rather interesting behavior regarding setting the src property of an image element, in that the browser will attempt to fetch the media after a microtask, regardless of whether or not the element has been (or will be!) inserted into the DOM. This is rather simple to demonstrate:

const img = document.createElement('img');
img.src = 'https://some.img.url';

Using the Network pane of a browser's devtools, you'll see the immediate fetch of the image.

FF Bugzilla Report: it's unclear whether this is a bug in FF & Safari or a spec issue, but it's been around for a while now.

Because of this, if one wants to set the image to lazy load, in Safari and FF, one needs to ensure the loading prop is set first, else it's effectively not set at all. Preact, React, and Vue mirror vanilla JS behavior, making loading an ordered prop in their JSX and SFCs.


As we try to decide what to do about this for Preact, I've been testing this behavior through multiple frameworks to see who accounts for this and who doesn't, but Solid's results have been exceptionally odd: lazy loading doesn't seem to work, regardless of property order, in FireFox and Chrome (tested desktop & mobile for both). However, it DOES work correctly on iOS Safari (I don't have a Mac so can't test desktop, sorry).

In the above mentioned frameworks, Chrome had been handling out-of-order properties just fine, meaning one could use <img src"..." loading="lazy" /> without any issue. I'm not quite sure what about Solid's implementation differs to cause Chrome to now have issues, or why Safari now works as expected, but something seems to be off.

This could very well be browser bug / spec issue territory, of course.

Your Example Website or App

https://github.com/rschristian/solid-lazy-img-bug

Steps to Reproduce the Bug or Issue

  1. yarn
  2. yarn dev (Alternatively, yarn build && yarn preview, issue can be reproduced in both)
  3. Open network pane in Firefox or Chrome
  4. Navigate to localhost:5173
  5. Notice the request for solid.svg, despite being quite far out of the viewport

Expected behavior

The image should be lazily loaded, only being requested when near the viewport / after a scroll.

Screenshots or Videos

No response

Platform

  • OS: Windows, Linux, iOS
  • Browser: FF & Chrome
  • Version: Latest of both

Additional context

No response

@ryansolid
Copy link
Member

Thanks @rschristian. Hmm.. I'd think the other framework most similar to our version would be Lit. We do a similar template cloning approach to rendering so. We clone all the elements. and then set the attribute before we attach it to the DOM.

Effectively the code ends up being:

const _tmpl = document.createElement("template");
_tmpl.innerHTML = `<div><div>Spacer</div><img loading="lazy" height="150" width="150">`;

const _el = _tmpl.content.firstChild.cloneNode(true),
    _el2 = _el.firstChild,
    _el3 = _el2.nextSibling;
_el2.style.setProperty("height", "200vh");
_el3.setAttribute("src", "/path/to/asset.svg");

@rschristian
Copy link
Author

rschristian commented Jul 28, 2023

I can add that Lit isn't subject to the aforementioned FF & Safari issue with its implementation or any problems in Chrome (that I saw; I have been testing 6+ frameworks across 6+ browsers, it's possible I missed something of course).

I'm not sure where or in what details you diverge from Lit, but that might be worth looking into first.

@ryansolid
Copy link
Member

ryansolid commented Aug 9, 2023

Ok I finally looked at this.

My results for your Solid example:

MacOS:
Chrome - delayed
Firefox - immediate
Safari - delayed

Windows
Chrome - delayed
Firefox - immediate

Basically I only see the issue in Firefox in both OSs. In Chrome changing the order didn't seem to matter either. But Firefox seems to always eagerly load.

I think I may have figured out the difference with Lit. They use document.importNode to clone instead of element.cloneNode this seems to "fix" the issue in Firefox. We already do this swap when there are custom elements I suppose we could always use it instead.

@ryansolid ryansolid added the bug Something isn't working label Sep 11, 2023
@danieltroger
Copy link

Also suffering from this. This coupled with my responsive image implementation causes insane delays where chrome literally locks up for seconds.

Welp, back to the basics with the image related stuff I guess

Screenshot 2023-10-25 at 10 13 29 Screenshot 2023-10-25 at 10 14 16

@ryansolid
Copy link
Member

I know the fix but it is slower. I didn't want to make a call here without figuring out of this is spec or a firefox bug.

@ghost
Copy link

ghost commented Feb 26, 2024

@ryansolid, I have the same problem without lazy loading, src attr delays render. The delay is small in Chrome, but it's large in Safari.

<img class={style.commentAvatar} src={local.avatar} />

image (3x elements)
image (5x elements)

if I just remove src:

<img class={style.commentAvatar} />

image (3x elements)

@ryansolid
Copy link
Member

To be fair this issue is about whether lazy works in the given browser. My testing indicated that only Firefox wasn't lazy loading images. That is something we are looking at fixing.

If images themselves are rendering slow that's a different thing and probably has nothing to do with Solid. If Chrome or Safari are locking up with images it comes down to how they are rendering them because all we are doing is setting the src attribute. That could be a reason to use lazy but that is outside this issue (unless lazy isn't working as the spec intends).

@ghost
Copy link

ghost commented Feb 26, 2024

@ryansolid, it can be fixed with setTimeout(() => /* set src attr */)
<img ref={el => setTimeout(() => el.src = local.src)}> works well

@ryansolid
Copy link
Member

Sure.. I'm saying that the src attribute slowing down rendering in the absense of loading="lazy" is expected browser behavior.

@ghost
Copy link

ghost commented Feb 26, 2024

there is no difference, both with loading="lazy" and without it still slowing down rendering

@beingflo
Copy link

beingflo commented Apr 1, 2024

Until it's clear if it's a Firefox bug or something that should be addressed in Solid, is there a workaround for this? Maybe using the Intersection Observer API manually would be the simplest approach?

@Antonio-Bennett
Copy link

Seems like this issue will be fixed in Svelte 5 maybe some inspiration can be taken from there?

@ryansolid
Copy link
Member

Seems like this issue will be fixed in Svelte 5 maybe some inspiration can be taken from there?

It looks like they are implementing my suggestion higher in the thread #1828 (comment)

I just have been hesitant because it means worse perf for everyone. But it is probably ultimately the right decision. We just haven't had a minor release in Solid for a while. Not sure when I want to get it in.

titoBouzout added a commit to potahtml/pota that referenced this issue May 27, 2024
…ix firefox has problems with `img`s and `loading="lazy"`. see solidjs/solid#1828
titoBouzout added a commit to potahtml/pota that referenced this issue May 27, 2024
…ix firefox has problems with `img`s and `loading="lazy"`. see solidjs/solid#1828
@ryansolid
Copy link
Member

Final going to be fixed soon in 1.9.

Fixed by commit: 2a3a198

@danieltroger
Copy link

danieltroger commented Oct 22, 2024

Seems like the fix unfortunately doesn't handle this case

It always defaults to using cloneNode() for these cases. I can reproduce the lazy images being loaded immediately in firefox when doing so. Vs hardcodeing loading="lazy" where they load when scrolling down as expected

<img
  loading={props.isPrioritisedImage_ ? "eager" : "lazy"}
  // Other attributes
/>

@ryansolid
Copy link
Member

Yeah limit of the compiler analysis. So dynamic expressions can't be hoisted. I suppose we could just look for the loading prop in general and assume that you want to do the slower method regardless of true/false.

@ryansolid ryansolid reopened this Oct 22, 2024
@titoBouzout
Copy link
Contributor

I was thinking the same, pretending loading is always lazy and then change it at runtime (as originally intended)? To clarify, <img loading={signal()} src="image.jpg"/> becomes <img loading="lazy" src="image.jpg"/> but then its changed as soon as importNoded so there should be no noticeable different I suppose.

@danieltroger
Copy link

I was thinking the same, pretending loading is always lazy and then change it at runtime (as originally intended)? To clarify, <img loading={signal()} src="image.jpg"/> becomes <img loading="lazy" src="image.jpg"/> but then its changed as soon as importNoded so there should be no noticeable different I suppose.

Yeah that would be great! Would appreciate that.

@titoBouzout
Copy link
Contributor

Sent ryansolid/dom-expressions#374

@ryansolid
Copy link
Member

ryansolid commented Oct 23, 2024

I was thinking the same, pretending loading is always lazy and then change it at runtime (as originally intended)? To clarify, <img loading={signal()} src="image.jpg"/> becomes <img loading="lazy" src="image.jpg"/> but then its changed as soon as importNoded so there should be no noticeable different I suppose.

Would just switching to importNode in these cases be enough? The reason I ask is if we ever switched to always use importNode we wouldn't need the extra logic.

@titoBouzout
Copy link
Contributor

titoBouzout commented Oct 23, 2024

I think the problematic use case is when src is static and also loading is dynamic. The template becomes <img src="image.jpg"/> and the image is loaded as soon as you call importedNode, but then it's too late for applying loading=lazy. For this reason, the idea is to make of the template <img loading=lazy src="image.jpg"/> when we know that the user is providing a dynamic loading. Not sure if that clarifies it. I'm not sure if I understood what you mean @ryansolid

A possible issue I could think of, is when both the src and loading are dynamic, IIRC the order of the props matters. Mmh, now that I think, doesn't matter anymore with this change because the template becomes <img loading=lazy/>

@ryansolid
Copy link
Member

I see. I was expecting it still to only fire when connected. Probably because I believe cloneNode didn't until then. But I guess this is another difference with importNode.

I'm surprised though that Lit wouldn't have this problem then. I imagine it importNode before apply dynamic expressions.

@titoBouzout
Copy link
Contributor

Thanks, I understand now what you said. I suspect you are right, but then something is off because the difference between importNode and cloneNode wouldn't have mattered in the first place. I will investigate properly and report back the findings.

Just for clarification my previous message is confusing the behaviour of an image when inside a template vs a regular div

@titoBouzout
Copy link
Contributor

titoBouzout commented Oct 24, 2024

Consideration: If I understand the specification correctly, when an image lazy loads, depends on so many factors, that we can consider it "unknown" and have practically no realistic expectations about it. Among implementation suggestions for "lazy load scroll margin" are:

  • view port patterns for the device
  • scrolling speed and momentum
  • network quality
  • use preferences

https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-load-root-margin


That being said, I went into the original report here, run the code and it works as expected (per the considerations above). ex: images are lazy loaded with solid v1.7.8 and even with solid v1.9.x

There are a few gotchas about that code:

  • in Firefox the original code works as expected, but once the image is loaded (because you scroll) then continuous refreshes will load the image (as if loading=lazy where not there). To trigger a "loading=lazy" you will need to CTRL+F5[hard-reload] (on which case it won't load the image till you scroll). I read somewhere that once the image is cached it will not respect the loading=lazy attribute. Seems like the only important consideration for them was network usage.
  • in Chrome it always loads the image, but this is because the spacer is not tall enough(at least for my device). If I change the spacer to be exactly above 2051px then it will load the image only when I do scroll. Crazy uh?

Ryan is correct and images won't load when used by cloneNode or importNode, it will only load the image when appending it to the document, such document.body.append(img) (proof on next message)


Unfortunately there's still a bug in Firefox, when a partial is cloned Firefox won't respect loading=lazy and will load the image right away, repro https://playground.solidjs.com/anonymous/ed4384c4-744d-4df0-9ed8-2b4c5a65edfa
You can also switch importNode and cloneNode but that doesn't make any difference. It also shows that images won't load when cloned/imported, but only when appended.

Just in case, it doesn't seem to be a problem with the container https://playground.solidjs.com/anonymous/c382f493-516b-4661-b968-ddee3d7e4dc9

I suppose frameworks, such as Lit, don't have this problem because they set the property dynamically and do not template-clone, which make them slow.

Svelte switched their mechanism to set the src in a microtask. https://github.com/sveltejs/svelte/blob/86ef18165f35d132f77b711263f00899f8134554/packages/svelte/src/internal/client/dom/elements/attributes.js#L399

I have entered this bug in mozilla https://bugzilla.mozilla.org/show_bug.cgi?id=1926802


Possible workarounds:

  1. setting the src dynamically doesn't work for me in Firefox (the image loads and doesn't respect lazy=loading) https://playground.solidjs.com/anonymous/f9080437-aa97-4383-bae2-2aafca085d17
  2. setting both, the loading and src dynamically does work for me (the image loads when I scroll) https://playground.solidjs.com/anonymous/5a937398-9b84-49fb-8823-cc5e144de4fc (this is setting the values after it has been appended to the document....)

@danieltroger
Copy link

danieltroger commented Oct 24, 2024

I'm not quite following at my normal reading speed. Just to confirm; you can reproduce my issue?

It's that when I'm doing this the image always loads immediately in firefox: Screenshot 2024-10-24 at 14 06 28

And when I do this it works as expected:
Screenshot 2024-10-24 at 14 06 09

@titoBouzout
Copy link
Contributor

titoBouzout commented Oct 24, 2024

Yes, I can reproduce your problem. I understand something new, that should have been evident by the comment on the Svelte code.

  1. If you change the image(loading, src) before it's appended to the document (as it happens in your example) then it doesn't work (firefox) https://playground.solidjs.com/anonymous/c045bde8-1b3b-4026-b76c-e44e7332ac33
  2. if you change the image after it has been appended to the document, then it works (firefox) https://playground.solidjs.com/anonymous/22962675-39c0-4cb0-88af-bb865e28fa62

I am trying to understand why inlining in your example loading="lazy" makes it work, but I am having a brain seizure. This Firefox behaviour is just horrible.

This falls into wontfix territory in my opinion, because introducing microtasks to fix this will cause race conditions with any "advanced" use of imgs in apps.

@danieltroger
Copy link

Hmm I don't understand why you need a microtask. All I need is the extra arguments (true, false) always being set for the call to template whenever loading is assigned to a signal value.

By the way, I think the solidJS playground runs an outdated solid-js version. I wanted to make an example: https://playground.solidjs.com/anonymous/cd61a8ba-d91a-439f-b91c-c6c99b2756ac but it didn't work.

If you check the code, it's because no extra arguments are passed to template()
Screenshot 2024-10-24 at 20 51 12

While if I run it with my local solid-js setup they are there:
Screenshot 2024-10-24 at 20 51 36

@danieltroger
Copy link

danieltroger commented Oct 24, 2024

Here try this, here the image loads as expected, when you scroll down:
importNode.html.zip
and here I've just removed the arguments to template() and it instantly loads, without scrolling down (bg gets red)

cloneNode.html.zip

So this would be the expected output for the given input:
expectedOutput.zip

It works perfectly for me. Make sure to have devtools open and cache disabled here for reliable re-tests:
Screenshot 2024-10-24 at 21 12 40

@titoBouzout
Copy link
Contributor

Thanks @danieltroger very useful, so uh, there is indeed a difference between cloneNode and importNode. The difference with the previous code I had is that I was cloning/importing the whole template, not the first child.

You can toggle useCloneNode on the following example (and click refresh button from the playground) to reproduce, it works with importNode (in firefox) and doesn't work with cloneNode (in firefox)
https://playground.solidjs.com/anonymous/bc0b3503-7cc8-4d69-bdf7-099007fb3dfc

It seems to also work with dynamic values https://playground.solidjs.com/anonymous/c7cc0e4a-fdc8-4419-8906-dfcd4263ffbc

I have updated the PR to remove adding loading=lazy by default ryansolid/dom-expressions#374

So I suppose we are good to go and this PR should in theory work.

@titoBouzout
Copy link
Contributor

I suppose this doesn't work in Svelte because they are doing as I did, clone the whole template https://github.com/sveltejs/svelte/blob/86ef18165f35d132f77b711263f00899f8134554/packages/svelte/src/internal/client/dom/template.js#L51-L62
They seem to have the possibility of multiple children, which we don't have.

@danieltroger
Copy link

It seems to also work with dynamic values https://playground.solidjs.com/anonymous/c7cc0e4a-fdc8-4419-8906-dfcd4263ffbc

Ah good find!

I have updated the PR to remove adding loading=lazy by default ryansolid/dom-expressions#374
So I suppose we are good to go and this PR should in theory work.

Niice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants