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)

enhancement
Not set
normal

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: nobody → emilio
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 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 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 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.
(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.
(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 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 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 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+
The way we ended up doing this, these patches need to land before part 3 in bug 1436048.
Blocks: 1436048
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
You need to log in before you can comment on or make changes to this bug.