Don't flush styles when creating a new animation via Element.animate
Categories
(Core :: DOM: Animation, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
(Blocks 1 open bug, )
Details
Attachments
(10 files, 1 obsolete file)
1.97 KB,
text/html
|
Details | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
I'm probably way off the mark here but let me explain my problem.
In bug 1253476 I have been trying to optimize how we handle filling animations, particularly for the test case in [1].
In profiling that I notice that we flush styles when we create a new animation. In particular KeyframeEffect::SetKeyframes flushes styles when it updates its properties.
However, that shouldn't be necessary. We should just be able to use the current computed style. If there are other pending style changes we'll update the properties when we process them.
When I make SetKeyframes not flush, however, tests like the following fail:
const div = createDiv(t);
const anim = div.animate({ visibility: ['hidden','visible'] },
{ duration: 1000, fill: 'both' });
anim.currentTime = 0;
assert_equals(getComputedStyle(div).visibility, 'hidden',
'Visibility when progress = 0');
What happens is that when we create the animation we successfully request a restyle on |div| but we don't flush styles yet.
When we go to read the visibility off gCS, however, we flush styles. As part of that we call WillComposeStyle on the animation's KeyframeEffect but we never call ComposeStyle on it.
Digging into it I get to the following stack for |div|:
style::traversal::compute_style
style::traversal::recalc_style_at
<style::gecko::traversal::RecalcStyleOnly<'recalc>
style::driver::traverse_dom
geckoservo::glue::traverse_subtree
Servo_TraverseSubtree
mozilla::ServoStyleSet::StyleDocument
mozilla::RestyleManager::DoProcessPendingRestyles
mozilla::PresShell::DoFlushPendingNotifications
nsIPresShell::FlushPendingNotifications
mozilla::dom::Document::FlushPendingNotifications
nsComputedDOMStyle::UpdateCurrentStyleSources
nsComputedDOMStyle::GetPropertyValue
nsComputedDOMStyle::GetPropertyValue
nsDOMCSSDeclaration::GetVisibility
mozilla::dom::CSS2Properties_Binding::get_visibility
In compute_style we have:
match target.share_style_if_possible(context) {
which ends up returning true meaning we never call resolve_style_with_default_parents
which is where we would normally get the animation_rules()
and produce the animated style.
In test_candidate
I don't see anything that would cause us to stop sharing style caches for this case so I'm wondering how this is supposed to behave.
Does this, for example, need a check that the elements don't have script-generated Web Animations applied to them?
Assignee | ||
Comment 1•3 years ago
|
||
Yep, adding a check to test_candidate
seems to fix it.
Assignee | ||
Comment 2•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b375ea24ce109e415f1cd722d01964488e012cb
The last patch in this series will cause test_restyles.html
to fail but I still need to debug why (since that second patch should make us restyle less not more). And I probably don't want to land this without that last patch because it is what will cause other tests to fail without the changes to test_candidate
.
Assignee | ||
Comment 3•3 years ago
|
||
Emilio points out that we could just use has_animations()
since we shouldn't be sharing styles for elements with CSS animations/transitions anyway due to:
(And I believe the reason that check doesn't work for us here, is we only apply it to element A, but it doesn't stop us from sharing with element B, where A has no animations but B does.)
Comment 4•3 years ago
|
||
What I don't quite understand is, IIUC, test_candidate is called only in normal traversal, and the computed style in question is actually animated one (visibility) so how do they interact each other?
Assignee | ||
Comment 5•3 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
What I don't quite understand is, IIUC, test_candidate is called only in normal traversal, and the computed style in question is actually animated one (visibility) so how do they interact each other?
I suspect it might be because this is the first styling of the element although I'm not entirely sure.
Comment 6•3 years ago
|
||
What I guessed is that there is a real issue and adding the animation check in test_candidate would be another wallpaper of it.
Comment 7•3 years ago
|
||
I suspect there's no other underlying issue. Let's say it's the first restyle of an element, there are two siblings, both having web animations (but not other CSS animations).
Sharing styles between them is just unsound, and right now we seem to be able to do it. In any case yeah, a reduced test-case that demonstrates the problem would be great. I would've imagined something like this to repro the problem, why doesn't it? Does the animation-only restyle paper over it?
<!doctype html>
<body>
<style>
span { display: inline-block; width: 100px; height: 100px; background-color: red; }
</style>
<script>
function createAnimatedSpan(color) {
let span = document.createElement("span");
span.animate([
{ backgroundColor: color },
{ backgroundColor: color },
], {
duration: Infinity,
iterations: Infinity,
});
return span;
}
onload = function() {
document.body.offsetTop;
let spans = [];
for (let i = 0; i < 20; ++i) {
spans.push(createAnimatedSpan('blue'));
spans.push(createAnimatedSpan('green'));
}
for (let span of spans)
document.body.appendChild(span);
}
</script>
Assignee | ||
Comment 8•3 years ago
|
||
I went and re-recorded this and confirmed that we don't do the animation restyle because this is the initial styling.
Specifically, we hit this condition and don't propagate the animation-only dirty hint up the tree:
As a result needs_animation_only_restyle
will be false in Servo_TraverseSubtree
:
Assignee | ||
Comment 9•3 years ago
|
||
For a reduced test case, the first test case here reproduces it consistently once you remove the style flush from KeyframeEffect::SetKeyframes
:
(As do a bunch of other test cases.)
Comment 10•3 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #8)
I went and re-recorded this and confirmed that we don't do the animation restyle because this is the initial styling.
Specifically, we hit this condition and don't propagate the animation-only dirty hint up the tree:
That was exactly what I guessed, we fail to add the animation-only dirty bit, actually it doesn't mean adding the dirty bit, it's ignored in the animation-only traversal though. We probably need to defer it in the second animation-only restyles?
(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
Sharing styles between them is just unsound,
I am not saying that sharing styles between them is a right thing to do. For example, in the case where the sibling elements have been already styled (in normal traversal), we never call test_candidate because we never set the normal dirty bit for the elements, but the animation works there.
Assignee | ||
Comment 11•3 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #2)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b375ea24ce109e415f1cd722d01964488e012cb
The last patch in this series will cause
test_restyles.html
to fail but I still need to debug why...
It turns out that what happens here is that UpdateProperties
returns early whenever the properties have not changed. However UpdateProperties
is also what updates our cumulative change hint.
So, if we don't flush style when setting up an animation we'll do the following:
- Call
UpdateProperties
with the old style- Build
mProperties
accordingly - Go to calculate the cumulative change hint but because
Element.mServoData
is null,ResolveServoStyleByAddingAnimation
will return null and we'll do:
mCumulativeChangeHint = ~nsChangeHint_Hints_CanIgnoreIfNotVisible;
- Build
- Restyle and fill in
Element.mServoData
- Call
UpdateProperties
again with the updated style- Notice that the calculated
mProperties
has not changed and return early, failing to updatemCumulativeChangeHint
- Notice that the calculated
- Then when we go to call
CanThrottle
→CanThrottleIfNotVisible
→CanIgnoreIfNotVisible
it will return:
NS_IsHintSubset(mCumulativeChangeHint, nsChangeHint_Hints_CanIgnoreIfNotVisible)
which will evaluate false sincemCumulativeChangeHint
has not been updated.
As a result, we'll fail to throttle the animation until something causes us to update the cumulative change hint.
We could possibly fix this by doing something like the following in UpdateProperties
:
if (!propertiesChanged) {
// Check if we need to update the cumulative change hint.
// One case where this happens is if we previously didn't have style data
// for our target and set it to ~nsChangeHint_Hints_CanIgnoreIfNotVisible
// instead.
if (mCumulativeChange == ~nsChangeHint_Hints_CanIgnoreIfNotVisible &&
mTarget && mTarget->Element->HasServoData()) {
CalculateCumulativeChangeHint(aStyle);
}
return;
}
I suspect there will still be many other cases where we need to update that hint but I'd rather not call CalculateCumulativeChangeHint
every time since it's very expensive.
Assignee | ||
Comment 12•3 years ago
|
||
Let's see how this goes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85debd29551b2520f6cf3b97f52681da33ade96e
Follow up test run to check the added test case doesn't fail intermittently (no idea which Android platforms I should be looking at):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aad3aaf59cd7422cc9c8fbedab5d2551b0603ad4
Assignee | ||
Comment 13•3 years ago
|
||
Repurposing this bug to focus on the perf issue all these changes are in service of. Specifically, this optimization is intended to improve performance on https://greensock.com/js/speed.html, and particularly to make sure that bug 1253476 doesn't cause us to regress performance there.
Assignee | ||
Comment 14•3 years ago
|
||
Looks like I still have two test failures to deal with:
- dom/animation/test/style/test_missing-keyframe-on-compositor.html
- layout/reftests/web-animations/stacking-context-transform-changing-effect.html
Assignee | ||
Comment 15•3 years ago
•
|
||
So it turns out that whether or not Element.animate flushes is observable and it doesn't flush in Chrome but does in Safari TP.
Suppose we have:
div {
opacity: 0.1;
width: 100px;
height: 100px;
background: black;
transition: 1s opacity linear;
}
And then we do:
// Just to be sure, flush style
getComputedStyle(div).opacity;
At this point the computed opacity of div
is 0.1.
Now, suppose we update the specified opacity to 0.2 but DON'T flush style:
div.style.opacity = '0.2';
Then trigger an animation on the same element:
div.animate({ opacity: [0.6, 1] }, 1000);
At this point if Element.animate()
flushes style it will cause us to trigger a transition from 0.1 to 0.2.
However, if Element.animate() does NOT flush style we will:
- Generate a new Animation animating from 0.6 to 1.
- Then on the next restyle we will have a before-change style opacity of 0.6 (since we bring declarative animations up-to-date as part of calculating the before-change style).
- And we we also have an after-change style of 0.6 (for the same reason).
Since the before-change style and after-change style have not changed, no transition will be generated.
Test case: https://codepen.io/birtles/pen/YBxxBd
Given that this is observable we should spec what the behavior is. I think we should definitely NOT flushing since requiring a flush would mean that Element.animate() both flushes AND dirties style. As a result, if the author generates a number of animations in a single animation frame, the browser would be required to flush multiple times per frame.
It's probably no coincidence that the Greensock test case[1] is much more performance in Chrome (doesn't flush) than in Firefox and Safari (do flush).
Assignee | ||
Comment 16•3 years ago
|
||
Spec issue: https://github.com/w3c/csswg-drafts/issues/3613
Assignee | ||
Comment 17•3 years ago
|
||
Regarding the first test failure in dom/animation/test/style/test_missing-keyframe-on-compositor.html
, a similar situation arises to that described in comment 15, although it's a little more subtle.
In test_missing-keyframe-on-compositor.html
we have an animation with an implicit from-keyframe. i.e. we have something like:
getComputedStyle(div).opacity; // Computed style is 0.1 now
div.style.opacity = '0.5';
div.animate({ opacity: 1 }, 1000);
If Element.animate()
does NOT flush style we will first generate a new Animation
animating from (null) to 1.
Then on the next restyle we will have a before-change style opacity of 0.1 (since that was the computed value of opacity at the previous style change event and there is no effect from bringing the script animation up-to-date).
For the after-change style one might expect it to be 0.5 but remember we brought animation styles up-to-date in order to calculate the before-change style.
Assuming those animation styles specify opacity of 0.1 and assuming the CSS cascade doesn't have any way of natively expressing the animation stack (which it doesn't yet), then after-change style will actually be 0.1. As a result, no transition will be generated.
That, at least, is consistent with the behavior when we have an explicit from keyframe.
Perhaps a little surprisingly, however, as a result of the restyle we will update the base style used by our animation from 0.1 to 0.5. But that still won't trigger a transition because on the next restyle both the before and after change style will be 0.5.
So, assuming the behavior proposed in the spec issue is accepted, I think the correct thing here might be to actually tweak test_missing-keyframe-on-compositor.html
to force a flush and trigger a transition.
Assignee | ||
Comment 18•3 years ago
|
||
This is a test case based on the description in comment 17 (mostly just for posterity).
Assignee | ||
Comment 19•3 years ago
|
||
Looking into the remaining failure in layout/reftests/web-animations/stacking-context-transform-changing-effect.html
what happens is that we have:
var elem = document.getElementById("test");
var anim = elem.animate(null, { duration: 100000 });
var newEffect = new KeyframeEffect(elem,
{ transform: ['none', 'none']},
100000);
anim.ready.then(function() {
anim.effect = newEffect;
requestAnimationFrame(function() {
document.documentElement.classList.remove("reftest-wait");
});
});
(The call to requestAnimationFrame
doesn't seem to be actually doing anything, for what it's worth. We call it from the ready promise callback which runs before rAF callbacks--so this doesn't actually wait for styling to happen.)
When we create the KeyframeEffect
, we will internally call SetKeyframes
and normally that would trigger a style flush fairly early on in the process.
However, if we drop that style flush, later in SetKeyframes
the style frame for elem
won't have been created yet. As a result, MaybeUpdateFrameForCompositor
will return early and never set the NS_FRAME_MAY_BE_TRANSFORMED
frame bit. As a result nsIFrame::IsTransformed
will return false.
What's more, when we later restyle and set the transformed style for the frame we'll fail the following assertion in ApplyRenderingChangeToTree
:
// We check StyleDisplay()->HasTransformStyle() in addition to checking
// IsTransformed() since we can get here for some frames that don't support
// CSS transforms.
NS_ASSERTION(!(aChange & nsChangeHint_UpdateTransformLayer) ||
aFrame->IsTransformed() ||
aFrame->StyleDisplay()->HasTransformStyle(),
"Unexpected UpdateTransformLayer hint");
So how is this supposed to work?
Normally we'll set NS_FRAME_MAY_BE_TRANSFORMED
when any of the following three things happen:
- We update the keyframes to include transforms or create a new KeyframeEffect (in
KeyframeEffect::SetKeyframes
), or - We retarget an effect at a new element (in
KeyframeEffect::SetTarget
), or - Newly create an
nsFrame
with transform animations applied to it (innsFrame::Init
, specifically this bit).
The trouble is, in this particular test we have the following sequence:
- Create a new
KeyframeEffect
. - Set up the keyframes and attempt to update the frame bits but fail because the style frame doesn't exist yet
- Create the
nsFrame
for the target element (as part of waiting foranim.ready
).
Unfortunately at this point we have not yet associated the createdKeyframeEffect
with an animation, so it's not relevant and not registered with theEffectSet
and hence not associated with thensFrame
's content. As a result we won't setNS_FRAME_MAY_BE_TRANSFORMED
isnsFrame::Init
. - Associate the effect with the animation causing it to register with the
EffectSet
and be included in styling. - Compose the
KeyframeEffect
updating the transform style if needed.
Unfortunately there's nothing at point 5 or beyond that will cause us to set NS_FRAME_MAY_BE_TRANSFORMED
on the target frame since we've already fully initialized the KeyframeEffect
and the nsFrame
independently.
As a result, in display list building we'll treat the frame as un-transformed and we'll fail the test (and the above-mentioned assertion too).
As far as I can tell, this is an existing problem. For example, I believe we can already cause the above assertion to fail with content like the following:
<style>
div { display: none }
</style>
<div id=div></div>
<script>
const animation = div.animate({ transform: ['none', 'none'] }, 1000);
animation.cancel();
setTimeout(() => {
div.style.display = 'block';
requestAnimationFrame(() => {
requestAnimationFrame(() => {
animation.play();
});
});
}, 500);
</script>
In terms of solutions, the most simple thing might be to just always set NS_FRAME_MAY_BE_TRANSFORMED
when we compose style and detect the transform property. Looking up the style frame each time might be suboptimal, so we could set a flag if we encounter null frame in MaybeUpdateFrameForCompositor
and only do it then. That would probably be a fairly safe and performant fix, but it does seem like a bit of a band-aid.
Alternatively, we could try to fix this in RestyleManager. For example, in RestyleManager::ProcessRestyledFrames
if we had nsChangeHint_UpdateTransformLayer
hint but no NS_FRAME_MAY_BE_TRANSFORMED
frame bit, we could call into the EffectSet, get it to iterate over its animations, and if it had any transform ones, have them set the frame bit and then replace nsChangeHint_UpdateTransformLayer
with nsChangeHint_AddOrRemoveTransform
. That seems more correct but more complicated.
Assignee | ||
Comment 20•3 years ago
|
||
For my own notes, I think we don't hit the part that adds NS_FRAME_MAY_BE_TRANSFORMED
whenever we have nsChangeHint_AddOrRemoveTransform
because we never add nsChangeHint_AddOrRemoveTransform
.
That hint would normally be added either:
RestyleManager::AddLayerChangesForAnimation
- but won't in this case because that is only triggered after we've generated a transform display item, but we won't do that here sinceIsTransformed()
returns falsensStyleDisplay::CalcDifference
- but probably won't in this case because the style doesn't actually change (just goes from 'none' to 'none'?)
So this bug probably only manifests when we have an animation starting with transform: none
.
Assignee | ||
Comment 21•3 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #19)
Alternatively, we could try to fix this in RestyleManager. For example, in
RestyleManager::ProcessRestyledFrames
if we hadnsChangeHint_UpdateTransformLayer
hint but noNS_FRAME_MAY_BE_TRANSFORMED
frame bit, we could call into the EffectSet, get it to iterate over its animations, and if it had any transform ones, have them set the frame bit and then replacensChangeHint_UpdateTransformLayer
withnsChangeHint_AddOrRemoveTransform
. That seems more correct but more complicated.
I looked into doing this.
One thing that's a bit odd is that RestyleManager::ProcessRestyledFrames
seems to assume that NS_FRAME_MAY_BE_TRANSFORMED
is accurate. For example, if it sees it is NOT set, it drops nsChangeHint_UpdatePostTransformOverflow
:
if (!(frame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED)) {
// Frame can not be transformed, and thus a change in transform will
// have no effect and we should not use the
// nsChangeHint_UpdatePostTransformOverflow hint.
hint &= ~nsChangeHint_UpdatePostTransformOverflow;
}
My patch then does almost the reverse:
if ((hint & nsChangeHint_UpdateTransformLayer) &&
!(frame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED) &&
frame->HasAnimationOfTransform()) {
// If we have an nsChangeHint_UpdateTransformLayer hint but no
// corresponding frame bit, it's possible we have a transform animation
// with transform style 'none' that was initialized independently from
// this frame and associated after the fact.
//
// In that case we set the frame bit and upgrade the change hint.
frame->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
hint &= ~nsChangeHint_ComprehensiveAddOrRemoveTransform;
}
Maybe that's ok. A couple of things I'm not sure about though:
- If this change does work, would it be valid to drop
KeyframeEffect::MaybeUpdateFrameForCompositor
altogether? (Would be nice if we could. There's too much stateful stuff happening inKeyframeEffect
producing bugs like this.) - Is upgrading the change hint here correct? Meaningful? Would leaving as
nsChangeHint_UpdateTransformLayer
be ok?
Anyway, let's see what try says:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0118bbc115bb4d46d2df1e8e4263dab4944bc024
Assignee | ||
Comment 22•3 years ago
|
||
A few extra try runs...
The whole stack for this bug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceec8c06c8b9b6d61211e03d6c7afb975461f271
With KeyframeEffect::MaybeUpdateFrameForCompositor
dropped entirely:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a2e75b7b87ec30a867e7f99f770e6ba99582974
With KeyframeEffect::MaybeUpdateFrameForCompositor
dropped entirely and not upgrading the hint:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29dc3cae7b9201e3f5813d1fbc3bcaf2c0549875
Assignee | ||
Comment 23•3 years ago
|
||
All the try runs look fine, so I guess I'll try putting it all up for review (including dropping MaybeUpdateFraomeForCompositor
) and see what my reviewers say :)
Assignee | ||
Comment 24•3 years ago
|
||
I've split off the work for writing WPT for this into bug 1525809.
Assignee | ||
Comment 25•3 years ago
|
||
Regarding the missing dom:: specifications, presumably this currently works
because in the unified build this file gets combined with something with "using
namespace mozilla::dom" but this will fail if the chunking of the unified build
changes.
Assignee | ||
Comment 26•3 years ago
|
||
Typically we set the NS_FRAME_MAY_BE_TRANSFORMED bit on a frame when one of the
following situations arises:
a. We update the keyframes of a KeyframeEffect to include transforms or we
create a new KeyframeEffect that animates transforms (in
KeyframeEffect::SetKeyframes), or
b. We retarget a KeyframeEffect with transforms at a new element (in
KeyframeEffect::SetTarget), or
c. We create an nsFrame with transform animations applied to it (in
nsFrame::Init), or
d. We get a nsChangeHint_AddOrRemoveTransform hint in
RestyleManager::ProcessRestyledFrames and decide to update the frame bit on
the frame and its continuations accordingly.
However, there are some situations where we can have a transform animation on
a frame where none of the above are triggered.
For example, if the style frame is not unavailable (e.g. a display:none
element) when the KeyframeEffect is initialized we will fail to the frame bit in
(a) and if we never retarget the effect we will never set reach (b).
Furthermore, if we have an animation that is "not relevant" (e.g. idle) and
hence not registered with the EffectSet when the frame is initialized we will
fail to set the frame bit in (c).
Finally, if the the animation does not produce a style change that causes the
nsChangeHint_AddOrRemoveTransform bit to be set (e.g. the transform animation
begins at 'none') we will not set the frame bit in (d).
As a result, we can end up failing to set the NS_FRAME_MAY_BE_TRANSFORMED bit
for some content.
The crashtest included in this patch produces such a case and, without the code
changes in this patch, will fail the assertion in ApplyRenderingChangeToTree[1]:
NS_ASSERTION(!(aChange & nsChangeHint_UpdateTransformLayer) ||
aFrame->IsTransformed() ||
aFrame->StyleDisplay()->HasTransformStyle(),
"Unexpected UpdateTransformLayer hint");
That is because although the nsChangeHint_UpdateTransformLayer bit will be set,
aFrame->IsTransformed() will return false because the frame bit has not been
set, and aFrame->StyleDisplay()->HasTransformStyle() will return false because
the animation sets the transform to 'none'.
Not only will this assertion fail, but once we cease flushing style as part of
triggering an animation later in this patch, the reftest
layout/reftests/web-animations/stacking-context-transform-changing-effect.html
will begin to fail. That reftest produces a similar situation to the crashtest
but it currently does not fail because the style flush that happens as part of
creating an animation ensures the style frame is available at the point when the
animation is triggered and hence case (a) from above is hit.
This patch addresses this by detecting the case where we have this change hint
set but not the corresponding frame bit, and setting the frame bit.
Depends on D18910
Assignee | ||
Comment 27•3 years ago
|
||
Now that we set the NS_FRAME_MAY_BE_TRANSFORMED frame bit when needed in
RestyleManager::ProcessRestyledFrames it seems redundant to also set it from
KeyframeEffect.
Furthermore, setting frame bits from KeyframeEffect is a little fragile since it
depends on the life cycle of the KeyframeEffect which is independent of the
nsFrame. If we can avoid doing that, we probably should.
Depends on D18911
Assignee | ||
Comment 28•3 years ago
|
||
We already avoid putting styles in the shared style cache for an element that
has animations[1] but if we are doing the initial style of two siblings where
the second has animations applied, there is nothing to stop us from trying to
share with the first (un-animated) element. This patch adds a check to prevent
sharing in that case since sharing style between animated elements is unsound
for the reasons described in [1].
A number of tests including:
testing/web-platform/tests/web-animations/animation-model/animation-types/visibility.html
will fail without this fix once we remove the style flush from
KeyframeEffect::SetKeyframes later in this patch series.
Depends on D18912
Assignee | ||
Comment 29•3 years ago
|
||
properties haven't changed if we failed to do so previously because we had no style data; r?hiro
Without this fix, various tests in
dom/animation/tests/mozilla/test_restyles.html will fail once we remove the
style flush from KeyframeEffect::SetKeyframes. That is because we will fail to
set mCumulativeChangeHint correctly when we initially set up the keyframe's
properties and then never update it since UpdateProperties will return early
when it determines the properties have not changed.
Depends on D18913
Assignee | ||
Comment 30•3 years ago
|
||
test_missing-keyframe-on-compositor.html assumes that calling Element.animate()
will flush style and trigger transitions. However, in this patch series we will
make Element.animate() stop flushing style (and the spec will also mandate
this). Once we do that, the expected transitions will no longer fire and this
test will fail. This patch updates the test to explicitly trigger transitions
before calling Element.animate().
Depends on D18914
Assignee | ||
Comment 31•3 years ago
|
||
A forthcoming spec change will require that Animatable.animate() and other
methods that mutate animations do not flush style:
https://github.com/w3c/csswg-drafts/issues/3613
Bug 1525809 will add web-platform-tests for this change once it is made (and
tweak the behavior introduced in this patch if necessary).
Currently Firefox and WebKit will flush styles when calling
Animatable.animate(). This is undesirable since this method will also
invalidate style. As a result, if content triggers multiple animations in
a single animation frame, it will restyle every time it creates an animation.
This patch removes the style flush from a number of these methods.
In general the style flush is not necessary. For example, we don't need to flush
style before getting the computed style to pass to UpdateProperties. That's
because if there are pending style changes, then UpdateProperties will be called
with the updated style once they are applied anyway. Flushing style first means
that we may end up resolving style twice, when once would be sufficient.
For GetKeyframes() however, when used on a CSS animation, it should return
the most up-to-date style so for that call site we do want to flush style.
The test case added in this patch will fail without the code changes in the
patch. Specifically, we will observe 10 non-animation restyles (from the
5 animations) if we flush styles from SetKeyframes.
Depends on D18915
Assignee | ||
Comment 32•3 years ago
|
||
Thanks for the feedback Hiro. I'll try and make failing test cases for the continuation and table cases although it might be next week before I finish them.
Here is a more complete try run of the current set of patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03ad2969b5ab0a4c9177475f15f458aec65287bd
Comment 33•3 years ago
|
||
That's a good news for me. It will take some time to review all the patches, especially for the final one. I am really worrying about the dropping the flush there, I know it's the right thing to do, but I'd been suffering from unstyled data
panics or crashes during Stylo development (IIRC it's August 2017), that's not the same case for this, so it might not be a problem at all now, but I am not 100% sure.
Assignee | ||
Comment 34•3 years ago
|
||
Yeah, there's no hurry at all. (Also, I will be on a slower computer next week so I will probably focus more on spec work.)
Yes I think those style flushes are papering over a few existing bugs. This patch series fixes some, but there might be more we discover. I want to land this towards the start of the cycle so we have time to find those bugs.
Updated•3 years ago
|
Assignee | ||
Comment 35•3 years ago
|
||
This is unrelated to this bug (the assertion can already fail without the
patches in this patch queue) but I uncovered it while writing the tests in the
next patch which will trip the assertion that is removed in this patch.
Depends on D18916
Assignee | ||
Comment 36•3 years ago
|
||
Depends on D19885
Updated•3 years ago
|
Assignee | ||
Comment 37•3 years ago
|
||
Looks like I overlooked one test failure in dom/animation/test/chrome/test_animation_properties.html
.
This does
var div = addDiv(t);
var animation = div.animate(subtest.frames, 100 * MS_PER_SEC);
assert_properties_equal(
animation.effect.getProperties(),
subtest.expected
);
This will fail with failures such as:
CSS variables are resolved to their corresponding values - CSS variables are resolved to their corresponding values: assert_equals: Values arrays do not match for left property expected "{ offset: 0, value: 10px, easing: linear, composite: replace }, { offset: 1, value: 100px, easing: undefined, composite: replace }" but got "{ offset: 0, value: 10px, easing: linear, composite: replace }, { offset: 1, value: auto, easing: undefined, composite: replace }"
Specifically the var(--var-100px)
from the test case is being resolved as auto
instead of 10px
.
Since getProperties() is a Chrome-only property there are two possible fixes here:
A. Flush inside getProperties() -- that will make the test pass but will possibly degrade performance in DevTools
B. Flush explicitly in the test
I lean towards B. A is arguably the right thing to do but I suspect in DevTools we never hit this case so we should prioritize performance instead.
Assignee | ||
Comment 38•3 years ago
|
||
Depends on D18915
Assignee | ||
Comment 39•3 years ago
|
||
moz-phab submit
seems to be breaking in all sorts of interesting ways...
Assignee | ||
Comment 40•3 years ago
|
||
Phabricator should be fixed now. Just one more patch left for review.
Assignee | ||
Comment 41•3 years ago
|
||
Thanks for the reviews!
Comment 42•3 years ago
|
||
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0d8937fbde0f Unified build fixes for KeyframeEffect.cpp and KeyframeUtils.cpp; r=hiro https://hg.mozilla.org/integration/autoland/rev/656ababf4eda Set NS_FRAME_MAY_BE_TRANSFORMED bit when we have nsChangeHint_UpdateTransformLayer; r=hiro https://hg.mozilla.org/integration/autoland/rev/1f26e2c082a8 Don't share styles when an element has animations applied to it; r=emilio https://hg.mozilla.org/integration/autoland/rev/c8b3e45cd031 Update the cumulative change hint even if a keyframe effect's properties haven't changed if we failed to do so previously because we had no style data; r=hiro https://hg.mozilla.org/integration/autoland/rev/d56add557dc1 Trigger transitions in test_missing-keyframe-on-compositor.html; r=hiro https://hg.mozilla.org/integration/autoland/rev/7973b5c9dccf Make test_animation_properties.html flush styles; r=hiro https://hg.mozilla.org/integration/autoland/rev/5dac0d4a3c44 Add a version of KeyframeEffect::GetTargetComputedStyle that does not flush style and use it; r=hiro https://hg.mozilla.org/integration/autoland/rev/1625e825876c Make KeyframeEffect::CreateComputedStyleForAnimationValue handle a null elementForResolve; r=hiro https://hg.mozilla.org/integration/autoland/rev/c6657979a1a6 Add tests for unstyled data; r=hiro
Comment 43•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d8937fbde0f
https://hg.mozilla.org/mozilla-central/rev/656ababf4eda
https://hg.mozilla.org/mozilla-central/rev/1f26e2c082a8
https://hg.mozilla.org/mozilla-central/rev/c8b3e45cd031
https://hg.mozilla.org/mozilla-central/rev/d56add557dc1
https://hg.mozilla.org/mozilla-central/rev/7973b5c9dccf
https://hg.mozilla.org/mozilla-central/rev/5dac0d4a3c44
https://hg.mozilla.org/mozilla-central/rev/1625e825876c
https://hg.mozilla.org/mozilla-central/rev/c6657979a1a6
Description
•