Closed Bug 1436048 Opened 2 years ago Closed 2 years ago

Change font-weight, font-stretch and font-style to use user-defined types

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: jfkthame, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 14 obsolete files)

156.16 KB, patch
emilio
: review+
jfkthame
: review+
Details | Diff | Splinter Review
24.36 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.03 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
4.44 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
135.05 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
78.33 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
16.24 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
font-weight should accept a (floating-point) number from 1..1000, in addition to the various keywords.

font-stretch adds percentage values to the existing keywords.

font-style accepts an angle after the 'oblique' keyword.
See Also: → 1436061
[ Triage 2017/02/20: P3 ]
Priority: -- → P3
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attached patch WIP patch for font-weight (obsolete) — Splinter Review
Depends on: 1452040
Depends on: 1452466
Comment on attachment 8964943 [details] [diff] [review]
WIP patch for font-weight

I'm going to update the WIP state. The code is now converted to use a user defined class and finally compiles. It's still a mess and incomplete, but jfkthame should find this useful to build on and to give feedback.
Attachment #8964943 - Attachment is obsolete: true
Attachment #8967030 - Attachment is patch: true
Comment on attachment 8967029 [details] [diff] [review]
p1 - have servo *specified* font weight use f32, not a u16

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

::: servo/components/style/values/specified/font.rs
@@ +77,5 @@
>                      _ => Err(()),
>                  }
>              }
> +            Token::Number { value, .. } => {
> +                // Once we properly support floating point as a specified weight

This is wrong (and was wrong). Mind updating it to support calc() properly? Instead of an f32 you'd need to store a Number, and parse it with Number::parse. Then call Number::to_computed_value in the to_computed_value impl.
Comment on attachment 8967030 [details] [diff] [review]
pt2 - use a user defined type (class) for font weight

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

::: gfx/thebes/gfxFont.h
@@ +123,5 @@
>      // baseline offset, used when simulating sub/superscript glyphs
>      float baselineOffset;
>  
> +    // The weight of the font: [1.0f - 1000.0f]
> +    FontWeightValue weight;

As a 16-bit field, this should go back to its previous location or somewhere like that. (stretch and style will end up 16-bit as well, so grouping the three will make sense)

@@ +187,5 @@
>      }
>  
>      PLDHashNumber Hash() const {
> +        return ((style + (systemFont << 7) /* +
> +            XXX(weight << 8) */ ) + uint32_t(size*1000) + int32_t(sizeAdjust*1000)) ^

We'll also need to incorporate these values into a hash in gfxUserFontSet; maybe we should give the types a ForHash() accessor that returns their internal value in a convenient integer form (even once their normal Value() is floating-point).

::: layout/style/FontFaceSet.cpp
@@ +981,5 @@
>  
>    // set up weight
>    aFontFace->GetDesc(eCSSFontDesc_Weight, val);
>    unit = val.GetUnit();
>    if (unit == eCSSUnit_Integer || unit == eCSSUnit_Enumerated) {

Presumably we'll need to handle eCSSUnit_Number here, once we're not restricted to integer weights.

@@ +1251,5 @@
>                                 nsCSSProps::kFontWeightKTable);
>    if (weightKeywordString.Length() > 0) {
>      weightKeyword = weightKeywordString.get();
>    } else {
> +    SprintfLiteral(weightKeywordBuf, "%f", float(aUserFontEntry->Weight()));

aUserFontEntry->Weight().ToFloat() ?

::: layout/style/ServoBindings.cpp
@@ +1345,5 @@
> +
> +float
> +Gecko_FontWeight_ToFloat(mozilla::FontWeightValue aWeight)
> +{
> +  return aWeight;

aWeight.ToFloat()?

(Should we require an explicit accessor to get the numeric value, rather than allowing implicit conversion? At the moment I'm inclined that way.)

::: layout/style/nsComputedDOMStyle.cpp
@@ +1973,5 @@
>    RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
>  
>    const nsStyleFont* font = StyleFont();
>  
> +  FontWeightValue weight = font->mFont.weight;

Should we .ToFloat() this in order to check the range and call SetNumber?

::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs
@@ +7,5 @@
>  use gecko_bindings::bindings;
>  use gecko_bindings::structs;
>  use gecko_bindings::structs::{nsCSSValue, nsCSSUnit};
>  use gecko_bindings::structs::{nsCSSValue_Array, nsCSSValueList};
> +//use gecko_bindings::structs::root::mozilla::FontWeightValue;

Presumably this isn't needed if we're using floats at the servo/gecko interface.

::: servo/components/style/properties/gecko.mako.rs
@@ +2607,5 @@
>  
>      pub fn clone_font_weight(&self) -> longhands::font_weight::computed_value::T {
> +        let weight: f32 = unsafe { Gecko_FontWeight_ToFloat(self.gecko.mFont.weight) };
> +        debug_assert!(weight <= (::std::u16::MAX as f32));
> +        longhands::font_weight::computed_value::T(weight as u16)

I don't understand why we're going via u16 here?
Now with `hg add gfx/src/FontPropertyTypes.h`
Attachment #8967030 - Attachment is obsolete: true
Comment on attachment 8967058 [details] [diff] [review]
pt2 - use a user defined type (class) for font weight

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

::: layout/style/nsCSSValue.h
@@ +384,5 @@
>                                    //  parameters.  First elem of array is name,
>                                    //  an nsCSSKeyword as eCSSUnit_Enumerated,
>                                    //  the rest of the values are arguments.
>  
> +  eCSSUnit_FontWeight   = 27,

This won't work, because it makes UnitHasArrayValue() return true, and then DoReset() crashes. Try something like 110 (beyond Number).
Comment on attachment 8967058 [details] [diff] [review]
pt2 - use a user defined type (class) for font weight

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

::: layout/style/FontFaceSet.cpp
@@ +981,5 @@
>  
>    // set up weight
>    aFontFace->GetDesc(eCSSFontDesc_Weight, val);
>    unit = val.GetUnit();
>    if (unit == eCSSUnit_Integer || unit == eCSSUnit_Enumerated) {

This will need to handle eCSSUnit_FontWeight now, right?
Comment on attachment 8967058 [details] [diff] [review]
pt2 - use a user defined type (class) for font weight

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

::: widget/android/nsLookAndFeel.cpp
@@ +8,5 @@
>  #include "nsXULAppAPI.h"
>  #include "nsLookAndFeel.h"
>  #include "gfxFont.h"
>  #include "gfxFontConstants.h"
> +#include "mozilla/FontPropetryTypes.h"

typo, s/FontPropetryTypes/FontPropertyTypes/

::: widget/headless/HeadlessLookAndFeelGTK.cpp
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "HeadlessLookAndFeel.h"
> +#include "mozilla/FontPropetryTypes.h"

typo, s/FontPropetryTypes/FontPropertyTypes/

::: widget/windows/nsLookAndFeel.cpp
@@ +9,5 @@
>  #include "nsStyleConsts.h"
>  #include "nsUXThemeData.h"
>  #include "nsUXThemeConstants.h"
>  #include "WinUtils.h"
> +#include "FontPropertyTypes.h"

needs "mozilla/" prefix
Thanks. I've integrated those changes into the other changes I've been making.
Attachment #8967029 - Attachment is obsolete: true
Attachment #8967058 - Attachment is obsolete: true
I expect this won't build on Windows and Linux yet, and will be orange on Mac:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65420e4583b1f958e57fea2b7eaeeee4a11e7579
Here's a run where I added some fixups on top of your patches (prev version); with this, Mac tests pass except for an expected M5 failure because we now accept "font-weight: 100.0".

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d980446223a05b9cecd8147110fdfccc5261ce28

I expect the change I made in GetFontWeight() is the wrong thing to do; instead, we should fix whatever code put the wrong value type into the nsCSSValue. But at least it made tests pass. :)
Blocks: 1451345
Comment on attachment 8967209 [details] [diff] [review]
pt2 - use a user defined type (class) for font weight

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

::: gfx/src/FontPropertyTypes.h
@@ +73,5 @@
> +  bool operator>(const FontFixedPointValue& aOther) const {
> +    return mEncoded > aOther.mEncoded;
> +  }
> +
> +  bool operator-(const FontFixedPointValue& aOther) const {

I don't think this operator wants to return a bool! :)

(I believe this accounts for some reftest failures on your try run, as it breaks the WeightDistance computation.)
Comment on attachment 8967209 [details] [diff] [review]
pt2 - use a user defined type (class) for font weight

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

::: layout/style/FontFaceSet.cpp
@@ +984,5 @@
>    unit = val.GetUnit();
> +  if (unit == eCSSUnit_FontWeight) {
> +    weight = val.GetFontWeight();
> +  } else if (unit == eCSSUnit_Enumerated) {
> +    weight = FontWeight(val.GetFloatValue());

No, you have to use GetIntValue() to extract an enumerated value.

@@ +994,5 @@
>    }
>  
> +  if (weight.ToFloat() == 0.0f) {
> +    weight = FontWeight::normal();
> +  }

I think this should be unnecessary once eCSSUnit_Enumerated is handled properly just above.
Try run with fixups that I'm hoping might get us through reftests intact.... https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c97cee066a3423a8caf8d26f472e646e340024.
And here's a run where I have swapped the FontPropertyTypes header for one that actually uses fixed-point representation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2574bfad53b895854840d2cc933ac4f8b5fdef7d. Let's see how this fares.
Depends on: 1453924
Attachment #8967208 - Attachment is obsolete: true
Attachment #8967209 - Attachment is obsolete: true
Attachment #8967745 - Flags: review?(emilio)
Comment on attachment 8967745 [details] [diff] [review]
part 1 - use a user defined type (class) for font weight

Emilio, can you review the Servo, bindings and nsCSSProps changes? jfkthame will review the rest.
Attachment #8967745 - Flags: review?(jfkthame)
Comment on attachment 8967745 [details] [diff] [review]
part 1 - use a user defined type (class) for font weight

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

The style changes look ok to me, mod those bits.

I'm still somewhat confused about why to use an integer representation for representing what needs to become floating-point font-weight.

I chatted with jwatt a bit and he answered me that too much precision usually destroys the font cache during animations, but it's unclear to me why we can't just clamp the precision on style/.

The only other advantage I can think of is that it saves 16 bits of memory, but... Is that worth given all the float to integer conversions that it entails? I think there's much more low-hanging fruit if we want to save that.

Anyway, assuming there's a good argument for storing the stuff as an integer, r=me on the style changes.

::: layout/style/nsCSSProps.h
@@ +90,5 @@
>  #define VARIANT_HKI  (VARIANT_HK | VARIANT_INTEGER)
>  #define VARIANT_HKL  (VARIANT_HK | VARIANT_LENGTH)
>  #define VARIANT_HKLP (VARIANT_HK | VARIANT_LP)
>  #define VARIANT_HKLPO (VARIANT_HKLP | VARIANT_NONE)
> +#define VARIANT_HKN  (VARIANT_HK | VARIANT_NUMBER)

This doesn't seem used?

::: servo/components/style/properties/gecko.mako.rs
@@ +2600,5 @@
>          }
>      }
>  
>      pub fn set_font_weight(&mut self, v: longhands::font_weight::computed_value::T) {
> +        unsafe { Gecko_FontWeight_SetFloat(&mut self.gecko.mFont.weight, v.0 as f32) };

So I'm not totally happy about all the FFI traffic here to set a number, and all the integer -> float -> integer -> float roundtrips, but I suspect this needs to change anyway for supporting <number> and such.

@@ +2606,5 @@
>      ${impl_simple_copy('font_weight', 'mFont.weight')}
>  
>      pub fn clone_font_weight(&self) -> longhands::font_weight::computed_value::T {
> +        let weight: f32 = unsafe { Gecko_FontWeight_ToFloat(self.gecko.mFont.weight) };
> +        debug_assert!(weight <= (::std::u16::MAX as f32));

No need for parenthesis, and should assert it's >= 0.
Attachment #8967745 - Flags: review?(emilio) → review+
Jonathan, mind taking a look at comment 24? What's the rationale for using an integer type that only exposes a float?
Flags: needinfo?(jfkthame)
This is a temporary stage, a followup patch will change the internals of FontWeight to store a 16-bit fixed-point value (but expose it as a float to the rest of the code, which doesn't need to know exactly how many bits of precision we're maintaining).
Flags: needinfo?(jfkthame)
Note that we'll also be replacing mWeight in gfxFontEntry with a range that holds two weights (min/max), and similarly for stretch and style - so if we switch to storing floats for all these things, we start bloating quite a bit, when in practice 16-bit fixed-point is adequate.
We also have the option of making the internal representation of FontWeight a float, if we decide minimizing int/float conversions is more important than saving space in the font/style objects; that may be a question worth examining from various points of view. I expect we should be able to hide that issue entirely within the class, though.
Comment on attachment 8967745 [details] [diff] [review]
part 1 - use a user defined type (class) for font weight

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

LGTM! Please do capitalize the FontWeight::Thin()/Normal()/Bold() static methods, as discussed on IRC. A couple other minor suggestions below.

::: gfx/2d/ScaledFontDWrite.cpp
@@ +136,5 @@
>      , mGamma(aGamma)
>      , mContrast(aContrast)
>  {
>    if (aStyle) {
> +    mStyle = SkFontStyle(int(aStyle->weight.ToFloat()),

We should probably round rather than just truncate here.

Maybe worth adding a ToIntRounded() accessor, rather than having to do it manually at callsite(s)?

::: gfx/thebes/gfxFont.cpp
@@ +4159,5 @@
>  
> +    if (weight > FontWeight(900))
> +        weight = FontWeight(900);
> +    if (weight < FontWeight(100))
> +        weight = FontWeight(100);

While touching these lines, you could add the missing braces.

::: gfx/thebes/gfxUserFontSet.h
@@ +405,5 @@
>                                              aKey->mURI->Hash(),
>                                              HashFeatures(aKey->mFontEntry->mFeatureSettings),
>                                              HashVariations(aKey->mFontEntry->mVariationSettings),
>                                              mozilla::HashString(aKey->mFontEntry->mFamilyName),
> +                                            aKey->mFontEntry->mWeight.ToFloat(),

Use .ForHash() here instead of .ToFloat(), to avoid doing floating-point conversion.
Attachment #8967745 - Flags: review?(jfkthame) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/837a6f4efa3e
part 1 - Use a user defined type for font weight everywhere. r=jfkthame,emilio
Tagging as leave-open since there will be additional patches to follow here.
Keywords: leave-open
This switches FontWeight over to use a fixed-point representation (and introduces corresponding FontStretch and FontStyle types, though they're not yet used). Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32382040f335ca78a5738b95509aec6cdc14e390.
Attachment #8967992 - Flags: review?(jwatt)
Assignee: jwatt → jfkthame
Oops, didn't mean to change the assignee here, sorry.
Assignee: jfkthame → jwatt
Patch to give gfxFontEntry a range of weights so as to support variations. This still wants some tidying-up, I think, but any early feedback would be great.
Attachment #8968074 - Flags: feedback?(jwatt)
Updated version of the WeightRange patch. I've pushed a try run to see how it goes, though most likely I've broken some stuff on non-macOS platforms for the time being... https://treeherder.mozilla.org/#/jobs?repo=try&revision=16610fca47282e947eaa9ae46259117d92d871cd.
Attachment #8968156 - Flags: feedback?(jwatt)
Attachment #8968074 - Attachment is obsolete: true
Attachment #8968074 - Flags: feedback?(jwatt)
Comment on attachment 8967992 [details] [diff] [review]
part 2 - Store FontWeight as a fixed-point value to support fractional font-weight values

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

::: gfx/src/FontPropertyTypes.h
@@ +18,5 @@
>   */
>  
>  namespace mozilla {
>  
> +// Generic template for font property type classes that use a fixed-point

Any reason not to use Doxygen comment style here?

@@ +29,5 @@
> +//   FractionBits - number of bits to use for the fractional part
> +//   Min, Max - [inclusive] limits to the range of values that may be stored
> +// Values are constructed from and exposed as floating-point, but stored
> +// internally as fixed point, so there will be a quantization effect on
> +// fractional values, depending on the number of fractional bits used.

Can you also add a comment here noting the advantages of using a fixed point representation. Size for one. The reduced number of fractional bits being desirable for better caching (and cache hits) too.

@@ +39,5 @@
> +  FontPropertyValue() = default;
> +  explicit FontPropertyValue(const FontPropertyValue& aOther) = default;
> +  FontPropertyValue& operator= (const FontPropertyValue& aOther) = default;
> +
> +  // Not sure what operators etc we'll want... create more as needed.

Sure, but I don't think we need to land this comment.

@@ +40,5 @@
> +  explicit FontPropertyValue(const FontPropertyValue& aOther) = default;
> +  FontPropertyValue& operator= (const FontPropertyValue& aOther) = default;
> +
> +  // Not sure what operators etc we'll want... create more as needed.
> +  bool operator== (const FontPropertyValue& aOther) const

We have only a handful of occurrences of operator== declared with a space before the opening parenthesis, but 4,514 occurrences of it without. Probably align with existing style and avoid the space (to avoid these lines being unnecessarily touched by any future automated rewrite if nothing else).

@@ +70,5 @@
> +  {
> +    return (mValue - aOther.mValue) * kInverseScale;
> +  }
> +
> +  // Return the raw internal representation, for purposes of hashing.

Doxygen (three /// or else /** ... */).

@@ +76,5 @@
> +  {
> +    return mValue;
> +  }
> +
> +protected:

Add a typedef here like:

  typedef T internal_type;

Then you can reference it in the subclasses as per my comments below.

@@ +79,5 @@
> +
> +protected:
> +  // Construct from a floating-point or integer value, checking that it is
> +  // within the allowed range and converting to fixed-point representation.
> +  // (Or should we accept any value but clamp to the valid range?)

Only if we think we need to, which I don't think we do.

@@ +103,5 @@
> +  // This is protected as it may not be the most appropriate accessor for a
> +  // given instance to expose. It's up to each individual property to provide
> +  // public accessors that forward to this as required.
> +  float ToFloat() const { return mValue * kInverseScale; }
> +  float ToIntRounded() const { return (mValue + kPointFive) >> FractionBits; }

Doesn't seem like this should return float.

@@ +110,5 @@
> +  static constexpr float kInverseScale = 1.0f / kScale;
> +  static constexpr float kMin = float(Min);
> +  static constexpr float kMax = float(Max);
> +  static const unsigned kFractionBits = FractionBits;
> +  static const T kPointFive = 1u << (kFractionBits - 1);

Probably worth a short comment.

@@ +117,5 @@
> +};
> +
> +
> +// font-weight: range 1..1000, fractional values permitted; keywords
> +// 'normal', 'bold' aliased to 400, 700; relative keywords 'lighter', 'bolder'

Doxygen comment. And add "respectively" before the semicolon.

@@ +130,3 @@
>  {
>  public:
>    // Ugh. We need a default constructor to allow this type to be used in the

I should have put this comment up in the base class (and we can point to that comment from each subclass if that seems appropriate). Can you move this up?

@@ +171,5 @@
> +  float ToFloat() const { return FontPropertyValue::ToFloat(); }
> +  float ToIntRounded() const { return FontPropertyValue::ToIntRounded(); }
> +
> +private:
> +  explicit FontWeight(uint16_t aValue)

Instead of uint16_t here, use FontPropertyValue::internal_type.

Same in the other places you're using uint16_t below.

@@ +185,3 @@
>  };
>  
> +// Percentage: must be >= 0%, normal is 100%; we arbitrarily limit to 1000%.

Doxygen style.

@@ +247,5 @@
> +  static const uint16_t kUltraExpanded  = 200u << kFractionBits;
> +};
> +
> +
> +// font-style: normal | italic | oblique <angle>?

Doxygen.

::: gfx/thebes/gfxDWriteFontList.h
@@ +122,2 @@
>  
> +        weight = std::max(100, std::min(900, weight));

MFBT has the clamped() function FWIW.

::: gfx/thebes/gfxGDIFont.cpp
@@ +444,5 @@
>  gfxGDIFont::FillLogFont(LOGFONTW& aLogFont, gfxFloat aSize)
>  {
>      GDIFontEntry *fe = static_cast<GDIFontEntry*>(GetFontEntry());
>  
> +    uint32_t weight;

Please make this LONG.

::: gfx/thebes/gfxGDIFontList.cpp
@@ +255,5 @@
>  }
>  
>  void
>  GDIFontEntry::FillLogFont(LOGFONTW *aLogFont,
> +                          uint32_t aWeight,

LONG.
Attachment #8967992 - Flags: review?(jwatt)
Attachment #8967992 - Attachment is obsolete: true
Comment on attachment 8968224 [details] [diff] [review]
part 2 - Store FontWeight as a fixed-point value to support fractional font-weight values

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

Looks good!

::: gfx/src/FontPropertyTypes.h
@@ +45,5 @@
>  public:
>    // Ugh. We need a default constructor to allow this type to be used in the
>    // union in nsCSSValue.  Furthermore we need the default and copy
>    // constructors to be "trivial" (i.e. the compiler implemented defaults that
> +  // do no initialization.

Closing parenthesis.

@@ +77,3 @@
>    }
>  
> +  float operator-(const FontPropertyValue& aOther) const

Probably still add a comment saying "The distance between two values".

@@ +141,5 @@
>  {
>  public:
> +  // See comment in FontPropertyValue regarding requirement for a trivial
> +  // default constructor.
> +  // Annoyingly we can't make the default implementations constexpr (at least

Probably also worth moving this "Annoyingly..." comment to the base class to keep it with the other comment. It would need to be weaked slightly to say something like "That would be nice to do in order to allow the methods of subclasses that always return the same value (for example, FontWeight::Thin()) to also be constexpr."

@@ +149,4 @@
>  
> +  // Construct from a numeric CSS font-weight value, as either float
> +  // or integer (the latter for convenience where we write constants
> +  // such as FontWeight(700) in code).

For the float variant, the comment just seems to state the obvious. Maybe leave that ctor uncommented, and move this comment to the int variant and just say something like "CSS font weights can have fractional values, but this constructor exists for convenience..."? And use Doxygen comment preferably.
Attachment #8968224 - Flags: review?(jwatt) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ceabd10aac2
part 2 - Store FontWeight as a fixed-point value to support fractional font-weight values. r=jwatt
Depends on: 1454596
Blocks: 1454597
Restricting the scope of this bug. We'll use bug 1454094 as the tracker now.
Summary: Update font-{style,weight,stretch} properties to accept additional values per CSS Fonts 4 → Change font-weight, font-stretch and font-style to use user-defined types
Comment on attachment 8968156 [details] [diff] [review]
(WiP) - Allow variation fonts to record a weight range in gfxFontEntry, and update font-matching to handle ranges

Obsoleting this; it will go into a separate bug.
Attachment #8968156 - Attachment is obsolete: true
Attachment #8968156 - Flags: feedback?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #43)
> Restricting the scope of this bug. We'll use bug 1454094 as the tracker now.

Just FTR, the tracker is actually bug 1454597.
Comment on attachment 8968646 [details] [diff] [review]
part 1 - Make gfxFcPlatformFontList::GetFTLibrary work before font system is fully up and running, so that the global FT_Library can be used during initialization of the font list itself

Oops, attached patch to the wrong bug; sorry for the noise.
Attachment #8968646 - Attachment is obsolete: true
Attachment #8969345 - Flags: review?(jwatt) → review+
Comment on attachment 8969568 [details] [diff] [review]
part 2.2 - Remove use of the internal_type typedef because it makes the stylo bindings unhappy

This makes me sad, but oh well. :/
Attachment #8969568 - Flags: review?(jwatt) → review+
Comment on attachment 8969568 [details] [diff] [review]
part 2.2 - Remove use of the internal_type typedef because it makes the stylo bindings unhappy

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

Once all this code is in the tree I can try to add it back.
So I got jwatts patches and made them build, the current tree is at:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8d6780b7326b7250d76f5e22d5a77d6acfa665e

I _think_ that should make pass most if not all the css-fonts-4 tests. We need to see whether there's any bug lurking around there though.
Depends on: 1454883
Depends on: 1455358
I did a minor fixup today:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=40aa6f32add1a89b50659de2f536d1540e302fb5&selectedJob=175018843

Looks like try is much happier with it. I need to go through all the test expectations and such.
Fixups by me incoming.
Attachment #8969643 - Attachment is obsolete: true
Attachment #8970090 - Flags: review?(jfkthame)
The test adjustments are:

  A subtest of variable-presentation-attribute.html is marked as failure,
  pending resolution of https://github.com/w3c/csswg-drafts/issues/2605.

  font-parse-numeric-stretch-style-weight.html is updated to:

    * handle calc per spec (invalid at computed-value time), see
      https://github.com/w3c/csswg-drafts/issues/2590

    * Accept 0% in font-stretch: https://github.com/w3c/csswg-drafts/issues/2591

    * Accept font-weight descriptor ranges where the start is greater than the
      end, per
      https://drafts.csswg.org/css-fonts-4/#descdef-font-face-font-weight, which
      says:

      > User agents must swap the computed value of the startpoint and endpoint
      > of the range in order to forbid decreasing ranges.

  font-shorthand.html is updated to presumably fix a copy-paste error, where
  "oblique 45deg" was expected to compute to "oblique", which is wrong.

The rest is removing failure annotations.
Attachment #8970095 - Flags: review?(jwatt)
Comment on attachment 8970090 [details] [diff] [review]
Rebased part 3 WIP.

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

r=me, modulo fixups that I know are on their way. :)

::: gfx/2d/ScaledFontDWrite.cpp
@@ +146,5 @@
>      , mContrast(aContrast)
>  {
>    if (aStyle) {
>      mStyle = SkFontStyle(aStyle->weight.ToIntRounded(),
>                           DWriteFontStretchFromStretch(aStyle->stretch),

ISTM this is going to need some kind of update to handle non-predefined stretch values. Currently, DWriteFontStretchFromStretch will just return UNDEFINED, which means we won't be passing the desired stretch through to the SkFontStyle at all.

But SkFontStyle doesn't seem to support arbitrary stretch values, so it's unclear to me what we should do with it here. Probably worth a followup to at least record the question.

::: gfx/thebes/gfxDWriteCommon.h
@@ +20,5 @@
>  #include <windows.h>
>  #include <dwrite.h>
>  
>  static inline DWRITE_FONT_STRETCH
> +DWriteFontStretchFromStretch(FontStretch aStretch)

AFAICS this is not used anywhere, so let's just delete it.

(The only caller of a function by this name is in ScaledFontDWrite.cpp, which provides its own static inline copy.)

::: gfx/thebes/gfxFont.cpp
@@ +4184,5 @@
> +    return (style + (systemFont << 7) + (weight.ForHash() << 8) +
> +            uint32_t(size*1000) + int32_t(sizeAdjust*1000)) ^
> +            nsRefPtrHashKey<nsAtom>::HashKey(language);
> +    */
> +}

I'd guess this might be hot enough to be worth keeping it inline?

::: gfx/thebes/gfxGDIFontList.h
@@ +113,1 @@
>      typedef mozilla::FontWeight FontWeight;

I think we should have these typedefs in gfxFontEntry, so the various subclasses don't need to repeat them. (But I'm fine with cleaning that up later if it keeps patch-management simpler.)

@@ +327,1 @@
>      typedef mozilla::FontWeight FontWeight;

Likewise, these should be provided by the superclass.

::: layout/style/ServoBindings.cpp
@@ +1349,5 @@
> +}
> +
> +void
> +Gecko_FontSlantStyle_SetFloat(mozilla::FontSlantStyle* aStyle,
> +                         float aFloat)

nit, indentation is wrong here (thanks to the name-change for FontSlantStyle, no doubt)

@@ +2357,5 @@
> +}
> +
> +void
> +Gecko_CSSValue_SetFontSlantStyle(nsCSSValueBorrowedMut aCSSValue,
> +                            float aStyle)

nit, indent

::: layout/style/ServoBindings.h
@@ +600,5 @@
> +void Gecko_FontStretch_SetFloat(mozilla::FontStretch* aStretch,
> +                                float aFloatValue);
> +float Gecko_FontSlantStyle_ToFloat(mozilla::FontSlantStyle aStyle);
> +void Gecko_FontSlantStyle_SetFloat(mozilla::FontSlantStyle* aStyle,
> +                              float aFloatValue);

nit, indent

::: layout/style/nsComputedDOMStyle.cpp
@@ +1979,5 @@
> +
> +  const nsStyleFont* font = StyleFont();
> +
> +  // Chrome does not return the actual slant angle when a non-default oblique
> +  // value is specified.  That's hard to do, so for now neither do we.

Do we have a followup filed for this (or is it already addressed in one of the related bugs/patches)?
Attachment #8970090 - Flags: review?(jfkthame) → review+
Comment on attachment 8970091 [details] [diff] [review]
Fixups to part 3 and Servo integration bits.

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

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +1202,5 @@
>          }
>  
>          if (LOG_FONTLIST_ENABLED()) {
>              LOG_FONTLIST(("(fontlist) added (%s) to family (%s)"
> +                 " with style: %s weight: %g stretch: %f%%"

%g, not %f I think.

::: gfx/thebes/gfxFontEntry.h
@@ -109,5 @@
>  };
>  
>  class gfxFontEntry {
>  public:
> -    typedef mozilla::FontWeight FontWeight;

Removing this and then having to namespace qualify seems undesirable.

::: gfx/thebes/gfxTextRun.cpp
@@ +2424,3 @@
>                  MOZ_LOG(log, LogLevel::Warning,\
>                         ("(%s) fontgroup: [%s] default: %s lang: %s script: %d "
> +                        "len %d weight: %g stretch: %f%% style: %s size: %6.2f %zu-byte "

%g, not %f I think.

@@ +2472,4 @@
>                      uint32_t runLen = runLimit - runStart;
>                      MOZ_LOG(log, LogLevel::Warning,\
>                             ("(%s) fontgroup: [%s] default: %s lang: %s script: %d "
> +                            "len %d weight: %g stretch: %f%% style: %s size: %6.2f "

%g, not %f I think.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +1040,5 @@
>      family->AddFontEntry(aUserFontEntry);
>  
>      if (LOG_ENABLED()) {
>          LOG(("userfonts (%p) added to \"%s\" (%p) style: %s weight: %g "
> +             "stretch: %f%% display: %d",

%g, not %f I think.

::: gfx/thebes/gfxUserFontSet.h
@@ +408,5 @@
>                                              HashFeatures(aKey->mFontEntry->mFeatureSettings),
>                                              HashVariations(aKey->mFontEntry->mVariationSettings),
>                                              mozilla::HashString(aKey->mFontEntry->mFamilyName),
>                                              aKey->mFontEntry->mWeight.ForHash(),
> +                                            // XXX Is this right?

It's a good question. Let's open a follow-up bug so we investigate properly.
Attachment #8970091 - Flags: review?(jwatt) → review+
Comment on attachment 8970095 [details] [diff] [review]
Fix and update WPT tests and expectations.

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

Thanks for that helpful commit message!
Attachment #8970095 - Flags: review?(jwatt) → review+
Comment on attachment 8970091 [details] [diff] [review]
Fixups to part 3 and Servo integration bits.

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

::: gfx/src/FontPropertyTypes.h
@@ +297,5 @@
>   * - Note that 'oblique 0deg' is distinct from 'normal' (should it be?)
>   */
>  class FontSlantStyle final : public FontPropertyValue<int16_t,8,-90,90>
>  {
> +  using InternalType = int16_t;

Is there a reason this class has "using InternalType..." while the others above do a "typedef ... InternalType"? Consistency would be nice...

::: gfx/thebes/gfxFcPlatformFontList.h
@@ +95,5 @@
>      // of the font data and the FT_Face
>      explicit gfxFontconfigFontEntry(const nsAString& aFaceName,
> +                                    mozilla::FontWeight aWeight,
> +                                    mozilla::FontStretch aStretch,
> +                                    mozilla::FontSlantStyle aStyle,

The namespace prefixes shouldn't be needed if gfxFontEntry provides the typedefs.

@@ +290,5 @@
>      gfxFontEntry*
>      LookupLocalFont(const nsAString& aFontName,
> +                    mozilla::FontWeight aWeight,
> +                    mozilla::FontStretch aStretch,
> +                    mozilla::FontSlantStyle aStyle) override;

Ditto, if gfxPlatformFontList provides typedefs.

::: gfx/thebes/gfxFontEntry.h
@@ -109,5 @@
>  };
>  
>  class gfxFontEntry {
>  public:
> -    typedef mozilla::FontWeight FontWeight;

Agreed (I've been making equivalent comments...)

::: layout/style/ServoBindings.cpp
@@ +1331,5 @@
>  
>  float
>  Gecko_FontStretch_ToFloat(mozilla::FontStretch aStretch)
>  {
> +  // Servo represents percentages with 1. being 100%.

Is that particularly useful in this case? Although font-stretch refers to "percentage", it's not a percentage *of* anything in particular, in the sense that it might be used to compute a new value by multiplying something. It's really an arbitrary scale where the spec chooses to define the minimum as zero and the 'normal' value as 100, that's all.

Or should we change the gecko representation to use smaller fractional values, so that default = 1.0 in gecko as well (so internally something like 2.14 rather than 8.8)?
(In reply to Jonathan Kew (:jfkthame) from comment #65)
> Comment on attachment 8970091 [details] [diff] [review]
> Fixups to part 3 and Servo integration bits.
> 
> Review of attachment 8970091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/src/FontPropertyTypes.h
> @@ +297,5 @@
> >   * - Note that 'oblique 0deg' is distinct from 'normal' (should it be?)
> >   */
> >  class FontSlantStyle final : public FontPropertyValue<int16_t,8,-90,90>
> >  {
> > +  using InternalType = int16_t;
> 
> Is there a reason this class has "using InternalType..." while the others
> above do a "typedef ... InternalType"? Consistency would be nice...

Yeah, agreed, this was me trying to fight a bit with bindgen, will use a consistent style everywhere.

> ::: gfx/thebes/gfxFcPlatformFontList.h
> @@ +95,5 @@
> >      // of the font data and the FT_Face
> >      explicit gfxFontconfigFontEntry(const nsAString& aFaceName,
> > +                                    mozilla::FontWeight aWeight,
> > +                                    mozilla::FontStretch aStretch,
> > +                                    mozilla::FontSlantStyle aStyle,
> 
> The namespace prefixes shouldn't be needed if gfxFontEntry provides the
> typedefs.

Yeah, will do.

> @@ +290,5 @@
> >      gfxFontEntry*
> >      LookupLocalFont(const nsAString& aFontName,
> > +                    mozilla::FontWeight aWeight,
> > +                    mozilla::FontStretch aStretch,
> > +                    mozilla::FontSlantStyle aStyle) override;
> 
> Ditto, if gfxPlatformFontList provides typedefs.

Ack

> ::: gfx/thebes/gfxFontEntry.h
> @@ -109,5 @@
> >  };
> >  
> >  class gfxFontEntry {
> >  public:
> > -    typedef mozilla::FontWeight FontWeight;
> 
> Agreed (I've been making equivalent comments...)
> 
> ::: layout/style/ServoBindings.cpp
> @@ +1331,5 @@
> >  
> >  float
> >  Gecko_FontStretch_ToFloat(mozilla::FontStretch aStretch)
> >  {
> > +  // Servo represents percentages with 1. being 100%.
> 
> Is that particularly useful in this case? Although font-stretch refers to
> "percentage", it's not a percentage *of* anything in particular, in the
> sense that it might be used to compute a new value by multiplying something.
> It's really an arbitrary scale where the spec chooses to define the minimum
> as zero and the 'normal' value as 100, that's all.

This is how all the percentages are represented in the style system, both in Servo's and Gecko's.

https://searchfox.org/mozilla-central/rev/fcd5cb3515d0e06592bba42e378f1b9518497e3d/layout/style/nsCSSValue.h#405

I'm not opposed to change it, but it's convenient that to apply the scaling you just need to multiply for it.

> Or should we change the gecko representation to use smaller fractional
> values, so that default = 1.0 in gecko as well (so internally something like
> 2.14 rather than 8.8)?

So perhaps changing the font types to follow the same pattern would be better, yeah... But that can be done in a followup.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #66)
> I'm not opposed to change it, but it's convenient that to apply the scaling
> you just need to multiply for it.

The point is that in this case it's not ever used to "apply scaling", it's just an arbitrary value that is an input to the font-selection algorithm and/or passed through to a variation-font implementation (where the scale expects the 'normal' value to be 100.0, so if we use a 1.0-based representation we'll need a multiplication there).

> > Or should we change the gecko representation to use smaller fractional
> > values, so that default = 1.0 in gecko as well (so internally something like
> > 2.14 rather than 8.8)?
> 
> So perhaps changing the font types to follow the same pattern would be
> better, yeah... But that can be done in a followup.

Sure, let's not mess with it today but perhaps adjust one way or the other in a followup.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad2ef987c9f
Use user defined types for font-stretch / font-style. r=jfkthame,jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/b61541f85293
Fix and update other WPT expectations in css-fonts-4 tests. r=jwatt
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdcc03769fbc
followup, fix for Windows build bustage. r=emilio(irc) on a CLOSED TREE
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10591 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged
I think we can call this FIXED.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
This one breaks Wayland Gtk+ builds - Bug 1457120.
See Also: → 1457120
Target Milestone: --- → mozilla61
This stuff is now documented:

https://developer.mozilla.org/en-US/docs/Web/CSS/font-style
https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight
https://developer.mozilla.org/en-US/docs/Web/CSS/font-stretch

I also added a rel note:

https://developer.mozilla.org/en-US/Firefox/Releases/61#CSS

For font-style, I updated the browser-compat data to talk about the angle after oblique, and added an example of this to the interactive example repo (these should be merged and start to appear on the page soon):

https://github.com/mdn/browser-compat-data/pull/2229
https://github.com/mdn/interactive-examples/pull/963

Will did the other two updates, and handled the necessary repo changes.

A review of this stuff would be really appreciated — thanks!

I also had a question about font-style's oblique + angle values — the description I've given in the ref doc basically says that the browser will match the angle value against the closest matching oblique face in the font-family, or if no oblique face exists, it will synthesize an appropriate rendering. This is what I inferred from the spec. But it seems to me that when you try to use a default system font for example, the angle values don't have an effect and you just get oblique or not oblique. I only saw an effect when I used a variable open type font.

So what should I write about this?
Flags: needinfo?(jwatt)
With almost all system font families, there will be an italic or oblique face installed as part of the family, and in that case, font-style:oblique <angle> will map to the available italic or oblique face, rather than synthesizing an oblique from the normal face.

An exception might be a family like Impact, which (at least on macOS) only has an upright face; or there are quite a lot of non-Latin fonts that don't come with "italic" faces. So an example like

  data:text/html,<div style="font:36px hiragino mincho pro">hello <span style="font-style:oblique -20deg">world

shows the <angle> being applied when a synthetic-oblique version of the CJK font is used. But an equivalent testcase with Times or Italic or most common Latin fonts will ignore it.
The MDN changes look fine to me, but probably a good idea to have jfkthame take a look over them as well.
Flags: needinfo?(jwatt) → needinfo?(jfkthame)
One correction for https://developer.mozilla.org/en-US/docs/Web/CSS/font-style: the default <angle> used for font-style:oblique is actually 14°, not 20° as currently stated. (The CSS WG resolved to change this; it hasn't been updated in the draft spec yet, but we implement the new value.)
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #79)
> One correction for
> https://developer.mozilla.org/en-US/docs/Web/CSS/font-style: the default
> <angle> used for font-style:oblique is actually 14°, not 20° as currently
> stated. (The CSS WG resolved to change this; it hasn't been updated in the
> draft spec yet, but we implement the new value.)

OK, cool, fixed.
You need to log in before you can comment on or make changes to this bug.