Cannot animate 'all' using Web Animations
Categories
(Core :: DOM: Animation, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox68 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(1 file)
Test case: https://output.jsbin.com/veriyab
My guess is that Servo_Property_IsAnimatable is returning false for 'all'.
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 1•5 years ago
|
||
It turns out we've deliberately made all not animatable in order to avoid attempting to animate display:
We should probably restore all to being animatable and just make sure to skip trying to animate display.
|
|
Assignee | |
Comment 2•5 years ago
|
||
We make sure to ignore display when we read keyframes for CSS animations (e.g. Servo_StyleSet_GetKeyframesForName, get_animated_properties) or from Web Animations (KeyframeUtils::IsAnimatableProperty) but we don't filter them out when building up a KeyframeEffect's set of (longhand, physical) properties from shorthands.
When we build up a KeyframeEffect's set of properties, AnimationValue::from_declaration will filter out non-animatable properties but it doesn't apply the check for 'display' since it is used by SMIL too (where 'display' is animatable).
We could try to add a check in AnimationValue::from_declaration but since that is used by SMIL it might be easiest to just check it in maybe_append_animation_value in Servo_GetComputedKeyframeValues instead.
We already have a crashtest that should fail if we try to animate display from Web Animations, but it might be worth adding a getProperties() chrome test for this too plus a WPT that all is, in fact, animatable.
|
||
Comment 4•5 years ago
|
||
You sure this is a regression from bug 1514086? I'm pretty sure it was not animatable before-hand either, see the commit message from that bug, which points to:
https://hg.mozilla.org/mozilla-central/rev/6884ba750aa3
Which was the regressing patch, and used shorthands_except_all() (so all returned false).
|
|
Assignee | |
Comment 5•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
You sure this is a regression from bug 1514086?
Nope. But I figured that since we were crashing with content with 'all' before that patch we were doing something with 'all' at that point.
|
|
Assignee | |
Comment 6•5 years ago
|
||
So, running ./mach mozregression --launch a5cc88a8a6c3 (the parent revision of 6884ba750aa3, i.e. bug 1504536) shows this used to work before then. Doing the same with the parent revision of bug 1514086 does not.
|
|
Assignee | |
Comment 7•5 years ago
|
||
|
|
||
Comment 8•5 years ago
|
||
(In reply to Brian Birtles (:birtles, away Apr 29 - May 6 for Golden Week) from comment #5)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
You sure this is a regression from bug 1514086?
Nope. But I figured that since we were crashing with content with 'all' before that patch we were doing something with 'all' at that point.
Sure, but that's because bug 1504536 made it return true for all. Before that it didn't use to. If I launch Firefox 64 (which doesn't have any of those bugs), I can see the same result as with today's nightly.
So this only worked since bug 1504536 landed and only until bug 1514086 restored the (broken) behavior.
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ffc6d61950a5 Allow animating the 'all' property from Web Animations; r=emilio
|
||
Comment 10•5 years ago
|
||
| bugherder | ||
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16583 for changes under testing/web-platform/tests
Description
•