Closed Bug 1454596 Opened 6 years ago Closed 6 years ago

Stylo changes for new CSS 4 changes for font-weight

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jwatt, Assigned: emilio)

References

()

Details

Attachments

(1 file, 2 obsolete files)

Apparently my patch in bug 1436048 [1] is going in the wrong direction. We need to use Number and a bunch of other changes.

1. https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1436048&attachment=8967208

Given time constraints Emilio is going to take over that work.
Comment on attachment 8968478 [details]
Bug 1454596: Update font-weight property and descriptor to css-fonts-4.

https://reviewboard.mozilla.org/r/237182/#review242986

::: layout/style/FontFaceSet.cpp:973
(Diff revision 1)
> +      return FontWeight(aVal.GetIntValue());
> +    case eCSSUnit_Normal:
> +    case eCSSUnit_Null:
> +      return FontWeight::Normal();
> +    case eCSSUnit_Pair:
> +      // TODO(jfkthame): Handle optional second value of the font descriptor.

Right, we'll need to update this once bug 1454598 is done; then we can return a WeightRange here instead of a single FontWeight.

If this is lands first, though, I'd be fine with us making that change in a followup.

::: servo/components/style/values/computed/font.rs:98
(Diff revision 1)
>      pub fn bolder(self) -> Self {
> -        if self.0 < 400 {
> -            FontWeight(400)
> -        } else if self.0 < 600 {
> -            FontWeight(700)
> +        if self.0 < 400. {
> +            FontWeight(400.)
> +        } else if self.0 < 600. {
> +            FontWeight(700.)
>          } else {
> -            FontWeight(900)
> +            FontWeight(900.)
>          }
>      }

I think this needs an additional test - if self.0 is already > 900, it should be returned unchanged, not snapped back to 900.0.

Also, now that weights are not necessarily multiples of 100, CSS-fonts-4 adjusts the thresholds here to 350 and 550.

(See table in https://drafts.csswg.org/css-fonts-4/#font-weight-numeric-values)

::: servo/components/style/values/computed/font.rs:109
(Diff revision 1)
>      pub fn lighter(self) -> Self {
> -        if self.0 < 600 {
> -            FontWeight(100)
> -        } else if self.0 < 800 {
> -            FontWeight(400)
> +        if self.0 < 600. {
> +            FontWeight(100.)
> +        } else if self.0 < 800. {
> +            FontWeight(400.)
>          } else {
> -            FontWeight(700)
> +            FontWeight(700.)
>          }

Needs similar adjustment to thresholds (550, 750) and extra test for the extreme case (weight already <= 100).

::: servo/components/style/values/specified/font.rs:33
(Diff revision 1)
>  const DEFAULT_SCRIPT_SIZE_MULTIPLIER: f64 = 0.71;
>  
> -#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToCss)]
> -/// A specified font-weight value
> +/// The minimum font-weight value per:
> +///
> +/// https://drafts.csswg.org/css-fonts-4/#font-weight-numeric-values
> +pub const MIN_FONT_WEIGHT: f32 = 0.;

Minimum should be 1.0, not zero.
Comment on attachment 8968478 [details]
Bug 1454596: Update font-weight property and descriptor to css-fonts-4.

https://reviewboard.mozilla.org/r/237182/#review243184

::: servo/components/style/gecko/rules.rs:85
(Diff revision 2)
> +            None => {
> +                nscssvalue.set_from(first);
> +                return;
> +            }

I would prefer we always set a pair, but that probably means we would suddenly add an allocation for majority of pages using `@font-face` nowadays... That is probably not a big problem though. Either way, I guess it's fine.

::: servo/components/style/properties/longhand/font.mako.rs:51
(Diff revision 2)
> -${helpers.predefined_type("font-weight",
> +${helpers.predefined_type(
> +    "font-weight",
> -                          "FontWeight",
> +    "FontWeight",
> -                          initial_value="computed::FontWeight::normal()",
> +    initial_value="computed::FontWeight::normal()",
> -                          initial_specified_value="specified::FontWeight::Normal",
> +    initial_specified_value="specified::FontWeight::Normal",
> -                          animation_value_type="ComputedValue",
> +    animation_value_type="Number",

It's not clear to me what does this change mean given bug 1454834. I suppose this is fine as far as tests pass.

::: servo/components/style/values/specified/font.rs:53
(Diff revision 2)
> -    /// Computed weight variant
> -    Weight(computed::FontWeight),
> -    /// System font varaint
> +    /// A `<number>`, with the additional constraints specified in:
> +    ///
> +    /// https://drafts.csswg.org/css-fonts-4/#font-weight-numeric-values
> +    ///
> +    /// See also https://github.com/w3c/csswg-drafts/issues/2590 for `calc()`
> +    /// handling.
> +    Weight(Number),

I would probably prefer we use `AbsoluteFontWeight` here to replace the weight variant as well as the two keywords. That should align better with how the spec describe it.

But it doesn't matter that much, I guess.

::: servo/components/style/values/specified/font.rs:57
(Diff revision 2)
> +    /// See also https://github.com/w3c/csswg-drafts/issues/2590 for `calc()`
> +    /// handling.

I don't really think it's worth an extra mentioning here, because the behavior should be clear per spec.

::: servo/components/style/values/specified/font.rs:67
(Diff revision 2)
>  }
>  
>  impl FontWeight {
>      /// Get a specified FontWeight from a gecko keyword
>      pub fn from_gecko_keyword(kw: u32) -> Self {
> -        computed::FontWeight::from_int(kw as i32)
> +        debug_assert!(kw as f32 <= MAX_FONT_WEIGHT);

Maybe assert that it is a multiple of 100 in the allowed range.

This function name is really confusing. Given gecko side code for this[1] it's probably correct. But we should have a more precise assertion so that if people changes value of the so-called keywords, we can catch it.


[1] https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/gfx/src/FontPropertyTypes.h#194-198

::: servo/components/style/values/specified/font.rs:121
(Diff revision 2)
> +                    weight.to_computed_value(context)
> +                        .max(MIN_FONT_WEIGHT)
> +                        .min(MAX_FONT_WEIGHT)

Ah, I really hope we have rust-lang/rust#44095 :/
Attachment #8968478 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8968581 [details]
Bug 1454596: Same update for font-stretch.

https://reviewboard.mozilla.org/r/237278/#review243206

::: servo/components/style/font_face.rs:396
(Diff revision 1)
>  
>          /// The weight of this font face
>          "font-weight" weight / mWeight: FontWeight,
>  
>          /// The stretch of this font face
> -        "font-stretch" stretch / mStretch: font_stretch::T,
> +        "font-stretch" stretch / mStretch: FontStretch,

Shouldn't this be a type of `(AbsoluteFontStretch, Option<AbsoluteFontStretch>)` like what you did for `FontWeight`?

::: servo/components/style/properties/gecko.mako.rs:2614
(Diff revision 1)
>          let weight: f32 = unsafe { Gecko_FontWeight_ToFloat(self.gecko.mFont.weight) };
>          longhands::font_weight::computed_value::T(weight)
>      }
>  
> +    pub fn set_font_stretch(&mut self, v: longhands::font_stretch::computed_value::T) {
> +        self.gecko.mFont.stretch = v.0 as i16;

This is pretty much wrong. Value in percentage is 1.0 for 100% while you are converting it into i16 directly.

Maybe you have plan to change type of stretch, but before that happen, it should really `* 100` before the conversion.

::: servo/components/style/properties/gecko.mako.rs:2620
(Diff revision 1)
> +    }
> +    ${impl_simple_copy('font_stretch', 'mFont.stretch')}
> +
> +    pub fn clone_font_stretch(&self) -> longhands::font_stretch::computed_value::T {
> +        let weight = self.gecko.mFont.stretch;
> +        longhands::font_stretch::computed_value::T(weight as f32)

Same here, you are converting number which are 100 for 100% into 1.0 for 100%, which is wrong.

::: servo/components/style/properties/longhand/font.mako.rs:353
(Diff revision 1)
>                          cx.style().get_font().gecko(),
>                          cx.device().pres_context()
>                      )
>                  }
> -                let weight = longhands::font_weight::computed_value::T::from_gecko_weight(system.weight);
> +                let font_weight = longhands::font_weight::computed_value::T::from_gecko_weight(system.weight);
> +                let font_stretch = Percentage(system.stretch as f32);

Similar here, the unit is wrong for the conversion.

::: servo/components/style/values/specified/font.rs:270
(Diff revision 1)
> +                ComputedPercentage(match *kw {
> +                    UltraCondensed => 50.,
> +                    ExtraCondensed => 62.5,
> +                    Condensed => 75.,
> +                    SemiCondensed => 87.5,
> +                    Normal => 100.,
> +                    SemiExpanded => 112.5,
> +                    Expanded => 125.,
> +                    ExtraExpanded => 150.,
> +                    UltraExpanded => 200.,
> +                })

This is really not how `Percentage` is supposed to work.
Attachment #8968581 - Flags: review?(xidorn+moz)
Comment on attachment 8968582 [details]
Bug 1454596: Test updates.

https://reviewboard.mozilla.org/r/237280/#review243208

::: layout/style/test/descriptor_database.js:34
(Diff revision 1)
> -		values: [ "normal", "400", "bold", "100", "200", "300", "500", "600", "700", "800", "900" ],
> -		invalid_values: [ "107", "399", "401", "699", "710", "bolder", "lighter" ]
> +		values: [ "normal", "400", "bold", "100", "200", "300", "500", "600",
> +				  "700", "800", "900", "107", "399", "401", "699", "710",
> +				  "calc(1001)", "calc(100 + 1)", "calc(1)", "100.6", "99" ],

Maybe add a `calc(0)` for valid value?

Also this is for descriptor, so you should probably also add pairs here.

::: layout/style/test/property_database.js:3601
(Diff revision 1)
>      type: CSS_TYPE_LONGHAND,
>      applies_to_first_letter: true,
>      applies_to_first_line: true,
>      applies_to_placeholder: true,
>      initial_values: [ "normal" ],
>      other_values: [ "ultra-condensed", "extra-condensed", "condensed", "semi-condensed", "semi-expanded", "expanded", "extra-expanded", "ultra-expanded" ],

Add test for font-stretch as well?

Probably nsComputedDOMStyle cannot handle it correctly yet... but that shouldn't be a problem I suppose.
Attachment #8968582 - Flags: review?(xidorn+moz)
Comment on attachment 8968581 [details]
Bug 1454596: Same update for font-stretch.

Yeah, let's keep the font-stretch stuff out until I can test it properly.
Attachment #8968581 - Attachment is obsolete: true
Blocks: 1454883
Attachment #8968582 - Attachment is obsolete: true
Comment on attachment 8968478 [details]
Bug 1454596: Update font-weight property and descriptor to css-fonts-4.

Xidorn, mind taking a look at the test changes in the patch? Thanks!
Attachment #8968478 - Flags: review+ → review?(xidorn+moz)
The test change looks fine, but it seems you didn't address any of issues in comment 6.
Comment on attachment 8968478 [details]
Bug 1454596: Update font-weight property and descriptor to css-fonts-4.

Test change looks fine. Please address issues raised previously.
Attachment #8968478 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> Comment on attachment 8968478 [details]
> Bug 1454596: Update font-weight property and descriptor to css-fonts-4.
> 
> Test change looks fine. Please address issues raised previously.

Yup, I wanted to get you to review the test change first, given your timezone, I'll fixup the rest when I come back to this (working on bug 1454162 atm).
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3330e2fce16d
Update font-weight property and descriptor to css-fonts-4. r=xidorn
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10531 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33032a57c49f
Followup, annotate the right test on a CLOSED TREE. r=me
Blocks: 1455358
https://hg.mozilla.org/mozilla-central/rev/3330e2fce16d
https://hg.mozilla.org/mozilla-central/rev/33032a57c49f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.