Skip to content
Permalink
Browse files
Align stroke-dasharray CSS property parsing with the specification
https://bugs.webkit.org/show_bug.cgi?id=249615

Reviewed by Simon Fraser.

Align `stroke-dasharray` CSS property parsing with the specification:
- https://svgwg.org/svg2-draft/painting.html#StrokeDashing
- https://svgwg.org/svg2-draft/painting.html#DataTypeDasharray

* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/stroke-dasharray-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/stroke-dasharray.html:
Update stroke-dasharray css-typed-on WPT test to match the specification. In
particular, <length> | <percentage> | <number> should be allowed. Negative
values shouldn't be allowed. Also, computed values should be converted to
lengths if numbers were provided.

* LayoutTests/imported/w3c/web-platform-tests/svg/painting/parsing/stroke-dasharray-invalid-expected.txt:
Rebaseline WPT test now that more checks are passing.

* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
Update LengthRawKnownTokenTypeNumberConsumer::consume() to stop ignoring the
UnitlessZeroQuirk parameter. Previously, no matter what value the caller was
passing for this parameter, we were allowing unitless zero. Update call sites
to pass UnitlessZeroQuirk::Allow to maintain pre-existing behavior for now.
However, I made sure to pass UnitlessZeroQuirk::Forbid in
consumeStrokeDasharray() since we want to make sure "0" gets parsed as a
<number>, not a <length> (covered by stroke-dasharray-valid.html WPT test).

(WebCore::CSSPropertyParserHelpers::consumeStrokeDasharray):
Fallback to parsing input as a <number> if parsing it as a <length-percentage>,
as per the specification.

* Source/WebCore/css/typedom/CSSUnitValue.cpp:
(WebCore::isValueOutOfRangeForProperty):
Update isValueOutOfRangeForProperty() to properly indicate that stroke-dasharray
doesn't allow negative values.

* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertSVGLengthValue):
(WebCore::Style::BuilderConverter::convertSVGLengthVector):
(WebCore::Style::BuilderConverter::convertStrokeDashArray):
Update our style builder converter to:
- Deal with stroke-dasharray values coming from CSS-Typed-OM, which may not have
  been converted to a CSSValueList yet (unlike values coming from the CSS parser).
- Request that numbers get converted to lengths (by using 'px' unit), now that we
  allow numbers at parser level (per the specification).

* Source/WebCore/svg/SVGLengthValue.cpp:
(WebCore::SVGLengthValue::fromCSSPrimitiveValue):
* Source/WebCore/svg/SVGLengthValue.h:

Canonical link: https://commits.webkit.org/258136@main
  • Loading branch information
cdumez committed Dec 20, 2022
1 parent b961cdd commit 36071c56fa58a4258963e403263ab1e3d984f82c
Show file tree
Hide file tree
Showing 9 changed files with 61 additions and 37 deletions.
@@ -5,14 +5,18 @@ PASS Can set 'stroke-dasharray' to CSS-wide keywords: unset
PASS Can set 'stroke-dasharray' to CSS-wide keywords: revert
PASS Can set 'stroke-dasharray' to var() references: var(--A)
PASS Can set 'stroke-dasharray' to the 'none' keyword: none
FAIL Setting 'stroke-dasharray' to a length: 0px throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
PASS Setting 'stroke-dasharray' to a length: -3.14em throws TypeError
FAIL Setting 'stroke-dasharray' to a length: 3.14cm throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
FAIL Setting 'stroke-dasharray' to a length: calc(0px + 0em) throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
FAIL Setting 'stroke-dasharray' to a percent: 0% throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
PASS Setting 'stroke-dasharray' to a percent: -3.14% throws TypeError
FAIL Setting 'stroke-dasharray' to a percent: 3.14% throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
FAIL Setting 'stroke-dasharray' to a percent: calc(0% + 0%) throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
PASS Can set 'stroke-dasharray' to a length: 0px
FAIL Can set 'stroke-dasharray' to a length: -3.14em assert_equals: unit expected "px" but got "em"
FAIL Can set 'stroke-dasharray' to a length: 3.14cm assert_equals: unit expected "px" but got "cm"
PASS Can set 'stroke-dasharray' to a length: calc(0px + 0em)
PASS Can set 'stroke-dasharray' to a percent: 0%
FAIL Can set 'stroke-dasharray' to a percent: -3.14% assert_approx_equals: expected -3.14 +/- 0.000001 but got 0
PASS Can set 'stroke-dasharray' to a percent: 3.14%
PASS Can set 'stroke-dasharray' to a percent: calc(0% + 0%)
PASS Can set 'stroke-dasharray' to a number: 0
PASS Can set 'stroke-dasharray' to a number: -3.14
PASS Can set 'stroke-dasharray' to a number: 3.14
PASS Can set 'stroke-dasharray' to a number: calc(2 + 3)
PASS Setting 'stroke-dasharray' to a time: 0s throws TypeError
PASS Setting 'stroke-dasharray' to a time: -3.14ms throws TypeError
PASS Setting 'stroke-dasharray' to a time: 3.14s throws TypeError
@@ -24,10 +28,6 @@ PASS Setting 'stroke-dasharray' to an angle: calc(0rad + 0deg) throws TypeError
PASS Setting 'stroke-dasharray' to a flexible length: 0fr throws TypeError
PASS Setting 'stroke-dasharray' to a flexible length: 1fr throws TypeError
PASS Setting 'stroke-dasharray' to a flexible length: -3.14fr throws TypeError
FAIL Setting 'stroke-dasharray' to a number: 0 throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
PASS Setting 'stroke-dasharray' to a number: -3.14 throws TypeError
FAIL Setting 'stroke-dasharray' to a number: 3.14 throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
FAIL Setting 'stroke-dasharray' to a number: calc(2 + 3) throws TypeError assert_throws_js: function "() => styleMap.set(propertyName, example.input)" did not throw
PASS Setting 'stroke-dasharray' to a transform: translate(50%, 50%) throws TypeError
PASS Setting 'stroke-dasharray' to a transform: perspective(10em) throws TypeError
PASS Setting 'stroke-dasharray' to a transform: translate3d(0px, 1px, 2px) translate(0px, 1px) rotate3d(1, 2, 3, 45deg) rotate(45deg) scale3d(1, 2, 3) scale(1, 2) skew(1deg, 1deg) skewX(1deg) skewY(45deg) perspective(1px) matrix3d(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16) matrix(1, 2, 3, 4, 5, 6) throws TypeError
@@ -14,7 +14,20 @@
'use strict';

runPropertyTests('stroke-dasharray', [
{ syntax: 'none' }
{ syntax: 'none' },
{
syntax: '<length>',
specified: assert_is_equal_with_range_handling,
},
{
syntax: '<percentage>',
specified: assert_is_equal_with_range_handling,
},
{
syntax: '<number>',
specified: assert_is_equal_with_range_handling,
computed: (_, result) => assert_is_unit('px', result)
},
]);

runUnsupportedPropertyTests('stroke-dasharray', [
@@ -4,6 +4,6 @@ PASS e.style['stroke-dasharray'] = "none 10px" should not set the property value
PASS e.style['stroke-dasharray'] = "20px / 30px" should not set the property value
PASS e.style['stroke-dasharray'] = "-40px" should not set the property value
PASS e.style['stroke-dasharray'] = "calc(2px + 3)" should not set the property value
FAIL e.style['stroke-dasharray'] = "calc(10% + 5)" should not set the property value assert_equals: expected "" but got "calc(5 + 10%)"
PASS e.style['stroke-dasharray'] = "calc(10% + 5)" should not set the property value
PASS e.style['stroke-dasharray'] = "calc(40 + calc(3px + 6%))" should not set the property value

@@ -605,13 +605,13 @@ struct LengthRawKnownTokenTypeDimensionConsumer {

struct LengthRawKnownTokenTypeNumberConsumer {
static constexpr CSSParserTokenType tokenType = NumberToken;
static std::optional<LengthRaw> consume(CSSParserTokenRange& range, const CSSCalcSymbolTable&, ValueRange valueRange, CSSParserMode parserMode, UnitlessQuirk unitless, UnitlessZeroQuirk)
static std::optional<LengthRaw> consume(CSSParserTokenRange& range, const CSSCalcSymbolTable&, ValueRange valueRange, CSSParserMode parserMode, UnitlessQuirk unitless, UnitlessZeroQuirk unitlessZero)
{
ASSERT(range.peek().type() == NumberToken);

auto& token = range.peek();

if (!shouldAcceptUnitlessValue(token.numericValue(), parserMode, unitless, UnitlessZeroQuirk::Allow))
if (!shouldAcceptUnitlessValue(token.numericValue(), parserMode, unitless, unitlessZero))
return std::nullopt;

if (auto validatedValue = validatedLengthRaw(token.numericValue(), valueRange)) {
@@ -1445,7 +1445,7 @@ RefPtr<CSSPrimitiveValue> consumePercentWorkerSafe(CSSParserTokenRange& range, V

RefPtr<CSSPrimitiveValue> consumeLength(CSSParserTokenRange& range, CSSParserMode parserMode, ValueRange valueRange, UnitlessQuirk unitless)
{
return consumeMetaConsumer<LengthConsumer>(range, { }, valueRange, parserMode, unitless, UnitlessZeroQuirk::Forbid, CSSValuePool::singleton());
return consumeMetaConsumer<LengthConsumer>(range, { }, valueRange, parserMode, unitless, UnitlessZeroQuirk::Allow, CSSValuePool::singleton());
}

#if ENABLE(VARIATION_FONTS)
@@ -1488,7 +1488,7 @@ static std::optional<LengthOrPercentRaw> consumeLengthOrPercentRaw(CSSParserToke
}

// FIXME: This doesn't work with the current scheme due to the NegativePercentagePolicy parameter
RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange& range, CSSParserMode parserMode, ValueRange valueRange, UnitlessQuirk unitless, NegativePercentagePolicy negativePercentagePolicy)
RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange& range, CSSParserMode parserMode, ValueRange valueRange, UnitlessQuirk unitless, UnitlessZeroQuirk unitlessZero, NegativePercentagePolicy negativePercentagePolicy)
{
auto& token = range.peek();

@@ -1502,13 +1502,13 @@ RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange& range, CSS
}

case DimensionToken:
return LengthCSSPrimitiveValueWithCalcWithKnownTokenTypeDimensionConsumer::consume(range, { }, valueRange, parserMode, unitless, UnitlessZeroQuirk::Forbid, CSSValuePool::singleton());
return LengthCSSPrimitiveValueWithCalcWithKnownTokenTypeDimensionConsumer::consume(range, { }, valueRange, parserMode, unitless, unitlessZero, CSSValuePool::singleton());

case NumberToken:
return LengthCSSPrimitiveValueWithCalcWithKnownTokenTypeNumberConsumer::consume(range, { }, valueRange, parserMode, unitless, UnitlessZeroQuirk::Forbid, CSSValuePool::singleton());
return LengthCSSPrimitiveValueWithCalcWithKnownTokenTypeNumberConsumer::consume(range, { }, valueRange, parserMode, unitless, unitlessZero, CSSValuePool::singleton());

case PercentageToken:
return PercentCSSPrimitiveValueWithCalcWithKnownTokenTypePercentConsumer::consume(range, { }, valueRange, parserMode, unitless, UnitlessZeroQuirk::Forbid, CSSValuePool::singleton());
return PercentCSSPrimitiveValueWithCalcWithKnownTokenTypePercentConsumer::consume(range, { }, valueRange, parserMode, unitless, unitlessZero, CSSValuePool::singleton());

default:
break;
@@ -3223,7 +3223,7 @@ static RefPtr<CSSPrimitiveValue> consumePositionComponent(CSSParserTokenRange& r
{
if (range.peek().type() == IdentToken)
return consumeIdent<CSSValueLeft, CSSValueTop, CSSValueBottom, CSSValueRight, CSSValueCenter>(range);
return consumeLengthOrPercent(range, parserMode, ValueRange::All, unitless, negativePercentagePolicy);
return consumeLengthOrPercent(range, parserMode, ValueRange::All, unitless, UnitlessZeroQuirk::Allow, negativePercentagePolicy);
}

static bool isHorizontalPositionKeywordOnly(const CSSPrimitiveValue& value)
@@ -6441,7 +6441,9 @@ RefPtr<CSSValue> consumeStrokeDasharray(CSSParserTokenRange& range)

RefPtr<CSSValueList> dashes = CSSValueList::createCommaSeparated();
do {
RefPtr<CSSPrimitiveValue> dash = consumeLengthOrPercent(range, SVGAttributeMode, ValueRange::NonNegative);
RefPtr<CSSPrimitiveValue> dash = consumeLengthOrPercent(range, HTMLStandardMode, ValueRange::NonNegative, UnitlessQuirk::Forbid, UnitlessZeroQuirk::Forbid);
if (!dash)
dash = consumeNumber(range, ValueRange::NonNegative);
if (!dash || (consumeCommaIncludingWhitespace(range) && range.atEnd()))
return nullptr;
dashes->append(dash.releaseNonNull());
@@ -108,7 +108,7 @@ RefPtr<CSSPrimitiveValue> consumeNumber(CSSParserTokenRange&, ValueRange);
RefPtr<CSSPrimitiveValue> consumeNumberOrPercent(CSSParserTokenRange&, ValueRange);

RefPtr<CSSPrimitiveValue> consumeLength(CSSParserTokenRange&, CSSParserMode, ValueRange, UnitlessQuirk = UnitlessQuirk::Forbid);
RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange&, CSSParserMode, ValueRange, UnitlessQuirk = UnitlessQuirk::Forbid, NegativePercentagePolicy = NegativePercentagePolicy::Forbid);
RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange&, CSSParserMode, ValueRange, UnitlessQuirk = UnitlessQuirk::Forbid, UnitlessZeroQuirk = UnitlessZeroQuirk::Allow, NegativePercentagePolicy = NegativePercentagePolicy::Forbid);

RefPtr<CSSPrimitiveValue> consumePercent(CSSParserTokenRange&, ValueRange);
RefPtr<CSSPrimitiveValue> consumePercentWorkerSafe(CSSParserTokenRange&, ValueRange, CSSValuePool&);
@@ -251,6 +251,7 @@ static bool isValueOutOfRangeForProperty(CSSPropertyID propertyID, double value,
case CSSPropertyScrollPaddingLeft:
case CSSPropertyScrollPaddingRight:
case CSSPropertyScrollPaddingTop:
case CSSPropertyStrokeDasharray:
case CSSPropertyStrokeMiterlimit:
case CSSPropertyStrokeWidth:
return value < 0;
@@ -153,8 +153,8 @@ class BuilderConverter {
static FontSelectionValue convertFontStretch(BuilderState&, const CSSValue&);
static FontSelectionValue convertFontStyle(BuilderState&, const CSSValue&);
static FontVariationSettings convertFontVariationSettings(BuilderState&, const CSSValue&);
static SVGLengthValue convertSVGLengthValue(BuilderState&, const CSSValue&);
static Vector<SVGLengthValue> convertSVGLengthVector(BuilderState&, const CSSValue&);
static SVGLengthValue convertSVGLengthValue(BuilderState&, const CSSValue&, ShouldConvertNumberToPxLength = ShouldConvertNumberToPxLength::No);
static Vector<SVGLengthValue> convertSVGLengthVector(BuilderState&, const CSSValue&, ShouldConvertNumberToPxLength = ShouldConvertNumberToPxLength::No);
static Vector<SVGLengthValue> convertStrokeDashArray(BuilderState&, const CSSValue&);
static PaintOrder convertPaintOrder(BuilderState&, const CSSValue&);
static float convertOpacity(BuilderState&, const CSSValue&);
@@ -1553,31 +1553,34 @@ inline bool BuilderConverter::convertSmoothScrolling(BuilderState&, const CSSVal
return downcast<CSSPrimitiveValue>(value).valueID() == CSSValueSmooth;
}

inline SVGLengthValue BuilderConverter::convertSVGLengthValue(BuilderState&, const CSSValue& value)
inline SVGLengthValue BuilderConverter::convertSVGLengthValue(BuilderState&, const CSSValue& value, ShouldConvertNumberToPxLength shouldConvertNumberToPxLength)
{
return SVGLengthValue::fromCSSPrimitiveValue(downcast<CSSPrimitiveValue>(value));
return SVGLengthValue::fromCSSPrimitiveValue(downcast<CSSPrimitiveValue>(value), shouldConvertNumberToPxLength);
}

inline Vector<SVGLengthValue> BuilderConverter::convertSVGLengthVector(BuilderState& builderState, const CSSValue& value)
inline Vector<SVGLengthValue> BuilderConverter::convertSVGLengthVector(BuilderState& builderState, const CSSValue& value, ShouldConvertNumberToPxLength shouldConvertNumberToPxLength)
{
auto& valueList = downcast<CSSValueList>(value);

Vector<SVGLengthValue> svgLengths;
svgLengths.reserveInitialCapacity(valueList.length());
for (auto& item : valueList)
svgLengths.uncheckedAppend(convertSVGLengthValue(builderState, item));
svgLengths.uncheckedAppend(convertSVGLengthValue(builderState, item, shouldConvertNumberToPxLength));

return svgLengths;
}

inline Vector<SVGLengthValue> BuilderConverter::convertStrokeDashArray(BuilderState& builderState, const CSSValue& value)
{
if (is<CSSPrimitiveValue>(value)) {
ASSERT(downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNone);
return SVGRenderStyle::initialStrokeDashArray();
if (auto* primitiveValue = dynamicDowncast<CSSPrimitiveValue>(value)) {
if (primitiveValue->valueID() == CSSValueNone)
return SVGRenderStyle::initialStrokeDashArray();

// Values coming from CSS-Typed-OM may not have been converted to a CSSValueList yet.
return Vector { convertSVGLengthValue(builderState, value, ShouldConvertNumberToPxLength::Yes) };
}

return convertSVGLengthVector(builderState, value);
return convertSVGLengthVector(builderState, value, ShouldConvertNumberToPxLength::Yes);
}

inline PaintOrder BuilderConverter::convertPaintOrder(BuilderState&, const CSSValue& value)
@@ -237,10 +237,13 @@ SVGLengthValue SVGLengthValue::blend(const SVGLengthValue& from, const SVGLength
return { WebCore::blend(fromValue.releaseReturnValue(), toValue, { progress }), to.lengthType() };
}

SVGLengthValue SVGLengthValue::fromCSSPrimitiveValue(const CSSPrimitiveValue& value)
SVGLengthValue SVGLengthValue::fromCSSPrimitiveValue(const CSSPrimitiveValue& value, ShouldConvertNumberToPxLength shouldConvertNumberToPxLength)
{
// FIXME: This needs to call value.computeLength() so it can correctly resolve non-absolute units (webkit.org/b/204826).
SVGLengthType lengthType = primitiveTypeToLengthType(value.primitiveType());
auto primitiveType = value.primitiveType();
if (primitiveType == CSSUnitType::CSS_NUMBER && shouldConvertNumberToPxLength == ShouldConvertNumberToPxLength::Yes)
primitiveType = CSSUnitType::CSS_PX;
SVGLengthType lengthType = primitiveTypeToLengthType(primitiveType);
return lengthType == SVGLengthType::Unknown ? SVGLengthValue() : SVGLengthValue(value.floatValue(), lengthType);
}

@@ -56,6 +56,8 @@ enum class SVGLengthNegativeValuesMode : uint8_t {
Forbid
};

enum class ShouldConvertNumberToPxLength : bool { No, Yes };

class SVGLengthValue {
WTF_MAKE_FAST_ALLOCATED;
public:
@@ -67,7 +69,7 @@ class SVGLengthValue {
static SVGLengthValue construct(SVGLengthMode, StringView, SVGParsingError&, SVGLengthNegativeValuesMode = SVGLengthNegativeValuesMode::Allow);
static SVGLengthValue blend(const SVGLengthValue& from, const SVGLengthValue& to, float progress);

static SVGLengthValue fromCSSPrimitiveValue(const CSSPrimitiveValue&);
static SVGLengthValue fromCSSPrimitiveValue(const CSSPrimitiveValue&, ShouldConvertNumberToPxLength = ShouldConvertNumberToPxLength::No);
Ref<CSSPrimitiveValue> toCSSPrimitiveValue(const Element* = nullptr) const;

SVGLengthType lengthType() const { return m_lengthType; }

0 comments on commit 36071c5

Please sign in to comment.