Bug 1527392 - Do not clamp computed width and height by min-/max- values. r=mats
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 11 May 2019 18:01:50 +0000
changeset 532360 d51f3432e142ef24333b125087f5fccc2fbc366a
parent 532359 6103006e3172f0ef258a30256b8cc7a6553e7f14
child 532361 411f5783f0466ee478325b2b32f3d13d74f077ec
child 532366 d8bd0fd8057945c2798d0cc489bd84a50607da95
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1527392, 197814
milestone68.0a1
first release with
nightly linux32
d51f3432e142 / 68.0a1 / 20190512093034 / files
nightly linux64
d51f3432e142 / 68.0a1 / 20190512093034 / files
nightly mac
d51f3432e142 / 68.0a1 / 20190512093034 / files
nightly win32
d51f3432e142 / 68.0a1 / 20190512093034 / files
nightly win64
d51f3432e142 / 68.0a1 / 20190512093034 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1527392 - Do not clamp computed width and height by min-/max- values. r=mats The spec says that when there's no box or the property doesn't apply, the computed value should be returned. That's not what we're doing right now, we're clamping by min-/max- values, which is wrong. This patch makes us match other engines and the spec, and it's an attempt to get interop on resolved values in: https://github.com/w3c/csswg-drafts/issues/3678 WebKit fails the WPT test, but due to a different reason: https://bugs.webkit.org/show_bug.cgi?id=197814 Differential Revision: https://phabricator.services.mozilla.com/D30780
layout/generic/test/test_bug522632.html
layout/style/nsComputedDOMStyle.cpp
layout/style/nsComputedDOMStyle.h
layout/style/test/test_bug1292447.html
layout/style/test/test_bug391221.html
testing/web-platform/tests/css/cssom/getComputedStyle-resolved-min-max-clamping.html
--- a/layout/generic/test/test_bug522632.html
+++ b/layout/generic/test/test_bug522632.html
@@ -10,17 +10,17 @@ https://bugzilla.mozilla.org/show_bug.cg
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=522632">Mozilla Bug 522632</a>
 <p id="display"></p>
 <div id="content">
   <table border="1">
     <tr>
       <td style="height: 200px; padding: 0; margin: 0; border: none">
-        <span style="height: 0; min-height: 50%" id="foo"></span>
+        <div style="height: 0; min-height: 50%" id="foo"></div>
       </td>
     <tr>
   </table>  
 </div>
 <pre id="test">
 <script type="application/javascript">
 
 /** Test for Bug 522632 **/
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -1837,27 +1837,17 @@ already_AddRefed<CSSValue> nsComputedDOM
   }
 
   if (calcHeight) {
     AssertFlushedPendingReflows();
     nsMargin adjustedValues = GetAdjustedValuesForBoxSizing();
     val->SetAppUnits(mInnerFrame->GetContentRect().height +
                      adjustedValues.TopBottom());
   } else {
-    const nsStylePosition* positionData = StylePosition();
-
-    nscoord minHeight =
-        StyleCoordToNSCoord(positionData->mMinHeight,
-                            &nsComputedDOMStyle::GetCBContentHeight, 0, true);
-
-    nscoord maxHeight = StyleCoordToNSCoord(
-        positionData->mMaxHeight, &nsComputedDOMStyle::GetCBContentHeight,
-        nscoord_MAX, true);
-
-    SetValueToSize(val, positionData->mHeight, minHeight, maxHeight);
+    SetValueToSize(val, StylePosition()->mHeight);
   }
 
   return val.forget();
 }
 
 already_AddRefed<CSSValue> nsComputedDOMStyle::DoGetWidth() {
   RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
 
@@ -1874,27 +1864,17 @@ already_AddRefed<CSSValue> nsComputedDOM
   }
 
   if (calcWidth) {
     AssertFlushedPendingReflows();
     nsMargin adjustedValues = GetAdjustedValuesForBoxSizing();
     val->SetAppUnits(mInnerFrame->GetContentRect().width +
                      adjustedValues.LeftRight());
   } else {
-    const nsStylePosition* positionData = StylePosition();
-
-    nscoord minWidth =
-        StyleCoordToNSCoord(positionData->mMinWidth,
-                            &nsComputedDOMStyle::GetCBContentWidth, 0, true);
-
-    nscoord maxWidth = StyleCoordToNSCoord(
-        positionData->mMaxWidth, &nsComputedDOMStyle::GetCBContentWidth,
-        nscoord_MAX, true);
-
-    SetValueToSize(val, positionData->mWidth, minWidth, maxWidth);
+    SetValueToSize(val, StylePosition()->mWidth);
   }
 
   return val.forget();
 }
 
 already_AddRefed<CSSValue> nsComputedDOMStyle::DoGetMaxHeight() {
   RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
   SetValueToMaxSize(val, StylePosition()->mMaxHeight);
@@ -2214,27 +2194,25 @@ void nsComputedDOMStyle::SetValueToExtre
       return aValue->SetIdent(eCSSKeyword__moz_available);
     case StyleExtremumLength::MozFitContent:
       return aValue->SetIdent(eCSSKeyword__moz_fit_content);
   }
   MOZ_ASSERT_UNREACHABLE("Unknown extremum length?");
 }
 
 void nsComputedDOMStyle::SetValueToSize(nsROCSSPrimitiveValue* aValue,
-                                        const StyleSize& aSize, nscoord aMin,
-                                        nscoord aMax) {
+                                        const StyleSize& aSize) {
   if (aSize.IsAuto()) {
     return aValue->SetIdent(eCSSKeyword_auto);
   }
   if (aSize.IsExtremumLength()) {
     return SetValueToExtremumLength(aValue, aSize.AsExtremumLength());
   }
   MOZ_ASSERT(aSize.IsLengthPercentage());
-  SetValueToLengthPercentage(aValue, aSize.AsLengthPercentage(), true, aMin,
-                             aMax);
+  SetValueToLengthPercentage(aValue, aSize.AsLengthPercentage(), true);
 }
 
 void nsComputedDOMStyle::SetValueToMaxSize(nsROCSSPrimitiveValue* aValue,
                                            const StyleMaxSize& aSize) {
   if (aSize.IsNone()) {
     return aValue->SetIdent(eCSSKeyword_none);
   }
   if (aSize.IsExtremumLength()) {
@@ -2251,24 +2229,23 @@ void nsComputedDOMStyle::SetValueToLengt
     return aValue->SetIdent(eCSSKeyword_auto);
   }
   SetValueToLengthPercentage(aValue, aSize.AsLengthPercentage(),
                              aClampNegativeCalc);
 }
 
 void nsComputedDOMStyle::SetValueToLengthPercentage(
     nsROCSSPrimitiveValue* aValue, const mozilla::LengthPercentage& aLength,
-    bool aClampNegativeCalc, nscoord aMin, nscoord aMax) {
+    bool aClampNegativeCalc) {
   if (aLength.ConvertsToLength()) {
     nscoord result = aLength.ToLength();
     if (aClampNegativeCalc) {
       result = std::max(result, 0);
     }
-    // TODO(emilio, bug 1527392): It's wrong to clamp here.
-    return aValue->SetAppUnits(std::max(aMin, std::min(result, aMax)));
+    return aValue->SetAppUnits(result);
   }
   if (aLength.ConvertsToPercentage()) {
     float result = aLength.ToPercentage();
     if (aClampNegativeCalc) {
       result = std::max(result, 0.0f);
     }
     return aValue->SetPercent(result);
   }
@@ -2289,17 +2266,17 @@ void nsComputedDOMStyle::SetValueToLengt
   }
   result.Append(')');
   aValue->SetString(result);
 }
 
 void nsComputedDOMStyle::SetValueToCoord(
     nsROCSSPrimitiveValue* aValue, const nsStyleCoord& aCoord,
     bool aClampNegativeCalc, PercentageBaseGetter aPercentageBaseGetter,
-    const KTableEntry aTable[], nscoord aMinAppUnits, nscoord aMaxAppUnits) {
+    const KTableEntry aTable[]) {
   MOZ_ASSERT(aValue, "Must have a value to work with");
 
   switch (aCoord.GetUnit()) {
     case eStyleUnit_Normal:
       aValue->SetIdent(eCSSKeyword_normal);
       break;
 
     case eStyleUnit_Auto:
@@ -2307,30 +2284,29 @@ void nsComputedDOMStyle::SetValueToCoord
       break;
 
     case eStyleUnit_Percent: {
       nscoord percentageBase;
       if (aPercentageBaseGetter &&
           (this->*aPercentageBaseGetter)(percentageBase)) {
         nscoord val =
             NSCoordSaturatingMultiply(percentageBase, aCoord.GetPercentValue());
-        aValue->SetAppUnits(
-            std::max(aMinAppUnits, std::min(val, aMaxAppUnits)));
+        aValue->SetAppUnits(val);
       } else {
         aValue->SetPercent(aCoord.GetPercentValue());
       }
     } break;
 
     case eStyleUnit_Factor:
       aValue->SetNumber(aCoord.GetFactorValue());
       break;
 
     case eStyleUnit_Coord: {
       nscoord val = aCoord.GetCoordValue();
-      aValue->SetAppUnits(std::max(aMinAppUnits, std::min(val, aMaxAppUnits)));
+      aValue->SetAppUnits(val);
     } break;
 
     case eStyleUnit_Integer:
       aValue->SetNumber(aCoord.GetIntValue());
       break;
 
     case eStyleUnit_Enumerated:
       NS_ASSERTION(aTable, "Must have table to handle this case");
@@ -2345,27 +2321,25 @@ void nsComputedDOMStyle::SetValueToCoord
     case eStyleUnit_Calc:
       nscoord percentageBase;
       if (!aCoord.CalcHasPercent()) {
         nscoord val = aCoord.ComputeCoordPercentCalc(0);
         if (aClampNegativeCalc && val < 0) {
           MOZ_ASSERT(aCoord.IsCalcUnit(), "parser should have rejected value");
           val = 0;
         }
-        aValue->SetAppUnits(
-            std::max(aMinAppUnits, std::min(val, aMaxAppUnits)));
+        aValue->SetAppUnits(val);
       } else if (aPercentageBaseGetter &&
                  (this->*aPercentageBaseGetter)(percentageBase)) {
         nscoord val = aCoord.ComputeCoordPercentCalc(percentageBase);
         if (aClampNegativeCalc && val < 0) {
           MOZ_ASSERT(aCoord.IsCalcUnit(), "parser should have rejected value");
           val = 0;
         }
-        aValue->SetAppUnits(
-            std::max(aMinAppUnits, std::min(val, aMaxAppUnits)));
+        aValue->SetAppUnits(val);
       } else {
         nsStyleCoord::Calc* calc = aCoord.GetCalcValue();
         SetValueToCalc(calc, aValue);
       }
       break;
 
     case eStyleUnit_Degree:
       aValue->SetDegree(aCoord.GetAngleValue());
--- a/layout/style/nsComputedDOMStyle.h
+++ b/layout/style/nsComputedDOMStyle.h
@@ -290,29 +290,25 @@ class nsComputedDOMStyle final : public 
   /* Helper functions */
   void SetValueFromComplexColor(nsROCSSPrimitiveValue* aValue,
                                 const mozilla::StyleColor& aColor);
   void SetValueToPosition(const mozilla::Position& aPosition,
                           nsDOMCSSValueList* aValueList);
   void SetValueToURLValue(const mozilla::css::URLValue* aURL,
                           nsROCSSPrimitiveValue* aValue);
 
-  void SetValueToSize(nsROCSSPrimitiveValue* aValue, const mozilla::StyleSize&,
-                      nscoord aMinAppUnits = nscoord_MIN,
-                      nscoord aMaxAppUnits = nscoord_MAX);
+  void SetValueToSize(nsROCSSPrimitiveValue* aValue, const mozilla::StyleSize&);
 
   void SetValueToLengthPercentageOrAuto(nsROCSSPrimitiveValue* aValue,
                                         const LengthPercentageOrAuto&,
                                         bool aClampNegativeCalc);
 
   void SetValueToLengthPercentage(nsROCSSPrimitiveValue* aValue,
                                   const LengthPercentage&,
-                                  bool aClampNegativeCalc,
-                                  nscoord aMinAppUnits = nscoord_MIN,
-                                  nscoord aMaxAppUnits = nscoord_MAX);
+                                  bool aClampNegativeCalc);
 
   void SetValueToMaxSize(nsROCSSPrimitiveValue* aValue, const StyleMaxSize&);
 
   void SetValueToExtremumLength(nsROCSSPrimitiveValue* aValue,
                                 StyleExtremumLength);
 
   /**
    * Method to set aValue to aCoord.  If aCoord is a percentage value and
@@ -326,19 +322,17 @@ class nsComputedDOMStyle final : public 
    * aMaxAppUnits.
    *
    * XXXbz should caller pass in some sort of bitfield indicating which units
    * can be expected or something?
    */
   void SetValueToCoord(nsROCSSPrimitiveValue* aValue,
                        const nsStyleCoord& aCoord, bool aClampNegativeCalc,
                        PercentageBaseGetter aPercentageBaseGetter = nullptr,
-                       const KTableEntry aTable[] = nullptr,
-                       nscoord aMinAppUnits = nscoord_MIN,
-                       nscoord aMaxAppUnits = nscoord_MAX);
+                       const KTableEntry aTable[] = nullptr);
 
   /**
    * If aCoord is a eStyleUnit_Coord returns the nscoord.  If it's
    * eStyleUnit_Percent, attempts to resolve the percentage base and returns
    * the resulting nscoord.  If it's some other unit or a percentage base can't
    * be determined, returns aDefaultValue.
    */
   nscoord StyleCoordToNSCoord(const LengthPercentage& aCoord,
--- a/layout/style/test/test_bug1292447.html
+++ b/layout/style/test/test_bug1292447.html
@@ -231,19 +231,19 @@ doATest("width", "minwidth4-", 600, 0, t
 doATest("width", "maxwidth1-", 320, 0, true);
 doATest("width", "maxwidth2-", 400, 0, true);
 doATest("width", "maxwidth3-", 320, 0, true);
 doATest("width", "maxwidth4-", 440, 0, true);
 doATest("width", "minmaxwidth1-", 320, 0, true);
 doATest("width", "minmaxwidth2-", 320, 0, true);
 doATest("width", "minmaxwidth3-", 600, 0, true);
 doATest("width", "minmaxwidth4-", 600, 0, true);
-doATest("width", "minmaxwidth5-", 320, 0, true);
-doATest("width", "minmaxwidth6-", 320, 0, true);
-doATest("width", "minmaxwidth7-", 600, 0, true);
+doATest("width", "minmaxwidth5-", 400, 0, true);
+doATest("width", "minmaxwidth6-", 400, 0, true);
+doATest("width", "minmaxwidth7-", 400, 0, true);
 doATest("width", "minmaxwidth8-", 320, 0, true);
 doATest("width", "minmaxwidth9-", 320, 0, true);
 doATest("width", "minmaxwidth10-", 600, 0, true);
 doATest("width", "minmaxwidth11-", 600, 0, true);
 
 doATest("min-height", "minheight1-", 200, 25);
 doATest("min-height", "minheight2-", 600, 75);
 doATest("max-height", "maxheight1-", 320, 40);
@@ -263,19 +263,19 @@ doATest("height", "minheight4-", 600, 0,
 doATest("height", "maxheight1-", 320, 0, true);
 doATest("height", "maxheight2-", 400, 0, true);
 doATest("height", "maxheight3-", 0, 0, true);
 doATest("height", "maxheight4-", 0, 0, true);
 doATest("height", "minmaxheight1-", 320, 0, true);
 doATest("height", "minmaxheight2-", 320, 0, true);
 doATest("height", "minmaxheight3-", 600, 0, true);
 doATest("height", "minmaxheight4-", 600, 0, true);
-doATest("height", "minmaxheight5-", 320, 0, true);
-doATest("height", "minmaxheight6-", 320, 0, true);
-doATest("height", "minmaxheight7-", 600, 0, true);
+doATest("height", "minmaxheight5-", 400, 0, true);
+doATest("height", "minmaxheight6-", 400, 0, true);
+doATest("height", "minmaxheight7-", 400, 0, true);
 doATest("height", "minmaxheight8-", 200, 0, true);
 doATest("height", "minmaxheight9-", 200, 0, true);
 doATest("height", "minmaxheight10-", 600, 0, true);
 doATest("height", "minmaxheight11-", 600, 0, true);
 
 function style(id) {
   return document.defaultView.getComputedStyle($(id));
 }
--- a/layout/style/test/test_bug391221.html
+++ b/layout/style/test/test_bug391221.html
@@ -8,17 +8,17 @@ https://bugzilla.mozilla.org/show_bug.cg
   <script src="/tests/SimpleTest/SimpleTest.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=391221">Mozilla Bug 391221</a>
 <p id="display">
   <div id="width-ref" style="width: 2ch"></div>
 </p>
-<div id="content" style="display: none">
+<div id="content">
 
 <div id="one" style="width: 1000px; max-width: 2ch"></div>
 <div id="two" style="width: 0px; min-width: 2ch"></div>
 <div id="three" style="width: 1000ch; max-width: 2px"></div>
   
 </div>
 <pre id="test">
 <script class="testbody" type="text/javascript">
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom/getComputedStyle-resolved-min-max-clamping.html
@@ -0,0 +1,38 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSSOM: resolved values of the width and height when the element generates no box or are a non-replaced inline aren't clamped by min-width / max-width</title>
+<link rel="help" href="https://drafts.csswg.org/cssom/#resolved-value">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<span id="non-replaced-inline"></span>
+<div id="display-none" style="display: none"></div>
+<div id="display-contents" style="display: contents"></div>
+<script>
+test(function() {
+  for (const e of document.querySelectorAll("[id]")) {
+    const cs = getComputedStyle(e);
+    for (const prop of ["width", "height"]) {
+      e.style.setProperty("min-" + prop, "10px");
+      e.style.setProperty("max-" + prop, "50px");
+
+      e.style.setProperty(prop, "10%");
+      assert_equals(cs[prop], "10%", `${e.id}: ${prop} with percentages returns percentages`);
+
+      e.style.setProperty(prop, "15px");
+      assert_equals(cs[prop], "15px", `${e.id}: ${prop} with value in range returns computed value`);
+
+      e.style.setProperty(prop, "1px");
+      assert_equals(cs[prop], "1px", `${e.id}: ${prop} with value out of range isn't clamped by min-${prop}`);
+
+      e.style.setProperty(prop, "60px");
+      assert_equals(cs[prop], "60px", `${e.id}: ${prop} with value out of range isn't clamped by max-${prop}`);
+
+      e.style.removeProperty(prop);
+      e.style.removeProperty("min-" + prop);
+      e.style.removeProperty("max-" + prop);
+    }
+  }
+}, "Resolved value of width / height when there's no used value isn't clamped by min/max properties");
+</script>