Auto merge of #13159 - emilio:negative-calc, r=notriddle

style: Don't incorrectly clamp values in calc that might not be only lengths.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13159)
<!-- Reviewable:end -->

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)",

components/style/media_queries.rs

@@ -40,11 +40,12 @@ impl Range<specified::Length> {
                     => value.to_computed_value(initial_font_size, initial_font_size),
                 specified::Length::ViewportPercentage(value)
                     => value.to_computed_value(viewport_size),
-                specified::Length::Calc(val)
+                specified::Length::Calc(val, range)
-                    => val.compute_from_viewport_and_font_size(viewport_size,
+                    => range.clamp(
-                                                               initial_font_size,
+                        val.compute_from_viewport_and_font_size(viewport_size,
-                                                               initial_font_size)
+                                                                initial_font_size,
-                          .length(),
+                                                                initial_font_size)
+                           .length()),
                 specified::Length::ServoCharacterWidth(..)
                     => unreachable!(),
             }

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)) => {
+            specified::LengthOrNone::Length(specified::Length::Calc(calc, range)) => {
-                LengthOrNone::Length(calc.to_computed_value(context).length())
+                LengthOrNone::Length(range.clamp(calc.to_computed_value(context).length()))
             }
             specified::LengthOrNone::Length(value) => {
                 LengthOrNone::Length(value.to_computed_value(context))

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<CSSFloat> 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<FontRelativeLength>,
     pub rem: Option<FontRelativeLength>,
     pub percentage: Option<Percentage>,
-    /// 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<Length, ()> {
-        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,
+    fn parse_length_or_percentage(input: &mut Parser) -> Result<CalcLengthOrPercentage, ()> {
-                                  context: AllowedNumericType) -> Result<CalcLengthOrPercentage, ()> {
+        CalcLengthOrPercentage::parse(input, CalcUnit::LengthOrPercentage)
-        CalcLengthOrPercentage::parse(input, CalcUnit::LengthOrPercentage, context)
     }
 
     fn parse(input: &mut Parser,
-             expected_unit: CalcUnit,
+             expected_unit: CalcUnit) -> Result<CalcLengthOrPercentage, ()> {
-             context: AllowedNumericType) -> Result<CalcLengthOrPercentage, ()> {
         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| {
+                let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage));
-                    CalcLengthOrPercentage::parse_length_or_percentage(input, context)
-                }));
                 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| {
+                let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage));
-                    CalcLengthOrPercentage::parse_length_or_percentage(input, context)
-                }));
                 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| {
+                let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage));
-                    CalcLengthOrPercentage::parse_length_or_percentage(input, context)
-                }));
                 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| {
+                let calc = try!(input.parse_nested_block(CalcLengthOrPercentage::parse_length_or_percentage));
-                    CalcLengthOrPercentage::parse_length_or_percentage(input, context)
-                }));
                 Ok(LengthOrPercentageOrAutoOrContent::Calc(calc))
             },
             _ => Err(())

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}

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;

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),
+            }
+        }
     }
 }

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)",

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)",

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",

tests/wpt/mozilla/tests/css/negative-calc-cv-ref.html

@@ -0,0 +1,16 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<style>
+  .container {
+    width: 500px;
+  }
+  .kid {
+    height: 50px;
+    background: green;
+    width: 40px;
+  }
+</style>
+<div class="container">
+  <div class="kid"></div>
+</div>

tests/wpt/mozilla/tests/css/negative-calc-cv.html

@@ -0,0 +1,17 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>A negative length in a calc() expression isn't incorrectly clamped.</title>
+<link rel="match" href="negative-calc-cv-ref.html">
+<style>
+  .container {
+    width: 500px;
+  }
+  .kid {
+    height: 50px;
+    background: green;
+    width: calc(-10px + 10%);
+  }
+</style>
+<div class="container">
+  <div class="kid"></div>
+</div>