1524480 - Don't flush styles when creating a new animation via Element.animate

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P3)

Description

[Brian Birtles (:birtles)]

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?

[1] https://greensock.com/js/speed.html

Comment 1

[Brian Birtles (:birtles)]

Yep, adding a check to test_candidate seems to fix it.

Comment 2

[Brian Birtles (:birtles)]

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.

Comment 3

[Brian Birtles (:birtles)]

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:

https://searchfox.org/mozilla-central/rev/6e3cc153566f5f288ae768a2172385b8436d61dd/servo/components/style/sharing/mod.rs#597

(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

[Hiroyuki Ikezoe (:hiro)]

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?

Comment 5

[Brian Birtles (:birtles)]

(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

[Hiroyuki Ikezoe (:hiro)]

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

[Emilio Cobos Álvarez (:emilio)]

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>

Comment 8

[Brian Birtles (:birtles)]

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:

https://searchfox.org/mozilla-central/rev/60c4067b1cbb0f94d7dc2d7cdfa27ed579817fee/servo/components/style/gecko/wrapper.rs#825-828

As a result needs_animation_only_restyle will be false in Servo_TraverseSubtree:

https://searchfox.org/mozilla-central/rev/60c4067b1cbb0f94d7dc2d7cdfa27ed579817fee/servo/ports/geckolib/glue.rs#366-367

Comment 9

[Brian Birtles (:birtles)]

For a reduced test case, the first test case here reproduces it consistently once you remove the style flush from KeyframeEffect::SetKeyframes:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/web-animations/animation-model/animation-types/visibility.html

(As do a bunch of other test cases.)

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 11

[Brian Birtles (:birtles)]

(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;
• 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 update mCumulativeChangeHint
• Then when we go to call CanThrottle → CanThrottleIfNotVisible → CanIgnoreIfNotVisible it will return:
  NS_IsHintSubset(mCumulativeChangeHint, nsChangeHint_Hints_CanIgnoreIfNotVisible) which will evaluate false since mCumulativeChangeHint 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.

Comment 12

[Brian Birtles (:birtles)]

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

Comment 13

[Brian Birtles (:birtles)]

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.

Comment 14

[Brian Birtles (:birtles)]

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

Comment 15

[Brian Birtles (:birtles)]

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).

[1] https://greensock.com/js/speed.html

Comment 16

[Brian Birtles (:birtles)]

Spec issue: https://github.com/w3c/csswg-drafts/issues/3613

Comment 17

[Brian Birtles (:birtles)]

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.

Comment 18

[Brian Birtles (:birtles)]

Attachment: Test case for flushing with implicit from keyframes

This is a test case based on the description in comment 17 (mostly just for posterity).

Comment 19

[Brian Birtles (:birtles)]

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 (in nsFrame::Init, specifically this bit).

The trouble is, in this particular test we have the following sequence:

1. Create a new KeyframeEffect.
2. Set up the keyframes and attempt to update the frame bits but fail because the style frame doesn't exist yet
3. Create the nsFrame for the target element (as part of waiting for anim.ready).
   Unfortunately at this point we have not yet associated the created KeyframeEffect with an animation, so it's not relevant and not registered with the EffectSet and hence not associated with the nsFrame's content. As a result we won't set NS_FRAME_MAY_BE_TRANSFORMED is nsFrame::Init.
4. Associate the effect with the animation causing it to register with the EffectSet and be included in styling.
5. 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.

Comment 20

[Brian Birtles (:birtles)]

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 since IsTransformed() returns false
• nsStyleDisplay::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.

Comment 21

[Brian Birtles (:birtles)]

(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 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.

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 in KeyframeEffect 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

Comment 22

[Brian Birtles (:birtles)]

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

Comment 23

[Brian Birtles (:birtles)]

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 :)

Comment 24

[Brian Birtles (:birtles)]

I've split off the work for writing WPT for this into bug 1525809.

Comment 25

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Unified build fixes for KeyframeEffect.cpp and KeyframeUtils.cpp; r?hiro

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.

Comment 26

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Set NS_FRAME_MAY_BE_TRANSFORMED bit when we have nsChangeHint_UpdateTransformLayer; r?hiro

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.

[1] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/layout/base/RestyleManager.cpp#1191-1199

Depends on D18910

Comment 27

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Drop KeyframeEffect::MaybeUpdateFrameForCompositor; r?hiro

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

Comment 28

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Don't share styles when an element has animations applied to it; r?emilio

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.

[1] https://searchfox.org/mozilla-central/rev/6e3cc153566f5f288ae768a2172385b8436d61dd/servo/components/style/sharing/mod.rs#597

Depends on D18912

Comment 29

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - 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

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

Comment 30

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Trigger transitions in test_missing-keyframe-on-compositor.html; r?hiro

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

Comment 31

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Add a version of KeyframeEffect::GetTargetComputedStyle that does not flush style and use it; r?hiro

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

Comment 32

[Brian Birtles (:birtles)]

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

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 34

[Brian Birtles (:birtles)]

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.

Comment 35

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Make KeyframeEffect::CreateComputedStyleForAnimationValue handle a null elementForResolve; r?hiro

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

Comment 36

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Add tests for unstyled data; r?hiro

Depends on D19885

Comment 37

[Brian Birtles (:birtles)]

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.

Comment 38

[Brian Birtles (:birtles)]

Attachment: Bug 1524480 - Make test_animation_properties.html flush styles; r?hiro

Depends on D18915

Comment 39

[Brian Birtles (:birtles)]

moz-phab submit seems to be breaking in all sorts of interesting ways...

Comment 40

[Brian Birtles (:birtles)]

Phabricator should be fixed now. Just one more patch left for review.

Comment 41

[Brian Birtles (:birtles)]

Thanks for the reviews!

Comment 42

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

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