From 7ecee05e4a098788fcb5f2764914411ed8d0ad54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Sat, 1 Apr 2017 23:11:50 +0200 Subject: [PATCH] Style: Do not immediately convert absolute specified lengths The NoCalcLength::Absolute variant has been rewritten to accommodate other units than Au with the new AbsoluteLength enum. This avoids loss of precision for some operations. The conversion from floating point absolute lengths to integer application unit values adopts the same clamping limits as Gecko. --- components/script/dom/element.rs | 4 +- components/style/values/computed/length.rs | 7 +- components/style/values/specified/length.rs | 153 +++++++++++++++--- components/style/values/specified/mod.rs | 1 + tests/unit/style/parsing/length.rs | 17 +- tests/unit/style/properties/viewport.rs | 4 +- tests/unit/style/value.rs | 4 +- .../html/min-width-001.htm.ini | 3 - .../html/cssstyledeclaration-csstext.htm.ini | 12 -- .../html/min-width-001.htm.ini | 3 - tests/wpt/mozilla/meta/MANIFEST.json | 2 +- tests/wpt/mozilla/tests/mozilla/calc.html | 2 +- 12 files changed, 161 insertions(+), 51 deletions(-) delete mode 100644 tests/wpt/metadata-css/css-values-3_dev/html/min-width-001.htm.ini delete mode 100644 tests/wpt/metadata-css/mediaqueries-3_dev/html/min-width-001.htm.ini diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 4b3a6010a9a..58fd6c33e3f 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -561,7 +561,7 @@ impl LayoutElementHelpers for LayoutJS { } 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 { } 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))); diff --git a/components/style/values/computed/length.rs b/components/style/values/computed/length.rs index a65768ea2a1..cfdb4151750 100644 --- a/components/style/values/computed/length.rs +++ b/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())) } } diff --git a/components/style/values/specified/length.rs b/components/style/values/specified/length.rs index d6dcad04a3b..efc8b5405d9 100644 --- a/components/style/values/specified/length.rs +++ b/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}; @@ -220,14 +220,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 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(&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 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 `` 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 - Absolute(Au), // application units + /// An absolute length + /// + /// https://drafts.csswg.org/css-values/#absolute-length + Absolute(AbsoluteLength), /// A font-relative length: /// @@ -258,7 +366,7 @@ impl HasViewportPercentage for NoCalcLength { impl ToCss for NoCalcLength { fn to_css(&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 */ @@ -273,7 +381,7 @@ impl Mul 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!"), @@ -285,13 +393,13 @@ impl NoCalcLength { /// Parse a given absolute or relative dimension. pub fn parse_dimension(value: CSSFloat, unit: &str) -> Result { match_ignore_ascii_case! { unit, - "px" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_PX) as i32))), - "in" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_IN) as i32))), - "cm" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_CM) as i32))), - "mm" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_MM) as i32))), - "q" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_Q) as i32))), - "pt" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_PT) as i32))), - "pc" => Ok(NoCalcLength::Absolute(Au((value * AU_PER_PC) as i32))), + "px" => Ok(NoCalcLength::Absolute(AbsoluteLength::Px(value))), + "in" => Ok(NoCalcLength::Absolute(AbsoluteLength::In(value))), + "cm" => Ok(NoCalcLength::Absolute(AbsoluteLength::Cm(value))), + "mm" => Ok(NoCalcLength::Absolute(AbsoluteLength::Mm(value))), + "q" => Ok(NoCalcLength::Absolute(AbsoluteLength::Q(value))), + "pt" => Ok(NoCalcLength::Absolute(AbsoluteLength::Pt(value))), + "pc" => Ok(NoCalcLength::Absolute(AbsoluteLength::Pc(value))), // font-relative "em" => Ok(NoCalcLength::FontRelative(FontRelativeLength::Em(value))), "ex" => Ok(NoCalcLength::FontRelative(FontRelativeLength::Ex(value))), @@ -309,25 +417,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)) } } @@ -756,8 +867,8 @@ impl CalcLengthOrPercentage { match value { SimplifiedValueNode::Percentage(p) => percentage = Some(percentage.unwrap_or(0.) + p), - SimplifiedValueNode::Length(NoCalcLength::Absolute(Au(au))) => - absolute = Some(absolute.unwrap_or(0) + au), + SimplifiedValueNode::Length(NoCalcLength::Absolute(length)) => + absolute = Some(absolute.unwrap_or(0.) + Au::from(length).to_f32_px()), SimplifiedValueNode::Length(NoCalcLength::ViewportPercentage(v)) => match v { ViewportPercentageLength::Vw(val) => @@ -786,7 +897,7 @@ impl CalcLengthOrPercentage { } Ok(CalcLengthOrPercentage { - absolute: absolute.map(Au), + absolute: absolute.map(Au::from_f32_px), vw: vw, vh: vh, vmax: vmax, @@ -1061,7 +1172,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 @@ -1076,7 +1187,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(()) } diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index f5ba7b7fb1d..b96e8246843 100644 --- a/components/style/values/specified/mod.rs +++ b/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}; diff --git a/tests/unit/style/parsing/length.rs b/tests/unit/style/parsing/length.rs index eda3b3425e3..39407441aaf 100644 --- a/tests/unit/style/parsing/length.rs +++ b/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"); +} diff --git a/tests/unit/style/properties/viewport.rs b/tests/unit/style/properties/viewport.rs index 8b88d61f293..0f602ac4475 100644 --- a/tests/unit/style/properties/viewport.rs +++ b/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()); diff --git a/tests/unit/style/value.rs b/tests/unit/style/value.rs index 472a5177d30..4214b5ef1b5 100644 --- a/tests/unit/style/value.rs +++ b/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()); } diff --git a/tests/wpt/metadata-css/css-values-3_dev/html/min-width-001.htm.ini b/tests/wpt/metadata-css/css-values-3_dev/html/min-width-001.htm.ini deleted file mode 100644 index c887ed3bc7a..00000000000 --- a/tests/wpt/metadata-css/css-values-3_dev/html/min-width-001.htm.ini +++ /dev/null @@ -1,3 +0,0 @@ -[min-width-001.htm] - type: reftest - expected: FAIL diff --git a/tests/wpt/metadata-css/cssom-1_dev/html/cssstyledeclaration-csstext.htm.ini b/tests/wpt/metadata-css/cssom-1_dev/html/cssstyledeclaration-csstext.htm.ini index f0db23cead6..5f750141b21 100644 --- a/tests/wpt/metadata-css/cssom-1_dev/html/cssstyledeclaration-csstext.htm.ini +++ b/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 - diff --git a/tests/wpt/metadata-css/mediaqueries-3_dev/html/min-width-001.htm.ini b/tests/wpt/metadata-css/mediaqueries-3_dev/html/min-width-001.htm.ini deleted file mode 100644 index c887ed3bc7a..00000000000 --- a/tests/wpt/metadata-css/mediaqueries-3_dev/html/min-width-001.htm.ini +++ /dev/null @@ -1,3 +0,0 @@ -[min-width-001.htm] - type: reftest - expected: FAIL diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 74a6afd08f6..8db0b39e821 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -25171,7 +25171,7 @@ "testharness" ], "mozilla/calc.html": [ - "c239c441c90e4d4ed94458eae5b2defabb47e984", + "028fc71bdc9a99d552ba552036d38fb4eef11bc1", "testharness" ], "mozilla/canvas.initial.reset.2dstate.html": [ diff --git a/tests/wpt/mozilla/tests/mozilla/calc.html b/tests/wpt/mozilla/tests/mozilla/calc.html index 9826168e24b..2f35b363422 100644 --- a/tests/wpt/mozilla/tests/mozilla/calc.html +++ b/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)',