Animation builder and runner. #6092

dvoytenko · 2016-11-08T23:14:06Z

This is the first draft of animation builder and runner. Tests are on the way. Please do an early review.

Partial for #5910.

aghassemi

Still going through it, sending out some early comments.

aghassemi · 2016-11-09T23:48:01Z

extensions/amp-animation/0.1/amp-animation.js

+
+  /** @override */
+  activate() {
+    if (!this.configJson_) {


Maybe move the config parsing to buildCallback for earlier error reporting and also should improve perceived performance as less JS would run between user-interaction and the animation start.

aghassemi · 2016-11-09T23:56:08Z

extensions/amp-animation/0.1/web-animation-types.js

+
+
+/**
+ * @typedef {!WebMultiAnimationDef|!WebKeyframeAnimationDef}


Closure Compiler's New Type Inference (not enabled yet) will complain about this because of the circular dependencies between WebMultiAnimationDef and WebAnimationDef.

I always understood circular dependencies to be fine. Can we confirm it early on?

try gulp type-check --nti (there will be lots of errors but you can search and see if this typedef is flagged)

Just tried. Not sure if it prints all the errors, but nothing is reported against this case.

aghassemi · 2016-11-10T00:00:50Z

extensions/amp-animation/0.1/web-animations.js

+import {getVendorJsPropertyName} from '../../../src/style';
+import {isArray, isObject} from '../../../src/types';
+import {
+  WebAnimationDef,


interesting way of doing this. Does it make sense to move the type defs to amp.extern?

Not yet. But possible in the future. At the very least, I'm definitely anticipating we'll move the web-animation-types to src

emarchiori · 2016-11-10T16:15:16Z

extensions/amp-animation/0.1/web-animation-types.js

+/**
+ * @mixes WebAnimationTimingDef
+ * @typedef {{
+ *   animations: !Array<!WebAnimationDef>,


Wouldn't it be enough for this to be:
animations: !Array<!WebKeyframeAnimationDef>,
I'm not sure I see the point of the nesting otherwise and that should solve the circular dependency mentioned before.

I though that it might be convenient to sometimes apply some animations parameters to all sub-animations. E.g. duration, delay, etc - to avoid copy-pasting them when they are exactly the same.

But if that seems like an over-complication - I'll reconsider.

Also, another nuance, I wanted all of the following to be valid configs:

Shortest key-frame only config:

{ target: target1, duration: 1000, keyframes: {opacity: 0} }

An array of animations:

[ { target: target1, duration: 1000, keyframes: {opacity: 1} }, { target: target2, duration: 500, delay: 500, keyframes: {opacity: 1} } ]

And an object + array:

{ duration: 1000, animations: [ { target: target1, keyframes: {opacity: 1} }, { target: target2, duration: 500, delay: 500, keyframes: {opacity: 1} } ] }

WDYT?

emarchiori · 2016-11-10T16:19:56Z

extensions/amp-animation/0.1/web-animation-types.js

+
+
+/**
+ * @typedef {!Object<string, *>|!Array<!Object<string, *>>}


Doesn't it make sense to make this use a typedef to clearly document that keyframes are only alowed to modify opacity and transform?

@typedef {{ offset: number, opacity: number, transform: string }}

Hmm. Good question. In other words, you'd like to enumerate all allowed right away? I was also going to include visibility and background to start with.

A bit of a difficulty with typedef based whitelist is that something like background-color cannot be defined there. Perhaps we'll just go with a Object<string, *> and clearly document whitelisted props?

The clearly documented whitelist seems fine by me.

emarchiori · 2016-11-10T16:21:27Z

extensions/amp-animation/0.1/web-animations.js

+      // Property -> keyframes form.
+      const object = /** {!Object<string, *>} */ (spec.keyframes);
+      keyframes = {};
+      for (const prop in object) {


This, and below, will allow any animatable property to be animated. Is validation that only opacity and transform animate happening somewhere else?

Thanks for reminding! We'll add whitelist right away.

aghassemi · 2016-11-10T16:55:17Z

extensions/amp-animation/0.1/web-animations.js

+
+  /** @param {!WebMultiAnimationDef} unusedSpec */
+  onMultiAnimation(unusedSpec) {
+    dev().assert(null, 'not implemented');


Can there ever be an implementation here? This whole class looks like an @abstract to me.

It is. But I thought it was out pattern for @abstract, since dev.assert are removed from the binary. Is it not? Could you provide the right way?

I was thinking to just annotate the class with @abstract to make that clear.

Done. Added to the class and methods as well.

aghassemi · 2016-11-10T17:23:35Z

extensions/amp-animation/0.1/web-animations.js

+        keyframes.push(clone(frame));
+      }
+    } else {
+      // No a known form of keyframes spec.


remove 'a'

Took a bit of reading to get what onKeyframeAnimation does, but it is fairly clear what it does after. Some summary comments on onKeyframeAnimation may help future readers.

Good idea. Will add.

aghassemi · 2016-11-10T17:37:17Z

extensions/amp-animation/0.1/web-animations.js

+   */
+  measure_(target, prop) {
+    const index = this.targets_.indexOf(target);
+    if (!this.computedStyleCache_[index]) {


I believe we need to invalidate this cache after animation run finishes otherwise a second animation run on specs that have fill-mode forward or both ( or if style changes with a user interaction between the first and second run ) could yield the wrong value

This is meant to be fire-and-forget class for this reason. I think it should be natural for a scanner - it scans only once.

aghassemi · 2016-11-10T17:40:18Z

extensions/amp-animation/0.1/web-animations.js

+ * @template T
+ */
+function clone(x) {
+  if (!x) {


JSON.parse(JSON.stringify(obj)); is simpler (and I would guess likely faster)

Do you mean that would work better than clone?

I mean as an implementation of clone().

function clone(x) { return JSON.parse(JSON.stringify(x)); }

I agree. It'll also throw an error on cyclic dependencies, instead of the infinite loop we have now. 😀

New code not pushed yet?

Just pushed.

aghassemi · 2016-11-10T17:40:51Z

extensions/amp-animation/0.1/web-animations.js

+ * @return T
+ * @template T
+ */
+function clone(x) {


move to /utils/clone.js ?

I had it there :) Everyone hated it and it was eventually removed. In this case, however, we definitely need it... /cc @jridgewell

Ok. Reimplemented as JSON.parse(stringify)

jridgewell · 2016-11-10T22:04:31Z

extensions/amp-animation/0.1/web-animations.js

+    /** @type {!WebKeyframesDef} */
+    let keyframes;
+
+    if (isObject(spec.keyframes)) {


Holy parsing batman.

jridgewell · 2016-11-10T22:07:13Z

extensions/amp-animation/0.1/web-animations.js

+   * @private
+   */
+  mergeTiming_(newTiming, prevTiming) {
+    const duration = newTiming.duration != null ?


_.defaults?

It does a bit of a parsing here, in terms of Number() and such.

aghassemi · 2016-11-11T19:37:16Z

extensions/amp-animation/0.1/web-animations.js

@@ -306,15 +335,15 @@ export class MeasureScanner extends Scanner {

    // Validate.
    if (this.validate_) {
-      user().assert(!isNaN(duration),
+      user().assert(duration >= 0,


you may want to call isFiniteNumber() (from types.js) for all of these numbers first ( since for instance '' >= 0 is true

I'm calling Number(duration) above, so it can't be a '' anymore.

* Animation scanner and runner. * presubmit fixes * tests * more tests * types fixed * docs * minor docs fixes

Animation scanner and runner.

aa33625

dvoytenko added the NEEDS REVIEW label Nov 8, 2016

dvoytenko assigned cramforce Nov 8, 2016

cramforce assigned aghassemi and chenshay Nov 8, 2016

presubmit fixes

5159f07

aghassemi reviewed Nov 10, 2016

View reviewed changes

emarchiori reviewed Nov 10, 2016

View reviewed changes

aghassemi reviewed Nov 10, 2016

View reviewed changes

jridgewell reviewed Nov 10, 2016

View reviewed changes

tests

b7baae2

aghassemi approved these changes Nov 11, 2016

View reviewed changes

Dima Voytenko added 4 commits November 11, 2016 17:19

more tests

2131dfb

types fixed

a57f211

docs

827c400

minor docs fixes

c1312f7

dvoytenko merged commit 9058780 into ampproject:master Nov 14, 2016

dvoytenko deleted the anim2 branch November 14, 2016 21:54

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Nov 15, 2016

Animation builder and runner. (ampproject#6092)

6439cb7

* Animation scanner and runner. * presubmit fixes * tests * more tests * types fixed * docs * minor docs fixes

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Animation builder and runner. (ampproject#6092)

1866ec1

* Animation scanner and runner. * presubmit fixes * tests * more tests * types fixed * docs * minor docs fixes

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Animation builder and runner. (ampproject#6092)

3a5ce7d

* Animation scanner and runner. * presubmit fixes * tests * more tests * types fixed * docs * minor docs fixes



		/**
		* @typedef {!WebMultiAnimationDef\|!WebKeyframeAnimationDef}



		/**
		* @typedef {!Object<string, >\|!Array<!Object<string, >>}

Animation builder and runner. #6092

Animation builder and runner. #6092

Conversation

dvoytenko commented Nov 8, 2016

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emarchiori Nov 10, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emarchiori Nov 10, 2016 •

edited