Closed Bug 1456547 Opened 2 years ago Closed 2 years ago

Treat missing @font-face weight/style/stretch descriptors as 'auto', allowing full use of any variation axes in the font

Categories

(Core :: Layout: Text and Fonts, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

Currently, if a @font-face rule omits any of the font-{weight,style,stretch} descriptors, this is treated as representing the initial value of that property, per CSS Fonts spec.[1] ("If these descriptors are omitted, initial values are assumed.")

In particular, this means that if a variable font is loaded without explicitly providing descriptors that specify the _range_ of weight/style/stretch to be supported, it will be treated as having a single-value "range" ('normal'), and will not respond to the corresponding properties as authors would most naturally expect.

This behavior is unhelpful to authors, and not consistently implemented by the various UAs currently shipping variable-font support. (Chrome implements this; Safari and Edge don't.)

The CSSWG recently resolved to address this by introducing an 'auto' value as the initial value for these descriptors; see [2] for details.

We should go ahead and implement this as part of our variable fonts support.


[1] https://drafts.csswg.org/css-fonts-4/#font-prop-desc
[2] https://github.com/w3c/csswg-drafts/issues/2485#issuecomment-380477779
Priority: -- → P2
From a rendering point of view, we can implement this even without adding any explicit support for parsing the 'auto' descriptor value; the absence of any descriptor (resulting in eCSSValue_Null when the FontFaceSet wants to set up a new font entry) is enough.

In this case, we still need to set the 'normal' property value on the gfxFontEntry, as the font matching algorithm depends on this to choose correctly among multiple faces in a family; but we can also set a flag indicating that the variation value applied (once the face has been chosen) should not be clamped to the range recorded in the face, but allowed to use whatever the resource supports.
This results in the expected (intuitive) behavior when an author omits the descriptors from @font-face, aligning us with Safari's implementation. (And deviating from Chrome, which follows a stricter reading of the current draft spec; but as noted above, the WG has resolved to change this.)
Attachment #8971171 - Flags: review?(jwatt)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
We'll still need a Stylo patch to properly expose the 'auto' initial value of these descriptors.
Descriptors are currently defined in https://searchfox.org/mozilla-central/source/servo/components/style/font_face.rs and their conversion to Gecko's nsCSSValue is defined in https://searchfox.org/mozilla-central/source/servo/components/style/gecko/rules.rs

Feel free to ni? me if you have any question.
FTR, an alternative approach I considered here would be to give the three [Property]Range classes a special "auto" state that would be interpreted as "normal" for font-matching purposes but we'd know not to clamp the used value. This could be done by adding a flag within the range classes, or by special "magic" values of the min/max fields (which would avoid enlarging the structs), but that seemed a bit tricksy/hackish.

Also, this would add a cost to the font-matching process, as the range would always have to be checked for this special flag/value, whereas with the current patch, the flag only matters at the final stage when we're preparing the settings for rendering; font-matching doesn't need to know anything about it. So on balance, I felt the separate flags were the simpler option.
Comment on attachment 8971171 [details] [diff] [review]
When weight/stretch/style descriptor is omitted from a @font-face rule, the corresponding variation axis should not be clamped to the 'normal' value

Review of attachment 8971171 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFontEntry.h
@@ +428,5 @@
> +    enum class RangeFlags : uint8_t {
> +        eNoFlags     = 0,
> +        eAutoWeight  = (1 << 0),
> +        eAutoStretch = (1 << 1),
> +        eAutoStyle   = (1 << 2)

I'd rather this were called eAutoSlant (or eAutoSlantStyle). I know this is to mirror the 'font-style' property, but annoyingly often the overloaded term "style" (sometimes meaning a "collections of a bunch of CSS style 'stuff'" and sometimes meaning "slant") ends up interleaved in the same parts of the code. In fact that overloading is annoying even in non-adjacent code. I think it would be sensible to move in the direction of avoiding leakage of the (nowadays) poor naming of 'font-style' into the internal code.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +162,5 @@
>             mVariationSettings == aVariationSettings &&
>             mLanguageOverride == aLanguageOverride &&
>             mSrcList == aFontFaceSrcList &&
>             mFontDisplay == aFontDisplay &&
> +           mRangeFlags == aRangeFlags &&

I wonder if we're being overly strict here.

@@ -971,5 @@
>      return entry.forget();
>  }
>  
> -already_AddRefed<gfxUserFontEntry>
> -gfxUserFontSet::CreateUserFontEntry(

FWIW this sort of unrelated removal is worth a comment when requesting review so your reviewer doesn't have to spend time figuring out inheritence etc. ;) Better yet, a separate patch.
Attachment #8971171 - Flags: review?(jwatt) → review+
(In reply to Jonathan Watt [:jwatt] from comment #6)
> Comment on attachment 8971171 [details] [diff] [review]
> When weight/stretch/style descriptor is omitted from a @font-face rule, the
> corresponding variation axis should not be clamped to the 'normal' value
> 
> Review of attachment 8971171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFontEntry.h
> @@ +428,5 @@
> > +    enum class RangeFlags : uint8_t {
> > +        eNoFlags     = 0,
> > +        eAutoWeight  = (1 << 0),
> > +        eAutoStretch = (1 << 1),
> > +        eAutoStyle   = (1 << 2)
> 
> I'd rather this were called eAutoSlant (or eAutoSlantStyle).

Fair enough, I'll change it.

(The one that annoys me more, but we're stuck with thanks to CSS, is "stretch" which really should be "width". But that ship sailed long ago....)

> ::: gfx/thebes/gfxUserFontSet.cpp
> @@ +162,5 @@
> >             mVariationSettings == aVariationSettings &&
> >             mLanguageOverride == aLanguageOverride &&
> >             mSrcList == aFontFaceSrcList &&
> >             mFontDisplay == aFontDisplay &&
> > +           mRangeFlags == aRangeFlags &&
> 
> I wonder if we're being overly strict here.

I don't think so. If there are two @font-face rules, identical except that one includes "font-weight: normal" and the other has no font-weight descriptor, we need to maintain the distinction because they'll behave differently at rendering time: the one with the explicit descriptor value will always render at that weight, while the no-descriptor one will vary in response to the font-weight property that's applied.

> FWIW this sort of unrelated removal is worth a comment when requesting
> review so your reviewer doesn't have to spend time figuring out inheritence
> etc. ;) Better yet, a separate patch.

Yeah, sorry!
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93cf389546fd
When weight/stretch/style descriptor is omitted from a @font-face rule, the corresponding variation axis should not be clamped to the 'normal' value. r=jwatt
https://hg.mozilla.org/mozilla-central/rev/93cf389546fd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Summary: Support 'auto' as the default value for @font-face weight/style/stretch descriptors → Treat missing @font-face weight/style/stretch descriptors as 'auto', allowing full use of any variation axes in the font
You need to log in before you can comment on or make changes to this bug.