Race condition between animation start and animation timeout

[aidesigner]

I believe I have found an issue using the custom animation trigger (animator.animate(...)). The aurelia-animator-css.js has a fixed timeout (this.animationTimeout = 50), which was causing my animations to not start 50% of the time. Basically the animation needs to start, trigger "webkitAnimationStart animationstart", and get to this line of code "_this7._addAnimationToStack(animId);" within 50ms or the timeout (setTimeout) triggers and all is lost. I increased the timer to 100ms and all is good, but not sure if this arbitrary time increase is a robust fix. Given it is hard to predict the worst case environment/speed 100ms may not be sufficient.

[EisenbergEffect]

@zewa666 Can you look into this? We need a robust and performant way to handle this. We don't want unnecessary delays..and we don't want them to be too short either.

[zewa666]

@aidesigner it's pretty hard to test this since I cannot reproduce it. The delay of 50ms normally is far more then necessary since the event should immediately trigger. So essentially the delay is necessary for items which are annotated with au-animate but have no CSS animation defined.

Can you maybe try to isolate the issue somehow to make it reproducible?

[michaelmalonenz]

@EisenbergEffect @zewa666 I think I have reproduced this bug after the latest release. Please find the code (based on the skeleton) here: https://github.com/michaelmalonenz/skeleton-navigation/tree/bug/animator-css%2328

[EisenbergEffect]

@zewa666 Please take a look when as soon as you can. Thanks!

[michaelmalonenz]

Ok, maybe it's a different bug - upping the this.animationTimeout to 500ms did nothing to solve the issue. It still is an interaction with animator-css, though (the issue doesn't present without it installed).

[zewa666]

@michaelmalonenz thank you very much for creating the sample, sadly though I just can take a better look at it next week since I'm currently on vacation and stuck with a horribly old macbook and bad work conditions like no stable internet etc. :angry:

[zewa666]

@aidesigner and @michaelmalonenz is this issue still present with the latest release of the skeleton navigation app? I still can't reproduce it and wonder if it maybe had either to do with some old issues or another module used in combination with the animator. @michaelmalonenz potentially the issue happened in combination with aurelia-dialog.

Please check again, I will take a closer look at the issue this week.

[michaelmalonenz]

@zewa666 The issue I am seeing is exactly what @npelletm was seeing in #29 and like their issue, mine is only present when I am using FontAwesome fa fa-spin classes on at least one of the elements. I am using Windows 10 and it presents on Chrome, IE, Firefox and Edge.

My sample repo is here: https://github.com/michaelmalonenz/skeleton-navigation/tree/bug/animator-css%2328

[zewa666]

@michaelmalonenz ok so it seems the issue is related to the show attribute. we'll get that sorted out now that we know the root-cause

[zewa666]

@aidesigner and @michaelmalonenz can you re-test with the latest beta. I guess the fixes for the other issue should have helped here as well

[michaelmalonenz]

@zewa666 I'm still seeing the issue. I've committed the package updates to the repo I mentioned above, so let me know if I've missed something.

[zewa666]

@michaelmalonenz I cannot follow the example as you bind towards a variable createRequestSent which is never defined, so I'm not sure what the example should demonstrate.

[michaelmalonenz]

Oh dear, that is embarrassing. Nevertheless, I have now corrected the code and I still see the issue.

[zewa666]

ok cool now I see the issue, still in the show binding ... hmm have to debug that one better.

[insidewhy]

I'm seeing this issue with a repeat.for and a small list of elements. I have to up the animationTimeout to 100 for a list of about 10 items. For 40 items I need to up it to 500 or I don't see animations at all.

[insidewhy]

In the afformentioned case I'm not using show.bind at all.

[insidewhy]

The only way I could get animations to work is to increase the animation timeout to a really high value. However this then triggers another bug with this code and animations can start interfering with each other. I've opened a fix for that in this pull request

[insidewhy]

It seems this is fairly easy to reproduce, just include a repeat.for element with a non-trivial body and keep adding items to the array you're repeating. It depends on the browser but eventually once you've added enough items to the array this race condition triggers. With chrome on linux it triggers very easily.

I'm thinking that to check if an animation will occur that an element's style should be compared with its getComputedStyle() right after the au-enter-active class has been added.

[insidewhy]

I've found a way to solve this, will be opening a PR later today.

[michaelmalonenz]

After updating all to the latest releases, this issue has been fixed for me. Thanks @ohjames @zewa666 and @EisenbergEffect for your responses on this

[zewa666]

if we can get a response from @aidesigner I'd gladly close this issue. Thanks for the great work and contribution @ohjames

[insidewhy]

Even after this fix there is still one very serious race condition when using repeat.for with any animator that leads to loss of sync between the model and view, then eventual crashes.

One more fix will be necessary, either in ViewSlot or the repeat code.

[joelcoxokc]

I had a few concerns about the initial Animator. This has been an issue for me for a long while, I have been toying with it for quite some time. My biggest use case is animating viewsSlots with high performance across browsers and platforms, like IOS and Android

So here is my solution.

I actually basically rebuilt the Animator.

However, I do not know all of the use cases, so it may need to be tested pretty well.

First I am going to post about the Property class that I created, This class will handle all css browser prefixing and event browser prefixing.

Then I will show the updated Animator

// @each static
// @returns all necessary
//   - HTMLElement.style properties
//   - cssRule properties
//   - EventNames properties
class Property {
  static animation(): PropertyInstruction {}
  static transition(): PropertyInstruction {}
  static transform(): PropertyInstruction {}

  /**
   * The following is an example instance from calling Property.animation()
   * All properties will be cached, So everything is not rebuilt each time. 
   * Consider we are using an older version of Mozilla
   */

  // The AnimationEvent will come from an actually tested event.type
  // Most Browsers it will not have a prefix, So this is just an example
  events = {
    animationstart:     'MozAnimationStart',
    animationend:       'MozAnimationEnd',
    animationiteration: 'MozAnimationIteration',
  },
  // dom properties are for using the Element.prototype.style
  dom = {
    animation:               'MozAnimation',
    animationName:           'MozAnimationName',
    animationDelay:          'MozAnimationDelay',
  },
  // css properties are for what ever you can think of.
  css = {
    animation:               '-moz-Animation',
    animationName:           '-moz-Animation-name',
    animationDelay:          '-moz-Animation-delay',
  }
}

/**
 * setStyle
 * You can stick to the default browser properties, and this will handle any
 * necessary browser prefix
 * @propertyName the css PropertyName
 * @value the css PropertyValue
 * @return {Boolean} Whether or not the property was set
 */
Property.prototype.setStyle(element: DOM.Element, propertyName: String, value: String): Boolean {}

/**
 * assignStyle
 * Simply iterates over the properties Object and calls setStyle
 */
Property.prototype.assignStyle(element: DOM.Element, properties: Object): Boolean {}

/**
 * getComputedValue
 * This method will handle any necessary browser prefixing
 * will call styles = window.getComputedStyle(element)
 * will call styles.getPropertyValue(propertyName)
 * @return {CSSRule} The css value found or null
 */
Property.prototype.getComputedValue(element: DOM.Element, propertyName: String): CSSRule {}

interface PropertyEvent {
  handler: Function; // Callback used in the event
  bound: Boolean; // Whether the event is bound or not
  triggered: Boolean; // Whether the event has been triggered or not
  dispose: Function; // function to call, that will unbind the listener
}

/**
 * subscribe
 * Creates an event Listener and returns a PropertyEvent
 * This method will also handle any browser prefixes
 * See PropertyEvent interface above
 *
 * You can also pass aliases instead of the full name
 * Example 'start' === 'animationstart'
 * @usage
 *  let anim = Property.animation();
 *  let subscription = anim.subscribe(element, 'start', ()=>{
 *     subscription.triggered === true
 *     subscription.bound === true
 *     subscription.dispose();
 *  },true);
 */

Property.prototype.subscribe(element:DOM.Element, eventName: String, handler: Function, bubbles:Boolean): PropertyEvent {}

You may be wondering why all of this is necessary... I am using this to easily access necessary information on elements, without checking the property prefixes on every animation.

This will also clean up the necessary logic needed in the actual animator, so that we can do some more interesting things.

The next post will be about the actual animator.

[joelcoxokc]

So Here is the deal, It is necessary that you do not view the code, until you understand the following

I have created an AnimationInstruction class This class has a lifeCycle of arrays, Each array contains classNames that will be either added or removed at different points throughout the animation lifeCycle

Each AnimationInstruction instance will contain three Objects

instance = {
  add:lifeCycle,
  remove:lifeCycle,
  contains:Object<Array>,
}

The add and remove objects contains the arrays of classNames I just explained.

The contains object will also be an array of classNames except will be used for conditions

So here is the lifeCycle

lifeCycle = {
  begin: Array<className>, // When animator[enter|leave|...] is invoked
  prepare:Array<className>, // When you should add the initial anim class like au-enter
  start:Array<className>, // When the animationstart Event is triggered
  end:Array<className>, // When the animationend Event is triggered
  done:Array<className>, // After animationend is triggered and unbound
  activate:Array<className>, // After the animation start and end events are bound
  clean:Array<className>, // After all events are unbound, and for final cleanup
}

Here is an example of creating an AnimationInstruction instance for animator.enter

return new AnimationInstruction({
  name: 'au-leave',
  useDoneClasses: true, // Condition for done classes
  suppressEvents: false, // Boolean for suppressing events
  add: {
    prepare: ['au-enter'],
    activate: ['au-enter-active'],
    done: ['au-entered'],
  },
  remove: {
    begin: ['au-entered', 'au-left'], // Done classes will be conditional
    end: ['au-enter-active', 'au-enter'],
    clean: ['au-enter-active', 'au-enter'],
  },
  contains: {
    stagger: ['au-stagger', 'au-stagger-enter'],
    canAnimate: ['au-animate'],
  },
  doneClasses: ['au-entered', 'au-left']
});

When the animation runs, At each point in the lifeCycle, It will add any classes under instruction.add.<point> It will remove any classes under instruction.remove.<point>

Using this lifecycle, You can now imagine how much easier it will be to manage animation states. This also makes the animator itself much less complex.

If this does not make sense please let me know.

I will submit A review PR on Monday so we can all preview the code.

After submitting the code, I will explain how I solved the timeout issue!

[EisenbergEffect]

@joel. A couple of questions:

1. Are you intending to change the public interface of the animator or is this just a set of internal implementation changes? We can't change the public interface at this point...
2. Can you be clear about what this actually improves? Does it improver performance? Have you measured that? In what scenarios? Or is this just a refactoring/cleanup/simplification?
3. Have you run all the existing unit tests for the animator? are they passing?
4. What scenarios are you targeting? Have you testing both screen transitions and list animations? Did you try out your implementation with the skeleton-navigation app?

[joelcoxokc]

@EisenbergEffect 1: Public Interface Remains 2: Yes and Yes,

• Performance:
  • We don't check for browser prefixes on every animation.
  • We don't create four event Listeners for Every Animation
  • Handling keyframeLookup based on preference
  • setTimeout was never being cleared (which was causing the timeout issue)
  • Also unit tests were not unbinding events (also causing invalid tests to pass)
• Refactor:
  • The animator is Much Smaller, yet there are more files
  • All browser prefixing and cssProperty lookup now abstracted from CssAnimator
  • Lifecycle based animations, Which provides a clean classList API
• Tests:
  • All unit tests pass
  • The only changes I made to unit tests is properly unbinding Events
• Scenarios:
  • All scenarios working smoothly

[EisenbergEffect]

Ok, that sounds good. @joelcoxokc Can you create a new PR with the refactor. When that's in place, we'll have @zewa666 do as review. Assuming everything looks good, we can merge it in.

[insidewhy]

I'll also check over the PR, I've got a bunch of recent experience hunting down race conditions to do with the animator and repeat.for. I'm glad it's being refactored, the existing code badly needs to be refactored, giant sections of code are cut and paste throughout the file with only small modifications. Prefix handling to me isn't a big deal, should only be a very tiny part of this

@joelcoxokc I removed the setTimeout usage (except during the stagger where it is necessary), this method of detecting animation completion was not good enough. You've checked out the most recent work? You should also make sure you registered for the "animation end" event before receiving the "animation start" event as this ended up being another race condition.

[insidewhy]

Sorry one more question:

setTimeout was never being cleared (which was causing the timeout issue)

What is "the timeout issue" here? If it's the one referred to by this github issue I worry you may be missing some things.

[joelcoxokc]

@ohjames I have the latest, with the setTimeout under stagger. Under the hood it is primarily doing the same thing just a few minor changes. Events are properly bound.

I will be submitting the code here in about 20 minutes.

I am just doing some documentation

[joelcoxokc]

@EisenbergEffect @zewa666 @ohjames Ok So the API is similar to what I explained above. I made A few minor Changes.

The Property class

I removed the static Property,animation|transition|transform

Now Property is an exported instance. and all necessary Properties, events, and css Properties exist in the same format on the Property Instance.

ExampleUsage

import {Property} from './property';

var subscription = Property.subscribe(element, 'animationstart', (e)=> {
  subscription.triggered === true;
  subscription.dispose();
  subscription.bound === false;
});
subscription.bound === true;

Property.css.animationName === 'animation-name';
Property.css.animationName === '-moz-animation-name'; // Older Moz Browser Example

Property.dom.animationName === 'animationName';
Property.dom.animationName === 'MozAnimationName'; // Older Moz Browser Example

Property.events.animationstart === 'animationstart';
Property.events.animationstart === 'mozAnimationStart'; // Older Moz Browser Example

Here is the interface for the lifecycle.

You can see how I use the lifecycle by looking at LifeCycle.prototype.run

At each step I call LifeCycle.prototype.runStep

runStep will handle all element classList manipulation as well as event triggers.

Here is the LifeCycleStrategy Interface

interface LifeCycleStep {
  addClass: Array|String, // Array of classNames
  removeClass: Array|String, // Array of classNames
  dispatch: String, // Event to be dispatched
}

interface LifeCycleStrategy {
  name: String;
  canStagger: Boolean;
  suppressEvents: Boolean;
  begin: Array<LifeCycleStep>;
  prepare: Array<LifeCycleStep>;
  activate: Array<LifeCycleStep>;
  done: Array<LifeCycleStep>;
  timout: Array<LifeCycleStep>;
  animationStart: Array<LifeCycleStep>;
  animationEnd: Array<LifeCycleStep>;
}

Each step is an Array, because, lets say you want to dispatch an event before you add a class

lifecycle = {
   animationEnd:[
      {dispatch:'some:event:name'},
      {addClass:'my-class-nam'}
   ]
   .....
}

[insidewhy]

@aidesigner I believe my PRs in #33 and #35 fix this issue, could you close it?

However you should be aware that there are still race conditions and that animations + repeat.for are still not something I would consider using: see https://github.com/aurelia/templating-resources/issues/183

[aidesigner]

Thanks for all the effort to address this issue. With Joel's complete rework (Upcoming merge) of the Animator I have a high degree of confidence the initial issue is solved. Unfortunately I no longer have my initial setup as I switched the Aurelia animator interface (great abstraction) to use Velocity. Should any issue arise I will open a new linked issue.

[EisenbergEffect]

@aidesigner Are you referring to Joel's animator PR from above. (We haven't merged it yet.) Did you try that and find that it solved the issues?

[aidesigner]

@EisenbergEffect My comments where directed at Joel's animator PR and I have updated my previous comments. With Joel's complete internal restructure my feeling is that any timing issue would be a variation and constitute a separate ticket. I am inclined to try it out at the next opportunity because of quality/effort in the PR.

[insidewhy]

@aidesigner the issue is already solved with my recent PRs, you can test it today. Joel's rewrite removes a lot of duplication that currently exists in the code though which seems great, looking forward to testing it.