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-fonts-4] font-display says it's valid in @font-feature-values but it isn't an at-rule #2973

Closed
rsheeter opened this issue Jul 31, 2018 · 30 comments

Comments

@rsheeter
Copy link

https://drafts.csswg.org/css-fonts-4/#font-feature-values-syntax appears to say only at-rules are allowed in @font-feature-values.

https://drafts.csswg.org/css-fonts-4/#font-display-font-feature-values seems to say font-display - not an at-rule - is valid in @font-feature-values.

Apologies if I'm horribly misunderstanding but ... how does this work? Is @font-feature-values Roboto { font-display: swap; } valid? If not, how do you use these together?

@litherum
Copy link
Contributor

Yep. I'm not sure why we support putting font-display in @font-feature-values when you could instead put it in your @font-face.

@rsheeter
Copy link
Author

rsheeter commented Jul 31, 2018 via email

@tabatkins
Copy link
Member

It's allowed in @font-feature-values so you can apply a font-display even if you don't have control over the @font-face rules themselves - as mentioned, this is useful for the Google Fonts case.

The only actual requirement is that at-rules can't contain both style rules and declarations; since @font-feature-values doesn't contain either yet, it's fine to make it contain the font-display declaration. It just needs to be defined to do so.

@litherum
Copy link
Contributor

litherum commented Aug 1, 2018

if you don't have control over the @font-face rules themselves - as mentioned, this is useful for the Google Fonts

I don't understand; Google Fonts has control over the @font-face rules.

@svgeesus
Copy link
Contributor

svgeesus commented Aug 1, 2018

Yep. I'm not sure why we support putting font-display in @font-feature-values when you could instead put it in your @font-face.

We seem to allow it in both places, and I can see reasons for allowing it in both, but @rsheeter is correct that the spec is not currently self consistent regarding the content model of @font-feature-values.

I don't understand; Google Fonts has control over the @font-face rules.

Yes, but a content author who is using Google-hosted fonts (and Google-hosted @-import-ed stylesheets) doesn't. But they can add @font-feature-values to their own stylesheets.

@fantasai fantasai added the css-fonts-4 Current Work label Aug 9, 2018
@svgeesus svgeesus self-assigned this Aug 17, 2018
@rsheeter
Copy link
Author

To give a little context for Google Fonts, we don't know what font-display our users want so we'd have to add a param to let the user specify it. Adding a param breaks cache sharing between sites that would otherwise share. Our stats suggest that cross-site caching is currently quite effective and we'd like to keep it that way. If we were to end up with numerous parameters to tune the @font-face in various ways the cache fragmentation could be significant.

@drott
Copy link
Collaborator

drott commented Sep 24, 2018

Adding a param breaks cache sharing between sites that would otherwise share. Our stats suggest that cross-site caching is currently quite effective and we'd like to keep it that way. If we were to end up with numerous parameters to tune the @font-face in various ways the cache fragmentation could be significant.

@rsheeter, could you clarify the level of caching you are referring to? I am right in assuming you mean a cross-site Google Fonts server side cache for the CSS that is delivered to clients? Or browser side caching of @font-faces?

@litherum
Copy link
Contributor

font-display has nothing to do with font features. I don't think this particular mechanism is a good fit.

@lilles
Copy link
Member

lilles commented Sep 25, 2018

Regarding syntax, at least Gecko has shipped @font-feature-values accepting <rule-list>, so they will only recognize @swash in the latter rule.

  @font-feature-values family {
    font-display: block;
    @swash { swishy: 1; flowing: 2; }
  }
  @font-feature-values family {
    dummy { }
    @swash { swishy: 1; flowing: 2; }
  }

@KenjiBaheux
Copy link

@litherum I see your point about font-display not being a "font feature" but the hook to the family seemed quite fitting. Any alternative suggestion?

@lilles
Copy link
Member

lilles commented Oct 19, 2018

Could a new rule for @font-face default values be an alternative?

@font-face-defaults <family-name># {
  <declaration-list>
}

@lilles
Copy link
Member

lilles commented Nov 26, 2018

Any feedback on this, or a counter-proposal?

Could a new rule for @font-face default values be an alternative?

@font-face-defaults <family-name># {
  <declaration-list>
}

@KenjiBaheux
Copy link

This looks reasonable to me.

aarongable pushed a commit to chromium/chromium that referenced this issue Nov 27, 2018
This CL implements support for specifying a default value for
font-display for @font-face rules which do not provide a font-display
descriptor. See spec [1]. The implementation is currently behind a flag
which is off by default (not tested, not in experimental).

For intent-to-implement, see [4].

There is a question if we should mix descriptors with @-rules in
@font-feature-values because an unrecognized descriptor would be seen as
part of the following rule as the start of a qualified ruled which means
forward-compatibility could break. Also, there is a question if this is a
font feature value and should be added to that @-rule at all. Both these
issue are discussed/mentioned in [2].

Since using a descriptor directly in @font-feature values is not
compatible with Gecko, we're wrapping the font-display descriptor in a
block with no prelude for now.

Note that the default font-display cannot currently apply to fonts loaded
by FontFace.load() because the dictionary passed to FontFace have default
values which effectively overrides any @font-feature-values
descriptors[3].


[1] https://drafts.csswg.org/css-fonts-4/#font-display-font-feature-values
[2] w3c/csswg-drafts#2973
[3] w3c/csswg-drafts#3188
[4] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/C6taiCkYayU

Bug: 777846

Change-Id: I290f23e7489afeef64812d7604417268c2464455
Reviewed-on: https://chromium-review.googlesource.com/c/1222966
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611056}
@litherum
Copy link
Contributor

Would the @font-face-defaults rule apply to all @font-family blocks that share the font name?

It’s a little unfortunate to make an entirely new at-rule just for a single descriptor. What else might we eventually want to put in that rule?

I wonder if instead of having a proliferation of different kinds of at-rules, we could instead do something like WebIDL’s “partial interfaces” or Swift’s “extensions” to allow you to re-open a previously closed block.

@tabatkins
Copy link
Member

...oooh, that's a good idea. Hm.

How about this?

@partial font-face {
 ...
}

The @partial rule looks identical to a normal at-rule, just with the normal at-rule's name pushed forward as a plain ident and @partial being the name instead. It otherwise has the same syntax as the extended at-rule; additional prelude comes after the ident, etc. (So, for example, counter styles would be extended with @partial counter-style foo {...}.)

At-rules have to define that they're extensible; it's expected that every "closed" at-rule will be. They must define precisely how to match themselves against the "main" at-rule: for example, @partial counter-style is matched according to the counter style name; @partial font-face is matched according to the font-family descriptor and the various styling descriptors that are relevant for matching, so it extends all compatible @font-face rules.

All the @partial rules are collected and merged in order of appearance, as usual. They extend whichever version of the "main" rule that won the cascade (for all existing ones, this is just the last one). Relative ordering of @partial vs "main" rules is unimportant; the winning partial declarations are all ordered after the "main" declarations.

Worked out example:

@partial counter-style foo {
  prefix: "pre";
  suffix: "post";
}

@counter-style foo {
  system: cyclic;
  symbols: A B C;
}

@partial counter-style foo {
  suffix: "after";
  negative: "neg";
}

@counter-style foo {
  system: alphabetic;
  symbols: A B C;
  prefix: "before";
}

This is equivalent to:

@counter-style foo {
  system: alphabetic;
  symbols: A B C;
  prefix: "pre";
  suffix: "after";
  negative: "neg";
}

For font-face, the following would extend all faces of the "foo" family with a font-display value:

@partial font-face {
  font-family: "foo";
  font-display: optional;
}

If you wanted to only extend the bold faces, you could do:

@partial font-face {
  font-family: "foo";
  font-weight: bold;
  font-display: optional;
}

Thoughts?

@litherum
Copy link
Contributor

litherum commented Nov 27, 2018

I presume that if you don't have any font-family, font-weight, font-style, or font-stretch descriptors in the @partial font-face, then the contents of the @partial font-face would be applied to every @font-face (thereby letting you set a default font-display for every web font in the document). Presumably, adding any of those descriptors would filter the set of @font-face blocks the @partial applies to. (And the filtering would be done on range intersection, presumably, because you could have font-weight: 300 800;.) And any other descriptors like font-feature-settings wouldn't filter, but would instead be applied to the @font-face the same way that font-display would be applied.

I think you're right that, if we want something like @partial, at-rules will have to opt-in, and when they opt-in, they will have to specify matching rules like what I just described above for @font-face.

I'm not sure that the cascading behavior you described above is the best behavior, but I'll discuss it with people on our team who know more about that.

@tabatkins
Copy link
Member

I presume that if you don't have any...

Yes, that's exactly what the last example is about.

I'm not sure that the cascading behavior you described above is the best behavior, but I'll discuss it with people on our team who know more about that.

The other possibility is that @partial is closed and last-wins too, but then you might want to write @partial partial ... ^_^

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed font-display says it's valid in @font-feature-values but it isn't an at-rule.

The full IRC log of that discussion <dael> Topic: font-display says it's valid in @font-feature-values but it isn't an at-rule
<dael> github: https://github.com//issues/2973#issuecomment-442167124
<dael> TabAtkins: A while back got a requirt to override font descriptor for fonts they don't control. Specifically google fonts. Google fonts control that and due to caching don't want arbitrary things in there. Idea is add font-display to font feature values which kinda adds additional information and we can expose that
<chris> q+ to note this is very interesting but seems more CSS Syntax than CSS Fonts 4. Would also like to see the proposal more fleshed out.
<dael> TabAtkins: Right now font-feature-values doesn't take descriptors. Needs minor spec edits for that.
<dael> TabAtkins: myles objected because seems ad hoc. We want to define additional fontface, but they cascade atomically. Rather then trying to come up with special fix for this one problem we thought why not try and fix the general problem and have a way to explicitly extend these @rules
<myles> i didn't "object"
<myles> just said it would be a shame if we did it
<dael> TabAtkins: IN thread had proposal for @partial rule that looks like an @rule where partial is the first ident. Whatever is in there is matched with the appropriate @rule and override what's in it with the partial. @partialFontFace and then put the descriptor. Font family or a weight or what have you. CHanges the font display value of font faces loaded from another source
<dael> TabAtkins: Only a handful of atomic cascade now, but this lets you handle that override case
<dael> TabAtkins: Proposal in thread if we want it. Probably a new WD, doesn't fit in Fonts spec. I'm happy to edit and persue if people are interested, but want interest before more time
<dael> leaverou: How to determine which @rule it overrides?
<dbaron> q+
<dael> myles: The way this would have to work is each @rule has to opt in. By default none get a change and we opt in @rules. When it's opted in it would have to spec how the partial rules match with the base @rule. For Fonts there are 3 descriptors that would match, different for other @ rules
<dael> TabAtkins: Font family has to match and the partial has to be = or less restrictive match on style, stretch, and weight descriptors.
<chris> q?
<dael> TabAtkins: If you only want the bolds you can specifiy that, or you can allow for everything
<dael> TabAtkins: Defined by each @rule when it claims I'm allowed to be extended. Rules are ad hoc for what the @rule does
<astearns> ack chris
<Zakim> chris, you wanted to note this is very interesting but seems more CSS Syntax than CSS Fonts 4. Would also like to see the proposal more fleshed out.
<myles> q+
<dael> chris: I agree with this belonging in sep spec. Also have questions on how matching works. I understand how it works for each @rule differently, but I'd like a definition that says see each @rule for matching and there's a common term the @rules can refer to.
<fantasai> The contents of @supports already cascades
<dael> chris: Seems generally useful, but I'd like more examples and where it won't work, like I assume not on @supports
<astearns> ack dbaron
<dael> chris: I think it's great, it's really interesting
<dael> dbaron: This feels like it's a confusing mech to me. Applies to some but not other @rules. Applies to those that don't cascade and turns them into cascading @rules
<chris> kind of like inherit applies to non-inheritable propertes, which is also a bit weird :)
<dael> dbaron: I think it's confusing enough that some do that and some don't. What TabAtkins said about @font-face where it interacts differently also sounds weird and confusing
<fantasai> +1 to dbaron
<leaverou> +1 to dbaron as well
<dael> dbaron: I feel it might be better handled as extensions to each @rule that needs this mech. Feels like avariant, not a new @partial rule
<dael> TabAtkins: THat's what it is, I'm just unifying it under one name to be recognizable. THe stuff after the @partial is what that @rule wants to do its extension grammar with
<dael> chris: I think as unified as poss on extension mech is better than duplicating half the @rules
<astearns> ack myles
<dael> myles: That's the direction I was going to mention
<dael> myles: 2 pieces, one to open a closed blocka nd add stuff. Then there's the specific idea of the fonts problem where different people want to change different parts.
<dael> myles: Such a general solution would solve this use case. I don't think this use case is suffiecnt for this complicated thing. If there are other use cases the combination of them could motivate.
<dael> astearns: AS long as solutions are generalizable. I'd like to see other rules we'd want to open and see if enough similar. I'm a little scared with each rule can define how overwritten with partial rule
<dael> TabAtkins: All reasonable.
<dael> TabAtkins: The particular use case for fonts is strong. I'd like to address that. If we're not going for general until we find more things, I'd push to solvet his use case
<dael> myles: Rather then giving up I think somebody should do research to see if there are use cases. We're asking the question, not answering it
<dael> astearns: I think general solution merits a bit more work. WIll need non-font use cases so we can evaluate general solution outside this thing that needs to be solved. Agreed we need to solve this and we should take time to see if general solution is the way to do. More specific @rules is a rats nest we've added to for decades and normalizing would be good
<dael> fantasai: I don't think a generic rule is normalizing anything. Agree with dbaron that if we're going to do it...we might decide on a specific syntax, but the @rule should be named after its @rule and not be generic
<myles> @partial font-face @partial-font-face @font-face-partial
<myles> @font-partial-face ????
<dael> TabAtkins: I don't care about @parial fontface or @fontface.partial I jsut want to see partials as a thing
<myles> @key-partial-frames
<dael> TabAtkins: Let's look at use cases, see if we can find decent ones for other cases of extension. Maybe there's a good reason to extend something like keyframes.
<dael> astearns: Alright. Sound good to everyone?
<dael> astearns: Alright, thanks.

@LeaVerou
Copy link
Member

(typed my comment too slowly to be recorded in the minutes, so I'm reposting here)

Besides concerns about use cases, I'm also a bit concerned that we are introducing a mechanism to override that is so different than the cascade and completely breaks with reordering. It's two different mental models for doing very similar things, which is a usability antipattern. I was wondering if we can somehow "match" the defined @font-face (or other) rule and override it in a more cascade-like manner.

@astearns astearns removed the Agenda+ label Jan 8, 2019
@superpoincare
Copy link

Reposting google/fonts#358 (comment) here:

Hi all,

I want to take this opportunity to point to a potential issue with @font-feature-values

I came across this tweet:

https://twitter.com/pjchrobak/status/1059229076987301888

It seems removing semicolon in font-display in @font-feature-values breaks Firefox CSS below that for older versions.

Keep in mind, that if you add
@font-feature-values FontName { font-display: whatever; } without semicolon after value (what minifiers do), older versions of Firefox stop render whole CSS after this descriptor 🤯

I tested this on Browserstack with Firefox 56 in MacOS Mojave and the tweet seems right!

image

Firefox 57 onward fine but this raises questions about the syntax.

@LeaVerou
Copy link
Member

Firefox 57 onward fine but this raises questions about the syntax.

How so? This sounds like a browser bug to me, and a fixed one at that.

@superpoincare
Copy link

Firefox has ESR versions for organisations and 60 is the latest but organisations sometimes take time to upgrade and 52 may also be in use.

@svgeesus
Copy link
Contributor

Thanks for the concern, but I think changing the entire CSS descriptor syntax to accommodate a bug in Firefox 56- (in case people are still using Firefox 52 long term support version) is not warranted.

Thanks for pushing on the Google Fonts people to support font-display !

(Posted using Firefox 67a1)

SergioOpenPeer pushed a commit to webrtc-uwp/chromium-tools that referenced this issue May 10, 2019
This CL implements support for specifying a default value for
font-display for @font-face rules which do not provide a font-display
descriptor. See spec [1]. The implementation is currently behind a flag
which is off by default (not tested, not in experimental).

For intent-to-implement, see [4].

There is a question if we should mix descriptors with @-rules in
@font-feature-values because an unrecognized descriptor would be seen as
part of the following rule as the start of a qualified ruled which means
forward-compatibility could break. Also, there is a question if this is a
font feature value and should be added to that @-rule at all. Both these
issue are discussed/mentioned in [2].

Since using a descriptor directly in @font-feature values is not
compatible with Gecko, we're wrapping the font-display descriptor in a
block with no prelude for now.

Note that the default font-display cannot currently apply to fonts loaded
by FontFace.load() because the dictionary passed to FontFace have default
values which effectively overrides any @font-feature-values
descriptors[3].


[1] https://drafts.csswg.org/css-fonts-4/#font-display-font-feature-values
[2] w3c/csswg-drafts#2973
[3] w3c/csswg-drafts#3188
[4] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/C6taiCkYayU

Bug: 777846

Change-Id: I290f23e7489afeef64812d7604417268c2464455
Reviewed-on: https://chromium-review.googlesource.com/c/1222966
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#611056}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 5d39943cf7bd8db8aa1f874f3444affca39e408f
@rsheeter
Copy link
Author

In chatting with @litherum face to face we realized we never mentioned user impact of this ticket hanging open. So, better late than never: we delayed shipping font-display for Google Fonts for quite some time waiting for this to be resolved one way or another.

It would be helpful to clarify whether setting font-display outside @font-face in some fashion is ever going to be supported or to close this as Won't Fix if it just isn't going to happen.

@svgeesus
Copy link
Contributor

Hi @rsheeter

So is this

Today, in the "Speed at Scale" talk at Google I/O we announced that font-display will soon be shipping to Google Fonts in the form of a query parameter.

putting it inside an at-rule?

@superpoincare
Copy link

@svgeesus

It's already live!

Check: https://fonts.googleapis.com/css?family=Open+Sans&display=swap

@svgeesus
Copy link
Contributor

svgeesus commented Jul 26, 2021

Sooo this long-reported bug is still open. As of today the spec says:

@font-feature-values <family-name># { <rule-list> }

<feature-value-block> = <font-feature-value-type> { <declaration-list> }

<font-feature-value-type> = @stylistic | @historical-forms | @styleset | @character-variant
  | @swash | @ornaments | @annotation

which means that the font-display descriptor for @font-feature-values can't actually be used thee. (The same descriptor can, of course, be used in @font-face.

To fix this, like @tabatkins said:

The only actual requirement is that at-rules can't contain both style rules and declarations; since @font-feature-values doesn't contain either yet, it's fine to make it contain the font-display declaration. It just needs to be defined to do so.

@font-feature-values <family-name># { <rule-list>  |  font-display }

<feature-value-block> = <font-feature-value-type> { <declaration-list> }

<font-feature-value-type> = @stylistic | @historical-forms | @styleset | @character-variant
  | @swash | @ornaments | @annotation

but also,

An @font-feature-values rule’s prelude contains a list of font family names, followed by a block containing multiple <feature-value-block>s, a special type of subsidiary at-rule. Each <feature-value-block> contains declarations mapping author-chosen human-friendly names (such as "flowing") to feature indexes for the associated feature.

to

An @font-feature-values rule’s prelude contains a list of font family names, followed by a block containing zero or more optional multiple <feature-value-block>s, (a special type of subsidiary at-rule) and an optional font-display descriptor

Each <feature-value-block> contains declarations mapping author-chosen human-friendly names (such as "flowing") to feature indexes for the associated feature.

Right? Plus some examples, like the one @lilles posted

@rsheeter @litherum sound good?

Edit: OK no. CSS Syntax 3 says

Similarly, the production represents a list of rules, and may only be used in grammars as the sole value in a block. It represents that the contents of the block must be parsed using the consume a list of rules algorithm.

@svgeesus
Copy link
Contributor

It's already live!

Check: https://fonts.googleapis.com/css?family=Open+Sans&display=swap

Right, but that is putting it inside @font-face which was already clearly allowed.

@svgeesus
Copy link
Contributor

@tabatkins what is the current status on relaxing mixing at-rules and declarations? We already decided to allow this right, to nest media queries inside rules? Or was it feature queries? Should I wait until Syntax 3 is fixed?

@tabatkins
Copy link
Member

There's nothing wrong with it on the Syntax side; you can't mix declarations with style rules (except with special circumstances, like Nesting), but you can mix either with at-rules. (You'll want to switch the grammar to <declaration-list>, is all.)

The OM needs to make sure it exposes both .style and .childRules or whatever.

mfreed7 pushed a commit to mfreed7/csswg-drafts that referenced this issue Aug 4, 2021
ns-rsilva pushed a commit to ns-rsilva/chromium that referenced this issue Apr 25, 2024
This CL implements support for specifying a default value for
font-display for @font-face rules which do not provide a font-display
descriptor. See spec [1]. The implementation is currently behind a flag
which is off by default (not tested, not in experimental).

For intent-to-implement, see [4].

There is a question if we should mix descriptors with @-rules in
@font-feature-values because an unrecognized descriptor would be seen as
part of the following rule as the start of a qualified ruled which means
forward-compatibility could break. Also, there is a question if this is a
font feature value and should be added to that @-rule at all. Both these
issue are discussed/mentioned in [2].

Since using a descriptor directly in @font-feature values is not
compatible with Gecko, we're wrapping the font-display descriptor in a
block with no prelude for now.

Note that the default font-display cannot currently apply to fonts loaded
by FontFace.load() because the dictionary passed to FontFace have default
values which effectively overrides any @font-feature-values
descriptors[3].


[1] https://drafts.csswg.org/css-fonts-4/#font-display-font-feature-values
[2] w3c/csswg-drafts#2973
[3] w3c/csswg-drafts#3188
[4] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/C6taiCkYayU

Bug: 777846

Change-Id: I290f23e7489afeef64812d7604417268c2464455
Reviewed-on: https://chromium-review.googlesource.com/c/1222966
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611056}
Former-commit-id: 5d39943cf7bd8db8aa1f874f3444affca39e408f
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