layout: Use the Stylist to get list of @font-face rules - #46271
Merged
Merged
Conversation
simonwuelker
requested review from
Loirooriol,
gterzian,
mrobinson and
nicoburns
as code owners
July 4, 2026 21:43
simonwuelker
commented
Jul 4, 2026
Comment on lines
+1
to
+3
| [font-face-007.html] | ||
| [@font-face from shadow applies to :host] | ||
| expected: FAIL |
Member
Author
There was a problem hiding this comment.
All major browsers besides safari fail these three tests because they don't implement tree scoped names. Now we do too, for the same reason.
The fact that we passed them before was just luck, because we didn't restrict @font-face rules to their shadow root at all.
Member
There was a problem hiding this comment.
I think the reason we are failing these now is that stylesheet rules inside of shadow roots are not included in the call to iter_extra_data_origins(), so there is a real regression here as previously shadow root-level stylesheets were completely mixed with document-level ones. I think that's an acceptable regression until we properly integrate them into the cascade (either scoped to their shadow roots or not -- as only WebKit seems to do this properly).
Member
Author
There was a problem hiding this comment.
I think the reason we are failing these now is that stylesheet rules inside of shadow roots are not included in the call to iter_extra_data_origins()
Yes, each shadow root has a AuthorStyles with its own CascadeData, seperate from the one the Stylist holds.
simonwuelker
force-pushed
the
stylist-font-face-rules
branch
from
July 4, 2026 21:53
bd28786 to
6cfca54
Compare
|
🔨 Triggering try run (#28721206402) for Linux (WPT) |
|
Test results for linux-wpt from try job (#28721206402): Flaky unexpected result (40)
Stable unexpected results that are known to be intermittent (15)
Stable unexpected results (3)
|
|
|
simonwuelker
marked this pull request as draft
July 4, 2026 22:52
simonwuelker
force-pushed
the
stylist-font-face-rules
branch
from
July 7, 2026 12:58
6cfca54 to
cd9d4db
Compare
Signed-off-by: Simon Wülker <[email protected]>
Signed-off-by: Simon Wülker <[email protected]>
Signed-off-by: Simon Wülker <[email protected]>
Signed-off-by: Simon Wülker <[email protected]>
simonwuelker
force-pushed
the
stylist-font-face-rules
branch
from
July 7, 2026 16:07
cd9d4db to
17c077b
Compare
simonwuelker
added a commit
to simonwuelker/servo
that referenced
this pull request
Jul 7, 2026
servo#46344) The tests rely on the same font being used in all layout queries. However, its possible that the `Ahem` fonts loads inbetween queries, causing timing-dependent failures. This surfaces in servo#46271 because that PR changes the timing of font loads. When we wait for `document.fonts.ready` then the referenced PR reliably passes both tests. cc @dshin-moz Signed-off-by: Simon Wülker <[email protected]>
|
🔨 Triggering try run (#28880797909) for Linux (WPT) |
|
Test results for linux-wpt from try job (#28880797909): Flaky unexpected result (45)
Stable unexpected results that are known to be intermittent (18)
|
|
✨ Try run (#28880797909) succeeded. |
simonwuelker
marked this pull request as ready for review
July 7, 2026 17:05
mrobinson
requested changes
Jul 9, 2026
mrobinson
left a comment
Member
There was a problem hiding this comment.
Nice work! I have a few comments, but this already looks much better than what we have.
Comment on lines
+153
to
+160
| if let Some(window) = DomRoot::downcast::<Window>(self.global()) { | ||
| let document = window.Document(); | ||
| if document.stylesheets_changed_since_last_reflow() { | ||
| window.reflow(cx, ReflowGoal::LayoutQuery(QueryMsg::BoxArea)); | ||
| document.switch_font_face_set_to_loading_if_needed(cx); | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I don't think we want to do a synchronous reflow here every time we get the promise. Could you instead just call self.switch_to_loading() if document.stylesheets_changed_since_last_reflow()? A later layout will resolve the new promise created in that case.
Member
Author
There was a problem hiding this comment.
But that would be incorrectly return a unfulfilled promise if the stylesheets that changed did not include new web fonts right? Can web content observe that the promise is not resolved immediately?
Member
There was a problem hiding this comment.
There doesn't seem to be a way to synchronously detect if a promise has been fulfilled: https://stackoverflow.com/questions/30564053/how-can-i-synchronously-determine-a-javascript-promises-state. In any case, a small amount of unusual behavior here is probably a better cost to pay than a synchronous reflow.
Member
There was a problem hiding this comment.
A bit more context: calling document.fonts.ready is a common thing to do in script and it would be both web content visible and unexpected if that did a synchronous reflow.
Member
Author
There was a problem hiding this comment.
Another problem I just thought of is that FontFaceSet is supposed to fire events when it switches to loading or finishes loading fonts. We don't implement that yet iirc, but it means that we don't want to switch to loading unless we really should.
https://drafts.csswg.org/css-font-loading/#FontFaceSet-events
Member
There was a problem hiding this comment.
The page could detect it, but that behavior still seems better than a synchronous reflow. :/ If you can think of a better way to to avoid it that's fine though...
Member
Author
There was a problem hiding this comment.
Hmm, I'm not enthused but we can try it and see if things break I suppose.
Member
There was a problem hiding this comment.
Apologies, it seems like Blink and Gecko both flush layout when getting the promise, so we should likely be doing that to. I think you can restore the original code here before landing.
Comment on lines
+1
to
+3
| [font-face-007.html] | ||
| [@font-face from shadow applies to :host] | ||
| expected: FAIL |
Member
There was a problem hiding this comment.
I think the reason we are failing these now is that stylesheet rules inside of shadow roots are not included in the call to iter_extra_data_origins(), so there is a real regression here as previously shadow root-level stylesheets were completely mixed with document-level ones. I think that's an acceptable regression until we properly integrate them into the cascade (either scoped to their shadow roots or not -- as only WebKit seems to do this properly).
|
🔨 Triggering try run (#29014102680) for Linux (WPT) |
|
Test results for linux-wpt from try job (#29014102680): Flaky unexpected result (42)
Stable unexpected results that are known to be intermittent (15)
|
|
✨ Try run (#29014102680) succeeded. |
Member
Author
|
this is ready for another round of review. |
mrobinson
approved these changes
Jul 9, 2026
simonwuelker
force-pushed
the
stylist-font-face-rules
branch
from
July 9, 2026 15:36
a4e2ce3 to
92f1a14
Compare
Signed-off-by: Simon Wülker <[email protected]>
simonwuelker
force-pushed
the
stylist-font-face-rules
branch
from
July 9, 2026 15:38
92f1a14 to
62b5749
Compare
simonwuelker
enabled auto-merge
July 9, 2026 15:44
This was referenced Jul 10, 2026
pull Bot
pushed a commit
to Patreos998/servo
that referenced
this pull request
Jul 10, 2026
…6436) This is something I had written for servo#46271 but I got confused about if/when multiple `@font-face` rules with the same family name can apply at the same time. We now properly remove `@font-face` rules when a rule with higher cascade priority is inserted and we don't insert them when a conflicting rule with higher priority already exists. Testing: A new test starts to pass (and is no longer intermittent) Fixes servo#35935 Fixes servo#35520 --------- Signed-off-by: Simon Wülker <[email protected]>
shubhamg13
pushed a commit
to shubhamg13/servo
that referenced
this pull request
Jul 16, 2026
This was supposed to be implemented as part of servo#46271 but I forgot to hook up the code that actually skips dispatching a new request when there is already one to the same URL. Testing: This should not change existing behaviour but *may* reduce intermittency by virtue of making fewer network requests. Signed-off-by: Simon Wülker <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Instead of processing every font face rule as stylesheets are modified, process
all rules, properly cascaded by Stylo, right before layout. This fixes an issues with
flakiness around font loading and layers and avoids searching the stylesheet for
font face rules in
O(n).Fixes: #45975
Fixes: #36094.
Fixes: #35935.