Closed Bug 1292001 Opened 8 years ago Closed 5 years ago

Make sure reversing an existing transition still works if the effect is removed/replaced

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla75
Tracking Flags:
Tracking Status
firefox75 --- fixed

People

(Reporter: boris, Assigned: birtles)

References

Details

Attachments

(9 files)

Bug 1292001 - Move transition-timing-function from keyframe easing to effect easing; r?boris
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Add tests for calling setKeyframes on effects for CSSTransitions; r?boris
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Modernize test_transitions_replacement_on_busy_frame.html; r?boris
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Move transition start value replacing behavior to the CSSTransition; r?boris
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Drop TransitionProperty() and ToValue() from ElementPropertyTransition; r?boris
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Make transition reversing behavior work even when the entire effect is replaced; r?boris
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Drop ElementPropertyTransition; r?boris
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Update expected easing for transitions in DevTools tests; r?daisuke
47 bytes, text/x-phabricator-request
Details | Review
Bug 1292001 - Fix lint warnings on a CLOSED TREE; a=lint-fix
47 bytes, text/x-phabricator-request
Details | Review
After implementing Bug 1049975, we support SetEffect for CSSTransition, but we have to make sure it still works if its effect is removed or replaced by a new KeyframeEffect, instead of an ElementPropertyTransition, while this transition is running. According to Bug 1049975 comment 44, we still need to do some work to handle the timing differently if the new transition reverses an existing one [1]. [1] http://searchfox.org/mozilla-central/rev/740bb4ed16d070cf5d466231b30e80b9204aec54/layout/style/nsTransitionManager.cpp#716-753
Priority: -- → P3
Blocks: 1178660
This doesn't need to block the Animation interface.
No longer blocks: 1178660
Blocks: 1398037

Let's see what breaks by moving transition-timing-function to effect easing...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b84016acb08c0f354327848d517b4ab0991b94b6

That's not all there is to this bug, however. Reproducing the relevant part of my comment from bug 1049975...

Suppose I want to create a new type of color transition that instead of animating directly from red -> green, animates red -> orange -> green.

In future I guess I will write something like:

  // transitionstart is defined in CSS transitions level 2
  elem.addEventListener('transitionstart', () => {
    // The getAnimations filter parameters are yet to be specced
    const colorTransitions =
      elem.getAnimations({ transitionProperty: 'width' });
    if (colorTransitions.length != 1) {
      return;
    }
    const colorTranition = colorTransitions[0];
    // Make effect mutable
    colorTransition.effect = new KeyframeEffect(colorTransition.effect);
    effect.setKeyframes({ color: [ 'red', 'orange', 'green' ] });
  });

  // Trigger transition
  elem.style.transition = 'color 2s ease-in';
  getComputedStyle(elem).color;
  elem.style.color = 'green';

Now, the important thing at this point is that if we do:

  elem.style.transitionProperty = 'none';

The modified transition should be cancelled. So far, I think this is easy.

The question is, what should happen if we do:

  elem.style.color = 'red';

Normally we would detect that we are reversing an animation and calculate how
far we are into the original transition and use that to shorten the new
transition.

I think we can still support that. The information we need from oldPT is
basically:

  1. Its endpoint value (so we can skip creating redundant transitions)
  2. Its effective start value (mStartForReversingTest)
  3. Its CurrentValuePortion -- this is purely based on timing so we don't need to
    store anything. However currently we store the timing function as on the
    first keyframe. It seems like it would be better to store the timing function
    as an effect-level timing function since it is less likely to be overwritten
    then.
  4. Its reverse portion

As for the replaced transition behavior -- I think we can skip that if the
effect is not a ElementPropertyTransition. It's just a performance optimization
that we can say simply doesn't apply once you replace the effect.

So, to make this work we'd basically need to:

  • Move mStartForReversingTest, mReversePortion from ElementPropertyTransition to
    CSSTransition
  • Add mTransitionProperty to CSSTransition
  • Probably add mTransitionToValue to CSSTransition
    (We might be able to be clever about this and, for example, only assign it
    when the effect is replaced with something that that is not an
    ElementPropertyTransition. Or we could even use mTransitionProperty to try to
    look up the end-point in the new set of keyframes--I'm not sure if that would
    always work, however.)
  • Set the timing function on transitions as an effect-level easing (rather than
    keyframe-level easing)

The above patch simply addresses the last point. Beyond that we'll need to check what other browsers do.

Assignee: nobody → brian
Status: NEW → ASSIGNED

Test case for changing the keyframes only: https://jsfiddle.net/gnhfsy27/

Given that we now no longer require substituting in a whole new effect in order to modify a transition, I don't know that this bug is strictly necessary any more. As long as we make sure reversing works correctly when using setKeyframes(), it would be fine if it doesn't work correctly when supplying a completely different KeyframeEffect (although that distinction will obviously need to be specified).

We still need to do a bit of work to get setKeyframes() to work properly with transitions but I will file separate bugs for that.

Summary: Make sure reversing an exist transition still works if the effect is removed/replaced → Make sure reversing an existing transition still works if the effect is removed/replaced

Maybe I can just do it all in this bug after all. For my notes, I think what we need is:

  • Part 1 - Move the easing to effect (WIP patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4b46e6cdd3d5c0f1b339f73bde0aa6aebadf389)
  • Part 2a - Write tests for setting the keyframes on a transition including
    • Correctly interpolating with the new values
    • Correct reversing behavior
    • When the set of keyframes is empty
    • (These will likely trip up assertions in ElementPropertyTransition which assumes it always has 2 keyframes so we'll want to combine 2a and 2b when landing)
  • Part 2b - Get tests in part 2a to pass. This will require reworking ElementPropertyTransition to deal with having different numbers of keyframes.
  • Part 3a - Write tests for setting the effect of a CSSTransition to a different KeyframeEffect or even to null including:
    • Reversing behavior
    • What else?
  • Part 3b - Get tests in part 3a to pass. This will likely involve moving mStartForReversingTest, mReversePortion from ElementPropertyTransition to CSSTransition
  • Add spec text to CSS Transition 2 that mentions that the reversing behavior is based on the original transition property and original transition end-point and is not based on any changes made via the API (since such changes could involve removing the transition property altogether).

This is yet to be specified although it is already tested in CSS Transitions
WPT. It is also needed in order to produce the expected reversing behavior when
CSSTransition's keyframes are replaced.

Looks like moz-phab is broken. There should be 7 patches here. I'll try again in 1hr or so.

In the next patch we want to add a test for transitions whose start value is
replaced when the transition has been modified using the Web Animations API.
However, first we update the existing test for this replacing behavior to
make it more readable so we can copy it.

Depends on D64517

This is needed so that later we can make the effect of transitions replaceable.

Note that the test added in this patch will fail without the code changes in
this patch.

Depends on D64518

These are no longer needed and they make assumptions about the shape of the
keyframes/properties which are not valid if the keyframes are replaced.

Depends on D64519

Depends on D64522

Pushed by bbirtles.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9dbefe504221 Move transition-timing-function from keyframe easing to effect easing; r=boris https://hg.mozilla.org/integration/autoland/rev/f35daa06e7b6 Add tests for calling setKeyframes on effects for CSSTransitions; r=boris https://hg.mozilla.org/integration/autoland/rev/4f12b279af44 Modernize test_transitions_replacement_on_busy_frame.html; r=boris https://hg.mozilla.org/integration/autoland/rev/a94cb2c7e518 Move transition start value replacing behavior to the CSSTransition; r=boris https://hg.mozilla.org/integration/autoland/rev/851b689f6593 Drop TransitionProperty() and ToValue() from ElementPropertyTransition; r=boris https://hg.mozilla.org/integration/autoland/rev/111e88a3f2a9 Make transition reversing behavior work even when the entire effect is replaced; r=boris https://hg.mozilla.org/integration/autoland/rev/7ecffcc58457 Drop ElementPropertyTransition; r=boris
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/22033 for changes under testing/web-platform/tests
Upstream PR was closed without merging

The transition-timing-function is now applied to the effect as opposed to the
individual keyframes.

Depends on D64523

Flags: needinfo?(brian)
Pushed by bbirtles.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d8822b74c84 Move transition-timing-function from keyframe easing to effect easing; r=boris https://hg.mozilla.org/integration/autoland/rev/ce94ec6c9c91 Add tests for calling setKeyframes on effects for CSSTransitions; r=boris https://hg.mozilla.org/integration/autoland/rev/4e487c8eeb87 Modernize test_transitions_replacement_on_busy_frame.html; r=boris https://hg.mozilla.org/integration/autoland/rev/3772ef2b71c9 Move transition start value replacing behavior to the CSSTransition; r=boris https://hg.mozilla.org/integration/autoland/rev/cff32676317c Drop TransitionProperty() and ToValue() from ElementPropertyTransition; r=boris https://hg.mozilla.org/integration/autoland/rev/9cf0f778c49f Make transition reversing behavior work even when the entire effect is replaced; r=boris https://hg.mozilla.org/integration/autoland/rev/d23c3553d173 Drop ElementPropertyTransition; r=boris https://hg.mozilla.org/integration/autoland/rev/f08f38bb2448 Update expected easing for transitions in DevTools tests; r=daisuke
Pushed by bbirtles.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c8ae100649c8 Fix lint warnings on a CLOSED TREE; a=lint-fix
Upstream PR was closed without merging
Upstream PR merged by stephenmcgruer
Regressions: 1633442
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: