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)
Core
CSS Parsing and Computation
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.
Reporter | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
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 7•6 years ago
|
||
mozreview-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 8•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 9•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968582 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
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)
Comment 13•6 years ago
|
||
The test change looks fine, but it seems you didn't address any of issues in comment 6.
Comment 14•6 years ago
|
||
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+
Assignee | ||
Comment 15•6 years ago
|
||
(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).
Comment 16•6 years ago
|
||
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.
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3330e2fce16d https://hg.mozilla.org/mozilla-central/rev/33032a57c49f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•