Fix font face resolution when renderer is blocked [chromium/src : main]
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
The failing test looks unrelated to your change; I think it is caused by:
https://chromium-review.googlesource.com/c/chromium/src/+/6533918
I will reach out to the author
Related details
- Shelley Vohr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
The failing test looks unrelated to your change; I think it is caused by:
https://chromium-review.googlesource.com/c/chromium/src/+/6533918
I will reach out to the author
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
The last failing test should be fixed now that this has landed:
https://chromium-review.googlesource.com/c/chromium/src/+/6869149
Related details
- Shelley Vohr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
if (d->IsActive() && d->IsLoadCompleted() &&Rune LillesveenSome of these conditions are incorrect in this context. For example, `!d->LoadEventStarted()` conflicts with:
Also, this code is only scheduled to run after the document has completed a layout, so `DidFirstLayout()` cannot be `false`.
I think there's a simpler and more correct way to do this:
```
void FontFaceSetDocument::FireDoneEventIfPossible() {
Document* d = GetDocument();
if (!d || !d->View()) {
return;
}// FireDoneEventIfPossible gets scheduled via PostTask at the end of a
// successful style+layout update. An invalidation may have occurred in
// the interim, so update style and layout synchronously here.
d->UpdateStyleAndLayout(DocumentUpdateReason::kUnknown);// These values can change during style+layout update, so check them
// *after* the call to UpdateStyleAndLayout.
if (should_fire_loading_event_) {
return;
}
if (!ShouldSignalReady()) {
return;
}FireDoneEvent();
}
```I *think* that should work.
It should be possible to write a unit test for this; adding futhark@ who might be able to help with that aspect.
Stefan ZagerShould be possible to create a wpt too, I think? Not sure I fully understand the issue, though. Is this an iframe only issue? Is it that the `ready` will not resolve in the iframe while the parent document is render-blocked, or is it just the iframe that is render-blocked?
Stefan ZagerHere's the electron bug:
https://github.com/electron/electron/issues/42284
Sounds like iframe only, which makes sense since only iframes throttle rendering.
I don't really know how electron works, but I can make an educated guess about the behavior of the test case in that bug...
The first call to `window.test()` adds an iframe in response to `DOMContentLoaded` in the top-level doc. At that point, the top-level window is hidden but also has not yet done the mandatory one rendering update after document load. When the mandatory update happens, it causes `document.fonts.ready` to resolve in both the top doc and the iframe.
The second call to `window.test()` happens while the top-level doc is shown, so rendering updates are happening and `document.fonts.ready` resolves as expected.
The third call to `window.test()` happens while the window is hidden and presumably rendering updates are paused. This is not render throttling as implemented in blink, because that code only affects cross-origin iframes, and the test case doesn't have cross-origin iframes. Rather, this is the suppression of main frame updates that occurs when an entire browser window is hidden; the implementation is in the browser process. This is where we get stuck, because the iframe will never do its initial rendering update. Previously, `document.fonts.ready` would still resolve because we do an opportunistic layout update during `Document::ImplicitClose()` in the iframe; but once the change mentioned in this CL's description landed we started suppressing the opportunistic initial layout, so we never schedule or run `FontFaceSetDocument::DidLayout`, which is where the promise gets resolved.
I'm not totally sure how to recreate this setup in a WPT, since we don't have access to electron API's like `window.hide()`. It *might* be possible to reproduce it using a cross-origin iframe moved outside the viewport to trigger render throttling. But in a unit test we can assert that the promise resolves in a microtask after `Document::ImplicitClose()` in the iframe, even if we don't run `UpdateLifecyclePhases`.
Shelley VohrActually, this probably doesn't require an iframe to reproduce in electron. You could probably do something like this instead:
```
var fonts = ['testfont1', 'testfont2', 'testfont3'];
window.test = async (str) => {
console.log('start', str);
let font = fonts.pop();
document.stylesheets[0].insertRule(`@font-face { font-family:"${font}"; src:url("path/to/${font}.woff2") }`);
document.stylesheets[0].insertRule(`div#${font} { font-family:"${font}" }`;
let div = document.createElement('div');
div.innerHTML = "Hello, world!";
div.id = font;
document.body.appendChild(div);console.log('before')
await iframe.contentWindow.document.fonts.ready;
console.log('after')
};
```Not sure if this is easier or harder to make into a WPT (the font files need to exist).
Stefan ZagerI might need some guidance for where to add the test/related files as I've not added a WPT before but i'm more than happy to have a go!
Rune Lillesveenfuthark@ -- this change lgtm, but I do think it needs a test. Is there an existing unit test suite that would be appropriate? I can help the author write the actual test.
Stefan ZagerWhen I said wpt, I thought "render blocking" meant the render blocking that happens because we're waiting for render blocking resources, but this is about render throttling? Then I don't know if you can do it with wpt. And I don't know what would be a suitable unit test framework for throttling.
Shelley VohrI'll take a look this week and provide guidance for writing the test.
Shelley VohrStefan Zager are you still able to provide some guidance here?
Stefan Zagerhi @fut...@chromium.org @sza...@chromium.org - I’d still love to land this if possible so if you could point me to next steps for a test that'd be great.
Test added.
Related details
- Rune Lillesveen
- Shelley Vohr
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Shelley Vohr
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
Document* d = GetDocument();I know this was already here, but `s/d/document`, please. Just `d` is a terrible variable name.
FontReadyPromiseResolvesWhileRenderThrottled) {Nice test.
Related details
- Shelley Vohr
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stefan Zager
| Code-Review | +1 |
1 comment
I know this was already here, but `s/d/document`, please. Just `d` is a terrible variable name.
Done
Related details
- Shelley Vohr
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Shelley Vohr
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/css/font_face_set_document.cc
Insertions: 3, Deletions: 3.
@@ -142,8 +142,8 @@
}
void FontFaceSetDocument::FireDoneEventIfPossible() {
- Document* d = GetDocument();
- if (!d || !d->View()) {
+ Document* document = GetDocument();
+ if (!document || !document->View()) {
return;
}
@@ -154,7 +154,7 @@
// FireDoneEventIfPossible gets scheduled via PostTask at the end of a
// successful style+layout update. An invalidation may have occurred in
// the interim, so update style and layout synchronously here.
- d->UpdateStyleAndLayout(DocumentUpdateReason::kUnknown);
+ document->UpdateStyleAndLayout(DocumentUpdateReason::kUnknown);
// These values can change during style+layout update, so check them
// *after* the call to UpdateStyleAndLayout.
```
Change information
Fix font face resolution when renderer is blocked
As a result of:
https://chromium-review.googlesource.com/c/chromium/src/+/5290838
... the FontFaceSet promise in e.g. contentWindow.document.fonts.ready
will never resolve while the renderer is blocked. This CL takes an
approach similar to that taken in MediaQueryList in order to enable the
promise to be resolved.
Bug: chromium:324572951
- M third_party/blink/renderer/core/css/font_face_set_document.cc
- M third_party/blink/renderer/core/display_lock/display_lock_utilities_test.cc
- M third_party/blink/renderer/core/loader/render_blocking_resource_manager_test.cc
Code-Review: +1 by Mason Freed, +1 by Stefan Zager, +1 by Rune Lillesveen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |