Closed Bug 1454883 Opened 6 years ago Closed 6 years ago

Update font-stretch 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

(5 files)

This is blocked on making font-stretch its own type for bug 1436061.
Pending review comments and fixing other misc stuff potentially, since this was mostly untested.

Just don't want to lose track of it.
Comment on attachment 8969257 [details]
Bug 1454883: Update font-stretch to css-fonts-4.

Wrong bug...
Attachment #8969257 - Attachment is obsolete: true
Attachment #8969257 - Flags: review?(karlt)
Blocks: 1455358
Comment on attachment 8969257 [details]
Bug 1454883: Update font-stretch to css-fonts-4.

https://reviewboard.mozilla.org/r/237982/#review243986

r=me with the comments addressed.

::: servo/components/style/properties/gecko.mako.rs:2614
(Diff revision 2)
>          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).0 as i16;

As far as you still need casting, this is wrong. It should be multiplied by 100 before casting to i16, otherwise it's likely breaking existing behavior.

When the C++ code eventually support float stretch, the casting as well as the multiply should be removed together.

::: servo/components/style/properties/gecko.mako.rs:2622
(Diff revision 2)
> +
> +    pub fn clone_font_stretch(&self) -> longhands::font_stretch::computed_value::T {
> +        use values::computed::Percentage;
> +        use values::generics::NonNegative;
> +
> +        let stretch = self.gecko.mFont.stretch as f32;

Likewise, this should divide 100 after converting to f32.

::: servo/components/style/values/computed/font.rs:33
(Diff revision 2)
>  use values::specified::font::{self as specified, MIN_FONT_WEIGHT, MAX_FONT_WEIGHT};
>  use values::specified::length::{FontBaseSize, NoCalcLength};
>  
>  pub use values::computed::Length as MozScriptMinSize;
>  pub use values::specified::font::{FontSynthesis, MozScriptSizeMultiplier, XLang, XTextZoom};
> +pub use values::computed::NonNegativePercentage as FontStretch;

I would suggest you wrap it in a struct so that we may implement ToCss for it which restores the keywords for those specific values.

::: servo/components/style/values/computed/percentage.rs:71
(Diff revision 2)
> +impl ToAnimatedValue for NonNegativePercentage {
> +    type AnimatedValue = Percentage;
> +
> +    #[inline]
> +    fn to_animated_value(self) -> Self::AnimatedValue {
> +        self.0
> +    }
> +
> +    #[inline]
> +    fn from_animated_value(animated: Self::AnimatedValue) -> Self {
> +        NonNegative(animated.clamp_to_non_negative())
> +    }
> +}

We should probably make this derivable... Maybe we can add a trait for `clamp_to_non_negative`?

::: servo/ports/geckolib/glue.rs:5442
(Diff revision 2)
> +    if font.font_stretch.get_system().is_some() {
> +        return false;
> +    }
> +    stretch.set_from(&font.font_stretch);

Have a feeling that using match here like the following would make the style look more consistent
```rust
match font.font_stretch.get_system() {
    Some(_) => return false,
    None => stretch.set_from(&font.font_stretch),
}
```
but it doesn't matter much.
Attachment #8969257 - Flags: review?(xidorn+moz) → review+
Assignee: nobody → emilio
The way we ended up doing this, these patches need to land before part 3 in bug 1436048.
Blocks: 1436048
Attachment #8969738 - Flags: review?(xidorn+moz) → review+
This was surprisingly caught by a test.
Attachment #8970096 - Flags: review?(xidorn+moz)
Comment on attachment 8970092 [details] [diff] [review]
Fix animation tests to account for font-stretch animating as a percentage.

Review of attachment 8970092 [details] [diff] [review]:
-----------------------------------------------------------------

This change looks reasonable to me. (though the commit message should be the same as the comment for this attachment (i.e. 'Fix animation tests to account...')).

But one thing I am concerned is that we will ship this feature without the pref?  If we had the pref, we have to set it in some .ini files,  if not (it seems not), the animation behavior will be changed, I think we should add the pref.  That's said, font-stretch is probably not so popular for animations, so it wouldn't be a big problem. Anyway, I defer to Brian's opinion.
Attachment #8970092 - Flags: review?(hikezoe)
Attachment #8970092 - Flags: review?(bbirtles)
Attachment #8970092 - Flags: review+
It's not clear to me what pref are you talking about. This enables fancy variable font usage, but it's otherwise unrelated to var fonts. The idea is not having this gated by any pref.
Flags: needinfo?(hikezoe)
Comment on attachment 8970092 [details] [diff] [review]
Fix animation tests to account for font-stretch animating as a percentage.

Review of attachment 8970092 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/smil/test/db_smilCSSFromTo.js
@@ +298,3 @@
>      new AnimTestcaseFromTo("expanded", "ultra-expanded",
> +                           { fromComp: "125%",
> +                             midComp: "162.5%",

My concern is here.  The animation value at 50% position will be changed to a bigger value, 162.5%, no?

Before the change (I am not sure which one is actually), the middle value of the interpolation  between 'expanded' and 'ultra-expanded' is 'extra-expanded' (i.e. 150%).  Am I missing something?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Created attachment 8970096 [details] [diff] [review]
> Let inspector know that font-stretch accepts percentages.
> 
> This was surprisingly caught by a test.

Wait, what? What test caught this?
Comment on attachment 8970092 [details] [diff] [review]
Fix animation tests to account for font-stretch animating as a percentage.

As per a brief chat with Emilio on IRC, I think this change is fine since Chrome and Safari have already shipped this (Emilio told me that.).
Flags: needinfo?(hikezoe)
Attachment #8970092 - Flags: review?(bbirtles)
Attachment #8970096 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87886520c599
Update font-stretch to css-fonts-4. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1091690e59
Only parse font-stretch keywords in the font shorthand. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/46996fb339f8
Fix animation tests to account for font-stretch animating as percentage. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb740fda0906
Let inspector know that font-stretch supports percentages. r=xidorn
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ded40b73ba23
Regenerate the devtools CSS database. r=me on a CLOSED TREE
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10590 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
You need to log in before you can comment on or make changes to this bug.