Bug 237073 - [css-cascade] Support 'revert' in @keyframes
Summary: [css-cascade] Support 'revert' in @keyframes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-22 19:28 PST by Oriol Brufau
Modified: 2022-02-24 13:21 PST (History)
4 users (show)

See Also:


Attachments
testcase (560 bytes, text/html)
2022-02-22 19:28 PST, Oriol Brufau
no flags Details
Patch (8.78 KB, patch)
2022-02-22 19:41 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2022-02-22 19:48 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (10.50 KB, patch)
2022-02-23 05:53 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (10.50 KB, patch)
2022-02-23 05:55 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2022-02-23 10:11 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2022-02-22 19:28:07 PST
Created attachment 452930 [details]
testcase

'initial', 'unset' and 'inherit' work well in @keyframes.
But 'revert' doesn't work, it just behaves as 'unset'.
It works in Blink: https://chromium-review.googlesource.com/c/chromium/src/+/2132249
Comment 1 Oriol Brufau 2022-02-22 19:41:06 PST
Created attachment 452931 [details]
Patch
Comment 2 Oriol Brufau 2022-02-22 19:48:07 PST
Created attachment 452933 [details]
Patch
Comment 3 Oriol Brufau 2022-02-23 05:53:03 PST
Created attachment 452968 [details]
Patch
Comment 4 Oriol Brufau 2022-02-23 05:55:37 PST
Created attachment 452969 [details]
Patch
Comment 5 Darin Adler 2022-02-23 08:55:21 PST
Comment on attachment 452969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452969&action=review

Seems fine as is, a few thoughts

> Source/WebCore/style/StyleResolver.cpp:283
> +    unsigned propertyCount = keyframe.properties().propertyCount();
> +    bool hasRevert = false;
> +    for (unsigned i = 0; i < propertyCount; ++i) {

I think in future we should add begin/end to StyleProperties so loops like this can be a modern style for loop instead of an indexed one like this. Not in this patch, but important for the future, I think.

> Source/WebCore/style/StyleResolver.cpp:284
> +        const auto& propertyReference = keyframe.properties().propertyAt(i);

This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.

> Source/WebCore/style/StyleResolver.cpp:292
> +        if (const CSSValue* value = propertyReference.value())
> +            hasRevert = hasRevert || value->isRevertValue();

I would write it this way:

    if (auto* value = propertyReference.value(); value && value->isRevertValue())
        hasRevert = true;

There are many different styles, but once we are doing an if, I think it’s best to include both checks in the if, it’s useful to use auto in a case like this to make sure we don’t accidentally upcast? If we had a helper function, could also consider:

    hasRevert = hasRevert || isRevertValue(propertyReference.value());

Might be nice to save the work of getting the value and type-checking it entirely once we found a revert, but also that doesn’t seem to be an important optimization.

> Source/WebCore/style/StyleResolver.cpp:309
> +    ElementRuleCollector collector(element, m_ruleSets, context.selectorMatchingState);
> +    collector.setPseudoElementRequest({ elementStyle.styleType() });
> +    if (hasRevert) {
> +        // In the animation origin, 'revert' rolls back the cascaded value to the user level.
> +        // Therefore, we need to collect UA and user rules.
> +        collector.setMedium(&m_mediaQueryEvaluator);
> +        collector.matchUARules();
> +        collector.matchUserRules();
> +    }
> +    collector.addAuthorKeyframeRules(keyframe);

This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.
Comment 6 Oriol Brufau 2022-02-23 09:15:42 PST
Comment on attachment 452969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452969&action=review

>> Source/WebCore/style/StyleResolver.cpp:309
>> +    collector.addAuthorKeyframeRules(keyframe);
> 
> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.

But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.
Comment 7 Darin Adler 2022-02-23 09:30:10 PST
Comment on attachment 452969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452969&action=review

>>> Source/WebCore/style/StyleResolver.cpp:309
>>> +    collector.addAuthorKeyframeRules(keyframe);
>> 
>> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.
> 
> But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.

Good point, we would have to make a copy. For now, I guess we can leave this as-is.

We can later add takeMatchResult which moves it instead of copying. For now, I we can leave this as-is, but I am not sure the current approach is ideal for memory use.
Comment 8 Oriol Brufau 2022-02-23 09:48:15 PST
Comment on attachment 452969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452969&action=review

>> Source/WebCore/style/StyleResolver.cpp:284
>> +        const auto& propertyReference = keyframe.properties().propertyAt(i);
> 
> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.

Not an expert, doesn't "auto" alone make an unnecessary copy? https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference
Comment 9 Darin Adler 2022-02-23 10:05:09 PST
Comment on attachment 452969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452969&action=review

>>> Source/WebCore/style/StyleResolver.cpp:284
>>> +        const auto& propertyReference = keyframe.properties().propertyAt(i);
>> 
>> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.
> 
> Not an expert, doesn't "auto" alone make an unnecessary copy? https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference

No.

The return value of propertyAt is a PropertyReference object. That object will always be copied (or moved). Using the "const auto&" syntax has no effect on that one way or the other.

If the return value was a reference, that would be different, and then I would suggest "auto&", not "const auto&".
Comment 10 Oriol Brufau 2022-02-23 10:11:13 PST
Created attachment 452997 [details]
Patch
Comment 11 Oriol Brufau 2022-02-23 10:16:00 PST
Comment on attachment 452969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452969&action=review

>>>> Source/WebCore/style/StyleResolver.cpp:284
>>>> +        const auto& propertyReference = keyframe.properties().propertyAt(i);
>>> 
>>> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.
>> 
>> Not an expert, doesn't "auto" alone make an unnecessary copy? https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference
> 
> No.
> 
> The return value of propertyAt is a PropertyReference object. That object will always be copied (or moved). Using the "const auto&" syntax has no effect on that one way or the other.
> 
> If the return value was a reference, that would be different, and then I would suggest "auto&", not "const auto&".

Ah, sure, somehow "PropertyReference" made my mind think it was a a reference XD.
Done.

>> Source/WebCore/style/StyleResolver.cpp:292
>> +            hasRevert = hasRevert || value->isRevertValue();
> 
> I would write it this way:
> 
>     if (auto* value = propertyReference.value(); value && value->isRevertValue())
>         hasRevert = true;
> 
> There are many different styles, but once we are doing an if, I think it’s best to include both checks in the if, it’s useful to use auto in a case like this to make sure we don’t accidentally upcast? If we had a helper function, could also consider:
> 
>     hasRevert = hasRevert || isRevertValue(propertyReference.value());
> 
> Might be nice to save the work of getting the value and type-checking it entirely once we found a revert, but also that doesn’t seem to be an important optimization.

Done.

>>>> Source/WebCore/style/StyleResolver.cpp:309
>>>> +    collector.addAuthorKeyframeRules(keyframe);
>>> 
>>> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.
>> 
>> But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.
> 
> Good point, we would have to make a copy. For now, I guess we can leave this as-is.
> 
> We can later add takeMatchResult which moves it instead of copying. For now, I we can leave this as-is, but I am not sure the current approach is ideal for memory use.

OK, I guess I will wait a bit in case Antti says something, otherwise land anyways.
Comment 12 EWS 2022-02-24 13:20:51 PST
Committed r290457 (247758@main): <https://commits.webkit.org/247758@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452997 [details].
Comment 13 Radar WebKit Bug Importer 2022-02-24 13:21:17 PST
<rdar://problem/89436026>