From e7e6e62ed36a5aa9798e9a12e5b4212736db25bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 22 Jan 2020 23:46:24 +0000 Subject: [PATCH] style: Remove font-size calc() hack to propagate keyword information. It does not make any sense with min() / max() / clamp. So just forget the keyword info when calc() is used. This also removes a bit of complex / hacky code. Differential Revision: https://phabricator.services.mozilla.com/D60663 --- components/style/gecko/wrapper.rs | 5 +-- components/style/values/specified/font.rs | 42 ++------------------- components/style/values/specified/length.rs | 12 +----- 3 files changed, 8 insertions(+), 51 deletions(-) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 9a1196a11b5..ba1c724d58f 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -956,9 +956,8 @@ impl FontMetricsProvider for GeckoFontMetricsProvider { let (wm, font) = match base_size { FontBaseSize::CurrentStyle => (style.writing_mode, style.get_font()), - // These are only used for font-size computation, and the first is - // really dubious... - FontBaseSize::InheritedStyleButStripEmUnits | FontBaseSize::InheritedStyle => { + // This is only used for font-size computation. + FontBaseSize::InheritedStyle => { (*style.inherited_writing_mode(), style.get_parent_font()) }, }; diff --git a/components/style/values/specified/font.rs b/components/style/values/specified/font.rs index 05a29c94087..86a00ef97df 100644 --- a/components/style/values/specified/font.rs +++ b/components/style/values/specified/font.rs @@ -574,11 +574,11 @@ impl KeywordInfo { /// Given a parent keyword info (self), apply an additional factor/offset to /// it. - pub fn compose(self, factor: f32, offset: CSSPixelLength) -> Self { + fn compose(self, factor: f32) -> Self { KeywordInfo { kw: self.kw, factor: self.factor * factor, - offset: self.offset * factor + offset, + offset: self.offset * factor, } } } @@ -901,7 +901,7 @@ impl FontSize { .get_parent_font() .clone_font_size() .keyword_info - .map(|i| i.compose(factor, CSSPixelLength::new(0.))) + .map(|i| i.compose(factor)) }; let mut info = None; let size = match *self { @@ -927,42 +927,8 @@ impl FontSize { base_size.resolve(context) * pc.0 }, FontSize::Length(LengthPercentage::Calc(ref calc)) => { - let parent = context.style().get_parent_font().clone_font_size(); - // if we contain em/% units and the parent was keyword derived, this is too - // Extract the ratio/offset and compose it - if (calc.em.is_some() || calc.percentage.is_some()) && parent.keyword_info.is_some() - { - let ratio = calc.em.unwrap_or(0.) + calc.percentage.map_or(0., |pc| pc.0); - // Compute it, but shave off the font-relative part (em, %). - // - // This will mean that other font-relative units like ex and - // ch will be computed against the old parent font even when - // the font changes. - // - // There's no particular "right answer" for what to do here, - // Gecko recascades as if the font had changed, we instead - // track the changes and reapply, which means that we carry - // over old computed ex/ch values whilst Gecko recomputes - // new ones. - // - // This is enough of an edge case to not really matter. - let abs = calc - .to_computed_value_zoomed( - context, - FontBaseSize::InheritedStyleButStripEmUnits, - ) - .unclamped_length(); - - info = parent.keyword_info.map(|i| i.compose(ratio, abs)); - } let calc = calc.to_computed_value_zoomed(context, base_size); - // FIXME(emilio): we _could_ use clamp_to_non_negative() - // everywhere, without affecting behavior in theory, since the - // others should reject negatives during parsing. But SMIL - // allows parsing negatives, and relies on us _not_ doing that - // clamping. That's so bonkers :( - calc.percentage_relative_to(base_size.resolve(context)) - .clamp_to_non_negative() + calc.resolve(base_size.resolve(context)) }, FontSize::Keyword(i) => { // As a specified keyword, this is keyword derived diff --git a/components/style/values/specified/length.rs b/components/style/values/specified/length.rs index 1ef76054382..3ac1f0a9762 100644 --- a/components/style/values/specified/length.rs +++ b/components/style/values/specified/length.rs @@ -71,10 +71,6 @@ pub enum FontBaseSize { CurrentStyle, /// Use the inherited font-size. InheritedStyle, - /// Use the inherited font-size, but strip em units. - /// - /// FIXME(emilio): This is very complex, and should go away. - InheritedStyleButStripEmUnits, } impl FontBaseSize { @@ -82,7 +78,7 @@ impl FontBaseSize { pub fn resolve(&self, context: &Context) -> computed::Length { match *self { FontBaseSize::CurrentStyle => context.style().get_font().clone_font_size().size(), - FontBaseSize::InheritedStyleButStripEmUnits | FontBaseSize::InheritedStyle => { + FontBaseSize::InheritedStyle => { context.style().get_parent_font().clone_font_size().size() }, } @@ -144,11 +140,7 @@ impl FontRelativeLength { } } - if base_size == FontBaseSize::InheritedStyleButStripEmUnits { - (Zero::zero(), length) - } else { - (reference_font_size, length) - } + (reference_font_size, length) }, FontRelativeLength::Ex(length) => { if context.for_non_inherited_property.is_some() {