Bug 204554 - transition-property is not computed correctly when transition-duration is set to "inherit"
Summary: transition-property is not computed correctly when transition-duration is set...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-24 02:51 PST by Antoine Quint
Modified: 2020-04-08 09:31 PDT (History)
14 users (show)

See Also:


Attachments
Test (508 bytes, text/html)
2019-11-24 02:51 PST, Antoine Quint
no flags Details
Patch (24.34 KB, patch)
2020-04-08 08:27 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (25.88 KB, patch)
2020-04-08 08:38 PDT, Antoine Quint
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2019-11-24 02:51:49 PST
Created attachment 384250 [details]
Test

See the attached test case which amounts to this simple markup:

<div style="transition-duration: 99s; transition-property: margin-left;"><pre style="transition-duration: inherit;"></li></div>

The transition-property for <pre> resolves to the empty string instead of "all".
Comment 1 Radar WebKit Bug Importer 2019-11-24 02:52:22 PST
<rdar://problem/57458091>
Comment 2 Antoine Quint 2019-11-25 03:07:48 PST
With the old animation engine, the RenderLayer for the animating <div> is removed after the animation completes and the <div> for the text is positioned.

In the new animation engine, the RenderLayer for the animating <div> still has a RenderLayer after the animation completes.
Comment 3 Antoine Quint 2019-11-25 05:37:14 PST
This last comment was for another bug, please disregard.
Comment 4 Antti Koivisto 2019-11-25 06:39:27 PST
I think I see the bug generated style building code. Besides duration this also inherits Animation::animationMode() enum which affects interpretation of Animation::property().


    static void applyInheritTransitionDuration(BuilderState& builderState)
    {
        auto& list = builderState.style().ensureTransitions();
        auto* parentList = builderState.parentStyle().transitions();
        size_t i = 0, parentSize = parentList ? parentList->size() : 0;
        for ( ; i < parentSize && parentList->animation(i).isDurationSet(); ++i) {
            if (list.size() <= i)
                list.append(Animation::create());
            list.animation(i).setDuration(parentList->animation(i).duration());
            list.animation(i).setAnimationMode(parentList->animation(i).animationMode());
        }

        // Reset any remaining animations to not have the property set.
        for ( ; i < list.size(); ++i)
            list.animation(i).clearDuration();
    }
Comment 5 Antti Koivisto 2019-11-25 06:43:05 PST
Code generation is in makeprop.pl
Comment 6 Antoine Quint 2020-04-07 07:47:05 PDT
Specifically, for this code, in generateAnimationPropertyInheritValueSetter.
Comment 7 Antoine Quint 2020-04-08 08:27:45 PDT
Created attachment 395812 [details]
Patch
Comment 8 Antoine Quint 2020-04-08 08:38:06 PDT
Created attachment 395813 [details]
Patch
Comment 9 Antti Koivisto 2020-04-08 08:59:39 PDT
Comment on attachment 395813 [details]
Patch

Nice. Sad that it doesn't flip any WPTs to PASS.
Comment 10 Antoine Quint 2020-04-08 09:03:40 PDT
(In reply to Antti Koivisto from comment #9)
> Comment on attachment 395813 [details]
> Patch
> 
> Nice. Sad that it doesn't flip any WPTs to PASS.

Not quite, but we pass one additional assertion call in one of the failing tests!
Comment 11 Antoine Quint 2020-04-08 09:31:27 PDT
Committed r259720: <https://trac.webkit.org/changeset/259720>