fix: src and loading in lazy loaded images returns right value immediately

[paoloricciuti]

Because of a weird Firefox+cloneNode bug nowadays if there's a lazy loaded image we remove the loading and src attributes and we reinstate them in the next animation frame (this will trick Firefox in doing it's job).

However this is problematic if the user access those attributes on mount or in an action. For example a case like this

<script>
	function check(node){
		console.log(node.loading);
		console.log(node.getAttribute("loading"));
		console.log(node.src);
		console.log(node.getAttribute("src"));
	}
</script>


<img 
	use:check
	loading="lazy"
	src="https://unsplash.it/300"
	alt="cool"
/>

because as you can see from the logs the values are completely unexpected.

I found a solution to this problem but it might have other implications that i'm not aware of...basically when we handle_lazy_img after removing the attributes i also define two properties on the element to handle loading and src "manually" (the same is true for getAttribute). This keeps the attribute "not there" for Firefox while also allowing the user to access it in JS if needed.

Do you think it's a reasonable solution?

P.s. i was not able to add a test for this because jsdom acts a bit weird with loading and it's always undefined so it never actually enters in the if inside handle_lazy_img.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.
• If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: a7c49f6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Playground

pnpm add https://pkg.pr.new/svelte@15141

[trueadm]

I'm really not sure about this. How common is it that someone will imperatively try and access these in an effect straight after?

I've been thinking that maybe in FF we just always use importNode instead of cloneNode. Last time I checked, the performance cliff was only really evident in Chromium browsers?

[paoloricciuti]

I'm really not sure about this. How common is it that someone will imperatively try and access these in an effect straight after?

I stumbled across this because a client was having problems...the problem is not exactly if the user access it but if it uses the element with some library that then access them. And I think it would actually be decently common to mount "audio widgets" and such.

Regardless I think it's a pretty weird behavior and we should fix it. I'm fine if this is not the fix 😁

[trueadm]

This isn't correct? It doesn't take into account relative pathing AFAIK which differs from the attribute value

[trueadm]

console.log(img.getAttribute("src")); // "images/foo.png"
console.log(img.src);                 // "http://example.com/path/images/foo.png"

[paoloricciuti]

Ugh...I knew something was gonna bit me 😁 I'll check it I can find a fix

[trueadm]

I believe the same issue with relative paths applies here too

[trueadm]

I'm really not sure about this. How common is it that someone will imperatively try and access these in an effect straight after?

I stumbled across this because a client was having problems...the problem is not exactly if the user access it but if it uses the element with some library that then access them. And I think it would actually be decently common to mount "audio widgets" and such.

Regardless I think it's a pretty weird behavior and we should fix it. I'm fine if this is not the fix 😁

Yeah I don't think this is the right approach. I'm sure there's something else we can do, or maybe we cover it by adding a temporary getter on the element in DEV for FF that says to do this in a rAF?

[paoloricciuti]

maybe we cover it by adding a temporary getter on the element in DEV for FF that says to do this in a rAF?

Well if we want to add it we should add it regardless of the browser.

Maybe we should try to use importNode for FF first)?

[Rich-Harris]

should we just change this to if (node.name === 'video' || node.name === 'img' || is_custom_element)?

svelte/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js

Lines 60 to 68 in 64f86ee

	if (node.name === 'video' || is_custom_element) {
	// cloneNode is faster, but it does not instantiate the underlying class of the
	// custom element until the template is connected to the dom, which would
	// cause problems when setting properties on the custom element.
	// Therefore we need to use importNode instead, which doesn't have this caveat.
	// Additionally, Webkit browsers need importNode for video elements for autoplay
	// to work correctly.
	context.state.metadata.context.template_needs_import_node = true;
	}

[trueadm]

should we just change this to if (node.name === 'video' || node.name === 'img' || is_custom_element)?

svelte/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js

Lines 60 to 68 in 64f86ee

	if (node.name === 'video' || is_custom_element) {
	// cloneNode is faster, but it does not instantiate the underlying class of the
	// custom element until the template is connected to the dom, which would
	// cause problems when setting properties on the custom element.
	// Therefore we need to use importNode instead, which doesn't have this caveat.
	// Additionally, Webkit browsers need importNode for video elements for autoplay
	// to work correctly.
	context.state.metadata.context.template_needs_import_node = true;
	}

That won't fix it. The parent templates need to have been cloned with importNode too for it to work – as we append into their fragments and if they don't match, then the bug persists. Such a bizarre FF bug.

[paoloricciuti]

But apparently importNode is actually faster on Firefox so we could use it for everything and get rid of this weirdness all together... I'm only scared about doing user agent checks at runtime

[trueadm]

But apparently importNode is actually faster on Firefox so we could use it for everything and get rid of this weirdness all together... I'm only scared about doing user agent checks at runtime

What about for WebKit/Safari?

[paoloricciuti]

But apparently importNode is actually faster on Firefox so we could use it for everything and get rid of this weirdness all together... I'm only scared about doing user agent checks at runtime

What about for WebKit/Safari?

Didn't check but we can just check what's faster and use that no? The bug for lazy should only be there for Firefox