Closed
Bug 1455358
Opened 6 years ago
Closed 6 years ago
Update font-style to css-fonts-4
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(4 files)
+++ This bug was initially created as a clone of Bug #1454883 +++ Same thing, same blockers.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → emilio
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8969368 [details] Bug 1455358: Update font-style to css-fonts-4. https://reviewboard.mozilla.org/r/238114/#review243856 ::: servo/components/style/values/specified/font.rs:243 (Diff revision 1) > + } > + } > +} > + > +/// The default angle for `font-style: oblique`. > +pub const DEFAULT_FONT_STYLE_OBLIQUE_ANGLE_DEGREES: f32 = 20.; Please make this 14 instead of 20. (The CSSWG decided on that last week: https://github.com/w3c/csswg-drafts/issues/2295#issuecomment-380486534)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8969368 [details] Bug 1455358: Update font-style to css-fonts-4. https://reviewboard.mozilla.org/r/238114/#review243996 r=me with the issue addressed. ::: servo/components/style/font_face.rs:30 (Diff revision 2) > #[cfg(feature = "gecko")] > use values::specified::font::{AbsoluteFontWeight, FontStretch as SpecifiedFontStretch}; > #[cfg(feature = "gecko")] > use values::specified::font::{SpecifiedFontFeatureSettings, SpecifiedFontVariationSettings}; > +#[cfg(feature = "gecko")] > +use values::specified::font::FontStyle as SpecifiedFontStyle; nit: I would usually do ```rust use values::specified::font as specified; ``` which can avoid repeatedly importing stuff from specified mod with aliasing like here and several line above for `FontStretch`. ::: servo/components/style/font_face.rs:174 (Diff revision 2) > + if *first != SpecifiedFontStyle::default_angle() { > + dest.write_char(' ')?; > + first.to_css(dest)?; > + } > + if first != second { > + dest.write_char(' ')?; > + second.to_css(dest)?; > + } Logic here is wrong. Consider ```css font-style: oblique 14deg 30deg; ``` the code here would serialize it to ```css font-style: oblique 30deg; ``` which is different than the original meaning. When the two values are different, both values should be there. We should have a test to catch this case.
Attachment #8969368 -
Flags: review?(xidorn+moz) → review+
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8969368 [details] Bug 1455358: Update font-style to css-fonts-4. https://reviewboard.mozilla.org/r/238114/#review244008 ::: commit-message-5365c:3 (Diff revision 2) > +This patch is meant to be squashed with the following two. This is an initial > +strawman implementation that I wrote to unblock jwatt, cleanups and fixes > +follow. I'm a bit concerned about squashing implementing some feature with cleanups. Is it broken as its own? If not I'd prefer we keep them separate...
Comment 9•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969368 [details] Bug 1455358: Update font-style to css-fonts-4. https://reviewboard.mozilla.org/r/238114/#review243996 > Logic here is wrong. Consider > ```css > font-style: oblique 14deg 30deg; > ``` > the code here would serialize it to > ```css > font-style: oblique 30deg; > ``` > which is different than the original meaning. When the two values are different, both values should be there. > > We should have a test to catch this case. Since the value is stored in the Rust side, this specific stuff should be testable without support from C++ code.
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8) > Comment on attachment 8969368 [details] > Bug 1455358: Update font-style to css-fonts-4. > > https://reviewboard.mozilla.org/r/238114/#review244008 > > ::: commit-message-5365c:3 > (Diff revision 2) > > +This patch is meant to be squashed with the following two. This is an initial > > +strawman implementation that I wrote to unblock jwatt, cleanups and fixes > > +follow. > > I'm a bit concerned about squashing implementing some feature with cleanups. > Is it broken as its own? If not I'd prefer we keep them separate... It's kinda broken, in the sense that animations will not clamp properly without the third patch... (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9) > Comment on attachment 8969368 [details] > Bug 1455358: Update font-style to css-fonts-4. > > https://reviewboard.mozilla.org/r/238114/#review243996 > > > Logic here is wrong. Consider > > ```css > > font-style: oblique 14deg 30deg; > > ``` > > the code here would serialize it to > > ```css > > font-style: oblique 30deg; > > ``` > > which is different than the original meaning. When the two values are different, both values should be there. > > > > We should have a test to catch this case. > > Since the value is stored in the Rust side, this specific stuff should be > testable without support from C++ code. Whoops, great catch! I'll add a test tomorrow. Yeah, I should be able to write this without waiting for the other patches.
Comment 11•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8) > > I'm a bit concerned about squashing implementing some feature with cleanups. > > Is it broken as its own? If not I'd prefer we keep them separate... > > It's kinda broken, in the sense that animations will not clamp properly > without the third patch... I'm not very concerned about this kind of breakage. As far as the patch builds as itself, and doesn't fail existing tests badly, it should be kept separate. Given that we probably don't have test for this stuff at all, I don't think squashing them together is a good idea.
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8969404 [details] Bug 1455358: Use a generic type in preparation to fix animation. https://reviewboard.mozilla.org/r/238154/#review244016 ::: servo/components/style/values/computed/font.rs:27 (Diff revision 1) > use style_traits::{CssWriter, ParseError, ToCss}; > use values::CSSFloat; > use values::animated::{ToAnimatedValue, ToAnimatedZero}; > use values::computed::{Angle, Context, Integer, NonNegativeLength, Number, ToComputedValue}; > -use values::generics::font::{FeatureTagValue, FontSettings}; > +use values::generics::font::{self as generics, FeatureTagValue, FontSettings}; > use values::generics::font::{KeywordInfo as GenericKeywordInfo, VariationValue}; You can remove the `KeywordInfo as GenericKeywordInfo` now, I guess. ::: servo/components/style/values/specified/font.rs:378 (Diff revision 1) > impl Parse for FontStyle { > fn parse<'i, 't>( > context: &ParserContext, > input: &mut Parser<'i, 't>, > ) -> Result<Self, ParseError<'i>> { > - Ok(try_match_ident_ignore_ascii_case! { input, > + Ok(FontStyle::Specified(SpecifiedFontStyle::parse(context, input)?)) Or `SpecifiedFontStyle::parse(context, input).map(FontStyle::Specified)`. ::: servo/ports/geckolib/glue.rs:5437 (Diff revision 1) > let family = unsafe { &mut *family }; > match font.font_family { > FontFamily::Values(list) => family.set_move(list.0), > FontFamily::System(_) => return false, > } > - match font.font_style { > + let specified_font_style = match font.font_style { You should probably add an empty line here and remove the one below, since specified_font_style is logically closer to the setting of `style` rather than `family`.
Attachment #8969404 -
Flags: review?(xidorn+moz) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8969405 [details] Bug 1455358: Fix clamping on animations. https://reviewboard.mozilla.org/r/238156/#review244026 ::: servo/components/style/values/computed/angle.rs:70 (Diff revision 1) > } > > + /// Return the value in degrees. > + pub fn degrees(&self) -> f32 { > + use std::f32::consts::PI; > + self.radians() * 360. / (2. * PI) Actually, `* (180. / PI)` is enough, and is potentially faster, because they are not completely equivalent, so compiler may not optimize it this way, but we don't really care about that difference.
Attachment #8969405 -
Flags: review?(xidorn+moz) → review+
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8969406 [details] Bug 1455358: Cleanup (a bit) system font copypasta. https://reviewboard.mozilla.org/r/238158/#review244028 r=me with the macro renamed. I would imagine that we can have a wrapper type one day to address this. ::: servo/components/style/values/specified/font.rs:31 (Diff revision 1) > use values::specified::{AllowQuirks, Angle, Integer, LengthOrPercentage, NoCalcLength, Number, Percentage}; > use values::specified::length::{FontBaseSize, AU_PER_PT, AU_PER_PX}; > > +// FIXME(emilio): The rest of the system font code is also copy-pasta, and > +// should be cleaned up. > +macro_rules! system_font_copy_pasta { `copy_pasta` is really a confusing name... Call it `system_font_methods` or something similar please.
Attachment #8969406 -
Flags: review?(xidorn+moz) → review+
Comment 15•6 years ago
|
||
The way we ended up doing this, these patches need to land before part 3 in bug 1436048.
Blocks: 1436048
Comment 16•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f1885763b29 Update font-style to css-fonts-4. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/eb75243cba98 Use a generic type in preparation to fix animation. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/4e937037938d Fix clamping on animations. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/e366b3efb748 Cleanup (a bit) system font copypasta. r=xidorn
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6f1885763b29 https://hg.mozilla.org/mozilla-central/rev/eb75243cba98 https://hg.mozilla.org/mozilla-central/rev/4e937037938d https://hg.mozilla.org/mozilla-central/rev/e366b3efb748
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•