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

Handle HTML Element onLoad events properly when they fire before React onLoad handlers have been attached #23316

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Feb 16, 2022

Description

This PR proposes a solution for the bug described in #13862

The approach here is to unconditionally re-trigger an img load/error by setting the src property in commitMount. The reason this works is if we end up with a load/error anytime prior to commitMount it will not invoke the onLoad / onError callbacks and the re-setting of the src property should trigger a secondary load event which executes in a later browser macro-task. timing wise this should slot in before or after passive effects depending on sync/concurrent render and task priority.

For load events that happen after commitMount they will play out undisturbed.

The reason we don't need similar behavior for commitUpdate is the src attribute is updated in the commit phase so changing src values are already synchronized to the commits.

We decided to not pursue audio/video/source in this PR to further explore this approach and performance implications for those element types.

Open Question

We opted out of specifying and asserting a specific behavior on Suspense fallbacks. currently when a load event fires after an img is suspended the event plays out even though the image is part of a suspended tree. We need to figure out what we want the event replaying behavior to be here

Remaining tasks

  • consider whether size addition is worth it for this edge case
    support more element types
  • support error instead of just load
  • confirm semantics around when load events can fire (element creation, src change, etc...)
  • rebase to cleanup commit history

@sizebot
Copy link

sizebot commented Feb 16, 2022

Comparing: a232744...ea891ce

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.10% 131.31 kB 131.44 kB +0.09% 42.00 kB 42.04 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.10% 135.96 kB 136.09 kB +0.02% 43.38 kB 43.39 kB
facebook-www/ReactDOM-prod.classic.js +0.18% 434.31 kB 435.10 kB +0.16% 79.46 kB 79.59 kB
facebook-www/ReactDOM-prod.modern.js +0.19% 420.82 kB 421.60 kB +0.18% 77.43 kB 77.57 kB
facebook-www/ReactDOMForked-prod.classic.js +0.18% 434.31 kB 435.10 kB +0.16% 79.46 kB 79.59 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against ea891ce

@sebmarkbage
Copy link
Collaborator

support more element types

The <link> tag doesn't load until inserted into the document. The <image> tag is deprecated. So I think the only other ones are <video>, <audio> and <source> which can have loadstart and error fire early.

Video and Audio are interesting because I believe they could actually even start autoplaying without being committed in some browsers. We might want to also fix that.

Another thing to consider is what semantics we want for hydration. For media in particular autoplaying should work before hydration, so a lot of events might fire before that. In general, the conclusion we've come to for hydration is that you can't reason about the state that happened before because you've lost access to the state things were in so we're not doing replaying in general. I think the same rationale applies here. The hydration code needs to read back the state to restore its internal state. Same as reading back the value of a text field upon hydration.

We could say that these other cases should also be treated as hydration but it might be easier to treat hydration as a special case and have the others just work.

@gnoff gnoff force-pushed the bugfix-defer-img-load branch 4 times, most recently from e78933a to 82e04e7 Compare February 28, 2022 20:21
early load events will be missed by onLoad handlers if they trigger before the tree is committed
to avoid this we reset the src property on the img element to cause the browser to re-load
the img.
}
return;
case 'img': {
if ((newProps: any).src) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sebmarkbage using props here instead of dom properties but is kinda arbitrary. At first I thought there was a distinction between null src and empty string src but I see that you disable both so we could use either. any reason to prefer one over the other?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really. One thing I was thinking was that maybe we should not set the src during render and only set it here so that it doesn't start loading until commit. In that case we'd have to use newProps.src.

This is relevant to trusted types, maybe it has to use the props for those? Not sure what happens if you read a src set by trusted types, does it return a trusted type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah yeah that is what I was asking about with #23316 (comment)

what are trusted types?

@sebmarkbage sebmarkbage merged commit 086fa8e into facebook:main Feb 28, 2022
return props.text;
}

// function AsyncText(props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed (temporarily) the suspense test case at Seb's suggestion

I briefly described the issue above. I'm not yet familiar enough with the Suspense to speak to how this would behave ideally.

Open Question

We opted out of specifying and asserting a specific behavior on Suspense fallbacks. currently when a load event fires after an img is suspended the event plays out even though the image is part of a suspended tree. We need to figure out what we want the event replaying behavior to be here

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use xit() for a skipped test so that it shows up in the list and so that there is no ambiguity about whether the code is there accidentally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

xit gets linted against since it’s something you do temporarily and disabling the lint makes a similar accidental point.

Since xit is a temporary measure that you use while developing I feel like it’s the wrong primitive for checking in disabled code.

@gnoff gnoff deleted the bugfix-defer-img-load branch March 1, 2022 18:29
@gnoff gnoff restored the bugfix-defer-img-load branch March 1, 2022 21:46
@gnoff gnoff deleted the bugfix-defer-img-load branch March 1, 2022 23:25
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
early load events will be missed by onLoad handlers if they trigger before the tree is committed
to avoid this we reset the src property on the img element to cause the browser to re-load
the img.

Co-authored-by: Josh Story <story@hey.com>
gnoff added a commit to gnoff/react that referenced this pull request Jul 16, 2024
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
gnoff added a commit to gnoff/react that referenced this pull request Jul 25, 2024
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
gnoff added a commit that referenced this pull request Jul 25, 2024
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
github-actions bot pushed a commit that referenced this pull request Jul 25, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants