Bug 1609737 - Forbid accessing the length and percentage parts of a LengthPercentage separately. r=#style draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 16 Jan 2020 18:20:46 +0000
changeset 2598856 9f090382ec55b55be7f271705fda4c82e60948a0
parent 2598325 7e0886a94d70b8696d6fc0481d9f9ae12b85c41a
child 2598857 86a6bffe61730c15ee72127f8a11bee1060981ff
push id476906
push userreviewbot
push dateThu, 16 Jan 2020 18:21:04 +0000
treeherdertry@86a6bffe6173 [default view] [failures only]
bugs1609737
milestone74.0a1
Bug 1609737 - Forbid accessing the length and percentage parts of a LengthPercentage separately. r=#style Summary: This is just not a thing you can do if you have min() / max() / etc, as the min / max value may depend on the percentage basis. Test Plan: Reviewers: #style Subscribers: Bug #: 1609737 Differential Diff: PHID-DIFF-bgmeitmrdnzlf6fblqyo
accessible/base/StyleInfo.cpp
layout/style/ServoStyleConstsInlines.h
layout/style/nsComputedDOMStyle.cpp
layout/style/nsStyleUtil.cpp
servo/ports/geckolib/cbindgen.toml
servo/ports/geckolib/glue.rs
testing/web-platform/meta/css/css-logical/parsing/max-block-size-computed.html.ini
testing/web-platform/meta/css/css-logical/parsing/max-inline-size-computed.html.ini
testing/web-platform/meta/css/css-logical/parsing/min-block-size-computed.html.ini
testing/web-platform/meta/css/css-logical/parsing/min-inline-size-computed.html.ini
testing/web-platform/meta/css/css-position/parsing/bottom-computed.html.ini
testing/web-platform/meta/css/css-position/parsing/left-computed.html.ini
testing/web-platform/meta/css/css-position/parsing/right-computed.html.ini
testing/web-platform/meta/css/css-position/parsing/top-computed.html.ini
testing/web-platform/meta/css/css-sizing/parsing/max-height-computed.html.ini
testing/web-platform/meta/css/css-sizing/parsing/max-width-computed.html.ini
testing/web-platform/meta/css/css-sizing/parsing/min-height-computed.html.ini
testing/web-platform/meta/css/css-sizing/parsing/min-width-computed.html.ini
testing/web-platform/meta/svg/geometry/parsing/sizing-properties-computed.svg.ini
--- a/accessible/base/StyleInfo.cpp
+++ b/accessible/base/StyleInfo.cpp
@@ -32,26 +32,27 @@ void StyleInfo::TextAlign(nsAString& aVa
       aValue);
 }
 
 void StyleInfo::TextIndent(nsAString& aValue) {
   aValue.Truncate();
 
   const auto& textIndent = mComputedStyle->StyleText()->mTextIndent;
   if (textIndent.ConvertsToLength()) {
-    aValue.AppendFloat(textIndent.LengthInCSSPixels());
+    aValue.AppendFloat(textIndent.ToLengthInCSSPixels());
     aValue.AppendLiteral("px");
     return;
   }
   if (textIndent.ConvertsToPercentage()) {
     aValue.AppendFloat(textIndent.ToPercentage() * 100);
     aValue.AppendLiteral("%");
     return;
   }
-  // FIXME: This doesn't handle calc in any meaningful way?
+  // FIXME: This doesn't handle calc in any meaningful way? Probably should just
+  // use the Servo serialization code...
   aValue.AppendLiteral("0px");
 }
 
 void StyleInfo::Margin(Side aSide, nsAString& aValue) {
   MOZ_ASSERT(mElement->GetPrimaryFrame(),
              " mElement->GetPrimaryFrame() needs to be valid pointer");
   aValue.Truncate();
 
--- a/layout/style/ServoStyleConstsInlines.h
+++ b/layout/style/ServoStyleConstsInlines.h
@@ -541,61 +541,49 @@ LengthPercentage LengthPercentage::FromA
 }
 
 LengthPercentage LengthPercentage::FromPercentage(float aPercentage) {
   LengthPercentage l;
   l.percentage = {TAG_PERCENTAGE, {aPercentage}};
   return l;
 }
 
-CSSCoord LengthPercentage::LengthInCSSPixels() const {
-  if (IsLength()) {
-    return AsLength().ToCSSPixels();
-  }
-  if (IsPercentage()) {
-    return 0;
-  }
-  return AsCalc().length.ToCSSPixels();
-}
-
-float LengthPercentage::Percentage() const {
-  if (IsLength()) {
-    return 0;
-  }
-  if (IsPercentage()) {
-    return AsPercentage()._0;
-  }
-  return AsCalc().percentage._0;
-}
-
 bool LengthPercentage::HasPercent() const {
   return IsPercentage() || (IsCalc() && AsCalc().has_percentage);
 }
 
 bool LengthPercentage::ConvertsToLength() const { return !HasPercent(); }
 
 nscoord LengthPercentage::ToLength() const {
   MOZ_ASSERT(ConvertsToLength());
   return IsLength() ? AsLength().ToAppUnits() : AsCalc().length.ToAppUnits();
 }
 
+CSSCoord LengthPercentage::ToLengthInCSSPixels() const {
+  MOZ_ASSERT(ConvertsToLength());
+  return IsLength() ? AsLength().ToCSSPixels() : AsCalc().length.ToCSSPixels();
+}
+
 bool LengthPercentage::ConvertsToPercentage() const {
   if (IsPercentage()) {
     return true;
   }
   if (IsCalc()) {
     auto& calc = AsCalc();
     return calc.has_percentage && calc.length.IsZero();
   }
   return false;
 }
 
 float LengthPercentage::ToPercentage() const {
   MOZ_ASSERT(ConvertsToPercentage());
-  return Percentage();
+  if (IsPercentage()) {
+    return AsPercentage()._0;
+  }
+  return AsCalc().percentage._0;
 }
 
 bool LengthPercentage::HasLengthAndPercentage() const {
   return IsCalc() && !ConvertsToLength() && !ConvertsToPercentage();
 }
 
 bool LengthPercentage::IsDefinitelyZero() const {
   if (IsLength()) {
@@ -604,25 +592,32 @@ bool LengthPercentage::IsDefinitelyZero(
   if (IsPercentage()) {
     return AsPercentage()._0 == 0.0f;
   }
   auto& calc = AsCalc();
   return calc.length.IsZero() && calc.percentage._0 == 0.0f;
 }
 
 CSSCoord LengthPercentage::ResolveToCSSPixels(CSSCoord aPercentageBasis) const {
-  return LengthInCSSPixels() + Percentage() * aPercentageBasis;
+  if (IsLength()) {
+    return AsLength().ToCSSPixels();
+  }
+  if (IsPercentage()) {
+    return AsPercentage()._0 * aPercentageBasis;
+  }
+  auto& calc = AsCalc();
+  return calc.length.ToCSSPixels() + calc.percentage._0 * aPercentageBasis;
 }
 
 template <typename T>
 CSSCoord LengthPercentage::ResolveToCSSPixelsWith(T aPercentageGetter) const {
   static_assert(std::is_same<decltype(aPercentageGetter()), CSSCoord>::value,
                 "Should return CSS pixels");
   if (ConvertsToLength()) {
-    return LengthInCSSPixels();
+    return ToLengthInCSSPixels();
   }
   return ResolveToCSSPixels(aPercentageGetter());
 }
 
 template <typename T, typename U>
 nscoord LengthPercentage::Resolve(T aPercentageGetter,
                                   U aPercentageRounder) const {
   static_assert(std::is_same<decltype(aPercentageGetter()), nscoord>::value,
@@ -646,25 +641,24 @@ nscoord LengthPercentage::Resolve(T aPer
 }
 
 nscoord LengthPercentage::Resolve(nscoord aPercentageBasis) const {
   return Resolve([=] { return aPercentageBasis; }, NSToCoordFloorClamped);
 }
 
 template <typename T>
 nscoord LengthPercentage::Resolve(T aPercentageGetter) const {
-  static_assert(std::is_same<decltype(aPercentageGetter()), nscoord>::value,
-                "Should return app units");
   return Resolve(aPercentageGetter, NSToCoordFloorClamped);
 }
 
 template <typename T>
 nscoord LengthPercentage::Resolve(nscoord aPercentageBasis,
                                   T aPercentageRounder) const {
-  return Resolve([=] { return aPercentageBasis; }, aPercentageRounder);
+  return Resolve([aPercentageBasis] { return aPercentageBasis; },
+                 aPercentageRounder);
 }
 
 void LengthPercentage::ScaleLengthsBy(float aScale) {
   if (IsLength()) {
     AsLength().ScaleBy(aScale);
   }
   if (IsCalc()) {
     AsCalc().length.ScaleBy(aScale);
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -2281,31 +2281,18 @@ void nsComputedDOMStyle::SetValueToLengt
   if (aLength.ConvertsToPercentage()) {
     float result = aLength.ToPercentage();
     if (aClampNegativeCalc) {
       result = std::max(result, 0.0f);
     }
     return aValue->SetPercent(result);
   }
 
-  // TODO(emilio): This intentionally matches the serialization of
-  // SetValueToCalc, but goes against the spec. Update this when possible.
-  RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
-  nsAutoString tmp, result;
-  result.AppendLiteral("calc(");
-  val->SetAppUnits(CSSPixel::ToAppUnits(aLength.LengthInCSSPixels()));
-  val->GetCssText(tmp);
-  result.Append(tmp);
-  if (aLength.HasPercent()) {
-    result.AppendLiteral(" + ");
-    val->SetPercent(aLength.Percentage());
-    val->GetCssText(tmp);
-    result.Append(tmp);
-  }
-  result.Append(')');
+  nsAutoString result;
+  Servo_LengthPercentage_ToCss(&aLength, &result);
   aValue->SetString(result);
 }
 
 nscoord nsComputedDOMStyle::StyleCoordToNSCoord(
     const LengthPercentage& aCoord, PercentageBaseGetter aPercentageBaseGetter,
     nscoord aDefaultValue, bool aClampNegativeCalc) {
   MOZ_ASSERT(aPercentageBaseGetter, "Must have a percentage base getter");
   if (aCoord.ConvertsToLength()) {
--- a/layout/style/nsStyleUtil.cpp
+++ b/layout/style/nsStyleUtil.cpp
@@ -251,28 +251,25 @@ bool nsStyleUtil::ThreadSafeIsSignifican
 // For a replaced element whose concrete object size is no larger than the
 // element's content-box, this method checks whether the given
 // "object-position" coordinate might cause overflow in its dimension.
 static bool ObjectPositionCoordMightCauseOverflow(
     const LengthPercentage& aCoord) {
   // Any nonzero length in "object-position" can push us to overflow
   // (particularly if our concrete object size is exactly the same size as the
   // replaced element's content-box).
-  if (aCoord.LengthInCSSPixels() != 0.) {
-    return true;
+  if (!aCoord.ConvertsToPercentage()) {
+    return !aCoord.ConvertsToLength() || aCoord.ToLengthInCSSPixels() != 0.0f;
   }
 
   // Percentages are interpreted as a fraction of the extra space. So,
   // percentages in the 0-100% range are safe, but values outside of that
   // range could cause overflow.
-  if (aCoord.HasPercent() &&
-      (aCoord.Percentage() < 0.0f || aCoord.Percentage() > 1.0f)) {
-    return true;
-  }
-  return false;
+  float percentage = aCoord.ToPercentage();
+  return percentage < 0.0f || percentage > 1.0f;
 }
 
 /* static */
 bool nsStyleUtil::ObjectPropsMightCauseOverflow(
     const nsStylePosition* aStylePos) {
   auto objectFit = aStylePos->mObjectFit;
 
   // "object-fit: cover" & "object-fit: none" can give us a render rect that's
--- a/servo/ports/geckolib/cbindgen.toml
+++ b/servo/ports/geckolib/cbindgen.toml
@@ -243,17 +243,17 @@ renaming_overrides_prefixing = true
 "OriginFlags" = "OriginFlags"
 "ServoTraversalFlags" = "ServoTraversalFlags"
 "ServoStyleSetSizes" = "ServoStyleSetSizes"
 
 [export.body]
 "CSSPixelLength" = """
   inline nscoord ToAppUnits() const;
   inline bool IsZero() const;
-  float ToCSSPixels() const { return _0; }
+  CSSCoord ToCSSPixels() const { return _0; }
   inline void ScaleBy(float);
 """
 
 "LengthPercentageUnion" = """
   using Self = StyleLengthPercentageUnion;
 
   // TODO(emilio): cbindgen should be able to generate these in the body of the
   // union, but it seems it's only implemented for structs, not unions.
@@ -291,21 +291,20 @@ renaming_overrides_prefixing = true
   inline StyleCalcLengthPercentage& AsCalc();
 
   static inline Self Zero();
   static inline Self FromAppUnits(nscoord);
   static inline Self FromPixels(CSSCoord);
   static inline Self FromPercentage(float);
 
   inline void ScaleLengthsBy(float);
-  inline CSSCoord LengthInCSSPixels() const;
-  inline float Percentage() const;
   inline bool HasPercent() const;
   inline bool ConvertsToLength() const;
   inline nscoord ToLength() const;
+  inline CSSCoord ToLengthInCSSPixels() const;
   inline bool ConvertsToPercentage() const;
   inline bool HasLengthAndPercentage() const;
   inline float ToPercentage() const;
   inline bool IsDefinitelyZero() const;
   inline CSSCoord ResolveToCSSPixels(CSSCoord aPercentageBasisInCSSPixels) const;
   template<typename T> inline CSSCoord ResolveToCSSPixelsWith(T aPercentageGetter) const;
   template<typename T, typename U>
   inline nscoord Resolve(T aPercentageGetter, U aPercentRoundingFunction) const;
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -6862,8 +6862,16 @@ pub unsafe extern "C" fn Servo_StyleArcS
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn Servo_LoadData_GetLazy(
     source: &url::LoadDataSource,
 ) -> *const url::LoadData {
     source.get()
 }
+
+#[no_mangle]
+pub extern "C" fn Servo_LengthPercentage_ToCss(
+    lp: &computed::LengthPercentage,
+    result: &mut nsAString
+) {
+    lp.to_css(&mut CssWriter::new(result)).unwrap();
+}
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-logical/parsing/max-block-size-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[max-block-size-computed.html]
-  [Property max-block-size value 'calc(20% + 10px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-logical/parsing/max-inline-size-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[max-inline-size-computed.html]
-  [Property max-inline-size value 'calc(20% + 10px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-logical/parsing/min-block-size-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[min-block-size-computed.html]
-  [Property min-block-size value 'calc(20% + 10px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-logical/parsing/min-inline-size-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[min-inline-size-computed.html]
-  [Property min-inline-size value 'calc(20% + 10px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-position/parsing/bottom-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[bottom-computed.html]
-  [Property bottom value 'calc(50% + 60px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-position/parsing/left-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[left-computed.html]
-  [Property left value 'calc(50% + 60px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-position/parsing/right-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[right-computed.html]
-  [Property right value 'calc(50% + 60px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-position/parsing/top-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[top-computed.html]
-  [Property top value 'calc(50% + 60px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-sizing/parsing/max-height-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[max-height-computed.html]
-  [Property max-height value 'calc(10% + 40px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-sizing/parsing/max-width-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[max-width-computed.html]
-  [Property max-width value 'calc(10% + 40px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-sizing/parsing/min-height-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[min-height-computed.html]
-  [Property min-height value 'calc(10% + 40px)']
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-sizing/parsing/min-width-computed.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[min-width-computed.html]
-  [Property min-width value 'calc(10% + 40px)']
-    expected: FAIL
-
--- a/testing/web-platform/meta/svg/geometry/parsing/sizing-properties-computed.svg.ini
+++ b/testing/web-platform/meta/svg/geometry/parsing/sizing-properties-computed.svg.ini
@@ -1,15 +1,9 @@
 [sizing-properties-computed.svg]
-  [resolved value is computed value when display is none]
-    expected: FAIL
-
-  [resolved value is computed value when display is contents]
-    expected: FAIL
-
   [Property width value 'calc(50% + 1.5em)']
     expected: FAIL
 
   [Property height value '40%']
     expected: FAIL
 
   [Property height value 'calc(50% + 1.5em)']
     expected: FAIL