CSS transition start value is not computed correctly #7816

paulrouget · 2015-10-01T16:25:22Z

In the example below, the transform:translateX value is incremented every 2 seconds. A CSS transition animates the div. But during the animation, the start value is not the value set via the style attribute, but the initial value set in the CSS rule.

<style>
  div {
    position: absolute;
    top: 10px;
    left: 10px;
    width: 100px;
    height: 100px;
    background: red;
    transform: translateX(-100px);
    transition: transform 1500ms linear;
  }
</style>

<body>
  <div></div>
</body>

<script>
  var i = 0;
  setInterval(() => {
    var transform = `transform:translateX(${i * 50}px)`;
    document.querySelector('div').setAttribute('style', transform);
    i = (i + 1) % 4;
  }, 2000);
</script>

EDIT

Here is an example that highlight all the problems I found while working on this issue:

<style>
  div {
    position: absolute;
    top: 10px;
    left: 10px;
    width: 100px;
    height: 100px;
    background: red;
    transition: all 2000ms linear;
    opacity: 0.1;
  }

  div.foo {
    width: 200px;
    opacity: 1;
    transform: translateY(200px);
  }
</style>

<div></div>

<script>
  var div = document.querySelector('div');
  setTimeout(() => div.classList.toggle('foo'), 1000);
  setTimeout(() => div.classList.toggle('foo'), 4000);
  setTimeout(() => div.classList.toggle('foo'), 5000);
</script>

The text was updated successfully, but these errors were encountered:

paulrouget · 2015-10-01T16:26:01Z

Maybe related to #7781.

jdm · 2015-10-01T16:49:23Z

Presumably we're getting incorrect values in http://mxr.mozilla.org/servo/source/components/layout/animation.rs#22 . The trick is figuring out why that's happening.

paulrouget · 2015-10-06T08:03:07Z

The new style is reset to old style in start_transitions_if_applicable:

property_animation.update(new_style, 0.0);

then it's supposed to be set to the final style in cascade_node_pseudo_element:

animation.property_animation.update(&mut *Arc::make_mut(style), 1.0);

But cascade_node is not called once the animation is over.

paulrouget · 2015-10-06T12:06:56Z

@pcwalton why is this_style reset to its initial value if there's a transition?

paulrouget · 2015-10-08T11:49:55Z

Can animations be interrupted or updated? Doesn't appear to work. Did it ever work or it's not implemented yet?

pcwalton · 2015-10-08T14:14:24Z

@paulrouget I'm confused by what "reset to its initial value" means here—could you elaborate?

It's not safe to call cascade_node_pseudo_element during transitions because they run off the main thread.

paulrouget · 2015-10-08T15:52:40Z

@paulrouget I'm confused by what "reset to its initial value" means here—could you elaborate?

Sure. Keep in mind that I'm really not familiar with this code, so feel free to tell me if I'm totally off track :)

I'm looking at when animation::start_transitions_if_applicable is called (https://mxr.mozilla.org/servo/source/components/layout/css/matching.rs#483):

// Trigger transitions if necessary. This will reset `this_style` back to its old value if
// it did trigger a transition.

… and in animation::start_transitions_if_applicable (https://mxr.mozilla.org/servo/source/components/layout/animation.rs#24):

// Set the property to the initial value.
property_animation.update(new_style, 0.0);

So in cascade_node_pseudo_element, we do not update the style until the animation is finished. The final update happens here: https://mxr.mozilla.org/servo/source/components/layout/css/matching.rs#449

Without digging too much, I just assumed that it meant that when a DOM element goes from transition:scale(1) to transition:scale(2), we keep the computed style at transition:scale(1) until the transition is done, then update to transition:scale(2).

By "reset to its initial value", I meant that we do not switch to transition:scale(2) immediately.

Assuming I assumed right:

Like you said, we can't update it during the transition, so it's done at the end. But:

Why is it done at the end and not before the transition start? It doesn't impact the transition itself. And what about getComputedStyle() in the middle of a transition?
Even if we want to do it at the end, the above-mentionned code is not called at the end of the transition (and really, that's the only thing I think we need to fix for this bug).
If a transition on the same property happens in the middle of the transition, where would we find the current computed value to compute the new animation?

paulrouget · 2015-10-09T05:12:01Z

~~Hmm, there's something wrong in my reasoning. The computed style (as seen from JS), is correctly updated during the transition and, of course, layout is properly updated overtime.~~ Edit: nope. See next comment.

paulrouget · 2015-10-14T07:19:55Z

The computed style (as seen from JS), is correctly updated during the transition and, of course, layout is properly updated overtime.

Not all of them! opacity style (getComputedStyle().opacity) is not updated. width is.

  div {
    position: absolute;
    top: 10px;
    left: 10px;
    width: 100px;
    height: 100px;
    background: red;
    transition: all 1000ms linear;
    opacity: 0.1;
  }

  div.foo {
    width: 200px;
    opacity: 1;
    transform: translateY(200px);
  }

3 properties are being animated.

From div to div.foo, on screen, everything is animated correctly. The computed style (as seen from JS) of width is properly updated. Opacity is not (stays at 0.1). Transform can't be printed (empty string), so I can't tell, but I guess it behaves like the opacity property.

From div.foo to div, on screen, transform and opacity are properly reset to their initial value (none and 0.1), but without the transition (because Servo animates from the very initial state, skipping the div.foo values). And the width property stays at 200px. As for the computed style, it is stuck at the div.foo values for the 3 properties.

Opacity and transform are properties that do not require reflow. They have that in common. So I guess that explain why they behave differently (something like OMTA in Gecko).

So here, I see 3 issues, and I don't know how much they are related:

computed style (as seen from JS) of opacity and transforms are never updated
width of the div (on screen and computed style as seen from JS) is not updated on the 2nd animation
For 2 animations chaining (state A -> B -> C), on the 2nd animation (B -> C), the opacity and transform values at t0 come from A, not from B

paulrouget · 2015-10-14T07:39:44Z

Commenting out this line:

https://mxr.mozilla.org/servo/source/components/layout/animation.rs#33

solves all the issues mentioned above.

But I'm not sure to understand why it's here in the first place.

There's a very short and barely noticeable flickering at the beginning of the animation where the width of the div jumps to its final value, and then, it's properly animated. I guess this is what the reset to 0.0 is for. So then, what would be missing is to call cascade_node_pseudo_element once the animation is over I guess.

paulrouget · 2015-10-14T07:44:01Z

@pcwalton: can you please explain why this line is necessary: https://mxr.mozilla.org/servo/source/components/layout/animation.rs#33

Can you confirm that cascade_node_pseudo_element should be called at the end of the animation to trigger this code: https://mxr.mozilla.org/servo/source/components/layout/css/matching.rs#449 - if so, how can this be done?

paulrouget · 2015-10-14T09:29:16Z

I guess we could do that on Msg::ChangeRunningAnimationsState.

pcwalton · 2015-10-15T16:55:32Z

You can't call cascade_node_pseudo_element during animations. That's for nodes, not for fragments.

pcwalton · 2015-10-15T16:56:05Z

To be more specific, there is no access to the DOM during animations. It's forbidden, because JS could be running concurrently during a transition (which is the whole point of OMT animations).

pcwalton · 2015-10-15T16:58:35Z

That line is trying to set the animation to its original start value, but I guess the start value for the animation isn't properly taking the cascade into account. We will need to fix the code that initializes the animation. You can't access the DOM during animations, so this has to be all set up properly in advance.

paulrouget · 2015-10-23T05:48:39Z

We should keep a list of finished animations, then at the next cascade, we can update the property value. Also, if animations are not finished, we should cancel them and update the style according to the current time.

@pcwalton does this require some big changes or it's just a matter of piping the right things together? In other words, do you think I can fix that or it's better if you take care of it?

metajack · 2015-11-02T16:43:55Z

Pinging @pcwalton

This is blocking browser.html progress.

pcwalton · 2015-11-03T00:43:28Z

@paulrouget I don't think it'll be too tough—a medium-sized change, perhaps?

paulrouget · 2015-11-03T03:07:36Z

In cascade_node_pseudo_element, at the beginning of the function, when we go through the animations and update the style to its progress=1.0 value, we should not assume the animation is over and update it to the its real progress value.

Something like replacing:

animation.property_animation.update(&mut *Arc::make_mut(style), 1.0);

with:

let now = clock_ticks::precise_time_s();
let mut progress = (now - animation.start_time) / animation.duration();
if progress > 1.0 {
  progress = 1.0
}
if progress > 0.0 {
  animation.property_animation.update(&mut *Arc::make_mut(style), progress);
}

This appears to work.

We should also cancel the animation at this point (not sure how yet).

Now, we also need to handle finished animations here, but finished animations are not accessible at this point. So in components/layout/animation.rs#update_animation_state, I'd suggest that we fill a finished_animations array (the same way we have a running_animations array), that we would flush in cascade_node_pseudo_element after we updated the style to its progress=1.0 value.

paulrouget · 2015-11-03T03:57:55Z

@pcwalton could you take a quick look at this commit: b8379dd

I just want to know if that looks like the proper approach.

I still need to cancel animations in cascade_node_pseudo_element and flush the finished_animations array.

pcwalton · 2015-11-04T03:31:42Z

Seems reasonable to me.

paulrouget · 2015-11-09T11:44:37Z

Well, I think this is beyond what I can do. Things are getting hairy when trying to cancel the animation on a property that is being modified in cascade_node. Maybe someone else can take over. Summary of what (I think) should be done:

save a list of finished animation, then, at the next cascade, the property value can be updated (at progress=1.0) (see a quick & dirty implementation here: b8379dd). The finished animation list should be flushed once the new property value has been computed.
on each cascade, update the value of the animated properties (also addressed in b8379dd)
cancel animations on properties that have changed. Maybe we need a sender/receiver for canceled animations. update_animation_state would check if there's any canceled animations and update the running animation list.

metajack · 2015-11-10T23:22:36Z

@pcwalton agreed to take this

pcwalton · 2015-11-24T23:14:55Z

I have a fix for this locally. Testing performance now.

animations complete or are interrupted. This adds a new pair of reader-writer locks. I measured the performance of style recalculation on Wikipedia and the overhead of the locks was not measurable. Closes servo#7816.

@paulrouget

Write animated values into the `ComputedValues` structures when animations complete or are interrupted. This adds a new pair of reader-writer locks. I measured the performance of style recalculation on Wikipedia and the overhead of the locks was not measurable. Closes #7816. cc @paulrouget r? @glennw  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8670)

animations complete or are interrupted. This adds a new pair of reader-writer locks. I measured the performance of style recalculation on Wikipedia and the overhead of the locks was not measurable. Closes servo#7816.

paulrouget mentioned this issue Oct 1, 2015

Servo + Browser.html: webplatform bugs browserhtml/browserhtml#562

Closed

92 tasks

jdm added the A-layout/uncategorized label Oct 1, 2015

paulrouget mentioned this issue Oct 19, 2015

CSS transitions do not always work #7781

Closed

metajack assigned pcwalton Nov 10, 2015

pcwalton mentioned this issue Nov 25, 2015

Write animated values into the ComputedValues structures when animations complete or are interrupted. #8670

Merged

bors-servo closed this as completed in #8670 Nov 25, 2015

paulrouget added the A-browserhtml/P3 label Jan 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS transition start value is not computed correctly #7816

CSS transition start value is not computed correctly #7816

paulrouget commented Oct 1, 2015

paulrouget commented Oct 1, 2015

jdm commented Oct 1, 2015

paulrouget commented Oct 6, 2015

paulrouget commented Oct 6, 2015

paulrouget commented Oct 8, 2015

pcwalton commented Oct 8, 2015

paulrouget commented Oct 8, 2015

paulrouget commented Oct 9, 2015

paulrouget commented Oct 14, 2015

paulrouget commented Oct 14, 2015

paulrouget commented Oct 14, 2015

paulrouget commented Oct 14, 2015

pcwalton commented Oct 15, 2015

pcwalton commented Oct 15, 2015

pcwalton commented Oct 15, 2015

paulrouget commented Oct 23, 2015

metajack commented Nov 2, 2015

pcwalton commented Nov 3, 2015

paulrouget commented Nov 3, 2015

paulrouget commented Nov 3, 2015

pcwalton commented Nov 4, 2015

paulrouget commented Nov 9, 2015

metajack commented Nov 10, 2015

pcwalton commented Nov 24, 2015

CSS transition start value is not computed correctly #7816

CSS transition start value is not computed correctly #7816

Comments

paulrouget commented Oct 1, 2015

paulrouget commented Oct 1, 2015

jdm commented Oct 1, 2015

paulrouget commented Oct 6, 2015

paulrouget commented Oct 6, 2015

paulrouget commented Oct 8, 2015

pcwalton commented Oct 8, 2015

paulrouget commented Oct 8, 2015

paulrouget commented Oct 9, 2015

paulrouget commented Oct 14, 2015

paulrouget commented Oct 14, 2015

paulrouget commented Oct 14, 2015

paulrouget commented Oct 14, 2015

pcwalton commented Oct 15, 2015

pcwalton commented Oct 15, 2015

pcwalton commented Oct 15, 2015

paulrouget commented Oct 23, 2015

metajack commented Nov 2, 2015

pcwalton commented Nov 3, 2015

paulrouget commented Nov 3, 2015

paulrouget commented Nov 3, 2015

pcwalton commented Nov 4, 2015

paulrouget commented Nov 9, 2015

metajack commented Nov 10, 2015

pcwalton commented Nov 24, 2015