Reftests for font loading

[gsnedders]

This has come up several times recently.

At the moment, we can't test font loading with reftests because they are defined to wait on fonts loading.

That said, as long as we want it to be possible to run tests over WebDriver this will always be difficult given WebDriver doesn't provide us with any timing guarantees. As I wrote in an email recently:

in the case of any runner based on WebDriver we basically cannot provide strong guarantees about screenshot timing (the worst case is arguably something like wptrunner running Chrome on Android: the JS calls the execute async script callback, which then has to pass over the USB connection to the adb host, then get processed by ChromeDriver (when ChromeDriver receives the message, this has now completed the CDP communication and the browser will go back to spinning the event loop, AIUI), which then gets sent on to wptrunner over a TCP/IP socket, which then sends a request to ChromeDriver to take a screenshot, which then sends a CDP message (pausing the event loop, AIUI) to take a screenshot, etc. The more latency there is in this system the less we have guarantees about timing, and the harder it is to take screenshots at exact times to test things like font loading.

cc/ @foolip @fred-wang @Hexcles @youennf @jgraham @thejohnjansen

[Hexcles]

Thanks for filing the issue @gsnedders !

Let me fill in more context. I had this discussion earlier with @foolip when trying to deflake reftests with webfonts on Blink's test runner. Since webfont loading don't necessarily block the load event (which is another issue of its own), we decided to implement the same hack in Blink as wptrunner: to wait for fonts.ready (injected script).

However, there was the question about what to do with reftest-wait.

• wptrunner waits for fonts.ready in both normal reftests and those using reftest-wait.
• In Blink we decided not to wait for fonts.ready if reftest-wait is used. The argument back then was so that test author could have more control and hence test the timing of font loading. This leads to an inconsistency between Blink and wpt test runners (https://crbug.com/848804).

There's at least one test upstreamed from Blink that relies on the behaviour of Blink test runner: css/css-fonts/font-display/font-display.html (orignally from https://codereview.chromium.org/2895123002 by @irori) (@gsnedders' attempt to change Blink's behaviour to match wptrunner would cause this test to fail in Blink.)

However, it might not be feasible to change the wptrunner's behaviour because 1) some browsers already block the load event on webfont loading and 2) on some configuration (e.g. mobile as pointed out by @gsnedders ) it's hard or impossible to enforce strong requirements of screenshot timing.

[gsnedders]

Since webfont loading don't necessarily block the load event (which is another issue of its own)

This is https://github.com/w3c/csswg-drafts/issues/1088.

1) some browsers already block the load event on webfont loading

Gecko does, Chrome seems to too? (see discussion in above CSS WG issue)

2) on some configuration (e.g. mobile as pointed out by @gsnedders ) it's hard or impossible to enforce strong requirements of screenshot timing

I mean, we can if we can somehow pause the browser at the point of reftest-wait being removed and we can guarantee that the callback that removes it happens at exactly the right time?

[foolip]

It's not necessary to be able to synchronously force a screenshot for the timing control to be useful. All that is needed is that when the screenshot is requested, the rendering doesn't change in the following milliseconds or hundreds of milliseconds at most. This isn't very hard to achieve by trickling the font resource.

Tests like this already exist, and without this control it's not clear how to write them instead: https://chromium-review.googlesource.com/c/chromium/src/+/1082319/4/third_party/WebKit/LayoutTests/TestExpectations#2799

[gsnedders]

Until w3c/csswg-drafts#1088 is resolved, I think to do this we'd need a flag meaning "take the screenshot as soon as this is removed, regardless of whether the load event has fired".

I don't know if we can change the semantics of "reftest-wait" to avoid waiting for the load event; a quick grep through usage tends to show it statically exists and is removed dynamically (almost?) everywhere, so it should be safe, provided we wait for the first paint after it is removed?

[foolip]

I don't know if we can change the semantics of "reftest-wait" to avoid waiting for the load event

We probably can. @Hexcles and I had a lot of "fun" with this a while back when fixing http://crbug.com/507054, in this code: https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc?l=368&rcl=dab7ed26d647862149d4c4b5e6d044fbe235571e

To know if it's safe, we'd just implement the new behavior and see if anything begins to fail.

However, I realize there's a problem here. If we don't wait for the load event, then when should we look to see if the reftest-wait attribute is on document.documentElement? DOMContentLoaded?

[gsnedders]

However, I realize there's a problem here. If we don't wait for the load event, then when should we look to see if the reftest-wait attribute is on document.documentElement? DOMContentLoaded?

Presumably DOMContentLoaded?

[foolip]

Sure, that makes sense. So first wait for DOMContentLoaded, and check for the reftest-wait attribute in that callback. If it's present use MutationObserver to wait until it's removed, and if not wait for load + document.fonts.ready.

[gsnedders]

@jgraham how do you feel about changing behaviour here?

[jgraham]

Nervous. It makes us incompatible with gecko reftests, so even if we fix any cases where we get different results because of this change, we will likely have people expecting the old behaviour. I also wonder how stable the tests will be if we require the screenshot between DOMContentLoaded and load; for WebDriver implementations there's a full round trip before the screenshot itself is taken, so you need to be careful to ensure that the page doesn't do any layout in that time.

That said, used with care, it does open up some new test scenarios. I at least think we want to test any change carefully before deploying it.

[foolip]

https://crbug.com/848804 is still open, and I'm surprised we haven't learned of any differences in results that are because of this. The discrepancy doesn't seem very consequential, but I think it'd be best to have Chromium align here.

If we have a strong use case for not waiting for the document.fonts.ready promise, having a new opt-in for that probably makes the most sense, maybe class="refest-wait reftest-dont-wait-for-fonts".