Closed
Bug 1436048
Opened 6 years ago
Closed 6 years ago
Change font-weight, font-stretch and font-style to use user-defined types
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
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.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Updated•6 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
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
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Attachment #8967030 -
Attachment is patch: true
Comment 6•6 years ago
|
||
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.
Reporter | ||
Comment 7•6 years ago
|
||
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?
Assignee | ||
Comment 8•6 years ago
|
||
Now with `hg add gfx/src/FontPropertyTypes.h`
Attachment #8967030 -
Attachment is obsolete: true
Reporter | ||
Comment 9•6 years ago
|
||
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).
Reporter | ||
Comment 10•6 years ago
|
||
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?
Reporter | ||
Comment 11•6 years ago
|
||
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
Assignee | ||
Comment 12•6 years ago
|
||
Thanks. I've integrated those changes into the other changes I've been making.
Assignee | ||
Comment 13•6 years ago
|
||
Attachment #8967029 -
Attachment is obsolete: true
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #8967058 -
Attachment is obsolete: true
Assignee | ||
Comment 15•6 years ago
|
||
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
Reporter | ||
Comment 16•6 years ago
|
||
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. :)
Reporter | ||
Comment 17•6 years ago
|
||
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.)
Reporter | ||
Comment 18•6 years ago
|
||
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.
Reporter | ||
Comment 19•6 years ago
|
||
Try run with fixups that I'm hoping might get us through reftests intact.... https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c97cee066a3423a8caf8d26f472e646e340024.
Reporter | ||
Comment 20•6 years ago
|
||
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.
Reporter | ||
Comment 21•6 years ago
|
||
Once more, with less asserting: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f37f30dadebd71107f562dc4c9624f0a22664d4c.
Assignee | ||
Comment 22•6 years ago
|
||
Attachment #8967208 -
Attachment is obsolete: true
Attachment #8967209 -
Attachment is obsolete: true
Attachment #8967745 -
Flags: review?(emilio)
Assignee | ||
Comment 23•6 years ago
|
||
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 24•6 years ago
|
||
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+
Comment 25•6 years ago
|
||
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)
Reporter | ||
Comment 26•6 years ago
|
||
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)
Reporter | ||
Comment 27•6 years ago
|
||
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.
Reporter | ||
Comment 28•6 years ago
|
||
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.
Reporter | ||
Comment 29•6 years ago
|
||
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+
Comment 30•6 years ago
|
||
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
Reporter | ||
Comment 31•6 years ago
|
||
Tagging as leave-open since there will be additional patches to follow here.
Keywords: leave-open
Comment 32•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/837a6f4efa3e
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/837a6f4efa3e
Reporter | ||
Comment 34•6 years ago
|
||
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)
Reporter | ||
Updated•6 years ago
|
Assignee: jwatt → jfkthame
Reporter | ||
Comment 35•6 years ago
|
||
Oops, didn't mean to change the assignee here, sorry.
Assignee: jfkthame → jwatt
Reporter | ||
Comment 36•6 years ago
|
||
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)
Reporter | ||
Comment 37•6 years ago
|
||
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)
Reporter | ||
Updated•6 years ago
|
Attachment #8968074 -
Attachment is obsolete: true
Attachment #8968074 -
Flags: feedback?(jwatt)
Assignee | ||
Comment 38•6 years ago
|
||
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)
Reporter | ||
Comment 39•6 years ago
|
||
Updated, and pushed to try to check whether I broke anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d476cc687252d43068cb6b9969d77250e88dfda7.
Attachment #8968224 -
Flags: review?(jwatt)
Reporter | ||
Updated•6 years ago
|
Attachment #8967992 -
Attachment is obsolete: true
Assignee | ||
Comment 40•6 years ago
|
||
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+
Comment 41•6 years ago
|
||
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
Comment 42•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ceabd10aac2
Assignee | ||
Comment 43•6 years ago
|
||
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
Reporter | ||
Comment 44•6 years ago
|
||
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)
Reporter | ||
Comment 45•6 years ago
|
||
(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.
Reporter | ||
Comment 46•6 years ago
|
||
Reporter | ||
Comment 47•6 years ago
|
||
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
Assignee | ||
Comment 48•6 years ago
|
||
Not yet building.
Reporter | ||
Comment 49•6 years ago
|
||
Attachment #8969345 -
Flags: review?(jwatt)
Assignee | ||
Updated•6 years ago
|
Attachment #8969345 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 50•6 years ago
|
||
Attachment #8969323 -
Attachment is obsolete: true
Assignee | ||
Comment 51•6 years ago
|
||
Attachment #8969417 -
Attachment is obsolete: true
Reporter | ||
Comment 52•6 years ago
|
||
Attachment #8969568 -
Flags: review?(jwatt)
Assignee | ||
Comment 53•6 years ago
|
||
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 54•6 years ago
|
||
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.
Assignee | ||
Comment 55•6 years ago
|
||
Attachment #8969451 -
Attachment is obsolete: true
Comment 56•6 years ago
|
||
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.
Comment 57•6 years ago
|
||
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.
Comment 58•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8595367dc40cab66cac90f0e6812dea4b86321e7
Comment 59•6 years ago
|
||
Fixups by me incoming.
Attachment #8969643 -
Attachment is obsolete: true
Attachment #8970090 -
Flags: review?(jfkthame)
Comment 60•6 years ago
|
||
Attachment #8970091 -
Flags: review?(jwatt)
Comment 61•6 years ago
|
||
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)
Reporter | ||
Comment 62•6 years ago
|
||
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+
Assignee | ||
Comment 63•6 years ago
|
||
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+
Assignee | ||
Comment 64•6 years ago
|
||
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+
Reporter | ||
Comment 65•6 years ago
|
||
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)?
Comment 66•6 years ago
|
||
(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.
Reporter | ||
Comment 67•6 years ago
|
||
(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.
Comment 68•6 years ago
|
||
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
Comment 69•6 years ago
|
||
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.
Comment 72•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ad2ef987c9f https://hg.mozilla.org/mozilla-central/rev/b61541f85293 https://hg.mozilla.org/mozilla-central/rev/fdcc03769fbc
Upstream PR merged
Comment 74•6 years ago
|
||
I think we can call this FIXED.
Comment 75•6 years ago
|
||
This one breaks Wayland Gtk+ builds - Bug 1457120.
Updated•6 years ago
|
Comment 76•6 years ago
|
||
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)
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Comment 77•6 years ago
|
||
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.
Assignee | ||
Comment 78•6 years ago
|
||
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)
Reporter | ||
Comment 79•6 years ago
|
||
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)
Comment 80•6 years ago
|
||
(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.
Description
•