Skip to content

Commit

Permalink
[Fiber] Ensure srcset and src are assigned last on img instances (
Browse files Browse the repository at this point in the history
facebook#30340)

Safari has a behavior (bug) where when you consturct an Image in
javascript if you set srcset before properties for `sizes` the brwoser
will download the largest image size because it starts to load before
you communicate the sizes information.


https://x.com/OliverJAsh/status/1812408504444989588?t=CVHPqBaUiF5-6DBPGERTDA

There are likely other combinations or property order assignment that
can cause problems such as setting crossorigin after assigning src or
srcset. Conceptually we should withold the src and srcSet from the Image
instance until last so all relevant other properties can be assigned
before actually initiating any network activity.

This is an unforunate amount of code for what is realistically a bug in
Browsers but it should allow us to avoid weird regressions depending on
prop object order.

I didn't change the preload prop order because I don't believe preload
links have the same issue (they are not fetched as eagerly I believe).

One nice benefit of this change though is the img case can move higher
in the switch which is likely optimal given it's a relatively common
tag. Previously it was as low as it was because it was part of the void
element set so it couldn't be elevated without elevating less common
tags

---------

Co-authored-by: Jan Kassens <jan@kassens.net>
  • Loading branch information
gnoff and kassens authored Jul 16, 2024
1 parent fee423e commit 0117239
Showing 1 changed file with 47 additions and 1 deletion.
48 changes: 47 additions & 1 deletion packages/react-dom-bindings/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,53 @@ export function setInitialProperties(
// Fast track the most common tag types
break;
}
// img tags previously were implemented as void elements with non delegated events however Safari (and possibly Firefox)
// begin fetching the image as soon as the `src` or `srcSet` property is set and if we set these before other properties
// that can modify the request (such as crossorigin) or the resource fetch (such as sizes) then the browser will load
// the wrong thing or load more than one thing. This implementation ensures src and srcSet are set on the instance last
case 'img': {
listenToNonDelegatedEvent('error', domElement);
listenToNonDelegatedEvent('load', domElement);
// Mostly a port of Void Element logic with special casing to ensure srcset and src are set last
let hasSrc = false;
let hasSrcSet = false;
for (const propKey in props) {
if (!props.hasOwnProperty(propKey)) {
continue;
}
const propValue = props[propKey];
if (propValue == null) {
continue;
}
switch (propKey) {
case 'src':
hasSrc = true;
break;
case 'srcSet':
hasSrcSet = true;
break;
case 'children':
case 'dangerouslySetInnerHTML': {
// TODO: Can we make this a DEV warning to avoid this deny list?
throw new Error(
`${tag} is a void element tag and must neither have \`children\` nor ` +
'use `dangerouslySetInnerHTML`.',
);
}
// defaultChecked and defaultValue are ignored by setProp
default: {
setProp(domElement, tag, propKey, propValue, props, null);
}
}
}
if (hasSrcSet) {
setProp(domElement, tag, 'srcSet', props.srcSet, props, null);
}
if (hasSrc) {
setProp(domElement, tag, 'src', props.src, props, null);
}
return;
}
case 'input': {
if (__DEV__) {
checkControlledValueProps('input', props);
Expand Down Expand Up @@ -1269,7 +1316,6 @@ export function setInitialProperties(
}
case 'embed':
case 'source':
case 'img':
case 'link': {
// These are void elements that also need delegated events.
listenToNonDelegatedEvent('error', domElement);
Expand Down

0 comments on commit 0117239

Please sign in to comment.