From d02c5b028162d9f7d5d3d7bb28fccf051b010d54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 1 Sep 2016 11:42:30 -0700 Subject: [PATCH] style: Don't incorrectly clamp values in calc that might not be only lengths. Expressions with percentages may be negative or positive at computed value time. So, we can only clamp lengths at computed value time, which is what the other browsers do. --- components/servo/Cargo.lock | 1 + components/style/media_queries.rs | 11 ++-- components/style/values/computed/mod.rs | 6 +- components/style/values/specified/mod.rs | 55 +++++-------------- components/style_traits/Cargo.toml | 1 + components/style_traits/lib.rs | 1 + components/style_traits/values.rs | 11 ++++ ports/cef/Cargo.lock | 1 + ports/geckolib/Cargo.lock | 1 + tests/wpt/mozilla/meta/MANIFEST.json | 24 ++++++++ .../tests/css/negative-calc-cv-ref.html | 16 ++++++ .../mozilla/tests/css/negative-calc-cv.html | 17 ++++++ 12 files changed, 97 insertions(+), 48 deletions(-) create mode 100644 tests/wpt/mozilla/tests/css/negative-calc-cv-ref.html create mode 100644 tests/wpt/mozilla/tests/css/negative-calc-cv.html diff --git a/components/servo/Cargo.lock b/components/servo/Cargo.lock index 7686aeb2102..758faadd19c 100644 --- a/components/servo/Cargo.lock +++ b/components/servo/Cargo.lock @@ -2268,6 +2268,7 @@ dependencies = [ name = "style_traits" version = "0.0.1" dependencies = [ + "app_units 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "cssparser 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)", "heapsize 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/components/style/media_queries.rs b/components/style/media_queries.rs index ed6dd2607bd..0537fc9fdac 100644 --- a/components/style/media_queries.rs +++ b/components/style/media_queries.rs @@ -40,11 +40,12 @@ impl Range { => value.to_computed_value(initial_font_size, initial_font_size), specified::Length::ViewportPercentage(value) => value.to_computed_value(viewport_size), - specified::Length::Calc(val) - => val.compute_from_viewport_and_font_size(viewport_size, - initial_font_size, - initial_font_size) - .length(), + specified::Length::Calc(val, range) + => range.clamp( + val.compute_from_viewport_and_font_size(viewport_size, + initial_font_size, + initial_font_size) + .length()), specified::Length::ServoCharacterWidth(..) => unreachable!(), } diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index 28fc7b3892f..7c13e1811e2 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -71,7 +71,7 @@ impl ToComputedValue for specified::Length { fn to_computed_value(&self, context: &Context) -> Au { match *self { specified::Length::Absolute(length) => length, - specified::Length::Calc(calc) => calc.to_computed_value(context).length(), + specified::Length::Calc(calc, range) => range.clamp(calc.to_computed_value(context).length()), specified::Length::FontRelative(length) => length.to_computed_value(context.style().get_font().clone_font_size(), context.style().root_font_size()), @@ -480,8 +480,8 @@ impl ToComputedValue for specified::LengthOrNone { #[inline] fn to_computed_value(&self, context: &Context) -> LengthOrNone { match *self { - specified::LengthOrNone::Length(specified::Length::Calc(calc)) => { - LengthOrNone::Length(calc.to_computed_value(context).length()) + specified::LengthOrNone::Length(specified::Length::Calc(calc, range)) => { + LengthOrNone::Length(range.clamp(calc.to_computed_value(context).length())) } specified::LengthOrNone::Length(value) => { LengthOrNone::Length(value.to_computed_value(context)) diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index 3bbdcb1ccb9..b0d02669154 100644 --- a/components/style/values/specified/mod.rs +++ b/components/style/values/specified/mod.rs @@ -190,14 +190,14 @@ pub enum Length { /// `Stylist::synthesize_rules_for_legacy_attributes()`. ServoCharacterWidth(CharacterWidth), - Calc(CalcLengthOrPercentage), + Calc(CalcLengthOrPercentage, AllowedNumericType), } impl HasViewportPercentage for Length { fn has_viewport_percentage(&self) -> bool { match *self { Length::ViewportPercentage(_) => true, - Length::Calc(ref calc) => calc.has_viewport_percentage(), + Length::Calc(ref calc, _) => calc.has_viewport_percentage(), _ => false } } @@ -209,7 +209,7 @@ impl ToCss for Length { Length::Absolute(length) => write!(dest, "{}px", length.to_f32_px()), Length::FontRelative(length) => length.to_css(dest), Length::ViewportPercentage(length) => length.to_css(dest), - Length::Calc(ref calc) => calc.to_css(dest), + Length::Calc(ref calc, _) => calc.to_css(dest), Length::ServoCharacterWidth(_) => panic!("internal CSS values should never be serialized"), } @@ -225,7 +225,7 @@ impl Mul for Length { Length::Absolute(Au(v)) => Length::Absolute(Au(((v as f32) * scalar) as i32)), Length::FontRelative(v) => Length::FontRelative(v * scalar), Length::ViewportPercentage(v) => Length::ViewportPercentage(v * scalar), - Length::Calc(_) => panic!("Can't multiply Calc!"), + Length::Calc(..) => panic!("Can't multiply Calc!"), Length::ServoCharacterWidth(_) => panic!("Can't multiply ServoCharacterWidth!"), } } @@ -469,8 +469,6 @@ pub struct CalcLengthOrPercentage { pub ch: Option, pub rem: Option, pub percentage: Option, - /// Whether the value returned can be negative at computed value time. - pub allowed_numeric_type: AllowedNumericType, } impl CalcLengthOrPercentage { @@ -623,17 +621,17 @@ impl CalcLengthOrPercentage { fn parse_length(input: &mut Parser, context: AllowedNumericType) -> Result { - CalcLengthOrPercentage::parse(input, CalcUnit::Length, context).map(Length::Calc) + CalcLengthOrPercentage::parse(input, CalcUnit::Length).map(|calc| { + Length::Calc(calc, context) + }) } - fn parse_length_or_percentage(input: &mut Parser, - context: AllowedNumericType) -> Result { - CalcLengthOrPercentage::parse(input, CalcUnit::LengthOrPercentage, context) + fn parse_length_or_percentage(input: &mut Parser) -> Result { + CalcLengthOrPercentage::parse(input, CalcUnit::LengthOrPercentage) } fn parse(input: &mut Parser, - expected_unit: CalcUnit, - context: AllowedNumericType) -> Result { + expected_unit: CalcUnit) -> Result { let ast = try!(CalcLengthOrPercentage::parse_sum(input, expected_unit)); let mut simplified = Vec::new(); @@ -700,7 +698,6 @@ impl CalcLengthOrPercentage { ch: ch.map(FontRelativeLength::Ch), rem: rem.map(FontRelativeLength::Rem), percentage: percentage.map(Percentage), - allowed_numeric_type: context, }) } @@ -787,23 +784,9 @@ impl CalcLengthOrPercentage { } } - // https://drafts.csswg.org/css-values/#calc-range - let mut percentage = self.percentage.map(|p| p.0); - if let AllowedNumericType::NonNegative = self.allowed_numeric_type { - if let Some(ref mut length) = length { - *length = cmp::max(*length, Au(0)); - } - - if let Some(ref mut percentage) = percentage { - if *percentage < 0. { - *percentage = 0.; - } - } - } - computed::CalcLengthOrPercentage { length: length, - percentage: percentage, + percentage: self.percentage.map(|p| p.0), } } } @@ -919,9 +902,7 @@ impl LengthOrPercentage { Token::Number(ref value) if value.value == 0. => Ok(LengthOrPercentage::Length(Length::Absolute(Au(0)))), Token::Function(ref name) if name.eq_ignore_ascii_case("calc") => { - let calc = try!(input.parse_nested_block(|input| { - CalcLengthOrPercentage::parse_length_or_percentage(input, context) - })); + let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage)); Ok(LengthOrPercentage::Calc(calc)) }, _ => Err(()) @@ -981,9 +962,7 @@ impl LengthOrPercentageOrAuto { Token::Ident(ref value) if value.eq_ignore_ascii_case("auto") => Ok(LengthOrPercentageOrAuto::Auto), Token::Function(ref name) if name.eq_ignore_ascii_case("calc") => { - let calc = try!(input.parse_nested_block(|input| { - CalcLengthOrPercentage::parse_length_or_percentage(input, context) - })); + let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage)); Ok(LengthOrPercentageOrAuto::Calc(calc)) }, _ => Err(()) @@ -1040,9 +1019,7 @@ impl LengthOrPercentageOrNone { Token::Number(ref value) if value.value == 0. => Ok(LengthOrPercentageOrNone::Length(Length::Absolute(Au(0)))), Token::Function(ref name) if name.eq_ignore_ascii_case("calc") => { - let calc = try!(input.parse_nested_block(|input| { - CalcLengthOrPercentage::parse_length_or_percentage(input, context) - })); + let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage)); Ok(LengthOrPercentageOrNone::Calc(calc)) }, Token::Ident(ref value) if value.eq_ignore_ascii_case("none") => @@ -1159,9 +1136,7 @@ impl LengthOrPercentageOrAutoOrContent { Token::Ident(ref value) if value.eq_ignore_ascii_case("content") => Ok(LengthOrPercentageOrAutoOrContent::Content), Token::Function(ref name) if name.eq_ignore_ascii_case("calc") => { - let calc = try!(input.parse_nested_block(|input| { - CalcLengthOrPercentage::parse_length_or_percentage(input, context) - })); + let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage)); Ok(LengthOrPercentageOrAutoOrContent::Calc(calc)) }, _ => Err(()) diff --git a/components/style_traits/Cargo.toml b/components/style_traits/Cargo.toml index 234d6a6c668..99ff18bd7ee 100644 --- a/components/style_traits/Cargo.toml +++ b/components/style_traits/Cargo.toml @@ -14,6 +14,7 @@ servo = ["heapsize", "heapsize_plugin", "serde", "serde_macros", "cssparser/heap_size", "cssparser/serde-serialization"] [dependencies] +app_units = "0.3" cssparser = "0.6" euclid = "0.10.1" heapsize = {version = "0.3.0", optional = true} diff --git a/components/style_traits/lib.rs b/components/style_traits/lib.rs index c55447d301a..6b538461d44 100644 --- a/components/style_traits/lib.rs +++ b/components/style_traits/lib.rs @@ -16,6 +16,7 @@ #![cfg_attr(feature = "servo", plugin(serde_macros))] #![cfg_attr(feature = "servo", plugin(heapsize_plugin))] +extern crate app_units; #[macro_use] extern crate cssparser; extern crate euclid; diff --git a/components/style_traits/values.rs b/components/style_traits/values.rs index eb6f3c13a55..747c1481bd1 100644 --- a/components/style_traits/values.rs +++ b/components/style_traits/values.rs @@ -63,6 +63,8 @@ macro_rules! __define_css_keyword_enum__actual { pub mod specified { + use app_units::Au; + #[repr(u8)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -79,5 +81,14 @@ pub mod specified { AllowedNumericType::NonNegative => value >= 0., } } + + #[inline] + pub fn clamp(&self, val: Au) -> Au { + use std::cmp; + match *self { + AllowedNumericType::All => val, + AllowedNumericType::NonNegative => cmp::max(Au(0), val), + } + } } } diff --git a/ports/cef/Cargo.lock b/ports/cef/Cargo.lock index 61a0c37c836..cf0541c2a12 100644 --- a/ports/cef/Cargo.lock +++ b/ports/cef/Cargo.lock @@ -2135,6 +2135,7 @@ dependencies = [ name = "style_traits" version = "0.0.1" dependencies = [ + "app_units 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "cssparser 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)", "heapsize 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/ports/geckolib/Cargo.lock b/ports/geckolib/Cargo.lock index 6f489176f8d..3a57edb0dfd 100644 --- a/ports/geckolib/Cargo.lock +++ b/ports/geckolib/Cargo.lock @@ -387,6 +387,7 @@ dependencies = [ name = "style_traits" version = "0.0.1" dependencies = [ + "app_units 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "cssparser 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)", "rustc-serialize 0.3.19 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 3297053b159..bbc79260a5f 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -3648,6 +3648,18 @@ "url": "/_mozilla/css/multiple_css_class_a.html" } ], + "css/negative-calc-cv.html": [ + { + "path": "css/negative-calc-cv.html", + "references": [ + [ + "/_mozilla/css/negative-calc-cv-ref.html", + "==" + ] + ], + "url": "/_mozilla/css/negative-calc-cv.html" + } + ], "css/negative_margin_uncle_a.html": [ { "path": "css/negative_margin_uncle_a.html", @@ -13030,6 +13042,18 @@ "url": "/_mozilla/css/multiple_css_class_a.html" } ], + "css/negative-calc-cv.html": [ + { + "path": "css/negative-calc-cv.html", + "references": [ + [ + "/_mozilla/css/negative-calc-cv-ref.html", + "==" + ] + ], + "url": "/_mozilla/css/negative-calc-cv.html" + } + ], "css/negative_margin_uncle_a.html": [ { "path": "css/negative_margin_uncle_a.html", diff --git a/tests/wpt/mozilla/tests/css/negative-calc-cv-ref.html b/tests/wpt/mozilla/tests/css/negative-calc-cv-ref.html new file mode 100644 index 00000000000..90bad307da2 --- /dev/null +++ b/tests/wpt/mozilla/tests/css/negative-calc-cv-ref.html @@ -0,0 +1,16 @@ + + +CSS Test Reference + +
+
+
diff --git a/tests/wpt/mozilla/tests/css/negative-calc-cv.html b/tests/wpt/mozilla/tests/css/negative-calc-cv.html new file mode 100644 index 00000000000..83607a3412e --- /dev/null +++ b/tests/wpt/mozilla/tests/css/negative-calc-cv.html @@ -0,0 +1,17 @@ + + +A negative length in a calc() expression isn't incorrectly clamped. + + +
+
+