Skip to content

[Fiber] use srcset to trigger load even on img mount #30351

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.

Sign up for GitHub

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

Merged
gnoff merged 1 commit into from
Jul 25, 2024
Merged

[Fiber] use srcset to trigger load even on img mount #30351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,8 +692,19 @@ export function commitMount(
}
return;
case 'img': {
// The technique here is to assign the src or srcSet property to cause the browser
// to issue a new load event. If it hasn't loaded yet it'll fire whenever the load actually completes.
// If it has already loaded we missed it so the second load will still be the first one that executes
// any associated onLoad props.
// Even if we have srcSet we prefer to reassign src. The reason is that Firefox does not trigger a new
// load event when only srcSet is assigned. Chrome will trigger a load event if either is assigned so we
// only need to assign one. And Safari just never triggers a new load event which means this technique
// is already a noop regardless of which properties are assigned. We should revisit if browsers update
// this heuristic in the future.
if ((newProps: any).src) {
((domElement: any): HTMLImageElement).src = (newProps: any).src;
Copy link
Collaborator

@sophiebits sophiebits Jul 16, 2024

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

Copy link
Collaborator Author

@gnoff gnoff Jul 17, 2024

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

Copy link
Collaborator

@sophiebits sophiebits Jul 17, 2024

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?

} else if ((newProps: any).srcSet) {
((domElement: any): HTMLImageElement).srcset = (newProps: any).srcSet;
}
return;
}
Expand Down