REGRESSION (r178156): CSS Parser incorrectly rejects valid calc() in padding-right...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 May 2015 20:27:50 +0000 (20:27 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 May 2015 20:27:50 +0000 (20:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144584
<rdar://problem/20796829>

Reviewed by Darin Adler.

Source/WebCore:

The CSS parser was rejecting calculated values at parsing time if it
considered the value was negative and the CSS property did not allow
negative values. However, doing so at this point will not always work
because we don't necessarily know the font-size yet (for e.g. for
calc(0.5em - 2px). Also, rejecting negative calculated values is not
the right behavior as the the specification. The specification says
we should clamp:
http://dev.w3.org/csswg/css-values-3/#calc-range

This patch updates validateCalculationUnit() to stop marking the value
as invalid if it is negative. Instead, let the CSSCalcValue's permitted
range clamp the value as needed.

This bug was causing the bottom graphic on aldentrio.com to not be
rendered properly.

Test: fast/css/negative-calc-values.html
      fast/css/padding-calc-value.html

* css/CSSParser.cpp:
(WebCore::CSSParser::validateCalculationUnit):

LayoutTests:

* fast/css/negative-calc-values-expected.txt: Added.
* fast/css/negative-calc-values.html: Added.
Add a layout test that assigns negative calc() values to properties
whose values cannot be negative to verify that values are clamped as
per the specification:
http://dev.w3.org/csswg/css-values-3/#calc-range

* fast/css/padding-calc-value-expected.txt: Added.
* fast/css/padding-calc-value.html: Added.
Add a layout test to test that using calc(.5em - 2px) for padding-right
CSS property works as intended. It used to be resolved as 0px instead
of "2*font-size - 2px".

* fast/css/text-shadow-calc-value-expected.txt:
* fast/css/text-shadow-calc-value.html:
Update test to match what the specification says:
http://dev.w3.org/csswg/css-values-3/#calc-range
"width: calc(5px - 10px);" is equivalent to "width: 0px;" since widths
smaller than 0px are not allowed.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@183765 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css/negative-calc-values-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/negative-calc-values.html [new file with mode: 0644]
LayoutTests/fast/css/padding-calc-value-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/padding-calc-value.html [new file with mode: 0644]
LayoutTests/fast/css/text-shadow-calc-value-expected.txt
LayoutTests/fast/css/text-shadow-calc-value.html
Source/WebCore/ChangeLog
Source/WebCore/css/CSSCalculationValue.h
Source/WebCore/css/CSSParser.cpp

index 1bcb383..b66cff6 100644 (file)
@@ -1,3 +1,31 @@
+2015-05-04  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r178156): CSS Parser incorrectly rejects valid calc() in padding-right property
+        https://bugs.webkit.org/show_bug.cgi?id=144584
+        <rdar://problem/20796829>
+
+        Reviewed by Darin Adler.
+
+        * fast/css/negative-calc-values-expected.txt: Added.
+        * fast/css/negative-calc-values.html: Added.
+        Add a layout test that assigns negative calc() values to properties
+        whose values cannot be negative to verify that values are clamped as
+        per the specification:
+        http://dev.w3.org/csswg/css-values-3/#calc-range
+
+        * fast/css/padding-calc-value-expected.txt: Added.
+        * fast/css/padding-calc-value.html: Added.
+        Add a layout test to test that using calc(.5em - 2px) for padding-right
+        CSS property works as intended. It used to be resolved as 0px instead
+        of "2*font-size - 2px".
+
+        * fast/css/text-shadow-calc-value-expected.txt:
+        * fast/css/text-shadow-calc-value.html:
+        Update test to match what the specification says:
+        http://dev.w3.org/csswg/css-values-3/#calc-range
+        "width: calc(5px - 10px);" is equivalent to "width: 0px;" since widths
+        smaller than 0px are not allowed.
+
 2015-05-04  Joseph Pecoraro  <pecoraro@apple.com>
 
         Unreviewed gardening. Fix lint error on mac-wk1.
diff --git a/LayoutTests/fast/css/negative-calc-values-expected.txt b/LayoutTests/fast/css/negative-calc-values-expected.txt
new file mode 100644 (file)
index 0000000..251352e
--- /dev/null
@@ -0,0 +1,29 @@
+Tests that negative calc() values are properly clamped when needed.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS testDiv.style['width'] is ""
+testDiv.style['width'] = '100px'
+testDiv.style['width'] = 'calc(5px - 10px)'
+PASS testDiv.style['width'] is "calc(-5px)"
+PASS window.getComputedStyle(testDiv).getPropertyValue('width') is "0px"
+testDiv.style['width'] = '100px'
+testDiv.style['width'] = 'calc(10% - 100px)'
+PASS testDiv.style['width'] is "calc(10% - 100px)"
+PASS window.getComputedStyle(testDiv).getPropertyValue('width') is "0px"
+testDiv.style['line-height'] = '10%'
+testDiv.style['line-height'] = 'calc(10% - 20%)'
+PASS testDiv.style['line-height'] is "calc(-10%)"
+PASS window.getComputedStyle(testDiv).getPropertyValue('line-height') is "0px"
+testPre.style['tab-size'] = '8'
+testPre.style['tab-size'] = 'calc(2 - 4)'
+PASS testPre.style['tab-size'] is "calc(-2)"
+PASS window.getComputedStyle(testPre).getPropertyValue('tab-size') is "0"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
+
+
diff --git a/LayoutTests/fast/css/negative-calc-values.html b/LayoutTests/fast/css/negative-calc-values.html
new file mode 100644 (file)
index 0000000..a5c7d9a
--- /dev/null
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<div id="testDiv" style="position: absolute;"></div>
+<pre id="testPre"><pre>
+<script>
+description("Tests that negative calc() values are properly clamped when needed.");
+
+var testDiv = document.getElementById("testDiv");
+var testPre = document.getElementById("testPre");
+
+shouldBeEmptyString("testDiv.style['width']");
+
+evalAndLog("testDiv.style['width'] = '100px'");
+evalAndLog("testDiv.style['width'] = 'calc(5px - 10px)'");
+shouldBeEqualToString("testDiv.style['width']", "calc(-5px)");
+shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('width')", "0px");
+
+evalAndLog("testDiv.style['width'] = '100px'");
+evalAndLog("testDiv.style['width'] = 'calc(10% - 100px)'");
+shouldBeEqualToString("testDiv.style['width']", "calc(10% - 100px)");
+shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('width')", "0px");
+
+evalAndLog("testDiv.style['line-height'] = '10%'");
+evalAndLog("testDiv.style['line-height'] = 'calc(10% - 20%)'");
+shouldBeEqualToString("testDiv.style['line-height']", "calc(-10%)");
+shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('line-height')", "0px");
+
+evalAndLog("testPre.style['tab-size'] = '8'");
+evalAndLog("testPre.style['tab-size'] = 'calc(2 - 4)'");
+shouldBeEqualToString("testPre.style['tab-size']", "calc(-2)");
+shouldBeEqualToString("window.getComputedStyle(testPre).getPropertyValue('tab-size')", "0");
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
diff --git a/LayoutTests/fast/css/padding-calc-value-expected.txt b/LayoutTests/fast/css/padding-calc-value-expected.txt
new file mode 100644 (file)
index 0000000..0b0d5e4
--- /dev/null
@@ -0,0 +1,10 @@
+Tests assigning a calculated value to 'padding' CSS property.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.getComputedStyle(testDiv).getPropertyValue('padding-right') is "11px"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/padding-calc-value.html b/LayoutTests/fast/css/padding-calc-value.html
new file mode 100644 (file)
index 0000000..aa039fd
--- /dev/null
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+
+<div id="testDiv" style="padding: 0 calc(.5em - 2px) 0 0; font-size: 26px;">
+</div>
+
+<script>
+description("Tests assigning a calculated value to 'padding' CSS property.");
+
+var testDiv = document.getElementById("testDiv");
+shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('padding-right')", "11px");
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
index 3126bf1..b5c2952 100644 (file)
@@ -11,8 +11,8 @@ testDiv.style['text-shadow'] = 'calc(-1 * 3px) calc(-2 * 3px) calc(3 * 3px) rgb(
 PASS testDiv.style['text-shadow'] is "rgb(255, 255, 255) calc(-3px) calc(-6px) calc(9px)"
 PASS window.getComputedStyle(testDiv).getPropertyValue('text-shadow') is "rgb(255, 255, 255) -3px -6px 9px"
 testDiv.style['text-shadow'] = 'calc(1 * 3px) calc(2 * 3px) calc(-3 * 3px) rgb(255, 255, 255)'
-PASS testDiv.style['text-shadow'] is previousStyle
-PASS window.getComputedStyle(testDiv).getPropertyValue('text-shadow') is previousComputedStyle
+PASS testDiv.style['text-shadow'] is "rgb(255, 255, 255) calc(3px) calc(6px) calc(-9px)"
+PASS window.getComputedStyle(testDiv).getPropertyValue('text-shadow') is "rgb(255, 255, 255) 3px 6px 0px"
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 517b72c..c1f86d5 100644 (file)
@@ -17,14 +17,11 @@ evalAndLog("testDiv.style['text-shadow'] = 'calc(-1 * 3px) calc(-2 * 3px) calc(3
 shouldBeEqualToString("testDiv.style['text-shadow']", "rgb(255, 255, 255) calc(-3px) calc(-6px) calc(9px)");
 shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('text-shadow')", "rgb(255, 255, 255) -3px -6px 9px")
 
-var previousStyle = testDiv.style['text-shadow'];
-var previousComputedStyle = window.getComputedStyle(testDiv).getPropertyValue('text-shadow');
-
-// Negative blur-radius is not allowed.
+// Negative blur-radius is not allowed so it should become 0.
 evalAndLog("testDiv.style['text-shadow'] = 'calc(1 * 3px) calc(2 * 3px) calc(-3 * 3px) rgb(255, 255, 255)'");
 // text-shadow should not be updated.
-shouldBe("testDiv.style['text-shadow']", "previousStyle");
-shouldBe("window.getComputedStyle(testDiv).getPropertyValue('text-shadow')", "previousComputedStyle")
+shouldBeEqualToString("testDiv.style['text-shadow']", "rgb(255, 255, 255) calc(3px) calc(6px) calc(-9px)");
+shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('text-shadow')", "rgb(255, 255, 255) 3px 6px 0px")
 
 </script>
 <script src="../../resources/js-test-post.js"></script>
index 47210a0..18df65a 100644 (file)
@@ -1,3 +1,33 @@
+2015-05-04  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r178156): CSS Parser incorrectly rejects valid calc() in padding-right property
+        https://bugs.webkit.org/show_bug.cgi?id=144584
+        <rdar://problem/20796829>
+
+        Reviewed by Darin Adler.
+
+        The CSS parser was rejecting calculated values at parsing time if it
+        considered the value was negative and the CSS property did not allow
+        negative values. However, doing so at this point will not always work
+        because we don't necessarily know the font-size yet (for e.g. for
+        calc(0.5em - 2px). Also, rejecting negative calculated values is not
+        the right behavior as the the specification. The specification says
+        we should clamp:
+        http://dev.w3.org/csswg/css-values-3/#calc-range
+
+        This patch updates validateCalculationUnit() to stop marking the value
+        as invalid if it is negative. Instead, let the CSSCalcValue's permitted
+        range clamp the value as needed.
+
+        This bug was causing the bottom graphic on aldentrio.com to not be
+        rendered properly.
+
+        Test: fast/css/negative-calc-values.html
+              fast/css/padding-calc-value.html
+
+        * css/CSSParser.cpp:
+        (WebCore::CSSParser::validateCalculationUnit):
+
 2015-05-04  Eric Carlson  <eric.carlson@apple.com>
 
         [Mac] Show wireless playback placard even when an element has custom controls
index e4d52e7..df7ceba 100644 (file)
@@ -92,7 +92,6 @@ public:
     CalculationCategory category() const { return m_expression->category(); }
     bool isInt() const { return m_expression->isInteger(); }
     double doubleValue() const;
-    bool isNegative() const { return m_expression->doubleValue() < 0; }
     bool isPositive() const { return m_expression->doubleValue() > 0; }
     double computeLengthPx(const CSSToLengthConversionData&) const;
     unsigned short primitiveType() const { return m_expression->primitiveType(); }
index 373b603..3882374 100644 (file)
@@ -1670,18 +1670,12 @@ bool CSSParser::validateCalculationUnit(ValueWithCalculation& valueWithCalculati
             isValid = true;
         if (!isValid && (unitFlags & FPositiveInteger) && calculation->isInt() && calculation->isPositive())
             isValid = true;
-        if (isValid && mustBeNonNegative && calculation->isNegative())
-            isValid = false;
         break;
     case CalcLength:
         isValid = (unitFlags & FLength);
-        if (isValid && mustBeNonNegative && calculation->isNegative())
-            isValid = false;
         break;
     case CalcPercent:
         isValid = (unitFlags & FPercent);
-        if (isValid && mustBeNonNegative && calculation->isNegative())
-            isValid = false;
         break;
     case CalcPercentLength:
         isValid = (unitFlags & FPercent) && (unitFlags & FLength);