Change lcp/image-upscaling.html to open about:blank explicitly #56018
Conversation
There was a problem hiding this comment.
Could we make this a variant instead of a new test to avoid duplication?
There was a problem hiding this comment.
We could; the downside of that is we end up changing the test ID of the corrected version, and thus lose the scoring of it historically.
|
Curiously, the Firefox stability job failed, showing a bunch of flakiness: https://github.com/web-platform-tests/wpt/pull/56018/checks?check_run_id=55328993318 (https://community-tc.services.mozilla.com/tasks/Fij0zBsDTQWkZThTFGeQtQ/runs/0/logs/live/public/logs/live.log is the underlying TC logs), and showing results regressing: https://wpt.fyi/results/largest-contentful-paint/image-upscaling.html?run_id=5099988792377344&run_id=5182589603414016 This makes me wonder if we're actually seeing the opposite bug in Firefox, where loading the empty URL works but about:blank doesn't? I wonder if this hints at another difference v. the HTML spec, and Firefox is loading an initial I'm welcome to suggestions of how to make this test work in both Firefox and Safari, avoiding any differences in |
See https://bugs.webkit.org/show_bug.cgi?id=301239; this also adds an lcp/image-upscaling-empty-url.html to ensure we maintain coverage of whatever the underlying bug here is.
gsnedders
force-pushed
the
lcp-window.open-about.blank
branch
from
0bc355d to
ad78961
Compare
Seemingly, waiting for either https://wpt.fyi/results/html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-window-open.html?run_id=5136351159910400&run_id=5179304544829440&run_id=5159212767510528&run_id=5098551958044672 seems to cover this difference in Firefox behaviour, though not the fact you can interact with the initial |
|
@npm1 Can you help review this? |
|
LGTM. @gsnedders do you think long-term the duplicate test can be removed, perhaps with a non-LCP test for whatever the window.open() issue turns out to be? |
Yeah, I'd hope so. |
|
Hey folks, this test perma times out in Firefox Nightly now after these changes. We recently changed how And the I filed this bug in Bugzilla here for tracking. |
After the changes in web-platform-tests#56018 this test started to rely on `pagereveal` since the way we load the page 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.
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.
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.
After the changes in #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.
7 participants
See https://bugs.webkit.org/show_bug.cgi?id=301239; this also adds an lcp/image-upscaling-empty-url.html to ensure we maintain coverage of whatever the underlying bug here is.
Also related: web-platform-tests/interop#1211.