Skip to content

Commit

Permalink
Only allow calc() combinations as per spec.
Browse files Browse the repository at this point in the history
We allowed calc expressions combining numbers and lengths, numbers and
percentages, and even a combination of numbers, percentages, and
lengths. None of those are allowed per the CSS Values and Units spec,
and it made it possible to combine user units, lengths, and percentages
into the same calc() for certain SVG CSS properties and attributes.

See also: w3c/csswg-drafts#4874

This improves SVG interop with Firefox. Firefox does not support calc()
for numbers, though.

Bug: 1061714
Change-Id: I9e5c492122242e51064310a40e28a233530e357c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2134251
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#756226}
  • Loading branch information
Rune Lillesveen authored and Commit Bot committed Apr 3, 2020
1 parent df7b138 commit 7377d9c
Show file tree
Hide file tree
Showing 17 changed files with 59 additions and 84 deletions.
49 changes: 13 additions & 36 deletions third_party/blink/renderer/core/css/css_math_expression_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,7 @@ double CSSMathExpressionNumericLiteral::ComputeLengthPx(
case kCalcAngle:
case kCalcFrequency:
case kCalcPercentLength:
case kCalcPercentNumber:
case kCalcTime:
case kCalcLengthNumber:
case kCalcPercentLengthNumber:
case kCalcOther:
NOTREACHED();
break;
Expand Down Expand Up @@ -285,43 +282,26 @@ bool CSSMathExpressionNumericLiteral::InvolvesPercentageComparisons() const {
// ------ End of CSSMathExpressionNumericLiteral member functions

static const CalculationCategory kAddSubtractResult[kCalcOther][kCalcOther] = {
/* CalcNumber */ {kCalcNumber, kCalcLengthNumber, kCalcPercentNumber,
kCalcPercentNumber, kCalcOther, kCalcOther, kCalcOther,
kCalcOther, kCalcLengthNumber, kCalcPercentLengthNumber},
/* CalcNumber */ {kCalcNumber, kCalcOther, kCalcOther, kCalcOther,
kCalcOther, kCalcOther, kCalcOther},
/* CalcLength */
{kCalcLengthNumber, kCalcLength, kCalcPercentLength, kCalcOther,
kCalcPercentLength, kCalcOther, kCalcOther, kCalcOther, kCalcLengthNumber,
kCalcPercentLengthNumber},
{kCalcOther, kCalcLength, kCalcPercentLength, kCalcPercentLength,
kCalcOther, kCalcOther, kCalcOther},
/* CalcPercent */
{kCalcPercentNumber, kCalcPercentLength, kCalcPercent, kCalcPercentNumber,
kCalcPercentLength, kCalcOther, kCalcOther, kCalcOther,
kCalcPercentLengthNumber, kCalcPercentLengthNumber},
/* CalcPercentNumber */
{kCalcPercentNumber, kCalcPercentLengthNumber, kCalcPercentNumber,
kCalcPercentNumber, kCalcPercentLengthNumber, kCalcOther, kCalcOther,
kCalcOther, kCalcOther, kCalcPercentLengthNumber},
{kCalcOther, kCalcPercentLength, kCalcPercent, kCalcPercentLength,
kCalcOther, kCalcOther, kCalcOther},
/* CalcPercentLength */
{kCalcPercentLengthNumber, kCalcPercentLength, kCalcPercentLength,
kCalcPercentLengthNumber, kCalcPercentLength, kCalcOther, kCalcOther,
kCalcOther, kCalcOther, kCalcPercentLengthNumber},
{kCalcOther, kCalcPercentLength, kCalcPercentLength, kCalcPercentLength,
kCalcOther, kCalcOther, kCalcOther},
/* CalcAngle */
{kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcAngle,
kCalcOther, kCalcOther, kCalcOther, kCalcOther},
{kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcAngle, kCalcOther,
kCalcOther},
/* CalcTime */
{kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcOther,
kCalcTime, kCalcOther, kCalcOther, kCalcOther},
{kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcTime,
kCalcOther},
/* CalcFrequency */
{kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcOther, kCalcOther,
kCalcOther, kCalcFrequency, kCalcOther, kCalcOther},
/* CalcLengthNumber */
{kCalcLengthNumber, kCalcLengthNumber, kCalcPercentLengthNumber,
kCalcPercentLengthNumber, kCalcPercentLengthNumber, kCalcOther, kCalcOther,
kCalcOther, kCalcLengthNumber, kCalcPercentLengthNumber},
/* CalcPercentLengthNumber */
{kCalcPercentLengthNumber, kCalcPercentLengthNumber,
kCalcPercentLengthNumber, kCalcPercentLengthNumber,
kCalcPercentLengthNumber, kCalcOther, kCalcOther, kCalcOther,
kCalcPercentLengthNumber, kCalcPercentLengthNumber}};
kCalcFrequency}};

static CalculationCategory DetermineCategory(
const CSSMathExpressionNode& left_side,
Expand Down Expand Up @@ -729,9 +709,6 @@ CSSPrimitiveValue::UnitType CSSMathExpressionBinaryOperation::ResolvedUnitType()
case kCalcFrequency:
return CSSPrimitiveValue::UnitType::kHertz;
case kCalcPercentLength:
case kCalcPercentNumber:
case kCalcLengthNumber:
case kCalcPercentLengthNumber:
case kCalcOther:
return CSSPrimitiveValue::UnitType::kUnknown;
}
Expand Down
11 changes: 3 additions & 8 deletions third_party/blink/renderer/core/css/css_math_expression_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,14 @@ class CSSNumericLiteralValue;
// The order of this enum should not change since its elements are used as
// indices in the addSubtractResult matrix.
enum CalculationCategory {
kCalcNumber = 0,
kCalcNumber,
kCalcLength,
kCalcPercent,
kCalcPercentNumber,
kCalcPercentLength,
kCalcAngle,
kCalcTime,
kCalcFrequency,
kCalcLengthNumber,
kCalcPercentLengthNumber,
kCalcOther
kCalcOther,
};

class CORE_EXPORT CSSMathExpressionNode
Expand Down Expand Up @@ -121,9 +118,7 @@ class CORE_EXPORT CSSMathExpressionNode

CalculationCategory Category() const { return category_; }
bool HasPercentage() const {
return category_ == kCalcPercent || category_ == kCalcPercentNumber ||
category_ == kCalcPercentLength ||
category_ == kCalcPercentLengthNumber;
return category_ == kCalcPercent || category_ == kCalcPercentLength;
}

// Returns the unit type of the math expression *without doing any type
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,18 +470,9 @@ CSSPrimitiveValue* ConsumeAlphaValue(CSSParserTokenRange& range,

bool CanConsumeCalcValue(CalculationCategory category,
CSSParserMode css_parser_mode) {
if (category == kCalcLength || category == kCalcPercent ||
category == kCalcPercentLength)
return true;

if (css_parser_mode != kSVGAttributeMode)
return false;

if (category == kCalcNumber || category == kCalcPercentNumber ||
category == kCalcLengthNumber || category == kCalcPercentLengthNumber)
return true;

return false;
return category == kCalcLength || category == kCalcPercent ||
category == kCalcPercentLength ||
(css_parser_mode == kSVGAttributeMode && category == kCalcNumber);
}

CSSPrimitiveValue* ConsumeLengthOrPercent(CSSParserTokenRange& range,
Expand Down Expand Up @@ -512,16 +503,7 @@ bool IsNonZeroUserUnitsValue(const CSSPrimitiveValue* value) {
value->GetDoubleValue() != 0;
}
const auto& math_value = To<CSSMathFunctionValue>(*value);
switch (math_value.Category()) {
case kCalcNumber:
return math_value.DoubleValue() != 0;
case kCalcPercentNumber:
case kCalcLengthNumber:
case kCalcPercentLengthNumber:
return true;
default:
return false;
}
return math_value.Category() == kCalcNumber && math_value.DoubleValue() != 0;
}

} // namespace
Expand Down
3 changes: 0 additions & 3 deletions third_party/blink/renderer/core/svg/svg_length.cc
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,7 @@ static bool IsSupportedCalculationCategory(CalculationCategory category) {
case kCalcLength:
case kCalcNumber:
case kCalcPercent:
case kCalcPercentNumber:
case kCalcPercentLength:
case kCalcLengthNumber:
case kCalcPercentLengthNumber:
return true;
default:
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@
from: '0%',
to: '100'
}, [
{at: -0.25, is: 'calc(0% + -25)'},
{at: -0.25, is: 'calc(0% + -25px)'},
{at: 0, is: '0%'},
{at: 0.25, is: 'calc(0% + 25)'},
{at: 0.5, is: 'calc(0% + 50)'},
{at: 0.75, is: 'calc(0% + 75)'},
{at: 0.25, is: 'calc(0% + 25px)'},
{at: 0.5, is: 'calc(0% + 50px)'},
{at: 0.75, is: 'calc(0% + 75px)'},
{at: 1, is: '100px'},
{at: 1.25, is: 'calc(0% + 125)'}
{at: 1.25, is: 'calc(0% + 125px)'}
]);
assertAttributeInterpolation({
property: 'cy',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
<!-- an1: Change width from 10 to 50 in 4s -->
<!-- an2: Change width from 10 to 100 in 4s starting at 5s -->
<rect width="10" height="100" fill="green">
<animate id="an1" attributeType="XML" attributeName="width" fill="remove" by="calc(4% + 8)" begin="0s" dur="4s"/>
<animate id="an2" attributeType="XML" attributeName="width" additive="replace" fill="freeze" by="calc(10% + 10)" begin="5s" dur="4s"/>
<animate id="an1" attributeType="XML" attributeName="width" fill="remove" by="calc(4% + 8px)" begin="0s" dur="4s"/>
<animate id="an2" attributeType="XML" attributeName="width" additive="replace" fill="freeze" by="calc(10% + 10px)" begin="5s" dur="4s"/>
</rect>

</svg>
Expand Down Expand Up @@ -73,4 +73,4 @@

window.animationStartsImmediately = true;

</script>
</script>
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,8 @@ PASS e.style['stroke-dashoffset'] = "-20%" should set the property value
FAIL e.style['stroke-dashoffset'] = "30" should set the property value assert_equals: serialization should be canonical expected "30px" but got "30"
PASS e.style['stroke-dashoffset'] = "40Q" should set the property value
PASS e.style['stroke-dashoffset'] = "calc(2em + 3ex)" should set the property value
PASS e.style['stroke-dashoffset'] = "calc(3)" should set the property value
PASS e.style['stroke-dashoffset'] = "calc(2 + 1)" should set the property value
PASS e.style['stroke-dashoffset'] = "calc(2 + (7 - 5))" should set the property value
Harness: the test ran to completion.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,8 @@ PASS e.style['stroke-width'] = "calc(2em + 3ex)" should set the property value
PASS e.style['stroke-width'] = "4%" should set the property value
PASS e.style['stroke-width'] = "5vmin" should set the property value
PASS e.style['stroke-width'] = "calc(50% + 60px)" should set the property value
PASS e.style['stroke-width'] = "calc(3)" should set the property value
PASS e.style['stroke-width'] = "calc(2 + 1)" should set the property value
PASS e.style['stroke-width'] = "calc(2 + (7 - 5))" should set the property value
Harness: the test ran to completion.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@
body { zoom: 200%; }
</style>
<svg id="svg" width="500" height="500" viewBox='0 0 1000 1000'>
<rect width='calc(50px + 50)' height='100' fill='green'/>
<rect width='calc(50px + 50px)' height='100' fill='green'/>
<rect x='110' width='calc(50 + 50)' height='100' fill='green'/>
</svg>
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 5 additions & 5 deletions third_party/blink/web_tests/svg/dom/SVGLength-calc-in-attr.html
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,10 @@
assert_calc_expression("calc(10mm + 10mm)", (20 * cssPixelsPerMillimeter));
assert_calc_expression("calc(20mm)", (20 * cssPixelsPerMillimeter));
assert_calc_expression("calc(10 + 10)", 20);
assert_calc_expression("calc(10mm + 10)", (10 * cssPixelsPerMillimeter) + 10);
assert_calc_expression("calc(10% + 10)", (10 * viewportWidthPercent()) + 10);
assert_calc_expression("calc(1cm + 2in + 1cm + 2)", (2 * cssPixelsPerInch) + (2 * cssPixelsPerCentimeter) + 2);
assert_calc_expression("calc(1cm + 2 + 1cm + 2in)", (2 * cssPixelsPerInch) + (2 * cssPixelsPerCentimeter) + 2);
assert_calc_expression("calc(10% + 10 + 2% + 10pc)", (12 * viewportWidthPercent()) + 10 + (10 * cssPixelsPerPica));
assert_calc_expression("calc(10mm + 10px)", (10 * cssPixelsPerMillimeter) + 10);
assert_calc_expression("calc(10% + 10px)", (10 * viewportWidthPercent()) + 10);
assert_calc_expression("calc(1cm + 2in + 1cm + 2px)", (2 * cssPixelsPerInch) + (2 * cssPixelsPerCentimeter) + 2);
assert_calc_expression("calc(1cm + 2px + 1cm + 2in)", (2 * cssPixelsPerInch) + (2 * cssPixelsPerCentimeter) + 2);
assert_calc_expression("calc(10% + 10px + 2% + 10pc)", (12 * viewportWidthPercent()) + 10 + (10 * cssPixelsPerPica));
}, "Tests calc() on presentation and non-presentation attr in svgLength");
</script>

0 comments on commit 7377d9c

Please sign in to comment.