FontFaceSet.add/remove shouldn't need to call FlushUserFontSet.
Categories
(Core :: Layout, task)
Tracking
()
People
(Reporter: emilio, Assigned: emilio)
References
Details
(Keywords: leave-open)
Attachments
(4 files)
Assignee | ||
Comment 1•3 years ago
|
||
They only deal with non-rule faces, so we don't need to flush (which
only ensures that rule faces are properly created, for the purposes of
this function).
Also add a couple stylistic cleanups.
Assignee | ||
Comment 2•3 years ago
|
||
There's no way these need to do a flush as they just return data from
the descriptors etc.
Similarly .loaded also relies on the font face set.
Assignee | ||
Comment 3•3 years ago
|
||
These are a bit more tricky because DescriptorUpdated() does check
whether we're in the font set, but only to invalidate it, so I think
it's not needed either (as if it's dirty, we'd invalidate it regardless
when the time comes).
Depends on D96341
Assignee | ||
Comment 4•3 years ago
|
||
This one is even more subtle. This could cause a call from SetStatus()
to OnFontFaceStatusChange() to not be done as part of the load()
function (if the face is about to get added to the set or such).
However, when it's actually added via UpdateRules, we'd properly
invalidate the relevant state as well, so I think this is also fine, and
our existing tests also agree.
Depends on D96342
Assignee | ||
Updated•3 years ago
|
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e7714e0e74a3 FontFace getters don't need to flush the fontset. r=heycam
Assignee | ||
Comment 6•3 years ago
|
||
Thanks to one of the examples cam found in the patch from comment 3, we found issues everywhere:
emilio
heycam: fun, the font API is so broken :(
heycam: (as in non-interoperable)
This probably shouldn't throw per your example:
<style>@font-face { font-family: Ahem; src: url('/tests/dom/base/test/Ahem.ttf'); }</style>
<iframe src="about:blank"></iframe>
<script>
onload = function() {
let f = [...document.fonts][0];
document.fonts.add(f);
document.querySelector("style").remove();
document.querySelector("iframe").contentDocument.fonts.add(f);
}
</script>
But it does because we flush the font face set of the wrong document. Blink will silently do nothing, ughhh
Well, actually blink seems like it'd happily share FontFace objects with other documents
Even if CSS-connected
This is all broken
heycam
hmmmm
emilio
Blink never throws in .add(), just silently returns. And the CSS-connected check is basically document->fontfacerules.contains(face), which is clearly broken if face doesn't come from document
Ugh, I'll paste this somewhere and file spec issues / bugs about this tomorrow.
would it make sense to make the "CSS-connected" flag immutable?
(as in, make it just mean "this came from a rule")
assuming people aren't doing this "grab face, remove sheet, try to do stuff with it", it seems like it'd nicely simplify the model...
So, browser/spec issue time...
Assignee | ||
Updated•3 years ago
|
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e2f1e514e84 FontFace.load() doesn't need to flush the user fontset. r=heycam
Comment 8•3 years ago
|
||
Backed out changeset 9e2f1e514e84 (Bug 1675853) for causing wpt failures in ch-pseudo-recalc-on-font-load.html
Backout link: https://hg.mozilla.org/integration/autoland/rev/8fae8913941cae2596f04cdad5ef9dc7d73c32b9
Failure log: https://treeherder.mozilla.org/logviewer?job_id=321153317&repo=autoland&lineNumber=1575
Comment 9•3 years ago
|
||
bugherder |
Assignee | ||
Comment 10•3 years ago
|
||
Should be green once bug 1675950 re-lands.
Comment 11•3 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/017aa3b39d73 FontFace.load() doesn't need to flush the user fontset. r=heycam
Comment 12•3 years ago
|
||
Backed out changeset 017aa3b39d73 (Bug 1675853) for causing wpt failures in ch-pseudo-recalc-on-font-load.html
Failure log: https://treeherder.mozilla.org/logviewer?job_id=321179115&repo=autoland&lineNumber=1581
Comment 13•3 years ago
|
||
Backout by malexandru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e5683b64ed3 Backed out changeset 017aa3b39d73 for causing wpt failures in ch-pseudo-recalc-on-font-load.html
Comment 14•3 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
Assignee | ||
Updated•3 years ago
|
Comment 15•2 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
Assignee | ||
Updated•2 years ago
|
Comment 16•2 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.
Assignee | ||
Updated•2 years ago
|
Comment 17•1 year ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.
Description
•