Closed
Bug 1253476
Opened 9 years ago
Closed 6 years ago
Collapse filling script-generated animations
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla69
People
(Reporter: birtles, Assigned: birtles)
References
(Blocks 1 open bug)
Details
(Whiteboard: [layout:backlog:2019q3:69][wptsync upstream])
Attachments
(19 files, 3 obsolete files)
|
(deleted),
text/plain
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
image/png
|
Details | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
text/x-phabricator-request
|
Details | |
|
(deleted),
text/x-phabricator-request
|
Details |
Object diagram
Currently someone can write:
elem.addEventListener('click', evt => {
evt.target.animate({ opacity: [ 0, 1 ] },
{ duration: 500, fill: 'forwards' });
});
And we'll generate a new animation every time elem is clicked. What's more, we'll keep it alive forever because it fills forwards. Over time the memory consumed will grow in an unbounded fashion.
This is fundamentally a spec issue since the spec requires getAnimations() to return filling animations. The solution at the spec level is not yet clear. Most recently we discussed collapsing filling animations into FillAnimation objects. Some notes are at [1] under the heading "Discarding forwards filling animations"
It's fairly involved so we may decide not to try implementing that.
Chrome currently ships Element.animate() but it internally collapses filling animations. It doesn't ship getAnimations(), however.
Our DevTools is built on top of getAnimations() so we need to come up with some way of collapsing these animations that won't break DevTools. If we decide to go ahead and implement FillAnimations then we might consider exposing them in DevTools too.
[1] https://docs.google.com/document/d/1lQbgJ0fZYGn1_qboj59G-MOILK33z4kmBdjgtSHF2gA/edit#
|
Assignee | |
Comment 1•9 years ago
|
||
Discussed with Cameron and agreed it is probably ok to ship Element.animate without this provided we fix it in the next release. This will be my top priority for Q2.
No longer blocks: 1245000
|
|
Assignee | |
Comment 2•9 years ago
|
||
Since it's nearly the weekend here, I thought I'd write down my current thoughts.
First, this is based on the proposal linked to at the end of comment 0. This proposal is super-complicated but I honestly can't think of anything better.
Assuming we go with that, we could do something like this:
* First, we need to to represent the fill values. Something like:
struct FillValue {
nsCSSProperty mProperty;
nsCSSValue mValue;
CompositorOperation mComposite; // Not implemented yet
};
* For each forwards-filling animation that we've collapsed, we need to
store all the properties that fill forwards. So we might use something
like:
typedef nsRefPtrHashtable<nsGenericHashKey<nsCSSProperty>,
FillValue> FillValueSet;
The reason for the hashmap is that when we collapse animations we'll
need to work out what properties are already set.
An alternative that may be simpler, is to simply keep around an
nsCSSPropertySet to track "set properties" and then when we collapse
go backwards from higher-priority to lower-priority and skip any
properties that are already set.
That's a pattern we use elsewhere and is probably better. Then we
could make FillValueSet a simple array.
* Next we need a way to represent a tree where the root node is
a FillEffect and each node contains an ordered sequence of children
where each child is either a FillValueSet or another FillEffect.
(This comes about because of the way the proposal allows handing out
FillAnimation objects at different stages and having them remain valid
so long as they are being kept alive.)
One approach is to use MaybeOneOf or Variant to represent this kind of
union of values.
So, something like:
typedef Variant<FillValueSet, FillEffect> FillValuesOrEffect;
struct FillEffect {
// Use SegmentedVector?
nsTArray<FillValuesOrEffect> mStack;
WeakReference<Animation> mAnimation;
// Walks through the stack and gets the value for each property
// (Used when building a new FillAnimation and when collapsing
// children.)
void AddValuesToSet(FillValueSet& aValueSet);
void Compact();
void Cancel();
};
The bit I'm currently stuck on is the data structure for mStack.
I'd like to use something like an array and keep it very simple and
also avoid too many small heap allocations. That would suggest that
any generated FillAnimation does *not* keep a reference back to the
FillEffect.
However, the one case where we might need that is when a FillAnimation
is cancelled. We *could* just implement that by searching through the
tree for the FillEffect whose mAnimation member points to
FillAnimation but that sounds quite inefficient if the author does:
elem.getAnimations({ type: 'fill' }).forEach(anim => anim.cancel());
Instead, I wonder if we can do a modified weak reference that tracks
whether a FillAnimation has been cancelled. When the FillAnimation is
cancelled we would set a flag on the proxy object / hashtable and mark
the EffectSet as needing compacting. Then inside Compact we could
implement the actual cancelling.
Likewise, whenever a FillAnimation is destroyed (due to GC), we would
set the same flag.
* As for the other parts of the implementation, Compact() would
essentially perform the following for each item in mStack:
- If we have a FillEffect whose corresponding FillAnimation is no
longer alive we call AddValuesToSet() on it and replace it with
a FillValueSet.
- If we have two FillValueSets in a row, we merge them into a single
FillValueSet.
- If we have a FillEffect whose corresponding FillAnimation has
been cancelled we should simply add nothing to the set and indicate
that we can be removed (perhaps we return false or something).
At the root level too we'd need to indicate if the FillEffect is now
empty.
* The next tricky bit is integrating this into EffectCompositor and
EffectSet. If possible, I'd like to avoid making FillEffect inherit
from AnimationEffectReadOnly or something like that since that sounds
heavyweight.
I'd also like to avoid introducing another abstract interface just for
this.
So, at the moment I'm thinking we could simple maintain two sets in
EffectSet: the hashset of nsRefPtr<KeyframeEffectReadOnly> that we
currently have representing running animations, and a separate lot of
FillEffects.
We'd need to annotate the root FillEffect with the mAnimationIndex of
the Animation it is replacing. Perhaps we have a separate Tuple value
for that.
Then, in EffectCompositor when we go to sort the effects for
compositing, we'd sort them into a single array of some sort of
union/Variant type. This would add a bit of conditional code but
I think it would be mostly confined to
EffectCompositor::ComposeAnimationRule.
EffectCompositor::UpdateCascadeResults will need similar treatment.
The other methods in EffectCompositor which deal with compositor
animations could ignore forwards-filling animations since we never
send them to the compositor.
One tricky area is UpdateEffectProperties. If an animation animates to
"3em" and the font-size changes, we need to make sure filling
animations using that value get updated (that's why we store the fill
value as an nsCSSValue in the first place). I'm not quite sure how
that will work out yet.
* The next question regards when we create FillEffects. Do we create
a FillEffect as soon as an animation begins forwards-filling?
Although that seems simplest and perhaps optimal, the complication is
if script still holds a reference to the original Animation and
restarts it. In that case we would need to drop the FillEffect and
re-add the re-started animation to the EffectSet.
On the other hand, we *need* to drop the reference to that Animation
or else we'll never be able to reclaim the memory associated with it.
I don't know if there's any way to determine if an object is being
kept alive by script only. Assuming there's not, then we probably do
need to find an efficient way to re-instate an Animation and drop its
FillEffect.
Perhaps it might be helpful is to imagine the behavior in the
following case:
const originalAnim = elem.animate({ opacity: 0 },
{ fill: 'forwards',
duration: 3000 });
originalAnim.finish();
const fillAnim = elem.getAnimations({ type: 'fill' })[0];
// fillAnim here returns an animation with a single keyframe with
// opacity: 0.
originalAnim.play();
// Is fillAnim now idle? Did it get cancelled? I guess it did.
That would suggest that we *do* actually create a FillEffect as soon
as an Animation starts filling forwards and we *do* have to find it
and remove it if the Animation is restarted.
But how do we even implement that? Particularly if we allow authors to
register a callback with fillAnim.oncancel?
Do we maintain a reference, somehow, from an Animation to its
FillAnimation?
This is all quite complicated.
(We'd also need to take care to handle dynamic changes to the fill
mode.)
In conclusion: I hope there is a simpler approach.
|
|
Assignee | |
Comment 3•8 years ago
|
||
I put a bit more thought into how to tackle this and here are my notes so far.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 5•7 years ago
|
||
I need to fix this before we ship implicit from/to keyframes since once we allow that people will want to generate these throw-away forwards filling animations more often.
Blocks: 1398037
|
||
Updated•6 years ago
|
Priority: -- → P3
|
|
Assignee | |
Comment 6•6 years ago
|
||
I started writing patches for this but I've come across a bump regarding how to represent fill values. At first I naively imagined we could just take the 100% keyframe values. However, it's more complicated than that:
* If direction is 'reverse' (or 'alternate' or 'alternate-reverse' with
a suitable iteration count) we'll want the 0% values
* If there is a non-integral iteration count we'll want the interpolated value
* Similarly if we have an integral iteration count and a non-integral
iteration start value
* If there is an iterationComposite mode we'll want the composited value
* If there are em-based units, we'll want them to continue to respond to changes
in the font-size which suggests we want to keep the _specified_ values
* Specified values can overlap when we have shorthands and longhands
* If we have an implicit keyframe and a non-integral iteration count we'll
actually need to represent a fraction of the interpolation between the
underlying value and the fixed keyframe value
Which all sounds like trying to interpolate specified values (recall that interpolation is defined between computed values, not specified values). For some types we can do that easily enough by using calc(), but not for all (e.g. interpolating between hsla() and a system color. There's no way to represent it as a specified value that preserves the system color-ness).
Perhaps for some cases we actually need to actually store a pair of properties and an offset. Even then, I'm not sure we can handle all cases.
For example, what if we have content that does:
-> Animation from underlying to rgba(...), iterationCount: 0.8
-> Animation from underlying to MenuText, iterationCount: 0.7, composite: add
How do we represent that fill value? It seems like we need a stack of values internally. And then I guess we just return the computed value via the API?
Only returning computed values from the API would be unfortunate because it means the API doesn't fully explain what's going on--there's some internal state that impacts the computed value that can't be inspected. On the other hand not storing specified values, and therefore not responding to changes in context would be worse.
Maybe we don't make FillEffect a subclass of KeyframeEffect--maybe it's a special thing that just returns the currently applied fill values as computed values and that's just the deal: FillEffects and FillAnimations don't tell you everything. They just represent a snapshot of what happened in the past. They might update, you can't know. If you really want full control, you need to keep all the original Animations alive or cancel that FillAnimation and replace it with something you do control.
The advantage of not exposing the internal representation is that it allows implementations to optimize it progressively. For example, you could decide to never coalesce animations with 'composite: add' initially, but just maintain the full stack. Later if such content becomes popular, you could further optimize that behavior--all without any observable changes or having to coordinate with specs and other browsers. This coalescing is pretty hard, so setting a low initial bar for implementations seems good.
|
|
Assignee | |
Comment 7•6 years ago
|
||
(I forgot to add to my previous analysis the result of applying a timing function and a non-integral iteration count.)
I think to represent fill values we need:
* A stack -- the current design already includes this. The change is that
a nested FillAnimation is also potentially represented by a stack so we
probably need to traverse into nested FillAnimations when composing.
* Potentially two specified values per property. e.g. animating from
'var(--inactive)' to 'var(--active)'
* The computed value(s) -- we should probably re-use the UpdateProperties
mechanism to store computed values that we update as needed.
* The composition modes.
It would be nice if we can also optimize for the common case: A single computed value (that is not context-sensitive).
That still doesn't answer how we deal with overlapping shorthands and longhands however. (Remember that we can have margin: 'var(--margin)' where '--margin' could be "4em" or "2px 90vw". i.e. we don't know how to map marginLeft to margin without knowing what the variable expands to.)
Perhaps we will end up having a bunch of FillValues of the form:
struct FillValue {
RefPtr<RawServoDeclarationBlock> mSpecifiedValue; // May be null if the value
// is not context-sensitive
AnimationValue mComputedValue; // This might also be null to represent the
// underlying value
dom::CompositeOperation mCompositeOperation;
}
struct FillSnapshot {
nsCSSPropertyID mProperty; // Could be a shorthand
nsTArray<FillValue> mValues; // Either 1 or 2 elements
//
// Should the above be just mozilla::Array<FillValue, 2> ?
//
// Or even:
//
// Maybe<FillValue> mFromValue;
// FillValue mToValue;
//
// ?
double mPortion; // Will be 1.0 if there is only one value
}
|
||
Comment 8•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #6)
> -> Animation from underlying to rgba(...), iterationCount: 0.8
> -> Animation from underlying to MenuText, iterationCount: 0.7, composite: add
This is an interesting test case. Awesome! Even it's just an animation without composite;
> -> Animation from underlying to MenuText, iterationCount: 0.7
I am pretty sure there is no test cases checking the filling animation value when the underlying style changes.
(I think Brian has already written such test cases locally)
|
|
Assignee | |
Comment 9•6 years ago
|
||
Yes, you're right. We have tests for context changes during the active interval:
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-context.html
But not during filling. We'll need to add them in this bug.
Testing for changes to system colors is probably going to be impossible (and not that important) but changes to font-size and variables should be possible. Testing for changes to viewport size (for vw/vh units) would be good but I don't think we can do that in wpt.
|
|
Assignee | |
Comment 10•6 years ago
|
||
I realized there's another case where this is going to be difficult:
-> Animation from 10px / composite: add to 50px / composite: replace with iterationCount: 0.5
at 50% of the way through that interval we're doing _both_ add and replace.
The question is, what should getKeyframes() return here?
We _could_ say that it returns the _pair_ of keyframes but what about if we have more than just one animation represented by the FillAnimation. e.g.:
-> Animation from 10px / composite: add to 50px / composite: replace with iterationCount: 0.5
-> Animation from underlying to 100px / composite: add
We can represent that internally using the structure described in comment 7, but what should getKeyframes() return?
Perhaps it should just represent the result as applied to the target element at the current time with composite: replace?
|
|
Assignee | |
Comment 11•6 years ago
|
||
Currently stuck working out how to get the corresponding specified value for each property. I'm not even sure it's going to be possible.
|
|
Assignee | |
Comment 12•6 years ago
|
||
Part of the problem of finding the specified value for each computed value is that we'd effectively need to navigate the specified to computed traversal in reverse including:
a. Logical -> physical property mapping
b. Overlapping shorthands / longhands
However, maybe rather than doing a precise reverse-mapping, we just do something like the following:
1. For each property we are trying to generate a fill snapshot for, find the relevant keyframe offset(s) for the interval / point to preserve [easy]
2. Look up all the original keyframes at that offset (which include the specified values) [hardest part here is just making sure we handle overlapping keyframes correctly]
3. At this point we could even just keep the keyframes as-is but that would be wasteful, but we could probably trim the ServoDeclarationBlock to those properties that could influence longhand physical property |p| by being either:
a) a shorthand that sets |p|
b) a logical property that _can_ map to |p|
c) a logical shorthand (not yet supported) that _can_ set |p|
So, for example, if we are generating the snapshot of an animation of margin-left, we'd save and trim any ServoDeclarationBlock that included 'margin', 'margin-left' (regardless or not of whether the 'margin' declaration clobbered the 'margin-left' one), 'margin-inline-start', etc. That sort of restricted reverse mapping seems tractable and, for the common case, would probably be pretty minimal in terms of storage.
Ideally we would also then be able to detect if any of the declarations in the preserved ServoDeclarationBlocks were context-sensitive, and, if none are, drop them altogether. I'm not sure how easy that is to do, however.
(And I'm hoping Emilio might have some hints here!)
|
|
Assignee | |
Comment 13•6 years ago
|
||
This is just a progress patch as a backup. It's probably got about 25% of the required functionality.
I'm currently working on KeyframeEffect::GetFillSnapshot. Then it will be FillAnimation::WrapAnimation. Then additional merging in FillAnimation::CompactEffects.
|
|
Assignee | |
Comment 14•6 years ago
|
||
Another complication: even though we don't animate custom properties yet, we need to maintain them along with any specified values since you can have:
@keyframes anim {
from {
background-color: blue;
}
to {
--green: green;
background-color: var(--green);
}
}
div {
animation: anim 3s forwards;
}
(Pen: https://codepen.io/birtles/pen/YjJaMY)
That will animate from blue to green and we need to remember that the fill-color is green in the specified value.
Ultimately, if --green itself is being animated we could just maintain it as a separate fill value (once we animate custom properties), but we'll still need to preserve un-animated custom properties I guess. Ideally we could collapse them here but I'm not sure if that's at all possible.
|
|
Assignee | |
Comment 15•6 years ago
|
||
The only other properties I know of that affect declarations in the same block are 'font-size' and 'direction'/'writing-mode'.
'direction'/'writing-mode' are not animatable so I'm pretty sure they just get ignored here. 'font-size' is animatable so if it appears in a keyframe we should end up preserving it as an fill property so there's no need to do any special handling for it, I believe.
So I think custom properties are the only case where we have a property that both affects declarations in the same block but which we don't otherwise preserve as an animatable property.
Digging into this a little further I see that we do actually preserve custom properties, but only for CSS Animations. This is a little tricky because if you inspect the result of getKeyframes() it appears we don't preserve them, but that's because we simply don't serialize them.[1]
Given that we don't coalesce CSS animations, I don't think we actually need to do anything special here for now.
[1] https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/animation/KeyframeEffect.cpp#1130-1134
|
|
Assignee | |
Comment 16•6 years ago
|
||
Here's a patch with a bit more filled-in.
My next steps here are:
- Try filling out UpdateProperties in FillNode / FillEffect
(Involves working out if we should _also_ set the snapshot when we have
a linked animation. I suspect we shouldn't.)
- Likewise for ComposeStyle and GetKeyframes
- Re-visit the linked list structure for nodes --- it's seems awfully fragile.
Perhaps we can make it a little more robust by not having nodes remove
themselves but just return something to the FillEffect that lets it know to
remove them (and free them at the same time).
If all that works out, then that probably covers the most unknown parts of the design and I should step back and focus on building up a subset of it that doesn't do any compacting at all but simply creates one FillAnimation per finished forwards-filling Animation. That would allow us to test the snapshotting behavior pretty well and could possibly even land by itself if need be.
Attachment #8998106 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 17•6 years ago
|
||
I got a little further with UpdateProperties but I'm starting to have doubts about this design.
The particular issue I'm facing at the moment is compositor animations. Filling animations can run on the compositor when they are part of a stack that runs on the compositor.[1]
The proposal in this bug is that we maintain a tree of filling animations.
The nodes in the tree are one of two types:
a) Animation values from filling animations that have gone away
b) Pointers to Animations (including FillAnimations) that are still in use (or simply yet to be GC'ed)
While many kinds of animation values (a) can be collapsed together, some can't. Also we can't collapse (a) with (b), however, since (b) could still change or be canceled.
So we end up with a list that might look like:
(a), (b), (a)
The reason this can become a tree is that (b) can point to a FillAnimation which itself might have such a list.
The tricky part, then, is what if our root FillAnimation is part of an effect stack that needs to run on the compositor?
Initially I was thinking that we'd have this tree of animations and when we compose style etc. we'd just walk that tree. However, we obviously don't want to send that tree to the compositor. We could easily flatten that tree into a series of intervals and send them but as I work on this I'm finding I have to write more and more special case FillAnimation code like this which is making me wonder if this design is wrong.
Ideally we could represent the whole tree of filling effects as a standard set of keyframes/properties in the root FillAnimation and send that to the compositor.
However, as pointed out in comment 6 and comment 7, it seems there are some things we can't represent as a standard set of keyframes/properties.
One of the examples from those comments is,
-> Animation from 10px / composite: add to 50px / composite: replace with iterationCount: 0.5
-> Animation from underlying to 100px / composite: add
The fill result for this is basically:
= (underlying + 10px) * 0.5 + 0.5 * 50px + 100px
Ideally, we would like to simplify this to something like:
= (underlying + 110px) * 0.5 + 0.5 * 150px
which we could then represent as regular keyframes: { 110px, composite: add } and { 150px }
But can be we actually do that?
We know that addition is not commutative[2] but what really need for this simplification to hold, is for addition to be distributive over interpolation. That is, we are trying to go from:
add(interpolate(add(underlying, 10px), 50px, 50%), 100px)
to:
interpolate(add(underlying, 10px, 100px), add(50px, 100px), 50%)
Or more generally:
add(interpolate(A, B, x%), C) == interpolate(add(A, C), add(B, C), x%)
Similarly for 'accumulate'.
For a few cases I tried, this seemed to hold but the rules for interpolating transform lists[3] are so full of edge cases I'm afraid there are many cases where it doesn't.
But supposing it does, or that we can fix the cases where it doesn't, we'd still need to generalize this further.
Consider this slightly extended version of the previous example that adds a non-intergral iterationCount to the second animation:
-> Animation from 10px / composite: add to 50px / composite: replace with iterationCount: 0.5
-> Animation from underlying to 100px / composite: add with iterationCount: 0.3
This is equivalent to:
α = interpolate(add(underlying, 10px), 50px, 50%)
result = interpolate(α, add(α, 100px), 30%)
Dropping the numbers and expanding it out:
interpolate(interpolate(add(underlying, A), B, X%), add(interpolate(add(underlying, A), B, X%), C), Y%)
And we want to turn that into something we can represent using regular keyframes.
Distributing add over interpolate simplifies it a little:
interpolate(interpolate(add(underlying, A), B, X%), interpolate(add(underlying, A, C), add(B, C), X%), Y%)
But it's still not something we can represent using keyframes which needs to be of the form:
interpolate(add(...), add(...), x%)
As far as I can tell, if we want to simplify that any further, we'd need to (re-)introduce a neutral/zero value concept. That is, we need to be able to take:
interpolate(add(underlying, A), B, X%)
And substitute an zero value for 'underlying', calculate the result and then end up with just:
add(underlying, <result>)
If we can do that, we can represent this using regular keyframes and it all gets a lot simpler.
I'm just dubious we can actually do that, especially when transform lists are involved. Issue #2204[4] seems to suggest likewise.
[1] https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/dom/animation/EffectCompositor.cpp#118-120
[2] since `translate(10px) rotate(45deg)` != `rotate(45deg) translate(10px)` and more generally matrix multiplication is not commutative
[3] https://drafts.csswg.org/css-transforms/#interpolation-of-transform-functions
[4] https://github.com/w3c/csswg-drafts/issues/2204
|
|
Assignee | |
Comment 18•6 years ago
|
||
> As far as I can tell, if we want to simplify that any further, we'd need to (re-)introduce a neutral/zero value concept. That is, we need to be able to take:
>
> interpolate(add(underlying, A), B, X%)
>
> And substitute an zero value for 'underlying', calculate the result and then end up with just:
>
> add(underlying, <result>)
On further thought, I don't think that's right. It doesn't capture the percentage of the underlying value to use in the weighted sum.
We really need to end up with a form such as:
interpolate(underlying, add(...), x%)
That is, ultimately the final result will be some portion of the underlying value, and some fixed value. The real question is whether or not we can calculate that fixed value while preserving all the nuance of the series of add/interpolate steps that would normally get us to arrive at a result.
Given our previous example:
-> Animation from 10px / composite: add to 50px / composite: replace with iterationCount: 0.5
-> Animation from underlying to 100px / composite: add with iterationCount: 0.3
For the first animation we're asking for:
50% of 10px + underlying, and 50% of 50px
which at least conceptually is equivalent to asking for:
50% of underlying, and 50% of 60px
i.e. { underlying }, { 60px }, progress: 0.5
For the second animation we're asking for:
70% of underlying, and 30% of 100px + underlying
which is conceptually equivalent to:
100% of underlying, and 30% of 100px
i.e. { underlying }, { 30px, composite: add }, progress: 0.3
There should be a way to harmonize those forms so that we end up with a single pair of values and a single progress value.
Normalizing the progress value is easy, but otherwise the behavior of the two different modes is pretty different.
I need to think about this more, but at this point I wonder if we could just normalize the progress values to 0.5 by extrapolating endpoints as needed, and then just combine all the endpoints together.
That's still going to run into possible trouble with transform lists but maybe they'll be solveable after all. https://github.com/w3c/csswg-drafts/issues/2204 seems to work for me now.
|
|
Assignee | |
Comment 19•6 years ago
|
||
I haven't quite clarified my thoughts here but I'm starting to suspect there might be a set of rules where we can accurately combine the different combinations:
* Fully-replaced fill value
* Fully-additive/accumulative fill value
* Fully-underlying fill value
* Partial underlying + replaced fill value
* Partial underlying + additive/accumulative fill value
* etc.
If that works then we would could stick to the internal representation outlined in comment 7 (and further developed in attachment 8999096 [details] [diff] [review]) but then periodically walk the tree of fill animations to end up with a single pair of values we use to represent the overall effect in a manner that still correctly responds to changes in the underlying value.
In a sense we would end up with three representations of the fill state:
1. The representation that preserves specified values so we can respond to changes in context (CSS variables etc.)
2. The representation we use for compositing, passing to the compositor etc. (computed values but with the effect of underlying values accurately represented)
3. The representation we return from getKeyframes() (computed values only, underlying values computed-in).
We _could_ merge (2) and (3) but it would require specifying some possibly non-trivial behavior. It might be easier to just require the computed values and let implementations work out how they're going to represent that internally. Certainly combining those could be a separate bug anyway.
The real question is just whether or not we can go from (1) to (2) without changing the result.
I wonder if there is some automated way to verify that. I could write some simple JS-based tests for it, and then just implement it and add assertions to compare the two results. I'm a bit concerned that it will be quite a bit of work to get the implementation up to the point where I could actually trust the results enough to verify the approach.
|
|
Assignee | |
Comment 20•6 years ago
|
||
(Just updating this in case I decide to work from home tomorrow.)
Attachment #8999096 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 21•6 years ago
|
||
I've written up my thoughts on this in a doc which I hope will serve both as a background for whatever unfortunate people have to review this and also a starting point for writing the spec text:
https://docs.google.com/document/d/1i2VfcfqyQdrmfTKPvPYbRCS1YY4hd--CZJgl8-TFYW8/edit?usp=sharing
It will no doubt change a lot as I continue working on this.
I've split up the work into four phases:
- Phase 1: 1-1 FillAnimation and Animation -- still using Animation for compositing though
- Phase 2: Composite from FillAnimation
- Phase 3: Allow a FillAnimation to represent multiple filling animations
- Phase 4: Compact FillAnimations
I've done the patches for Phase 1 and quite a few tests and hope to make progress on Phase 2 next week.
|
|
Assignee | |
Comment 22•6 years ago
|
||
Phase 1:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3da6276a05081c395a652004e23f7a0d870cd03d
(Although there's about one patch that doesn't show up there because it was in a previous try run)
|
|
Assignee | |
Comment 23•6 years ago
|
||
I started working on Phase 2 and it turns out we need quite a bit of extra work in order to get getKeyframes() to work on FillAnimations.
I'm currently trying to make sense of `servo-owned-types` and so on since it turns out I need to make a RawServoAnimatedValueMap on the Gecko side in order to implement this, and bindings.rs is producing duplicate definitions once I wrap that SERVO_ARC_TYPE.
|
|
Assignee | |
Comment 24•6 years ago
|
||
I'm going on PTO for a few days and will be busy for much of the rest of the month so I'm just putting up what I have so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa80eba15598f361a624360480b8ce8e4878543
|
|
Assignee | |
Comment 25•6 years ago
|
||
(For what it's worth, some of those tests are expected to fail. Getting `getKeyframes.html` to pass is my next task. Bug
1488122 is the reason for some of the other failures.)
|
|
Assignee | |
Comment 26•6 years ago
|
||
|
|
Assignee | |
Comment 27•6 years ago
|
||
I had another quick look at this today. My current status is I have getKeyframes.html passing and I'm working on making filling effects have their own copy of the keyframes they mirror--or actually a summarized version.
In attachment 8999842 [details] [diff] [review] I realized it will be more maintainable long-term if we can make these filling effects produce generic keyframes so that, for example, we don't need any special case logic for compositor animations. I'm currently working on making FillEffect's rehydrate keyframes from a snapshot summary from their mirrored Keyframes. The idea is that by the end of the patch queue we'll have a series of snapshots we flatten into a set of keyframes.
These snapshots record the progress we're using for each property at the point where it fills. One hiccup I've come across is that this progress can be > 1 or < 0. However, generally we never sample outside [0, 1] unless we have a certain kind of cubic bezier timing function. In order to represent these out of range filling values we'll either need to:
a) Re-constitute a timing function which will produce the required value (probably quite tricky).
b) Introduce a new type of timing function only used internally that produces the required value.
c) Preserve the initial timing function when needed and adjust the offsets appropriately.
I'm leaning towards (c) although (b) is also attractive since we could use the same mechanism for fractions between [0, 1] as well and not need to adjust/match up keyframe offsets.
|
|
Assignee | |
Comment 28•6 years ago
|
||
I dug into this a bit further and wrote up my findings in the doc where I've been describing all this: https://docs.google.com/document/d/1i2VfcfqyQdrmfTKPvPYbRCS1YY4hd--CZJgl8-TFYW8/edit#heading=h.9wtif6i5p3yj
From that doc:
"In summary, not all types have a suitable zero value that will allow us to combine stacks of effects into a simple set of keyframes that represents the whole stack."
In other words, it seems like we are going to need a fair amount of special case code for this after all :(
In particular, all our methods that dig through an animation's keyframe effect looking for things like "what is the maximum scale factor of this animation?", "does this have any geometric properties?", "can this animation run on the compositor?", etc. will need to deal with the possibility of an animation having multiple sets of keyframes. Furthermore, when we generate animations for the compositor we will need to potentially generate multiple compositor animations from a single Animation object.
That sounds like a lot of code that will only get tested in the rare case of multiple filling animations on the same property that can't be collapsed.
I wonder if we can spec this differently somehow to avoid that.
|
|
Assignee | |
Comment 29•6 years ago
|
||
I've been trying to work out what simplifications we actually need to hold in order to collapse stacks of values. My working so far follows.
In general:
result = (L acc(n) (U compA A)) interp(p) (L acc(n) (U compB B))
Limiting the set of composite operators to just 'replace' and 'add' for now we have three cases:
A) Both are 'replace'
result = (L acc(n) A) interp(p) (L acc(n) B)
Which produces a value independent of the underlying value.
B) Both are 'add'
result = (L acc(n) (U add A)) interp(p) (L acc(n) (U add B))
Here we hope that this is the same as:
result = U add [(L acc(n) A) interp(p) (L acc(n) B)]
=> Need to prove this holds
C) One side is 'add'
result = (L acc(n) (U add A)) interp(p) (L acc(n) (U replace B))
= (L acc(n) (U add A)) interp(p) (L acc(n) B)
We hope that this is basically the same as:
result = [U interp(p) 0] add [(L acc(n) A) interp(p) (L acc(n) B)]
=> Need to prove this holds
Supposing that is true and there is a suitable 0 for each type,
how will this help us to combine adjacent effects?
Suppose we have two effects, X and Y, where Y is on top of X.
Let's also suppose X has an from-comp of 'add' and Y as a to-comp of 'add'.
We have:
resultX = (L acc(n) (U add A)) interp(pX) (L acc(n) (U replace B))
= [U interp(pX) 0] add [(L acc(n) A) interp(pX) (L acc(n) B)]
= [U interp(pX) 0] add <some fixed value>
resultY = (L acc(n) (U replace A)) interp(pY) (L acc(n) (U add B))
= (L acc(n) A) interp(pY) (L acc(n) (U add B))
= [0 interp(pY) U] add [(L acc(n) A) interp(pY) (L acc(n) B)]
but U here is actually resultX, so we have:
resultY = [0 interp(pY) resultX] add [(L acc(n) A) interp(pY) (L acc(n) B)]
= {0 interp(pY) [(U interp(pX) 0) add <some fixed value>]} add [(L acc(n) A) interp(pY) (L acc(n) B)]
= {0 interp(pY) [(U interp(pX) 0) add <some fixed value>]} add <some fixed value>
At this point we hope that this is equivalent to
= 0 interp(pY*pX) U add <some other fixed value>
=> Need to prove this
i.e. we should be able to reduce a stack of such values into a portion of U combined with some fixed value.
If that all checks out, then at least for 'add' and 'replace' we should be ok.
There might be a way to get this to work with the 'accumulate' composite mode by adding a similar bucket for the amount of the underlying value to accumulate with but I'm really not sure if that will work.
Once we have the above tests written we should be able to add a test for composite. It should fail, but we might discover that it works for some subset of values (e.g. matrices only).
|
|
Assignee | |
Comment 30•6 years ago
|
||
This is my initial attempt at a test case to prove the first identity (B) in comment 29.
It still needs to incorporate iteration composite modes and values of p other than 0.5 but so far we can see that the identity _mostly_ holds. It fails for some particular skew() values but I need to generate actual visual tests from the failing tests to see if the difference is observable in those cases.
Unfortunately I'm not going to be able to test filters in the same way since they don't reduce to the same a canonical computed value in the same way. They would have to be tested by reftests.
|
|
Assignee | |
Comment 31•6 years ago
|
||
Test case finally finished running reporting:
> Tested 2097152 combination(s). 52946 Failure(s).
An example of some of the failing test cases:
> underlying: matrix(1, 0, 0, -1, 0, 0)
> from: translate(-10%)
> to: rotate(180deg)
> expected: matrix(-8.74228e-8, -1, -1, 8.74228e-8, -91.25, 0.00000797733)
> actual: matrix(-8.74228e-8, -1, -1, 8.74228e-8, -91.85, 0.00000802978)
> underlying: skew(720deg, 30deg)
> from: skewX(-180deg)
> to: skew(90deg, 90deg)
> expected: matrix(-3535, 1494.42, 3535.71, 5577.41, 0, 0)
> to: matrix(-3535, 1494.43, 3535.71, 5577.4, 0, 0)
I probably need to bump up the tolerance for comparisons.
|
|
Assignee | |
Comment 32•6 years ago
|
||
With a tolerance of 0.5 I get:
> Tested 2097152 combination(s). 17744 Failure(s).
Even so the failure seem to be mostly just precision issues:
> underlying: scale(2)
> from: translate(10%)
> to: rotate(90deg)
> expected: matrix(1.41421, 1.41421, -1.41421, 1.41421, 182.5, 0)
> actual: matrix(1.41421, 1.41421, -1.41421, 1.41421, 183.7, 0)
> underlying: scale(1, 0)
> from: translate(10%)
> to: skewY(-90deg)
> expected: matrix(3536.06, 0, -1767.68, 0, 91.85, 0)
> actual: matrix(3536.06, 0, -1767.68, 0, 91.25, 0)
I need to make up actual test content to see how noticeable this would be (I suspect the first one could be, the second one not).
|
|
Assignee | |
Comment 33•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #32)
> I need to make up actual test content to see how noticeable this would be (I
> suspect the first one could be, the second one not).
There is some slightly noticeable jiggle for the first case: https://codepen.io/birtles/pen/oQBrRN
However, thinking about this whole problem some more I'm concerned that even if addition is distributive, for many properties interpolation is not associative.
For example, transforms. When interpolating matrices we have logic to ensure we interpolate using the smaller angle. Although I've yet to prove it, I suspect there will be some content where if we interpolate the later effects first we will end up making a different decision about which angle is smaller.
Likewise, any list-based property where interpolation falls back to discrete when the function arguments don't match up could cause problems if we end up applying discrete interpolation at a different point.
I'm starting to think that maybe we should abandon trying to achieve a 100% solution. Most of the time the animation will fill using the final keyframe. Provided 'add' is associative, we can collapse adding animations.
We could perhaps keep 'accumulate' as a composite mode but just not try to collapse it.
Likewise, we _could_ keep context-sensitive values and just not collapse them (or only sometimes collapse them such as when they have the same units--although that would mean a lot of extra code).
Then when we find we have a stack of, say, more than 10 animations we could produce a console warning saying, "Your content is not able to be efficiently represented and is consuming a lot of memory. Details: <MDN link>."
Maybe the spec could even allow implementations to forcefully simplify in such cases.
|
|
Assignee | |
Comment 34•6 years ago
|
||
I worked on rehydrating keyframes in the FillEffect from a KeyframeEffect's snapshot but I noticed that we don't actually need to create keyframes in a FillEffect. In most places we are really only concerned with a KeyframeEffect's _properties_. Provided we store the snapshots on the FillEffect, we can regenerate the _properties_ as needed.
Most of the utility methods that do things with KeyframeEffects work on their _properties_ anyway, and we already override GetKeyframes() to generate values from the _properties_. Furthermore, SetKeyframes() should throw since FillEffect is read-only. So hopefully we can have FillEffects have an empty set of keyframes and just work on snapshots and properties. That should reduce the amount of special-case code.
Even when we come to compacting FillAnimations and have a single FillEffect that can represent multiple filling animations, including multiple filling animations targeting the same property, we can probably still just build multiple AnimationProperty objects for each property. Provided the snapshot array is sorted, and the mProperties array maintains the same order, it might just work. We may need special handling for what we pass to the compositor though, I've yet to check.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7787e6273282f2fe8f0f8185b04ec76693092d2
|
|
Assignee | |
Comment 35•6 years ago
|
||
|
|
Assignee | |
Comment 36•6 years ago
|
||
As per the breakdown in comment 21, this should represent the completion of phase 2 (of 4):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d65c619424241842109f16f5762d7cda74f563e
There will still be about ~4 test failures due to bug 1488122.
|
|
Assignee | |
Comment 37•6 years ago
|
||
For me own reference as much as anything, here is the basic structure I am following.
|
|
Assignee | |
Comment 38•6 years ago
|
||
Rebased and with a few more bits added:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dcb5ac4f71f1ffd6ddf98ebc5d76b0465341aab
|
|
Assignee | |
Comment 39•6 years ago
|
||
Got a little further on this today. However I'm concerned that this is so very very complicated that it will be a nightmare to maintain in the future.
Consider some of the steps that happens when an Animation that is contributing to a FillAnimation is seeked back so that it is no longer filling.
* Firstly, it needs to be removed from the FillAnimation.
* Then, if that FillAnimation tracks other Animations besides the re-activated one, we need to generate ONE or TWO new FillAnimations to track them (depending on if it is in the middle of the range represented by the FillAnimation or not).
* If the FillAnimation has a parent FillAnimation, we need to do the same to the parent FillAnimation too all the way up the tree.
* BUT, if the original FillAnimation ONLY represented other filling animations for which we no longer have an associated (Fill)Animation object, AND the original FillAnimation has a parent FillAnimation, then we need to pass its set of fill effects up to the parent. [This last step could be performed as a separate coalescing phase, however.]
This is obviously achievable, but given that this code is executed infrequently, it is highly likely to be a source of bugs and particularly in the future as others maintain it without necessarily understanding all invariants required by the spec and possible permutations.
I have two ideas for simplifying this.
1) Don't make any guarantees about object identity. In fact, _require_ that FillAnimations are always re-generated.
That is, suppose there is one filling animation on |elem|:
elem.getAnimations()[0] !== elem.getAnimations()[0]
This is less convenient for authors.
For example, suppose an app is rendering the animation state using something like React and doing shallow comparison. Such an app would continually re-render despite the fact that the animation objects are not changing.
Furthermore, any expando properties the app puts on the returned FillAnimation objects would not be on the objects returned when they next call `getAnimations` so they can't use expando properties to track which animation is which.
Inconvenience aside, this approach it would simplify the requirements for UAs significantly. Rather than being required to maintain a tree where various branches can be pruned due to GC, or have to be regenerated due to an animation being seeked, the implementation can maintain a flat list of filling effects and then just bundle them up into the minimal number of FillAnimations on-demand.
UAs would still need to record links out to any FillAnimation objects (e.g. with weak pointers) so that it didn't collapse a range when some incomplete sub-range of it might be canceled (unless we do (2) as well), but I _think_ it could simplify this problem considerably by allowing the UA to use a flat list rather than a tree.
2) Don't allow canceling filling animations.
This thought only occurred to me because today we were discussing how to remove an event listener that some third-party script registered, possibly using an anonymous function. In short, there's simply no way to do it. And yet, the Web has managed to get by without that.
So, we could just say: "you can only cancel animations if you retain the handle to the original Animation". Not great, but it would certainly simplify this.
My concern with following the original plan (doing the nice thing and not doing either (1) or (2)) is not only the maintenance burden (and inevitable security bugs) of the code it requires, but the decreased likelihood that other vendors will actually implement it. Other vendors have said they intend to implement it but I don't think they've really looked at what is involved.
|
|
Assignee | |
Comment 40•6 years ago
|
||
Hi Boris, I could use some help working out how to approach expando properties in this bug. Specifically, the idea in this bug is that when we do:
const animations = elem.getAnimations();
for any adjacent animations that are filling indefinitely, we lump them together and generate a new FillAnimation object to represent them. This allows us to internally coalesce these animations in some circumstances and avoid leaking memory.
The proposed spec text for this requires that each time we return a FillAnimation representing the same set of underlying animations, it returns the same FillAnimation object.
So, for example, the following should hold (assuming the first animation is a FillAnimation):
elem.getAnimations()[0] === elem.getAnimations()[0];
(The reason for requiring the same object be returned is that that seems to be the most useful thing for authors. e.g. I expect DevTools might annotate these objects so that it knows which ones it is already rendering and can therefore do a minimal update.)
That said, we want to GC these FillAnimations when they're no longer being used so I am expecting to maintain a hashmap of weak pointers to these objects on the Document, keyed on the set of animations they represent.
However, doing that would make GC observable if the author does:
elem.getAnimations()[0].expandoProp = 'yer';
// ... wait a while ...
const didGC = typeof elem.getAnimations()[0].expandoProp !== 'undefined';
So I am looking at adding another registry to the Document for mutated FillAnimations that simply keeps an owning reference to such animations. FillAnimations would then add and remove themselves as their state changes. In order to do that, however, I'd need to detect when such objects have expando properties and when they don't. Ideally, I'd also need some way to detect when that state changes (or else find a suitable place in the GC cycle to check it).
Does that seem possible? I dug through some of the proxy code and users of OverrideBuiltins but I couldn't find anywhere else where we're doing this so I suspect I'm approaching this the wrong way.
Flags: needinfo?(bzbarsky)
|
|
Assignee | |
Comment 41•6 years ago
|
||
I've basically finished implementing this now. There are only three remaining things to do:
- Deal with expando properties (waiting on ni for this).
I've already added a mechanism for preserving the life of FillAnimations that have event handlers etc. so hopefully this is just a matter of hooking into that. - Try making the generated
FillAnimations have zero duration so that they report theirplayStateas "finished" rather than "running". - Make DevTools render
FillAnimations.
All of those seem quite straightforward. I have the spec text ready (save possible modifications from part 2), web-platform-tests written, and all this work split up into 63 atomic patches that can be landed as four separate bugs (plus the DevTools work which likely needs to be interleaved).
The patch series is here:
https://hg.mozilla.org/try/pushloghtml?changeset=f9c12f30d64f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9c12f30d64f6530682ea90ba3b5c3b425239c6d
However... this is still much more complex than I would like.
I've simplified the approach considerably from my initial design, but I'm still deeply concerned that it will make future maintenance more difficult. (I am especially thinking about the impact on implementing GroupEffects here since other browser vendors have shown an interest in that recently.)
I've attached the WIP patch so that others can get a feel for the complexity this introduces.
At this stage, the only other approach I think is feasible here is to introduce some hard limit to the number of filling animations in an effect stack (e.g. ~100 per element or something of that sort). Once you go over that limit, the filling animations at the bottom of the stack get automatically canceled.
Attachment #8999842 -
Attachment is obsolete: true
|
||
Comment 42•6 years ago
|
||
The proposed spec text for this requires that each time we return a FillAnimation representing the same
set of underlying animations, it returns the same FillAnimation object.
OK, so it requires you to hold a strong reference to the FillAnimation object... That probably means you need to do that. If you don't want to do that, you might want to push back on the spec that requires it.
we want to GC these FillAnimations when they're no longer being used
Define "no longer being used"? For example, if they are used as weakmap keys, does that count as "used"? Because that would allow them to be gced if they're actually not referenced anywhere, but it sounds like the spec, as written, would require them to be kept alive if they're being used as weakmap keys, because that means they might get returned from this API.
There are probably other upcoming JS APIs (weakrefs, say) that would have similar issues.
One thing you could in theory do is keep strong refs to FillAnimations any time they preserve their wrapper (which means the C++ object is keeping the JS object alive, which we do in various cases like expandos, weakmaps, etc, when we don't want GC to be observable). But wrapper preservation is not really hookable right now; you can tell whether the wrapper is being preserved, but not when it enters that state. We could add yet another virtual call to that code, but that's not great either.
Fundamentally, this sounds like a spec that is going to be really hard to implement interoperably without just keeping the things alive. If it's assuming that they don't actually get kept alive and instead people do something that makes it look like they do, that's likely going to surface as interop bugs. :(
Flags: needinfo?(bzbarsky)
|
|
Assignee | |
Comment 43•6 years ago
|
||
Thanks so much Boris. I only came across the weakref case this morning when looking at a similar issue regarding the Element.pseudo() interface.
Wrapper preservation is indeed the concept I'm looking for here but if there's no way to detect when that happens without significant perf impact this could be a lot more involved.
I'm the author of the proposed spec text and I have serious concerns about it being implemented at all. The spec itself doesn't say "you can GC FillAnimations when they're no longer being used" it's just that the whole feature is designed to allow UAs to avoid leaking memory so they will probably want to do so.
The rationale behind the approach is outlined in the FillAnimation proposal and while I think it's the best option we've considered so far for fixing a difficult problem, I'm more and more persuaded we should do something much simpler, even if it's a little less convenient for authors. Your feedback makes me even more inclined to pursue the simpler approach. Thank you again.
|
|
||
Comment 44•6 years ago
|
||
but if there's no way to detect when that happens without significant perf impact
Well. More precisely, the impact would need to be measured. Maybe it's just not an issue.
|
|
Assignee | |
Comment 45•6 years ago
|
||
For posterity, I've put the patch series to implement the original FillAnimation proposal up on a GitHub branch:
https://github.com/birtles/gecko-dev/commits/fill-animation
It's about 63 patches long. It implements most of that proposal except the issues outline in comment 40 onwards regarding dealing with expando properties, weakrefs etc.
I'm currently working on a much simpler approach which I will put up for review if the spec PR gets merged.
|
|
Assignee | |
Comment 46•6 years ago
|
||
Spec text for this has finally been approved:
https://github.com/w3c/csswg-drafts/commit/db06d5f1c532c20960efa33fae8d0ae869976dde
|
|
Assignee | |
Comment 47•6 years ago
|
||
The in-class initializers are easier to maintain since you don't have to try and
match them up with the constructor initializer list (including matching the
order).
|
|
Assignee | |
Comment 48•6 years ago
|
||
Animation::UpdateTiming takes a SyncNotifyFlag parameter. This is passed to
UpdateFinishedState where it determines how we handle finish actions.
If it is async we queue a microtask where we re-evaluate if the animation is
finished or not before queuing events / resolving promises.
That allows code like the following to not trigger finish events:
const animation = elem.animate({...}, 1000);
animation.currentTime += 1000;
animation.effect.updateTiming({ duration: 2000 });
(Since the check that the animation is finished will run in a microtask after
the call to updateTiming.)
When the flag is "sync" we still don't actually run the finish actions
entirely synchronously: the finished promise is resolved synchronously, but
resolving a promise actually queues a microtask for each callback. Likewise, the
finish event is queued synchronously, but not dispatched.
Since there should be no opportunity for script to run between when we call
Animation::Tick and when we run the next microtask checkpoint (currently at the
end of DocumentTimeline::WillRefresh but that will change slightly in the next
patch in this series) there is no need to introduce the extra "async" microtask
for re-evaluating an animation's finished state. Instead it should be possible
to use the "sync" finishing behavior. Such a change should be unobservable to
Web content but will reduce indirection somewhat.
Depends on D30317
|
|
Assignee | |
Comment 49•6 years ago
|
||
According to the procedure to update animations and send events[1] the UA should
update all timelines first and then run a microtask checkpoint.
As a result, when we run callbacks for the finished promise on an Animation they
should see the fully up-to-date state of all animations, regardless of which
timeline they are attached to.
However, that is currently not the case since we run a microtask checkpoint
after updating each individual timeline.
This difference will become more significant later in this patch series when we
introduce another step--removing replaced animations--that also should happen
before we run the microtask checkpoint (so that the promise callbacks always see
a fully-up-to-date state).
This patch makes our handling a little more in line with the spec. It's not
quite the same because it's possible there may be other refresh driver observers
that trigger a microtask checkpoint in between ticking the different timelines
but that case is expected to be rare and fixing it would require maintaining
a separate queue for timeline observers that we run after all other observers--
so it is probably not necessary to fix that case at this stage.
The test added in this patch fails without the code changes in this patch.
[1] https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events
Depends on D30318
|
|
Assignee | |
Comment 50•6 years ago
|
||
Later in this patch series we will add a HashSet<OwningAnimationTarget> member
to EffectCompositor. This patch provides the necessary definitions to support
that.
Depends on D30319
|
|
Assignee | |
Comment 51•6 years ago
|
||
Depends on D30320
|
|
Assignee | |
Comment 52•6 years ago
|
||
This patch introduces the machinery for dispatching remove events but does not
actually cause removing to do anything to the output of the animation beyond
updating its replaceState member.
The expected behavior is defined in:
https://drafts.csswg.org/web-animations-1/#removing-replaced-animations
And the corresponding IDL members are defined in:
https://drafts.csswg.org/web-animations-1/#animation
https://drafts.csswg.org/web-animations-1/#enumdef-animationreplacestate
Tests for these events are added in the next patch in this series.
Depends on D30321
|
|
Assignee | |
Comment 53•6 years ago
|
||
Depends on D30322
|
|
Assignee | |
Comment 54•6 years ago
|
||
Depends on D30323
|
|
Assignee | |
Comment 55•6 years ago
|
||
https://drafts.csswg.org/web-animations-1/#dom-animation-persist
Depends on D30324
|
|
Assignee | |
Comment 56•6 years ago
|
||
And update the GitHub issue link at the same time since #3947 was duped to
#1837.
Depends on D30325
|
|
Assignee | |
Comment 57•6 years ago
|
||
Depends on D30326
|
|
Assignee | |
Comment 58•6 years ago
|
||
Depends on D30327
|
|
Assignee | |
Comment 59•6 years ago
|
||
After discussion with Apple and Google, we decided to drop the properties parameter to commitStyles that allows the author to select which properties to commit. For posterity, here is a patch for implementing that.
|
|
Assignee | |
Comment 60•6 years ago
|
||
Sorry for all the phabricator spam. Due to bug 1549614 we're getting a bunch of bogus coverity spam and each time I update the queue it seems to run again. (The clang-format warnings are legitimate however -- that's due to me disabling the auto-format plugin when it started eating patches a few weeks back.)
|
|
Assignee | |
Comment 61•6 years ago
|
||
|
||
Updated•6 years ago
|
Attachment #9063426 -
Attachment description: Bug 1253476 - Implement Animation.commitStyles; r?boris,emilio,bzbarsky! → Bug 1253476 - Implement Animation.commitStyles; r?boris,emilio,smaug,bzbarsky!
|
|
Assignee | |
Comment 62•6 years ago
|
||
Depends on D30327
|
||
Updated•6 years ago
|
Whiteboard: [layout:backlog:2019q3:69]
|
|
||
Updated•6 years ago
|
Attachment #9064397 -
Attachment description: Bug 1253476 - Return whether or not a declaration block was updated; r?emilio → Bug 1253476 - Use update() to update declarations from Servo_DeclarationBlock_SetPropertyToAnimatinValue; r?emilio
|
|
||
Updated•6 years ago
|
Attachment #9064397 -
Attachment description: Bug 1253476 - Use update() to update declarations from Servo_DeclarationBlock_SetPropertyToAnimatinValue; r?emilio → Bug 1253476 - Use update() to update declarations from Servo_DeclarationBlock_SetPropertyToAnimationValue; r?emilio
|
||
Comment 63•6 years ago
|
||
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e5b24c61e3a
Use in-class member initializers in Animation.h; r=hiro
https://hg.mozilla.org/integration/autoland/rev/861caf0b6714
Make Animation::Tick do finish actions synchronously; r=hiro
https://hg.mozilla.org/integration/autoland/rev/0e25f7b0790b
Run microtask checkpoint for updating timing after updating all timelines; r=hiro
https://hg.mozilla.org/integration/autoland/rev/a80ca3b22049
Add DefaultHasher implementation for OwningAnimationTarget; r=njn
https://hg.mozilla.org/integration/autoland/rev/f0273e033938
Add KeyframeEffect::GetPropertySet; r=boris
https://hg.mozilla.org/integration/autoland/rev/8642d6a60ad9
Add remove events; r=boris,bzbarsky
https://hg.mozilla.org/integration/autoland/rev/25238a996da4
Add tests that removing is triggered at the right time; r=boris
https://hg.mozilla.org/integration/autoland/rev/d540c77bd298
Don't composite removed animations; r=boris
https://hg.mozilla.org/integration/autoland/rev/16dbeeceab38
Implement Animation::Persist; r=boris,bzbarsky
https://hg.mozilla.org/integration/autoland/rev/b74b84baf717
Move IsRendered to Element; r=emilio
https://hg.mozilla.org/integration/autoland/rev/180d65431190
Implement Animation.commitStyles; r=boris,emilio,bzbarsky,smaug
https://hg.mozilla.org/integration/autoland/rev/f26b8ea63ae3
Use update() to update declarations from Servo_DeclarationBlock_SetPropertyToAnimationValue; r=emilio
https://hg.mozilla.org/integration/autoland/rev/26b7854271e1
Turn on auto-removing behavior by default in Nightly; r=boris
https://hg.mozilla.org/integration/autoland/rev/df36c132c6b0
Make whitespace in Web Animations WebIDL definitions more consistent; r=bzbarsky
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16917 for changes under testing/web-platform/tests
Whiteboard: [layout:backlog:2019q3:69] → [layout:backlog:2019q3:69][wptsync upstream]
Upstream PR merged
|
|
||
Comment 66•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/6e5b24c61e3a
https://hg.mozilla.org/mozilla-central/rev/861caf0b6714
https://hg.mozilla.org/mozilla-central/rev/0e25f7b0790b
https://hg.mozilla.org/mozilla-central/rev/a80ca3b22049
https://hg.mozilla.org/mozilla-central/rev/f0273e033938
https://hg.mozilla.org/mozilla-central/rev/8642d6a60ad9
https://hg.mozilla.org/mozilla-central/rev/25238a996da4
https://hg.mozilla.org/mozilla-central/rev/d540c77bd298
https://hg.mozilla.org/mozilla-central/rev/16dbeeceab38
https://hg.mozilla.org/mozilla-central/rev/b74b84baf717
https://hg.mozilla.org/mozilla-central/rev/180d65431190
https://hg.mozilla.org/mozilla-central/rev/f26b8ea63ae3
https://hg.mozilla.org/mozilla-central/rev/26b7854271e1
https://hg.mozilla.org/mozilla-central/rev/df36c132c6b0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox69:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
|
||
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•