Early / late property setup is not fully sound anymore
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox101 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
(Keywords: perf-alert)
Attachments
(2 files)
There are some cases where the font-size can depend on the writing-mode of the element, see:
Bug 1610815 adds a similar situation, in which you can get declaration-order-dependent font sizes. For example I'd expect these two elements to render with different sizes (you should be able to get the same result on trunk with a font with no zero-width character).
This is a bit on the edge-case side of things, but we should try to address it in a way that doesn't regress performance (much?).
It seems we should be able to, during this initial loop over the declarations, keep track of which longhands are we cascading, and avoid looping if there are no writing-mode dependent properties. It could actually even be an improvement if there are no early properties at all.
<!doctype html>
<style>
#a {
writing-mode: vertical-lr;
font-size: 100vi;
}
#b {
font-size: 100vi;
writing-mode: vertical-lr;
}
</style>
<div id="a">A</div>
<div id="b">B</div>
Hiro, can you confirm that this is an issue with your patch?
|
Assignee | |
Updated•4 years ago
|
|
||
Comment 1•4 years ago
|
||
Huh, I am quite surprised. Is the declaration order causing different results? Both font sizes look same size with the patch for bug 1610815. Though I modified the test case to use a moderate font-size, i.e. 30vi.
|
|
Assignee | |
Comment 2•4 years ago
|
||
I haven't tested it, this was only due to code inspection.
You're right the fonts are the same in that test-case, but they're the wrong size. They don't depend on declaration order because we compute_writing_mode after cascading early properties. But that means that we do the computation of font-size against the inherited writing-mode, which is not right. This test-case shows the issue more clearly.
And actually, it seems WebKit has the same bug.
|
|
Assignee | |
Comment 3•4 years ago
•
|
||
Ref: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff73759f975be8eed465da59a24197ecacb31b04
Patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=786b2abfcc1bed9c4bf757dd34f80441d1c30bae
Perf results (eventually): https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=ff73759f975be8eed465da59a24197ecacb31b04&newProject=try&newRevision=786b2abfcc1bed9c4bf757dd34f80441d1c30bae&framework=10&page=1
|
|
Assignee | |
Comment 4•4 years ago
|
||
This makes the worst case for cascade performance slightly more
expensive (4 rather than three declaration walks), but my hope is that
it will make the average case faster, since the best case is now just
two walks instead of three, and writing mode properties are somewhat
rare.
This needs a test, but needs to wait until the writing-mode dependent
viewport units land (will wait to land with a test).
|
|
Assignee | |
Comment 5•4 years ago
|
||
Talos looks reasonably good: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=ff73759f975be8eed465da59a24197ecacb31b04&newProject=try&newRevision=786b2abfcc1bed9c4bf757dd34f80441d1c30bae&framework=1&page=1&showOnlyComparable=1&showOnlyImportant=1
I'll land this before the vi/vh units and file a follow-up for the test.
|
||
Comment 7•4 years ago
|
||
| bugherder | ||
|
||
Comment 8•4 years ago
|
||
== Change summary for alert #33811 (as of Tue, 12 Apr 2022 19:02:19 GMT) ==
Improvements:
| Ratio | Test | Platform | Options | Absolute values (old vs new) |
|---|---|---|---|---|
| 11% | perf_reftest style-attr-1.html | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 4.06 -> 3.62 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=33811
Description
•