From c78f1b62def53884ecaee2e5df4eb19e4a6c25e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 12 Sep 2019 08:14:44 +0000 Subject: [PATCH] style: Remove LengthPercentage::was_calc. There should not be any behavior change between specifying a percentage using % or calc(%) per the resolution of https://github.com/w3c/csswg-drafts/issues/3482. Differential Revision: https://phabricator.services.mozilla.com/D43747 --- components/style/values/animated/length.rs | 3 - components/style/values/computed/length.rs | 61 +++++++------------ components/style/values/specified/position.rs | 1 - 3 files changed, 23 insertions(+), 42 deletions(-) diff --git a/components/style/values/animated/length.rs b/components/style/values/animated/length.rs index 73957618cea..04a0844dbe0 100644 --- a/components/style/values/animated/length.rs +++ b/components/style/values/animated/length.rs @@ -26,13 +26,10 @@ impl Animate for LengthPercentage { .animate(&other.unclamped_length(), procedure)?; let percentage = animate_percentage_half(self.specified_percentage(), other.specified_percentage())?; - let is_calc = - self.was_calc || other.was_calc || self.has_percentage != other.has_percentage; Ok(Self::with_clamping_mode( length, percentage, self.clamping_mode, - is_calc, )) } } diff --git a/components/style/values/computed/length.rs b/components/style/values/computed/length.rs index 88eb8f3955a..2643cd14150 100644 --- a/components/style/values/computed/length.rs +++ b/components/style/values/computed/length.rs @@ -85,25 +85,20 @@ pub struct LengthPercentage { /// Whether we specified a percentage or not. #[animation(constant)] pub has_percentage: bool, - /// Whether this was from a calc() expression. This is needed because right - /// now we don't treat calc() the same way as non-calc everywhere, but - /// that's a bug in most cases. - /// - /// Please don't add new uses of this that aren't for converting to Gecko's - /// representation, or to interpolate values. - /// - /// See https://github.com/w3c/csswg-drafts/issues/3482. - #[animation(constant)] - pub was_calc: bool, } -// FIXME(emilio): This is a bit of a hack that can disappear as soon as we share -// representation of LengthPercentage with Gecko. The issue here is that Gecko -// uses CalcValue to represent position components, so they always come back as -// was_calc == true, and we mess up in the transitions code. +// NOTE(emilio): We don't compare `clamping_mode` since we want to preserve the +// invariant that `from_computed_value(length).to_computed_value(..) == length`. // -// This was a pre-existing bug, though arguably so only in pretty obscure cases -// like calc(0px + 5%) and such. +// Right now for e.g. a non-negative length, we set clamping_mode to `All` +// unconditionally for non-calc values, and to `NonNegative` for calc. +// +// If we determine that it's sound, from_computed_value() can generate an +// absolute length, which then would get `All` as the clamping mode. +// +// We may want to just eagerly-detect whether we can clamp in +// `LengthPercentage::new` and switch to `AllowedNumericType::NonNegative` then, +// maybe. impl PartialEq for LengthPercentage { fn eq(&self, other: &Self) -> bool { self.length == other.length && @@ -133,7 +128,6 @@ impl LengthPercentage { length, percentage, AllowedNumericType::All, - /* was_calc = */ false, ) } @@ -148,14 +142,12 @@ impl LengthPercentage { length: Length, percentage: Option, clamping_mode: AllowedNumericType, - was_calc: bool, ) -> Self { Self { clamping_mode, length, percentage: percentage.unwrap_or_default(), has_percentage: percentage.is_some(), - was_calc, } } @@ -285,7 +277,6 @@ impl specified::CalcLengthPercentage { Length::new(length.min(f32::MAX).max(f32::MIN)), self.percentage, self.clamping_mode, - /* was_calc = */ true, ) } @@ -381,35 +372,29 @@ impl LengthPercentage { } /// Returns the clamped non-negative values. - /// - /// TODO(emilio): It's a bit unfortunate that this depends on whether the - /// value was a `calc()` value or not. Should it? #[inline] pub fn clamp_to_non_negative(self) -> Self { - if self.was_calc { + if let Some(p) = self.specified_percentage() { + // If we can eagerly clamp the percentage then just do that. + if self.length.is_zero() { + return Self::with_clamping_mode( + Length::zero(), + Some(p.clamp_to_non_negative()), + AllowedNumericType::NonNegative, + ); + } + return Self::with_clamping_mode( self.length, - self.specified_percentage(), + Some(p), AllowedNumericType::NonNegative, - self.was_calc, - ); - } - - debug_assert!(!self.has_percentage || self.unclamped_length() == Length::zero()); - if let Some(p) = self.specified_percentage() { - return Self::with_clamping_mode( - Length::zero(), - Some(p.clamp_to_non_negative()), - AllowedNumericType::NonNegative, - self.was_calc, - ); + ) } Self::with_clamping_mode( self.length.clamp_to_non_negative(), None, AllowedNumericType::NonNegative, - self.was_calc, ) } } diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index d3476bb9f65..37d07894273 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -301,7 +301,6 @@ impl ToComputedValue for PositionComponent { l, Some(p), length.clamping_mode, - /* was_calc = */ true, ) }, PositionComponent::Side(_, Some(ref length)) |