[Fiber] Ensure srcset and src are assigned last on img instances
#30340
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
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 This change updates the dom fiber implementation to ensure that sizes is added before srcset
gnoff
force-pushed
the
ensure-srcset-last
branch
from
512ff02
to
01ef646
Compare
gnoff
changed the title
img instancessrcset and src are assigned last on img instances
kassens
reviewed
Jul 15, 2024
There was a problem hiding this comment.
While you're here, also work around the second mentioned bug of loading=lazy needed before src?
eps1lon
approved these changes
Jul 15, 2024
| @@ -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) | |||
There was a problem hiding this comment.
Safari truly is the new IE.
|
Does this fix this bug too? I guess the img tag needs to be appended to its parent before .src is set. |
|
And does this need to do something with srcset: |
Co-authored-by: Jan Kassens <jan@kassens.net>
|
Comparing: 8b08e99...f34f75a Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
funny that's the first contribution I made to React I think :) I suspect yes it probably does need to include srcSet as well. I'll land in another PR
Yeah this is something I thought about when adding this. The composition with a picture parent is probably a problem and needs more consideration. Unfortunately it's not as simple a change so I'm not going to do it now. I wonder if we should take a stronger stance on |
this should solve that issue too b/c it makes sure src and srcSet go last |
github-actions bot
pushed a commit
that referenced
this pull request
Jul 16, 2024
#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> DiffTrain build for [0117239](0117239)
D'oh, of course. |
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
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>
felixshiftellecon
added a commit
to felixshiftellecon/react
that referenced
this pull request
Jul 24, 2024
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>
This was referenced Jul 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Safari has a behavior (bug) where when you consturct an Image in javascript if you set srcset before properties for
sizesthe 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