Auto merge of #16229 - tomhoule:fix-lengths, r=emilio

style: Do not immediately convert absolute specified lengths

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

This PR aims to solve issue #15729. I tried to follow the recommendations there as much as possible.

This is my first attempt at contributing to Servo, so this will probably need a lot of input, although I'm eager to make it as polished as possible.

- The base inaccuracy issue seems solved, as can be easily verified with the `console.log` based example in the issue.
- Very basic unit tests were added.

I have doubts mainly about the right way to represent these new enum variants for the various length units:

1. With new enum variants in `NoCalcLength` *and* newtypes (current solution)
2. With a `NoCalcLength::Absolute` variant that contains a new `AbsoluteLength` enum, but without newtypes
3. Same as solution 2 but with newtypes

- I mostly cared about unit tests until now but will investigate other types of tests
- Tests to check the clamping
- Write a proper commit message

Thanks for your time and feedback :)

---
<!-- 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 #15729.

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/16229)
<!-- Reviewable:end -->

components/script/dom/element.rs

@@ -561,7 +561,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
             }
             LengthOrPercentageOrAuto::Length(length) => {
                 let width_value = specified::LengthOrPercentageOrAuto::Length(
-                    specified::NoCalcLength::Absolute(length));
+                    specified::NoCalcLength::Absolute(specified::AbsoluteLength::Px(length.to_f32_px())));
                 hints.push(from_declaration(
                     shared_lock,
                     PropertyDeclaration::Width(width_value)));
@@ -590,7 +590,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
             }
             LengthOrPercentageOrAuto::Length(length) => {
                 let height_value = specified::LengthOrPercentageOrAuto::Length(
-                    specified::NoCalcLength::Absolute(length));
+                    specified::NoCalcLength::Absolute(specified::AbsoluteLength::Px(length.to_f32_px())));
                 hints.push(from_declaration(
                     shared_lock,
                     PropertyDeclaration::Height(height_value)));

components/style/values/computed/length.rs

@@ -10,7 +10,7 @@ use std::fmt;
 use style_traits::ToCss;
 use super::{Number, ToComputedValue, Context};
 use values::{Auto, CSSFloat, Either, ExtremumLength, None_, Normal, specified};
-use values::specified::length::{FontRelativeLength, ViewportPercentageLength};
+use values::specified::length::{AbsoluteLength, FontRelativeLength, ViewportPercentageLength};
 
 pub use super::image::{EndingShape as GradientShape, Gradient, GradientKind, Image};
 pub use super::image::{LengthOrKeyword, LengthOrPercentageOrKeyword};
@@ -22,7 +22,8 @@ impl ToComputedValue for specified::NoCalcLength {
     #[inline]
     fn to_computed_value(&self, context: &Context) -> Au {
         match *self {
-            specified::NoCalcLength::Absolute(length) => length,
+            specified::NoCalcLength::Absolute(length) =>
+                length.to_computed_value(context),
             specified::NoCalcLength::FontRelative(length) =>
                 length.to_computed_value(context, /* use inherited */ false),
             specified::NoCalcLength::ViewportPercentage(length) =>
@@ -34,7 +35,7 @@ impl ToComputedValue for specified::NoCalcLength {
 
     #[inline]
     fn from_computed_value(computed: &Au) -> Self {
-        specified::NoCalcLength::Absolute(*computed)
+        specified::NoCalcLength::Absolute(AbsoluteLength::Px(computed.to_f32_px()))
     }
 }
 

components/style/values/specified/length.rs

@@ -16,7 +16,7 @@ use std::ascii::AsciiExt;
 use std::ops::Mul;
 use style_traits::ToCss;
 use style_traits::values::specified::AllowedNumericType;
-use super::{Angle, Number, SimplifiedValueNode, SimplifiedSumNode, Time};
+use super::{Angle, Number, SimplifiedValueNode, SimplifiedSumNode, Time, ToComputedValue};
 use values::{Auto, CSSFloat, Either, FONT_MEDIUM_PX, HasViewportPercentage, None_, Normal};
 use values::ExtremumLength;
 use values::computed::{ComputedValueAsSpecified, Context};
@@ -202,14 +202,122 @@ impl CharacterWidth {
     }
 }
 
+/// Same as Gecko
+const ABSOLUTE_LENGTH_MAX: i32 = (1 << 30);
+const ABSOLUTE_LENGTH_MIN: i32 = - (1 << 30);
+
+/// Helper to convert a floating point length to application units
+fn to_au_round(length: CSSFloat, au_per_unit: CSSFloat) -> Au {
+    Au(
+        (length * au_per_unit)
+        .min(ABSOLUTE_LENGTH_MAX as f32)
+        .max(ABSOLUTE_LENGTH_MIN as f32)
+        .round() as i32
+    )
+}
+
+/// Represents an absolute length with its unit
+#[derive(Clone, PartialEq, Copy, Debug)]
+#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+pub enum AbsoluteLength {
+    /// An absolute length in pixels (px)
+    Px(CSSFloat),
+    /// An absolute length in inches (in)
+    In(CSSFloat),
+    /// An absolute length in centimeters (cm)
+    Cm(CSSFloat),
+    /// An absolute length in millimeters (mm)
+    Mm(CSSFloat),
+    /// An absolute length in quarter-millimeters (q)
+    Q(CSSFloat),
+    /// An absolute length in points (pt)
+    Pt(CSSFloat),
+    /// An absolute length in pica (pc)
+    Pc(CSSFloat),
+}
+
+impl AbsoluteLength {
+    fn is_zero(&self) -> bool {
+        match *self {
+            AbsoluteLength::Px(0.)
+            | AbsoluteLength::In(0.)
+            | AbsoluteLength::Cm(0.)
+            | AbsoluteLength::Mm(0.)
+            | AbsoluteLength::Q(0.)
+            | AbsoluteLength::Pt(0.)
+            | AbsoluteLength::Pc(0.) => true,
+            _ => false,
+        }
+    }
+}
+
+impl ToComputedValue for AbsoluteLength {
+    type ComputedValue = Au;
+
+    fn to_computed_value(&self, _: &Context) -> Au {
+        Au::from(*self)
+    }
+
+    fn from_computed_value(computed: &Au) -> AbsoluteLength {
+        AbsoluteLength::Px(computed.to_f32_px())
+    }
+}
+
+impl From<AbsoluteLength> for Au {
+    fn from(length: AbsoluteLength) -> Au {
+        match length {
+            AbsoluteLength::Px(value) => to_au_round(value, AU_PER_PX),
+            AbsoluteLength::In(value) => to_au_round(value, AU_PER_IN),
+            AbsoluteLength::Cm(value) => to_au_round(value, AU_PER_CM),
+            AbsoluteLength::Mm(value) => to_au_round(value, AU_PER_MM),
+            AbsoluteLength::Q(value) => to_au_round(value, AU_PER_Q),
+            AbsoluteLength::Pt(value) => to_au_round(value, AU_PER_PT),
+            AbsoluteLength::Pc(value) => to_au_round(value, AU_PER_PC),
+        }
+    }
+}
+
+impl ToCss for AbsoluteLength {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        match *self {
+            AbsoluteLength::Px(length) => write!(dest, "{}px", length),
+            AbsoluteLength::In(length) => write!(dest, "{}in", length),
+            AbsoluteLength::Cm(length) => write!(dest, "{}cm", length),
+            AbsoluteLength::Mm(length) => write!(dest, "{}mm", length),
+            AbsoluteLength::Q(length) => write!(dest, "{}q", length),
+            AbsoluteLength::Pt(length) => write!(dest, "{}pt", length),
+            AbsoluteLength::Pc(length) => write!(dest, "{}pc", length),
+        }
+    }
+}
+
+impl Mul<CSSFloat> for AbsoluteLength {
+    type Output = AbsoluteLength;
+
+    #[inline]
+    fn mul(self, scalar: CSSFloat) -> AbsoluteLength {
+        match self {
+            AbsoluteLength::Px(v) => AbsoluteLength::Px(v * scalar),
+            AbsoluteLength::In(v) => AbsoluteLength::In(v * scalar),
+            AbsoluteLength::Cm(v) => AbsoluteLength::Cm(v * scalar),
+            AbsoluteLength::Mm(v) => AbsoluteLength::Mm(v * scalar),
+            AbsoluteLength::Q(v) => AbsoluteLength::Q(v * scalar),
+            AbsoluteLength::Pt(v) => AbsoluteLength::Pt(v * scalar),
+            AbsoluteLength::Pc(v) => AbsoluteLength::Pc(v * scalar),
+        }
+    }
+}
+
 /// A `<length>` without taking `calc` expressions into account
 ///
 /// https://drafts.csswg.org/css-values/#lengths
 #[derive(Clone, PartialEq, Copy, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub enum NoCalcLength {
-    /// An absolute length: https://drafts.csswg.org/css-values/#absolute-length
+    /// An absolute length
-    Absolute(Au),  // application units
+    ///
+    /// https://drafts.csswg.org/css-values/#absolute-length
+    Absolute(AbsoluteLength),
 
     /// A font-relative length:
     ///
@@ -240,7 +348,7 @@ impl HasViewportPercentage for NoCalcLength {
 impl ToCss for NoCalcLength {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         match *self {
-            NoCalcLength::Absolute(length) => write!(dest, "{}px", length.to_f32_px()),
+            NoCalcLength::Absolute(length) => length.to_css(dest),
             NoCalcLength::FontRelative(length) => length.to_css(dest),
             NoCalcLength::ViewportPercentage(length) => length.to_css(dest),
             /* This should only be reached from style dumping code */
@@ -255,7 +363,7 @@ impl Mul<CSSFloat> for NoCalcLength {
     #[inline]
     fn mul(self, scalar: CSSFloat) -> NoCalcLength {
         match self {
-            NoCalcLength::Absolute(Au(v)) => NoCalcLength::Absolute(Au(((v as f32) * scalar) as i32)),
+            NoCalcLength::Absolute(v) => NoCalcLength::Absolute(v * scalar),
             NoCalcLength::FontRelative(v) => NoCalcLength::FontRelative(v * scalar),
             NoCalcLength::ViewportPercentage(v) => NoCalcLength::ViewportPercentage(v * scalar),
             NoCalcLength::ServoCharacterWidth(_) => panic!("Can't multiply ServoCharacterWidth!"),
@@ -267,13 +375,13 @@ impl NoCalcLength {
     /// Parse a given absolute or relative dimension.
     pub fn parse_dimension(value: CSSFloat, unit: &str) -> Result<NoCalcLength, ()> {
         match_ignore_ascii_case! { unit,
-            "px" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_PX) as i32))),
+            "px" => Ok(NoCalcLength::Absolute(AbsoluteLength::Px(value))),
-            "in" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_IN) as i32))),
+            "in" => Ok(NoCalcLength::Absolute(AbsoluteLength::In(value))),
-            "cm" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_CM) as i32))),
+            "cm" => Ok(NoCalcLength::Absolute(AbsoluteLength::Cm(value))),
-            "mm" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_MM) as i32))),
+            "mm" => Ok(NoCalcLength::Absolute(AbsoluteLength::Mm(value))),
-            "q" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_Q) as i32))),
+            "q" => Ok(NoCalcLength::Absolute(AbsoluteLength::Q(value))),
-            "pt" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_PT) as i32))),
+            "pt" => Ok(NoCalcLength::Absolute(AbsoluteLength::Pt(value))),
-            "pc" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_PC) as i32))),
+            "pc" => Ok(NoCalcLength::Absolute(AbsoluteLength::Pc(value))),
             // font-relative
             "em" => Ok(NoCalcLength::FontRelative(FontRelativeLength::Em(value))),
             "ex" => Ok(NoCalcLength::FontRelative(FontRelativeLength::Ex(value))),
@@ -291,25 +399,28 @@ impl NoCalcLength {
     #[inline]
     /// Returns a `zero` length.
     pub fn zero() -> NoCalcLength {
-        NoCalcLength::Absolute(Au(0))
+        NoCalcLength::Absolute(AbsoluteLength::Px(0.))
     }
 
     #[inline]
     /// Checks whether the length value is zero.
     pub fn is_zero(&self) -> bool {
-        *self == NoCalcLength::Absolute(Au(0))
+        match *self {
+            NoCalcLength::Absolute(length) => length.is_zero(),
+            _ => false
+        }
     }
 
     #[inline]
     /// Returns a `medium` length.
     pub fn medium() -> NoCalcLength {
-        NoCalcLength::Absolute(Au::from_px(FONT_MEDIUM_PX))
+        NoCalcLength::Absolute(AbsoluteLength::Px(FONT_MEDIUM_PX as f32))
     }
 
     /// Get an absolute length from a px value.
     #[inline]
     pub fn from_px(px_value: CSSFloat) -> NoCalcLength {
-        NoCalcLength::Absolute(Au((px_value * AU_PER_PX) as i32))
+        NoCalcLength::Absolute(AbsoluteLength::Px(px_value))
     }
 }
 
@@ -738,8 +849,8 @@ impl CalcLengthOrPercentage {
             match value {
                 SimplifiedValueNode::Percentage(p) =>
                     percentage = Some(percentage.unwrap_or(0.) + p),
-                SimplifiedValueNode::Length(NoCalcLength::Absolute(Au(au))) =>
+                SimplifiedValueNode::Length(NoCalcLength::Absolute(length)) =>
-                    absolute = Some(absolute.unwrap_or(0) + au),
+                    absolute = Some(absolute.unwrap_or(0.) + Au::from(length).to_f32_px()),
                 SimplifiedValueNode::Length(NoCalcLength::ViewportPercentage(v)) =>
                     match v {
                         ViewportPercentageLength::Vw(val) =>
@@ -768,7 +879,7 @@ impl CalcLengthOrPercentage {
         }
 
         Ok(CalcLengthOrPercentage {
-            absolute: absolute.map(Au),
+            absolute: absolute.map(Au::from_f32_px),
             vw: vw,
             vh: vh,
             vmax: vmax,
@@ -1043,7 +1154,7 @@ impl LengthOrPercentage {
         // TODO(emilio): Probably should use Number::parse_non_negative to
         // handle calc()?
         let num = input.expect_number()?;
-        Ok(LengthOrPercentage::Length(NoCalcLength::Absolute(Au((AU_PER_PX * num) as i32))))
+        Ok(LengthOrPercentage::Length(NoCalcLength::Absolute(AbsoluteLength::Px(num))))
     }
 
     /// Parse a non-negative length, treating dimensionless numbers as pixels
@@ -1058,7 +1169,7 @@ impl LengthOrPercentage {
         // handle calc()?
         let num = input.expect_number()?;
         if num >= 0. {
-            Ok(LengthOrPercentage::Length(NoCalcLength::Absolute(Au((AU_PER_PX * num) as i32))))
+            Ok(LengthOrPercentage::Length(NoCalcLength::Absolute(AbsoluteLength::Px(num))))
         } else {
             Err(())
         }

components/style/values/specified/mod.rs

@@ -29,6 +29,7 @@ pub use self::grid::{GridLine, TrackKeyword};
 pub use self::image::{AngleOrCorner, ColorStop, EndingShape as GradientEndingShape, Gradient};
 pub use self::image::{GradientKind, HorizontalDirection, Image, ImageRect, LengthOrKeyword};
 pub use self::image::{LengthOrPercentageOrKeyword, SizeKeyword, VerticalDirection};
+pub use self::length::AbsoluteLength;
 pub use self::length::{FontRelativeLength, ViewportPercentageLength, CharacterWidth, Length, CalcLengthOrPercentage};
 pub use self::length::{Percentage, LengthOrNone, LengthOrNumber, LengthOrPercentage, LengthOrPercentageOrAuto};
 pub use self::length::{LengthOrPercentageOrNone, LengthOrPercentageOrAutoOrContent, NoCalcLength, CalcUnit};

tests/unit/style/parsing/length.rs

@@ -2,9 +2,13 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+use cssparser::Parser;
+use media_queries::CSSErrorReporterTest;
 use parsing::parse;
-use style::parser::Parse;
+use style::parser::{Parse, ParserContext};
+use style::stylesheets::Origin;
 use style::values::specified::length::Length;
+use style_traits::ToCss;
 
 #[test]
 fn test_calc() {
@@ -13,3 +17,14 @@ fn test_calc() {
     assert!(parse(Length::parse, "calc(1px + 2px )").is_ok());
     assert!(parse(Length::parse, "calc( 1px + 2px)").is_ok());
 }
+
+#[test]
+fn test_length_literals() {
+    assert_roundtrip_with_context!(Length::parse, "0.33px", "0.33px");
+    assert_roundtrip_with_context!(Length::parse, "0.33in", "0.33in");
+    assert_roundtrip_with_context!(Length::parse, "0.33cm", "0.33cm");
+    assert_roundtrip_with_context!(Length::parse, "0.33mm", "0.33mm");
+    assert_roundtrip_with_context!(Length::parse, "0.33q", "0.33q");
+    assert_roundtrip_with_context!(Length::parse, "0.33pt", "0.33pt");
+    assert_roundtrip_with_context!(Length::parse, "0.33pc", "0.33pc");
+}

tests/unit/style/properties/viewport.rs

@@ -6,7 +6,7 @@ use app_units::Au;
 use style::properties::PropertyDeclaration;
 use style::properties::longhands::border_top_width;
 use style::values::HasViewportPercentage;
-use style::values::specified::{Length, NoCalcLength, ViewportPercentageLength};
+use style::values::specified::{AbsoluteLength, Length, NoCalcLength, ViewportPercentageLength};
 
 #[test]
 fn has_viewport_percentage_for_specified_value() {
@@ -20,7 +20,7 @@ fn has_viewport_percentage_for_specified_value() {
 
     let pabs = PropertyDeclaration::BorderTopWidth(Box::new(
         border_top_width::SpecifiedValue::from_length(
-            Length::NoCalc(NoCalcLength::Absolute(Au(100)))
+            Length::NoCalc(NoCalcLength::Absolute(AbsoluteLength::Px(Au(100).to_f32_px())))
         )
     ));
     assert!(!pabs.has_viewport_percentage());

tests/unit/style/value.rs

@@ -5,14 +5,14 @@
 use app_units::Au;
 use cssparser::Parser;
 use style::values::HasViewportPercentage;
-use style::values::specified::{ViewportPercentageLength, NoCalcLength};
+use style::values::specified::{AbsoluteLength, ViewportPercentageLength, NoCalcLength};
 use style::values::specified::length::{CalcLengthOrPercentage, CalcUnit};
 
 #[test]
 fn length_has_viewport_percentage() {
     let l = NoCalcLength::ViewportPercentage(ViewportPercentageLength::Vw(100.));
     assert!(l.has_viewport_percentage());
-    let l = NoCalcLength::Absolute(Au(100));
+    let l = NoCalcLength::Absolute(AbsoluteLength::Px(Au(100).to_f32_px()));
     assert!(!l.has_viewport_percentage());
 }
 

tests/wpt/metadata-css/css-values-3_dev/html/min-width-001.htm.ini

@@ -1,3 +0,0 @@
-[min-width-001.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/cssom-1_dev/html/cssstyledeclaration-csstext.htm.ini

@@ -3,15 +3,3 @@
   [uppercase value]
     expected: FAIL
 
-  [cssText order]
-    expected: FAIL
-
-  [another cssText order (non-alphabetical order)]
-    expected: FAIL
-
-  [whitespaces in value]
-    expected: FAIL
-
-  [invalid property does not appear]
-    expected: FAIL
-

tests/wpt/metadata-css/mediaqueries-3_dev/html/min-width-001.htm.ini

@@ -1,3 +0,0 @@
-[min-width-001.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/mozilla/meta/MANIFEST.json

@@ -25196,7 +25196,7 @@
    "testharness"
   ],
   "mozilla/calc.html": [
-   "c239c441c90e4d4ed94458eae5b2defabb47e984",
+   "028fc71bdc9a99d552ba552036d38fb4eef11bc1",
    "testharness"
   ],
   "mozilla/canvas.initial.reset.2dstate.html": [

tests/wpt/mozilla/tests/mozilla/calc.html

@@ -41,7 +41,7 @@ var widthTests = [
     ['calc(10px + 10em - 10px)', 'calc(10em + 0px)', '160px'],
 
     // Fold absolute units
-    ['calc(1px + 1pt + 1pc + 1in + 1cm + 1mm)', 'calc(155.88333333333333px)', '155.88333333333333px'],
+    ['calc(1px + 1pt + 1pc + 1in + 1cm + 1mm)', 'calc(155.91666666666666px)', '155.91666666666666px'],
 
     // Alphabetical order
     ['calc(0ch + 0px + 0pt + 0pc + 0in + 0cm + 0mm + 0rem + 0em + 0ex + 0% + 0vw + 0vh + 0vmin + 0vmax)',