Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-font-loading] document.fonts is too vagely defined #6126

Closed
MatsPalmgren opened this issue Mar 22, 2021 · 5 comments
Closed

[css-font-loading] document.fonts is too vagely defined #6126

MatsPalmgren opened this issue Mar 22, 2021 · 5 comments

Comments

@MatsPalmgren
Copy link

MatsPalmgren commented Mar 22, 2021

I'm adding a @font-face rule for a built-in font to the UA sheet in Gecko. This caused a ton of WPT failures because our implementation of document.fonts included it in the FontFaceSet it returns. The WPT framework itself makes the assumption (in /infrastructure/assumptions/document-fonts-ready.html) that document.fonts returns an empty set if the document itself hasn't added any font sources yet.

The spec says:
https://drafts.csswg.org/css-font-loading/#font-face-source

The value of the context’s fonts attribute is its font source, which provides all of the fonts used in font-related operations, unless defined otherwise.

and further:
https://drafts.csswg.org/css-font-loading/#document-font-face-set

The set entries for a document’s font source must be initially populated with all the CSS-connected FontFace objects from all of the CSS @font-face rules in the document’s stylesheets, in document order.

It's unclear from the quoted spec text (and the spec in general) whether document.fonts should be initially empty, or if it should include User and UserAgent origin font sources too.

We tend to think that it should be explicitly defined to start out empty, and thus that only Author origin font sources are exposed to the document. For a few reasons:

  1. it seems unlikely that web developers want to see fonts other than their own in document.fonts
  2. the semantics of FontFaceSet.delete (and clear) on a User or UserAgent font is unclear; it doesn't seem like a document should have the authority to remove either
  3. exposing User origin fonts to the document seems like a privacy / fingerprinting issue

Perhaps one should read from "in the document’s stylesheets, in document order" that only sheets linked, or embedded, from the document itself (and their @imports) should be included, but that's vague. A clear definition of "the document’s stylesheets" seems missing too.

CC @jfkthame

@tabatkins
Copy link
Member

I agree, it should only reflect Author (and User, presumably) faces.

I'm curious what you're adding a User Agent @font-face for? If it's a generic font, that won't work correctly (it'll be recognized by the quoted variant of its name as well, which the generics must not do), and if it's not a generic font, then you're exposing a new font to all pages that's guaranteed to exist.

@jfkthame
Copy link
Contributor

I agree, it should only reflect Author (and User, presumably) faces.

Why would it make sense to reflect User faces here? (In view of Mats' reasons (2) and (3), it seems undesirable.)

@tabatkins
Copy link
Member

Oh yeah, don't know what I was thinking. User sheets aren't exposed in document.stylesheets, so it would indeed be weird for fonts from them to show up in document.fonts.

@MatsPalmgren
Copy link
Author

I'm curious what you're adding a User Agent @font-face for?

It's a font with a handful of glyphs for rendering the list-style-type bullets / disclosure triangles + space. We're removing our legacy code path (bug 1542807) that used a special box type to render those. We'll now generate content and render that using the normal ::marker path.

If it's a generic font, that won't work correctly (it'll be recognized by the quoted variant of its name as well, which the generics must not do), and if it's not a generic font, then you're exposing a new font to all pages that's guaranteed to exist.

It's not a generic font. The working name is -moz-bullet-font (subject to change). Exposing it to all pages is intentional. Letting authors replace it with a @font-face overriding that name is also intentional.

@tabatkins
Copy link
Member

Luckily CSSOM already defines the precise term I need, so the spec should be well-defined now.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 12, 2021
…gin sheets from 'document.fonts'. r=jfkthame

Per w3c/csswg-drafts#6126 these were
never intended to be included in the first place so this is just
fixing a bug.  Note that I'm leaving them in the mRuleFaces array
so that the font loading machinery works the same as before.
I'm just excluding them when queried by document.fonts.

Differential Revision: https://phabricator.services.mozilla.com/D111694
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 14, 2021
…gin sheets from 'document.fonts'. r=jfkthame

Per w3c/csswg-drafts#6126 these were
never intended to be included in the first place so this is just
fixing a bug.  Note that I'm leaving them in the mRuleFaces array
so that the font loading machinery works the same as before.
I'm just excluding them when queried by document.fonts.

Differential Revision: https://phabricator.services.mozilla.com/D111694
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 15, 2021
…gin sheets from 'document.fonts'. r=jfkthame

Per w3c/csswg-drafts#6126 these were
never intended to be included in the first place so this is just
fixing a bug.  Note that I'm leaving them in the mRuleFaces array
so that the font loading machinery works the same as before.
I'm just excluding them when queried by document.fonts.

Differential Revision: https://phabricator.services.mozilla.com/D111694
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 15, 2021
…gin sheets from 'document.fonts'. r=jfkthame

Per w3c/csswg-drafts#6126 these were
never intended to be included in the first place so this is just
fixing a bug.  Note that I'm leaving them in the mRuleFaces array
so that the font loading machinery works the same as before.
I'm just excluding them when queried by document.fonts.

Differential Revision: https://phabricator.services.mozilla.com/D111694
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants