Skip to content
Permalink
Browse files
[web-animations] baseline-shift animation is incorrect
https://bugs.webkit.org/show_bug.cgi?id=248178

Reviewed by Darin Adler.

We fail the first subtest of the WPT test web-animations/responsive/baselineShift.html because we only
do part of the work required to animate the baseline-shift property and also fail to return the correct
computed value for it.

The baseline-shift property can be a <length-percentage> or a keyword. The <length-percentage> is contained
in SVGRenderStyle::baselineShiftValue() while the keyword is contained in SVGRenderStyle::baselineShift().
We create a dedicated animation wrapper that accounts for both of those methods.

Now that SVGRenderStyle has the correct value for baselineShift() when blending, we must also ensure that
we return a _computed_ value and not just serialize the value held in SVGRenderStyle::baselineShiftValue().
To that end, we change SVGLengthValue::toCSSPrimitiveValue() to convert the stored value using
SVGLengthContext::convertValueToUserUnits() when provided an Element. We also change that method to not be
static since its first parameter is an SVGLengthValue.

* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/baselineShift-expected.txt:
* LayoutTests/svg/css/parse-calc-length-expected.txt:
* LayoutTests/svg/css/parse-calc-length.html:
* LayoutTests/svg/css/scientific-numbers-expected.txt:
* LayoutTests/svg/css/scientific-numbers.html:
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:
(WebCore::strokeDashArrayToCSSValueList):
(WebCore::ComputedStyleExtractor::svgPropertyValue):
* Source/WebCore/svg/SVGLengthValue.cpp:
(WebCore::SVGLengthValue::toCSSPrimitiveValue const):
(WebCore::SVGLengthValue::toCSSPrimitiveValue): Deleted.
* Source/WebCore/svg/SVGLengthValue.h:

Canonical link: https://commits.webkit.org/256934@main
  • Loading branch information
graouts committed Nov 22, 2022
1 parent b10a458 commit 50e415a29df2b2c9ca9541dedf0cbda6878b1f95
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 17 deletions.
@@ -1,4 +1,4 @@

FAIL baselineShift responsive to style changes assert_equals: expected "80px" but got "baseline"
FAIL baselineShift responsive to inherited changes assert_equals: expected "sub" but got "baseline"
PASS baselineShift responsive to style changes
FAIL baselineShift responsive to inherited changes assert_equals: expected "super" but got "sub"

@@ -53,7 +53,7 @@ PASS computedStyle("stroke-dashoffset", "calc(40px + 60px)") is "100px"
PASS computedStyle("width", "calc(40px + 60px)") is "100px"
PASS computedStyle("x", "calc(40px + 60px)") is "100px"
PASS computedStyle("y", "calc(40px + 60px)") is "100px"
PASS computedStyle("baseline-shift", "calc(0% + 100px)") is "0"
PASS computedStyle("baseline-shift", "calc(0% + 100px)") is "0px"
PASS computedStyle("cx", "calc(0% + 100px)") is "calc(0% + 100px)"
PASS computedStyle("cy", "calc(0% + 100px)") is "calc(0% + 100px)"
PASS computedStyle("height", "calc(0% + 100px)") is "calc(0% + 100px)"
@@ -66,7 +66,7 @@ PASS computedStyle("stroke-dashoffset", "calc(0% + 100px)") is "calc(0% + 100px)
PASS computedStyle("width", "calc(0% + 100px)") is "calc(0% + 100px)"
PASS computedStyle("x", "calc(0% + 100px)") is "calc(0% + 100px)"
PASS computedStyle("y", "calc(0% + 100px)") is "calc(0% + 100px)"
PASS computedStyle("baseline-shift", "calc(100% - 100px)") is "0"
PASS computedStyle("baseline-shift", "calc(100% - 100px)") is "0px"
PASS computedStyle("cx", "calc(100% - 100px)") is "calc(100% - 100px)"
PASS computedStyle("cy", "calc(100% - 100px)") is "calc(100% - 100px)"
PASS computedStyle("height", "calc(100% - 100px)") is "calc(100% - 100px)"
@@ -63,7 +63,7 @@
testComputed("y", "calc(40px + 60px)", "100px");

// Test 'calc(0% + 100px)'.
testComputed("baseline-shift", "calc(0% + 100px)", "0");
testComputed("baseline-shift", "calc(0% + 100px)", "0px");
testComputed("cx", "calc(0% + 100px)", "calc(0% + 100px)");
testComputed("cy", "calc(0% + 100px)", "calc(0% + 100px)");
testComputed("height", "calc(0% + 100px)", "calc(0% + 100px)");
@@ -78,7 +78,7 @@
testComputed("y", "calc(0% + 100px)", "calc(0% + 100px)");

// Test 'calc(100% - 100px)'.
testComputed("baseline-shift", "calc(100% - 100px)", "0");
testComputed("baseline-shift", "calc(100% - 100px)", "0px");
testComputed("cx", "calc(100% - 100px)", "calc(100% - 100px)");
testComputed("cy", "calc(100% - 100px)", "calc(100% - 100px)");
testComputed("height", "calc(100% - 100px)", "calc(100% - 100px)");
@@ -82,11 +82,11 @@ PASS getComputedStyle(text).baselineShift is "-50px"

Test if value and 'em' still works
PASS getComputedStyle(text).baselineShift is "baseline"
PASS getComputedStyle(text).baselineShift is "50em"
PASS getComputedStyle(text).baselineShift is "800px"

Test if value and 'ex' still works
PASS getComputedStyle(text).baselineShift is "baseline"
PASS getComputedStyle(text).baselineShift is "50ex"
PASS getComputedStyle(text).baselineShift is "400px"

Trailing and leading whitespaces
PASS getComputedStyle(text).baselineShift is "baseline"
@@ -79,11 +79,11 @@

debug("");
debug("Test if value and 'em' still works");
test("50em", "50em");
test("50em", "800px");

debug("");
debug("Test if value and 'ex' still works");
test("50ex", "50ex");
test("50ex", "400px");

debug("");
debug("Trailing and leading whitespaces");
@@ -2429,6 +2429,33 @@ class PropertyWrapperFontSizeAdjust final : public PropertyWrapperGetter<std::op
}
};

class PropertyWrapperBaselineShift final : public PropertyWrapper<SVGLengthValue> {
WTF_MAKE_FAST_ALLOCATED;
public:
PropertyWrapperBaselineShift()
: PropertyWrapper(CSSPropertyBaselineShift, &RenderStyle::baselineShiftValue, &RenderStyle::setBaselineShiftValue)
{
}

private:
bool equals(const RenderStyle& a, const RenderStyle& b) const final
{
return a.svgStyle().baselineShift() == b.svgStyle().baselineShift() && PropertyWrapper::equals(a, b);
}

bool canInterpolate(const RenderStyle& from, const RenderStyle& to, CompositeOperation compositeOperation) const final
{
return from.svgStyle().baselineShift() == to.svgStyle().baselineShift() && PropertyWrapper::canInterpolate(from, to, compositeOperation);
}

void blend(RenderStyle& destination, const RenderStyle& from, const RenderStyle& to, const CSSPropertyBlendingContext& context) const final
{
auto& srcSVGStyle = !context.progress ? from.svgStyle() : to.svgStyle();
destination.accessSVGStyle().setBaselineShift(srcSVGStyle.baselineShift());
PropertyWrapper::blend(destination, from, to, context);
}
};

template <typename T>
class AutoPropertyWrapper final : public PropertyWrapper<T> {
WTF_MAKE_FAST_ALLOCATED;
@@ -3298,7 +3325,7 @@ CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap()

new PropertyWrapperColor(CSSPropertyLightingColor, &RenderStyle::lightingColor, &RenderStyle::setLightingColor),

new PropertyWrapper<SVGLengthValue>(CSSPropertyBaselineShift, &RenderStyle::baselineShiftValue, &RenderStyle::setBaselineShiftValue),
new PropertyWrapperBaselineShift,
new PropertyWrapper<SVGLengthValue>(CSSPropertyKerning, &RenderStyle::kerning, &RenderStyle::setKerning),
#if ENABLE(VARIATION_FONTS)
new PropertyWrapperFontVariationSettings(CSSPropertyFontVariationSettings, &RenderStyle::fontVariationSettings, &RenderStyle::setFontVariationSettings),
@@ -57,7 +57,7 @@ static Ref<CSSValue> strokeDashArrayToCSSValueList(const Vector<SVGLengthValue>&

auto list = CSSValueList::createCommaSeparated();
for (auto& length : dashes)
list->append(SVGLengthValue::toCSSPrimitiveValue(length));
list->append(length.toCSSPrimitiveValue());

return list;
}
@@ -129,7 +129,7 @@ RefPtr<CSSValue> ComputedStyleExtractor::svgPropertyValue(CSSPropertyID property
case CSSPropertyFill:
return adjustSVGPaint(svgStyle.fillPaintType(), svgStyle.fillPaintUri(), createColor(svgStyle.fillPaintColor()));
case CSSPropertyKerning:
return SVGLengthValue::toCSSPrimitiveValue(svgStyle.kerning());
return svgStyle.kerning().toCSSPrimitiveValue();
case CSSPropertyMarkerEnd:
if (!svgStyle.markerEndResource().isEmpty())
return CSSPrimitiveValue::create(makeString('#', svgStyle.markerEndResource()), CSSUnitType::CSS_URI);
@@ -155,7 +155,7 @@ RefPtr<CSSValue> ComputedStyleExtractor::svgPropertyValue(CSSPropertyID property
case BaselineShift::Sub:
return CSSPrimitiveValue::createIdentifier(CSSValueSub);
case BaselineShift::Length:
return SVGLengthValue::toCSSPrimitiveValue(svgStyle.baselineShiftValue());
return svgStyle.baselineShiftValue().toCSSPrimitiveValue(m_element.get());
}
ASSERT_NOT_REACHED();
return nullptr;
@@ -243,9 +243,16 @@ SVGLengthValue SVGLengthValue::fromCSSPrimitiveValue(const CSSPrimitiveValue& va
return lengthType == SVGLengthType::Unknown ? SVGLengthValue() : SVGLengthValue(value.floatValue(), lengthType);
}

Ref<CSSPrimitiveValue> SVGLengthValue::toCSSPrimitiveValue(const SVGLengthValue& length)
Ref<CSSPrimitiveValue> SVGLengthValue::toCSSPrimitiveValue(const Element* element) const
{
return CSSPrimitiveValue::create(length.valueInSpecifiedUnits(), lengthTypeToPrimitiveType(length.lengthType()));
if (is<SVGElement>(element)) {
SVGLengthContext context { downcast<SVGElement>(element) };
auto computedValue = context.convertValueToUserUnits(valueInSpecifiedUnits(), lengthType(), lengthMode());
if (!computedValue.hasException())
return CSSPrimitiveValue::create(computedValue.releaseReturnValue(), CSSUnitType::CSS_PX);
}

return CSSPrimitiveValue::create(valueInSpecifiedUnits(), lengthTypeToPrimitiveType(lengthType()));
}

ExceptionOr<void> SVGLengthValue::setValueAsString(StringView valueAsString, SVGLengthMode lengthMode)
@@ -28,6 +28,7 @@
namespace WebCore {

class CSSPrimitiveValue;
class Element;
class SVGLengthContext;

enum class SVGLengthType : uint8_t {
@@ -67,7 +68,7 @@ class SVGLengthValue {
static SVGLengthValue blend(const SVGLengthValue& from, const SVGLengthValue& to, float progress);

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

SVGLengthType lengthType() const { return m_lengthType; }
SVGLengthMode lengthMode() const { return m_lengthMode; }

0 comments on commit 50e415a

Please sign in to comment.