Changeset 287820 in webkit


Ignore:
Timestamp:
Jan 9, 2022 7:35:10 AM (6 months ago)
Author:
graouts@webkit.org
Message:

[Web Animations] getKeyframes() for a CSS Animation should not use computed style for keyframes
​https://bugs.webkit.org/show_bug.cgi?id=235008

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

Mark WPT progression.

  • web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt:

Source/WebCore:

Until now, when calling getKeyframes() on a keyframe effect associated with a CSS Animation, we would
serialize for each keyframe the computed style for the animated properties. However, the Web Aniations
spec says (​https://drafts.csswg.org/web-animations-1/#dom-keyframeeffect-getkeyframes) that the value
to use should "be the result of serializing the property value of declaration by passing declaration
to the algorithm to serialize a CSS value [CSSOM]."

One way we can get to the value as specified on the @keyframes rule rather than the computed style
is to access the StyleRuleKeyframe object directly as stored by the Style::Resolver. This keeps the
raw CSSValues as parsed.

To do this, we first refactor Style::Resolver::keyframeStylesForAnimation() such that all the code
that creates a Vector<Ref<StyleRuleKeyframe>> with deduplicated keyframe rules is in the new dedicated
method Style::Resolver::keyframeRulesForName().

Then, in KeyframeEffect::getKeyframes(), we can call into this function to obtain the set of keyframe
rules for the associated CSS Animation's name and read the style from there. Note that these values
may contain unsubstituted --var() values, so in that case for now we still use the computed style.

  • animation/KeyframeEffect.cpp:

(WebCore::KeyframeEffect::getKeyframes):

  • style/StyleResolver.cpp:

(WebCore::Style::Resolver::keyframeRulesForName):
(WebCore::Style::Resolver::keyframeStylesForAnimation):

  • style/StyleResolver.h:
Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r287817 r287820  
     12022-01-08  Antoine Quint  <graouts@webkit.org>
     2
     3        [Web Animations] getKeyframes() for a CSS Animation should not use computed style for keyframes
     4        https://bugs.webkit.org/show_bug.cgi?id=235008
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Mark WPT progression.
     9
     10        * web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt:
     11
    1122022-01-08  Simon Fraser  <simon.fraser@apple.com>
    213

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-animations/KeyframeEffect-getKeyframes.tentative-expected.txt

    r287707 r287820  
    1616PASS KeyframeEffect.getKeyframes() returns expected frames for an animation with multiple keyframes for the same time and with different but equivalent easing functions
    1717PASS KeyframeEffect.getKeyframes() returns expected frames for overlapping keyframes
    18 FAIL KeyframeEffect.getKeyframes() returns expected values for animations with filter properties and missing keyframes assert_equals: value for 'filter' on Keyframe #1 should match expected "blur(5px) sepia(60%) saturate(30%)" but got "blur(5px) sepia(0.6) saturate(0.3)"
     18PASS KeyframeEffect.getKeyframes() returns expected values for animations with filter properties and missing keyframes
    1919PASS KeyframeEffect.getKeyframes() returns expected values for animation with drop-shadow of filter property
    2020FAIL KeyframeEffect.getKeyframes() returns expected values for animations with text-shadow properties and missing keyframes assert_equals: value for 'textShadow' on Keyframe #0 should match expected "rgb(0, 0, 0) 1px 1px 2px, rgb(0, 0, 255) 0px 0px 16px, rgb(0, 0, 255) 0px 0px 3.2px" but got "rgb(0, 0, 0) 1px 1px 2px, rgb(0, 0, 255) 0px 0px 16px, rgb(0, 0, 255) 0px 0px 3.200000047683716px"
    21 FAIL KeyframeEffect.getKeyframes() returns expected values for animations with background-size properties and missing keyframes assert_equals: value for 'backgroundSize' on ComputedKeyframe #1 should match expected "50%, 6px, contain" but got "50%"
     21FAIL KeyframeEffect.getKeyframes() returns expected values for animations with background-size properties and missing keyframes assert_equals: value for 'backgroundSize' on ComputedKeyframe #0 after updating current style should match expected "30px, 40%, auto" but got "30px"
    2222FAIL KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values assert_equals: value for 'transform' on Keyframe #1 should match expected "translate(100px)" but got "matrix(1, 0, 0, 1, 100, 0)"
    2323PASS KeyframeEffect.getKeyframes() returns expected values for animations with CSS variables as keyframe values in a shorthand property
    2424PASS KeyframeEffect.getKeyframes() returns expected values for animations with a CSS variable which is overriden by the value in keyframe
    25 FAIL KeyframeEffect.getKeyframes() returns expected values for animations with only custom property in a keyframe assert_equals: value for 'transform' on Keyframe #0 should match expected "translate(100px)" but got "matrix(1, 0, 0, 1, 100, 0)"
     25FAIL KeyframeEffect.getKeyframes() returns expected values for animations with only custom property in a keyframe assert_equals: value for 'transform' on Keyframe #0 should match expected "translate(100px)" but got "translate(100px, 0px)"
    2626PASS KeyframeEffect.getKeyframes() reflects changes to @keyframes rules
    2727

  • trunk/Source/WebCore/ChangeLog

    r287819 r287820  
     12022-01-08  Antoine Quint  <graouts@webkit.org>
     2
     3        [Web Animations] getKeyframes() for a CSS Animation should not use computed style for keyframes
     4        https://bugs.webkit.org/show_bug.cgi?id=235008
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Until now, when calling getKeyframes() on a keyframe effect associated with a CSS Animation, we would
     9        serialize for each keyframe the computed style for the animated properties. However, the Web Aniations
     10        spec says (https://drafts.csswg.org/web-animations-1/#dom-keyframeeffect-getkeyframes) that the value
     11        to use should "be the result of serializing the property value of declaration by passing declaration
     12        to the algorithm to serialize a CSS value [CSSOM]."
     13
     14        One way we can get to the value as specified on the @keyframes rule rather than the computed style
     15        is to access the StyleRuleKeyframe object directly as stored by the Style::Resolver. This keeps the
     16        raw CSSValues as parsed.
     17
     18        To do this, we first refactor Style::Resolver::keyframeStylesForAnimation() such that all the code
     19        that creates a Vector<Ref<StyleRuleKeyframe>> with deduplicated keyframe rules is in the new dedicated
     20        method Style::Resolver::keyframeRulesForName().
     21
     22        Then, in KeyframeEffect::getKeyframes(), we can call into this function to obtain the set of keyframe
     23        rules for the associated CSS Animation's name and read the style from there. Note that these values
     24        may contain unsubstituted --var() values, so in that case for now we still use the computed style.
     25
     26        * animation/KeyframeEffect.cpp:
     27        (WebCore::KeyframeEffect::getKeyframes):
     28        * style/StyleResolver.cpp:
     29        (WebCore::Style::Resolver::keyframeRulesForName):
     30        (WebCore::Style::Resolver::keyframeStylesForAnimation):
     31        * style/StyleResolver.h:
     32
    1332022-01-09  Alan Bujtas  <zalan@apple.com>
    234

  • trunk/Source/WebCore/animation/KeyframeEffect.cpp

    r287707 r287820  
    640640        auto computedStyleExtractor = ComputedStyleExtractor(target, false, m_pseudoId);
    641641
     642        auto keyframeRules = [&]() -> const Vector<Ref<StyleRuleKeyframe>> {
     643            if (!is<CSSAnimation>(animation()))
     644                return { };
     645
     646            auto& backingAnimation = downcast<CSSAnimation>(*animation()).backingAnimation();
     647            auto* styleScope = Style::Scope::forOrdinal(*m_target, backingAnimation.nameStyleScopeOrdinal());
     648            if (!styleScope)
     649                return { };
     650
     651            return styleScope->resolver().keyframeRulesForName(m_blendingKeyframes.animationName());
     652        }();
     653
     654        auto keyframeRuleForKey = [&](double key) -> StyleRuleKeyframe* {
     655            for (auto& keyframeRule : keyframeRules) {
     656                for (auto keyframeRuleKey : keyframeRule->keys()) {
     657                    if (keyframeRuleKey == key)
     658                        return keyframeRule.ptr();
     659                }
     660            }
     661            return nullptr;
     662        };
     663
    642664        // We need to establish which properties are implicit for 0% and 100%.
    643665        HashSet<CSSPropertyID> zeroKeyframeProperties = m_blendingKeyframes.properties();
    … …  
    691713            // 3. For each animation property-value pair specified on keyframe, declaration, perform the following steps:
    692714            auto& style = *keyframe.style();
     715            auto* keyframeRule = keyframeRuleForKey(keyframe.key());
    693716            for (auto cssPropertyId : keyframe.properties()) {
    694717                if (cssPropertyId == CSSPropertyCustom)
    695718                    continue;
    696719                String idlValue = "";
    697                 if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(style, cssPropertyId, renderer))
    698                     idlValue = cssValue->cssText();
     720                if (keyframeRule) {
     721                    if (auto cssValue = keyframeRule->properties().getPropertyCSSValue(cssPropertyId)) {
     722                        if (!cssValue->hasVariableReferences())
     723                            idlValue = keyframeRule->properties().getPropertyValue(cssPropertyId);
     724                    }
     725                }
     726                if (idlValue.isEmpty()) {
     727                    if (auto cssValue = computedStyleExtractor.valueForPropertyInStyle(style, cssPropertyId, renderer))
     728                        idlValue = cssValue->cssText();
     729                }
    699730                addPropertyToKeyframe(cssPropertyId, idlValue);
    700731            }

  • trunk/Source/WebCore/style/StyleResolver.cpp

    r287524 r287820  
    308308}
    309309
    310 void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle* elementStyle, const ResolutionContext& context, KeyframeList& list)
    311 {
    312     list.clear();
    313 
    314     // Get the keyframesRule for this name.
    315     if (list.animationName().isEmpty())
    316         return;
     310const Vector<Ref<StyleRuleKeyframe>> Resolver::keyframeRulesForName(const AtomString& animationName)
     311{
     312    if (animationName.isEmpty())
     313        return { };
    317314
    318315    m_keyframesRuleMap.checkConsistency();
    319316
    320     KeyframesRuleMap::iterator it = m_keyframesRuleMap.find(list.animationName().impl());
     317    auto it = m_keyframesRuleMap.find(animationName.impl());
    321318    if (it == m_keyframesRuleMap.end())
    322         return;
     319        return { };
    323320
    324321    auto timingFunctionForKeyframe = [](Ref<StyleRuleKeyframe> keyframe) -> RefPtr<const TimingFunction> {
    … …  
    341338    };
    342339
    343     const StyleRuleKeyframes* keyframesRule = it->value.get();
     340    auto* keyframesRule = it->value.get();
    344341    auto* keyframes = &keyframesRule->keyframes();
    345342
    … …  
    357354    }();
    358355
     356    if (!hasDuplicateKeys)
     357        return *keyframes;
     358
    359359    // FIXME: If HashMaps could have Ref<> as value types, we wouldn't need
    360360    // to copy the HashMap into a Vector.
    361     Vector<Ref<StyleRuleKeyframe>> newKeyframesIfNecessary;
    362     if (hasDuplicateKeys) {
    363         // Merge keyframes with a similar offset and timing function.
    364         HashMap<KeyframeUniqueKey, RefPtr<StyleRuleKeyframe>> keyframesMap;
    365         for (auto& originalKeyframe : *keyframes) {
    366             auto timingFunction = uniqueTimingFunctionForKeyframe(originalKeyframe);
    367             for (auto key : originalKeyframe->keys()) {
    368                 if (auto keyframe = keyframesMap.get({ key, timingFunction }))
    369                     keyframe->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
    370                 else {
    371                     auto styleRuleKeyframe = StyleRuleKeyframe::create(MutableStyleProperties::create());
    372                     styleRuleKeyframe.ptr()->setKey(key);
    373                     styleRuleKeyframe.ptr()->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
    374                     keyframesMap.set({ key, timingFunction }, styleRuleKeyframe.ptr());
    375                     newKeyframesIfNecessary.append(styleRuleKeyframe);
    376                 }
     361    Vector<Ref<StyleRuleKeyframe>> deduplicatedKeyframes;
     362    // Merge keyframes with a similar offset and timing function.
     363    HashMap<KeyframeUniqueKey, RefPtr<StyleRuleKeyframe>> keyframesMap;
     364    for (auto& originalKeyframe : *keyframes) {
     365        auto timingFunction = uniqueTimingFunctionForKeyframe(originalKeyframe);
     366        for (auto key : originalKeyframe->keys()) {
     367            if (auto keyframe = keyframesMap.get({ key, timingFunction }))
     368                keyframe->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
     369            else {
     370                auto styleRuleKeyframe = StyleRuleKeyframe::create(MutableStyleProperties::create());
     371                styleRuleKeyframe.ptr()->setKey(key);
     372                styleRuleKeyframe.ptr()->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
     373                keyframesMap.set({ key, timingFunction }, styleRuleKeyframe.ptr());
     374                deduplicatedKeyframes.append(styleRuleKeyframe);
    377375            }
    378376        }
    379         keyframes = &newKeyframesIfNecessary;
    380     }
     377    }
     378
     379    return deduplicatedKeyframes;
     380}
     381
     382void Resolver::keyframeStylesForAnimation(const Element& element, const RenderStyle* elementStyle, const ResolutionContext& context, KeyframeList& list)
     383{
     384    list.clear();
     385
     386    auto keyframeRules = keyframeRulesForName(list.animationName());
     387    if (keyframeRules.isEmpty())
     388        return;
    381389
    382390    // Construct and populate the style for each keyframe.
    383     for (auto& keyframe : *keyframes) {
     391    for (auto& keyframeRule : keyframeRules) {
    384392        // Add this keyframe style to all the indicated key times
    385         for (auto key : keyframe->keys()) {
     393        for (auto key : keyframeRule->keys()) {
    386394            KeyframeValue keyframeValue(0, nullptr);
    387             keyframeValue.setStyle(styleForKeyframe(element, elementStyle, context, keyframe.ptr(), keyframeValue));
     395            keyframeValue.setStyle(styleForKeyframe(element, elementStyle, context, keyframeRule.ptr(), keyframeValue));
    388396            keyframeValue.setKey(key);
    389             if (auto timingFunctionCSSValue = keyframe->properties().getPropertyCSSValue(CSSPropertyAnimationTimingFunction))
     397            if (auto timingFunctionCSSValue = keyframeRule->properties().getPropertyCSSValue(CSSPropertyAnimationTimingFunction))
    390398                keyframeValue.setTimingFunction(TimingFunction::createFromCSSValue(*timingFunctionCSSValue.get()));
    391399            list.insert(WTFMove(keyframeValue));

  • trunk/Source/WebCore/style/StyleResolver.h

    r285630 r287820  
    140140
    141141    void addKeyframeStyle(Ref<StyleRuleKeyframes>&&);
     142    const Vector<Ref<StyleRuleKeyframe>> keyframeRulesForName(const AtomString&);
    142143
    143144    bool usesFirstLineRules() const { return m_ruleSets.features().usesFirstLineRules; }
Note: See TracChangeset for help on using the changeset viewer.