-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
[Fiber] use srcset to trigger load even on img mount #30351
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -694,6 +694,8 @@ export function commitMount( | |||
case 'img': { | |||
if ((newProps: any).src) { | |||
((domElement: any): HTMLImageElement).src = (newProps: any).src; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if srcset is present and this branch is hit, does load trigger? I might have assumed that it would ignore src completely when srcset is present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So firefox only triggers load when you set src
even if there is a srcset
. So I prefer src
here to trigger the load in FF. Chrome triggers it when either is set so setting src
if it exists is good enough but it will fall back if only srcset
is set. Safari never triggers a new load so the existing solution doesn't work and this change won't really do anything about that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind adding this as a comment?
Comparing: 0117239...0971bb4 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
|
@@ -694,6 +694,8 @@ export function commitMount( | |||
case 'img': { | |||
if ((newProps: any).src) { | |||
((domElement: any): HTMLImageElement).src = (newProps: any).src; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind adding this as a comment?
In facebook#23316 we fixed a bug where onload events were missed if they happened too early. This update adds support for srcset to retrigger the load event. Firefox unfortunately does not trigger a load even when you assign srcset so this won't work in every browser when you use srcset without src however it does close a gap in chrome at least
In #23316 we fixed a bug where onload events were missed if they happened too early. This update adds support for srcset to retrigger the load event. Firefox unfortunately does not trigger a load even when you assign srcset so this won't work in every browser when you use srcset without src however it does close a gap in chrome at least DiffTrain build for [7f217d1](7f217d1)
In #23316 we fixed a bug where onload events were missed if they happened too early. This update adds support for srcset to retrigger the load event. Firefox unfortunately does not trigger a load even when you assign srcset so this won't work in every browser when you use srcset without src however it does close a gap in chrome at least