Bug 204554

Summary: transition-property is not computed correctly when transition-duration is set to "inherit"
Product: WebKit Reporter: Antoine Quint <graouts>
Component: CSSAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, dstockwell, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, koivisto, kondapallykalyan, macpherson, menard, pdr, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test
none
Patch
none
Patch koivisto: review+

Antoine Quint
Reported 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".
Attachments
Test (508 bytes, text/html)
2019-11-24 02:51 PST, Antoine Quint
no flags
Patch (24.34 KB, patch)
2020-04-08 08:27 PDT, Antoine Quint
no flags
Patch (25.88 KB, patch)
2020-04-08 08:38 PDT, Antoine Quint
koivisto: review+
Radar WebKit Bug Importer
Comment 1 2019-11-24 02:52:22 PST
Antoine Quint
Comment 2 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.
Antoine Quint
Comment 3 2019-11-25 05:37:14 PST
This last comment was for another bug, please disregard.
Antti Koivisto
Comment 4 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(); }
Antti Koivisto
Comment 5 2019-11-25 06:43:05 PST
Code generation is in makeprop.pl
Antoine Quint
Comment 6 2020-04-07 07:47:05 PDT
Specifically, for this code, in generateAnimationPropertyInheritValueSetter.
Antoine Quint
Comment 7 2020-04-08 08:27:45 PDT
Antoine Quint
Comment 8 2020-04-08 08:38:06 PDT
Antti Koivisto
Comment 9 2020-04-08 08:59:39 PDT
Comment on attachment 395813 [details] Patch Nice. Sad that it doesn't flip any WPTs to PASS.
Antoine Quint
Comment 10 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!
Antoine Quint
Comment 11 2020-04-08 09:31:27 PDT
Note You need to log in before you can comment on or make changes to this bug.