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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Blocks 1 bug)

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

()

Attachments

(10 attachments, 1 obsolete attachment)

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
Assignee

Description

5 months ago

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

Flags: needinfo?(emilio)
Assignee

Comment 1

5 months ago

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

Flags: needinfo?(emilio)
Assignee

Comment 2

5 months 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: nobody → bbirtles
Status: NEW → ASSIGNED
Summary: Sharing style cache shares style even when one element has pending animation changes → Styles shared even when there are pending animation changes from Web Animations
Assignee

Comment 3

5 months 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:

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

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

5 months 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.

What I guessed is that there is a real issue and adding the animation check in test_candidate would be another wallpaper of it.

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

5 months 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:

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

Assignee

Comment 9

5 months ago

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

(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

5 months 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;
  • 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 CanThrottleCanThrottleIfNotVisibleCanIgnoreIfNotVisible 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.

Assignee

Comment 12

5 months 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

5 months 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.

Summary: Styles shared even when there are pending animation changes from Web Animations → Don't flush styles when creating a new animation via Element.animate
Assignee

Comment 14

5 months 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

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

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

Assignee

Comment 17

5 months 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

5 months ago

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

Assignee

Comment 19

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

Assignee

Comment 20

5 months 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:

So this bug probably only manifests when we have an animation starting with transform: none.

Assignee

Comment 21

5 months 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 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

Assignee

Comment 22

5 months 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

5 months 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

Updated

5 months ago
Blocks: 1525809
Assignee

Comment 24

5 months ago

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

Assignee

Comment 25

5 months 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

5 months 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.

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

Depends on D18910

Assignee

Comment 27

5 months 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

5 months 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.

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

Depends on D18912

Assignee

Comment 29

5 months 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

5 months 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

5 months 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

5 months 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

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

5 months 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.

Attachment #9041999 - Attachment description: Bug 1524480 - Update the cumulative change hint even if a keyframe effect's → 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
Assignee

Comment 35

4 months 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

4 months ago

Depends on D19885

Attachment #9041997 - Attachment is obsolete: true
Assignee

Updated

4 months ago
Blocks: 1528142
Assignee

Comment 37

4 months 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 39

4 months ago

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

Assignee

Comment 40

4 months ago

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

Assignee

Updated

4 months ago
Blocks: 1528151
Assignee

Comment 41

4 months ago

Thanks for the reviews!

Comment 42

4 months 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
You need to log in before you can comment on or make changes to this bug.