From 8308eb7ea14318f53b55f3289c2bb9b712265318 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?=
Date: Sat, 11 May 2019 18:01:50 +0000
Subject: [PATCH] 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
--HG--
extra : moz-landing-system : lando
---
layout/generic/test/test_bug522632.html | 2 +-
layout/style/nsComputedDOMStyle.cpp | 48 +++++--------------
layout/style/nsComputedDOMStyle.h | 12 ++---
layout/style/test/test_bug1292447.html | 12 ++---
layout/style/test/test_bug391221.html | 2 +-
...mputedStyle-resolved-min-max-clamping.html | 38 +++++++++++++++
6 files changed, 60 insertions(+), 54 deletions(-)
create mode 100644 testing/web-platform/tests/css/cssom/getComputedStyle-resolved-min-max-clamping.html
diff --git a/layout/generic/test/test_bug522632.html b/layout/generic/test/test_bug522632.html
index e9a7b01c6430..8a2614045c9d 100644
--- a/layout/generic/test/test_bug522632.html
+++ b/layout/generic/test/test_bug522632.html
@@ -15,7 +15,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=522632
diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
index ec93f6640e4d..8ab1b02216df 100644
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -1842,17 +1842,7 @@ already_AddRefed nsComputedDOMStyle::DoGetHeight() {
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();
@@ -1879,17 +1869,7 @@ already_AddRefed nsComputedDOMStyle::DoGetWidth() {
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();
@@ -2219,8 +2199,7 @@ void nsComputedDOMStyle::SetValueToExtremumLength(nsROCSSPrimitiveValue* aValue,
}
void nsComputedDOMStyle::SetValueToSize(nsROCSSPrimitiveValue* aValue,
- const StyleSize& aSize, nscoord aMin,
- nscoord aMax) {
+ const StyleSize& aSize) {
if (aSize.IsAuto()) {
return aValue->SetIdent(eCSSKeyword_auto);
}
@@ -2228,8 +2207,7 @@ void nsComputedDOMStyle::SetValueToSize(nsROCSSPrimitiveValue* aValue,
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,
@@ -2256,14 +2234,13 @@ void nsComputedDOMStyle::SetValueToLengthPercentageOrAuto(
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();
@@ -2294,7 +2271,7 @@ void nsComputedDOMStyle::SetValueToLengthPercentage(
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()) {
@@ -2312,8 +2289,7 @@ void nsComputedDOMStyle::SetValueToCoord(
(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());
}
@@ -2325,7 +2301,7 @@ void nsComputedDOMStyle::SetValueToCoord(
case eStyleUnit_Coord: {
nscoord val = aCoord.GetCoordValue();
- aValue->SetAppUnits(std::max(aMinAppUnits, std::min(val, aMaxAppUnits)));
+ aValue->SetAppUnits(val);
} break;
case eStyleUnit_Integer:
@@ -2350,8 +2326,7 @@ void nsComputedDOMStyle::SetValueToCoord(
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);
@@ -2359,8 +2334,7 @@ void nsComputedDOMStyle::SetValueToCoord(
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);
diff --git a/layout/style/nsComputedDOMStyle.h b/layout/style/nsComputedDOMStyle.h
index 0ffe239b7d87..d3c2415ad7b9 100644
--- a/layout/style/nsComputedDOMStyle.h
+++ b/layout/style/nsComputedDOMStyle.h
@@ -295,9 +295,7 @@ class nsComputedDOMStyle final : public nsDOMCSSDeclaration,
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&,
@@ -305,9 +303,7 @@ class nsComputedDOMStyle final : public nsDOMCSSDeclaration,
void SetValueToLengthPercentage(nsROCSSPrimitiveValue* aValue,
const LengthPercentage&,
- bool aClampNegativeCalc,
- nscoord aMinAppUnits = nscoord_MIN,
- nscoord aMaxAppUnits = nscoord_MAX);
+ bool aClampNegativeCalc);
void SetValueToMaxSize(nsROCSSPrimitiveValue* aValue, const StyleMaxSize&);
@@ -331,9 +327,7 @@ class nsComputedDOMStyle final : public nsDOMCSSDeclaration,
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
diff --git a/layout/style/test/test_bug1292447.html b/layout/style/test/test_bug1292447.html
index 925ff1dd62f8..2ca4d5ec905b 100644
--- a/layout/style/test/test_bug1292447.html
+++ b/layout/style/test/test_bug1292447.html
@@ -236,9 +236,9 @@ 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);
@@ -268,9 +268,9 @@ 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);
diff --git a/layout/style/test/test_bug391221.html b/layout/style/test/test_bug391221.html
index 2bc9a107405f..bf787111971a 100644
--- a/layout/style/test/test_bug391221.html
+++ b/layout/style/test/test_bug391221.html
@@ -13,7 +13,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=391221
-
+
diff --git a/testing/web-platform/tests/css/cssom/getComputedStyle-resolved-min-max-clamping.html b/testing/web-platform/tests/css/cssom/getComputedStyle-resolved-min-max-clamping.html
new file mode 100644
index 000000000000..e630377c3335
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom/getComputedStyle-resolved-min-max-clamping.html
@@ -0,0 +1,38 @@
+
+
+
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
+
+
+
+
+
+
+
+
+