Don't rely on the pagereveal event in lcp/image-upscaling.html #56629
Conversation
This file contains hidden or 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
Member
Author
|
cc @gsnedders, @smfr, @shaseley could you review this one? Thanks! |
smfr
approved these changes
Dec 10, 2025
gsnedders
requested changes
Dec 13, 2025
Member
gsnedders
left a comment
gsnedders
left a comment
There was a problem hiding this comment.
I'd like the re-run of the Safari job to complete to make sure this is fine, but my only real comment here is the indentation is off.
After the changes in web-platform-tests#56018 this test started to rely on `pagereveal` since the way we load the popup has changed. `pagereveal` is a new event type that's not implemented by every browsers yet and this test is for lcp rather than the CSS view transitions, so we shouldn't rely on that. On the other hand, if a browser doesn't expose the `pageshow` event it means that this is a sync `about:blank`, so the document should be immediately ready. That's why checking `popup.document.readyState` is simple and effective enough. Tested it with all major browsers and it seems like it's passing in all of them. Once Firefox lands the synchronous `about:blank` changes it would be good to remove this code all together.
canova
force-pushed
the
lcp-image-upscaling
branch
from
c6594c3 to
7aba0d9
Compare
canova
commented
Dec 15, 2025
Member
Author
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
The Safari failure seems unrelated since it failed in the before case too. I rebased the PR on top of the recent master branch now. Let's see if it's going to be different.
But I tested this manually on Safari and it was passing there.
gsnedders
approved these changes
Dec 15, 2025
Member
Yeah, the original run ran into an infrastructure issue, hence both before/after failing. The re-run looked fine, as does the run after rebasing. Anyhow, r+'d now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
After the changes in #56018 this test started to rely on
pagerevealsince the way we load the popup has changed.pagerevealis a new event type that's not implemented by every browsers yet and this test is for lcp rather than the CSS view transitions, so we shouldn't rely on that.On the other hand, if a browser doesn't expose the
pageshowevent it means that this is a syncabout:blank, so the document should be immediately ready. That's why checkingpopup.document.readyStateis simple and effective enough.Tested it with all major browsers and it seems like it's passing in all of them.
Once Firefox lands the synchronous
about:blankchanges it would be good to remove this code all together.Fixes #56620.