Skip to content
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

Image: Address onLoad not being called for SSR images #3612

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

liuyenwei
Copy link
Contributor

Summary

<Image /> accepts an onLoad callback which ends up being attached to the native img element's load event handler and is triggered once the image resource has finished loading.

The problem with this approach is that the onLoad handler does not get triggered if the image resource has already finished loading before the <Image /> component is rendered. The most common scenario of this happening is server-side rendering since the image request will be kicked off via the server-rendered HTML and, very likely, be completed by the time client hydration happens.

Change

This PR introduces a fix for this by attached a ref callback to the native img callback which will do the following:

  • check that _fixCompletedOnLoad is true, the img complete attribute is true, and the onLoad callback has not yet been triggered
  • if the above is true, we will call onLoad will a fake load Event. Because the load event has already happened, we need to construct our own.

Why is this in an experiment

I discovered this issue when debugging an impression drop with my Masonry V2 experiment. I found that, because my experiment resulted in the initial server rendered Pins not being unmounted/re-mounted, the image onLoad callbacks were never being triggered and consequently, we were not logging impressions.

I'm planning to test this change across both control and enabled groups of the Masonry V2 experiment to validate this and in parallel, will chat with metrics quality about rolling this out since it could have impression impact.

@liuyenwei liuyenwei requested a review from a team as a code owner June 5, 2024 16:52
Copy link

netlify bot commented Jun 5, 2024

Deploy Preview for gestalt ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 99b7294
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/6660a2cc93bb4c0008cfda00
😎 Deploy Preview https://deploy-preview-3612--gestalt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

const { _fixCompletedOnLoad } = this.props;
// For certain scenarios, such as server-side rendering, the image may already be loaded by the time the component is rendered resulting in the onLoad event not being triggered.
// To address these, we can use a ref callback and check whether the image is already loaded - if it is, we trigger the onLoad event manually.
if (_fixCompletedOnLoad && node?.complete && !this.onLoadCalled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!this.onLoadCalled

so handleLoad when image loads and we set this.onLoadCalled = true;. Then, when it renders we set the ref and we check onLoadCalled

handleLoad gets sets manually if this.onLoadCalled = true;.

"if it is, we trigger the onLoad event manually."

should && !this.onLoadCalled be && this.onLoadCalled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!this.onLoadCalled is here to guard against a scenario where the native img load callback is fired (and onLoad called) before the ref callback

i think the only situation where this could happen is:

  • image component renders
  • image request completes very quickly
  • ref callback is triggered

i think this would be an edge case but wanted to play it safe

@AlbertCarreras AlbertCarreras added the minor release Minor release label Jun 5, 2024
@AlbertCarreras AlbertCarreras self-requested a review June 5, 2024 17:37
@liuyenwei liuyenwei merged commit 876644e into pinterest:master Jun 5, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release Minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants