<amp-bodymovin-actions> Add actions play/pause/stop/seekTo and noautoplay attribute by nainar · Pull Request #14040 · ampproject/amphtml

nainar · 2018-03-15T23:54:01Z

Future work: (low priority can potentially file as GFIs)

Add animationData attribute (most useful)
Add speed attribute
Add direction attribute

…ovin

honeybadgerdontcare · 2018-03-16T00:12:14Z

extensions/amp-bodymovin-animation/validator-amp-bodymovin-animation.protoascii

    value_regex_casei: "(false|number|true)"
  }
+  attrs: {
+    name: "noautoplay"


may want to have value: "" as that would prevent it from just being any value. it'd have to be empty or itself (noautoplay=noautoplay).

honeybadgerdontcare

validation changes look good

nainar

@aghassemi could you TAL?

aghassemi · 2018-03-16T17:40:15Z

3p/bodymovinanimation.js

  });
 }
+
+window.addEventListener('message', parseMessage, false);


this should move inside bodymovinanimation and use global instead of window

aghassemi · 2018-03-16T22:19:20Z

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

 const TAG = 'amp-bodymovin-animation';

+/** @enum {number} */
+export const PLAYING_STATE = {


I was hoping calling an action in an invalid state is no-op in lottie player. Can you give it a try? If that's the cause, I recommend not keeping an internal playing state at all. Issues with internal state is they need to get synced with the player which may be unnecessary work and no trivial. For example currently we are already out of sync in autoplay case and also if loop is not infinite and player paused after x loops we stay in playing state.

Ahhh good catch - found one example of the lottie code here: https://github.com/airbnb/lottie-web/blob/master/build/player/lottie_light.js#L8938

Other functions also perform no-op if the status hasn't changed. Removing related code.

aghassemi · 2018-03-16T22:22:26Z

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

+    const message = JSON.stringify(dict({
+      'action': 'play',
+    }));
+    this.iframe_.contentWindow./*OK*/postMessage(message, '*');


this.iframe_ may not not exist when this is called. What we want to do is queue these requests until player is loaded and then send them down. So I recommend
1- creating a sendCommand_ method and using that here.
2- inside sendCommand_, chain to a playerReadyPromise, this requrest the 3p iframe to send a message back up when the SDK has loaded which would resolve this promise.

amp-youtube is a good example for this.

aghassemi · 2018-03-16T22:24:42Z

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

+  seekTo_(timeVal) {
+    const message = JSON.stringify(dict({
+      'action': 'goToAndStop',
+      'value': timeVal,


can we support percent as well? it is more useful than time ( allows scrollbound animations ). Hopefully Lottie player can give the final duration. (if loop=true, we should assume 1 loop)

aghassemi · 2018-03-16T22:27:16Z

3p/bodymovinanimation.js

+function parseMessage(event) {
+  const eventMessage = parseJson(getData(event));
+  const action = eventMessage['action'];
+  if (animationHandler) {


There is a comment below that if done, means animationHandler can never be null when you get messages.

aghassemi · 2018-03-16T22:29:31Z

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

+
+  seekTo_(timeVal) {
+    const message = JSON.stringify(dict({
+      'action': 'goToAndStop',


goToAndPlay makes more sense since pause is called separately here. I actually suggest calling it seekTo but removing pause from here and doing the pause in the 3P code instead. (idea being knowledge of Lottie-specific names should stays in the 3P implementation and not the player-generic component)

Done. Also the pause() call was redundant since I was accidentally calling lottie.goToAndPlay instead of lottie.goToAndPause

nainar

@aghassemi could you TAL?

nainar · 2018-03-19T15:44:45Z

3p/bodymovinanimation.js

+function parseMessage(event) {
+  const eventMessage = parseJson(getData(event));
+  const action = eventMessage['action'];
+  if (animationHandler) {


nainar · 2018-03-19T15:45:07Z

3p/bodymovinanimation.js

  });
 }
+
+window.addEventListener('message', parseMessage, false);


nainar · 2018-03-19T15:45:21Z

extensions/amp-bodymovin-animation/validator-amp-bodymovin-animation.protoascii

    value_regex_casei: "(false|number|true)"
  }
+  attrs: {
+    name: "noautoplay"


nainar · 2018-03-19T15:59:50Z

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

 const TAG = 'amp-bodymovin-animation';

+/** @enum {number} */
+export const PLAYING_STATE = {


Ahhh good catch - found one example of the lottie code here: https://github.com/airbnb/lottie-web/blob/master/build/player/lottie_light.js#L8938

Other functions also perform no-op if the status hasn't changed. Removing related code.

nainar · 2018-03-19T16:03:16Z

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

+
+  seekTo_(timeVal) {
+    const message = JSON.stringify(dict({
+      'action': 'goToAndStop',


Done. Also the pause() call was redundant since I was accidentally calling lottie.goToAndPlay instead of lottie.goToAndPause

nainar · 2018-03-19T17:44:47Z

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

+    const message = JSON.stringify(dict({
+      'action': 'play',
+    }));
+    this.iframe_.contentWindow./*OK*/postMessage(message, '*');


nainar · 2018-03-19T17:46:14Z

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

+  seekTo_(timeVal) {
+    const message = JSON.stringify(dict({
+      'action': 'goToAndStop',
+      'value': timeVal,


aghassemi · 2018-03-20T00:01:01Z

build-system/amp.extern.js

-bodymovin.loadAnimation
+var bodymovin;
+bodymovin.loadAnimation;
+var animationHandler;


looks like closure compiler doesn't like animationHadler definition here. It may need to become * @typedef {{...` above the declaration in the 3p script.

This comment isn't relevant to this patch. The amp.extern.js is fine in this run. 99 problems but amp.extern.js isn't one.

aghassemi · 2018-03-20T00:04:15Z

spec/amp-actions-and-events.md

+    <td>Stops the animation.</td>
+    <td><code>seekTo(time=INTEGER)</code></td>
+    <td>Sets the currentTime of the animation to the specified value and pauses animation. </td>
+  </tr>


add seekTo(percent=[0,1])

aghassemi · 2018-03-20T00:07:16Z

extensions/amp-bodymovin-animation/amp-bodymovin-animation.md


 Whether the animation should be looping or not. `true` by default. Values can be: `true`|`false`|`number`

+##### noautoplay (optional)


let's put a link to the actions .md file from here

## Actions Please see [AMP Action Documentation](link-to-the-heading) for the actions available on `amp-bodymovin-animation` component.

aghassemi · 2018-03-20T00:07:41Z

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

+    });
+  }
+
+  play_() {


nit: /** @provate */ js doc for everything here.

aghassemi · 2018-03-20T00:12:01Z

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

+   * @private
+   * */
+  sendCommand_(action, opt_valueType, opt_value) {
+    this.playerReadyPromise_.then(function(iframe) {


with few small exceptions in tests, we always use => instead of function

also no need to get the iframe out of promise and set it again as this_iframe just use this.iframe_

aghassemi · 2018-03-20T00:12:43Z

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

+    });
    return true;
  }
+


nit: jsdoc.

aghassemi · 2018-03-20T00:13:23Z

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

  }
+
+  handleBodymovinMessages_(event) {
+    if (event.source != this.iframe_.contentWindow) {


check for this.iframe_ ( as it can become null during unlayout callback) so if (this.iframe_ && event...)

aghassemi · 2018-03-20T00:14:42Z

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

      removeElement(this.iframe_);
      this.iframe_ = null;
    }
+    this.playerReadyPromise_ = new Promise(resolve => {


also needs a

if (this.unlistenMessage_) { this.unlistenMessage_(); }

aghassemi · 2018-03-20T00:21:23Z

3p/bodymovinanimation.js

+  } else if (action == 'stop') {
+    animationHandler.stop();
+  } else if (action == 'seekTo') {
+    animationHandler.goToAndStop(eventMessage['value'],


nice that Lottie already accepts percent as value!

I KNOW - I just need to read better next time. :o

@aghassemi / @nainar, where do you see that it accepts a percentage as value? I put together this JSFiddle to test this idea but it doesn't seem to work with values between 0 and 1. Thus, when I try to use amp-bodymovin-animation with amp-position-observer, it doesn't work. Any ideas?

Hi @jessepinho according to the API you also have to pass in an isFrame attribute set to true I believe for it to read it as a percentage value. Otherwise it uses a timebased value.

@nainar When isFrame is true, the first parameter is the actual frame number. So, to get it to be a percentage, you'd have to multiply that number by the total number of frames.

I can make a pull request to fix this if you're open to it!

@nainar See #15321

aghassemi · 2018-03-20T00:23:20Z

3p/bodymovinanimation.js

+    }));
+    global.parent. /*OK*/postMessage(message, '*');
  });
+  global.addEventListener('message', parseMessage, false);


I'd move this above global.parent. /*OK*/postMessage to indicate you only start listening after ready message is sent. This would guarantee parseMessage is not called when animationHandler is null.

nainar

@aghassemi could you TAL?

nainar · 2018-03-20T00:24:38Z

3p/bodymovinanimation.js

+  } else if (action == 'stop') {
+    animationHandler.stop();
+  } else if (action == 'seekTo') {
+    animationHandler.goToAndStop(eventMessage['value'],


I KNOW - I just need to read better next time. :o

nainar · 2018-03-20T00:25:13Z

3p/bodymovinanimation.js

+    }));
+    global.parent. /*OK*/postMessage(message, '*');
  });
+  global.addEventListener('message', parseMessage, false);


nainar · 2018-03-20T00:27:34Z

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

      removeElement(this.iframe_);
      this.iframe_ = null;
    }
+    this.playerReadyPromise_ = new Promise(resolve => {


nainar · 2018-03-20T00:28:07Z

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

+    });
    return true;
  }
+


nainar · 2018-03-20T00:28:32Z

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

  }
+
+  handleBodymovinMessages_(event) {
+    if (event.source != this.iframe_.contentWindow) {


nainar · 2018-03-20T00:29:26Z

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

+   * @private
+   * */
+  sendCommand_(action, opt_valueType, opt_value) {
+    this.playerReadyPromise_.then(function(iframe) {


nainar · 2018-03-20T00:30:19Z

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

+    });
+  }
+
+  play_() {


nainar · 2018-03-20T00:32:31Z

extensions/amp-bodymovin-animation/amp-bodymovin-animation.md


 Whether the animation should be looping or not. `true` by default. Values can be: `true`|`false`|`number`

+##### noautoplay (optional)


nainar · 2018-03-20T00:34:06Z

spec/amp-actions-and-events.md

+    <td>Stops the animation.</td>
+    <td><code>seekTo(time=INTEGER)</code></td>
+    <td>Sets the currentTime of the animation to the specified value and pauses animation. </td>
+  </tr>


nainar · 2018-03-20T01:08:40Z

build-system/amp.extern.js

-bodymovin.loadAnimation
+var bodymovin;
+bodymovin.loadAnimation;
+var animationHandler;


This comment isn't relevant to this patch. The amp.extern.js is fine in this run. 99 problems but amp.extern.js isn't one.

aghassemi · 2018-03-20T01:15:49Z

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

      }).then(() => {
-        return this.loadPromise(this.iframe_);
+        const loaded = this.loadPromise(this.iframe_);
+        this.playerReadyResolver_(loaded);


we shouldn't call playerReadyResolver_ here. actually we don't even need to listen to loadPromise anymore. we can just return this. playerReadyPromise_ (since playerReadyPromise_ won't be resolved until iframe has loaded and lottie SDK has loaded as well)

nainar · 2018-03-20T01:26:33Z

@aghassemi Made the change you asked for - also removed the temp buttons in bodymovin-animation.amp.html

nainar · 2018-03-20T16:04:40Z

@aghassemi could you merge this in?

* Allow meta http-equiv=x-dns-prefetch-control * Introduce excludes tagspec feature * Revision bump for #13782 * Revision bump for #14040 * Revision bump for #14157,#14127

Gregable · 2018-03-22T17:36:20Z

These validator changes will be live everywhere within an hour.

nainar added 30 commits March 1, 2018 14:26

WIP - Initial implementation

c4d5c82

First draft of code

2377934

remove unused test

7a46387

WIP

730541d

Fix aghassemi@ suggestions

0c1b1f4

remove WIP tests

80300b9

Merge branch 'master' of github.com:ampproject/amphtml into amp_bodym…

429add4

…ovin

Rename

b38b2a0

Stop using data prefixed variables

9565eb5

testing

38b2153

Add tentative tests

3c5de97

Add no-autoplay

4e2fe75

Add play/pause/stop actions

ba88af7

@aghassemi suggestions

e23b8ae

Test works

4870c85

Rebase

52c3c5f

Post rebase lint fixes

e5d57ef

Merge parent in

c6b528a

Fix all the compile issues

7c413b9

Validator fixes

a5b2e1c

Validator changes suggested by honeybadgerdontcare@

e610b6c

Merge branch 'amp_bodymovin' into amp_bodymovin_actions

e54f90f

Add seekTo

85c132b

more validator changes

4c33fc5

TIL: no commas in validator protoascii

d2f1ec8

Merge branch 'amp_bodymovin' into amp_bodymovin_actions

ad9f99e

Please BUILD

123585f

Merge branch 'amp_bodymovin' into amp_bodymovin_actions

67110ee

Merge branch 'master' of github.com:ampproject/amphtml into amp_bodym…

4a2cb58

…ovin

Merge branch 'amp_bodymovin' into amp_bodymovin_actions

4749181

gulp pr-check fixes

18461dc

nainar requested a review from aghassemi March 15, 2018 23:54

googlebot added the cla: yes label Mar 15, 2018

honeybadgerdontcare reviewed Mar 16, 2018

View reviewed changes

Validator and check-types clean up

8b2bbdc

honeybadgerdontcare approved these changes Mar 16, 2018

View reviewed changes

nainar added 2 commits March 16, 2018 12:02

Allow bodymovinanimation.js to use event-helper.js

6d4ab99

Add description for seekTo

2fd6e28

nainar commented Mar 16, 2018

View reviewed changes

aghassemi reviewed Mar 16, 2018

View reviewed changes

Rewrite as per aghassemi@ suggestions

4ac7cd6

nainar commented Mar 19, 2018

View reviewed changes

nainar added 2 commits March 19, 2018 16:00

Make sure only valid JSON objects are accepted

08c34ae

gulp lint fixes

efd7623

aghassemi reviewed Mar 20, 2018

View reviewed changes

aghassemi@ suggestions

cc64ffe

nainar commented Mar 20, 2018

View reviewed changes

aghassemi reviewed Mar 20, 2018

View reviewed changes

Remove redundant code and remove buttons

ffddbda

nainar force-pushed the amp_bodymovin_actions branch from d6b37d2 to d9eaa24 Compare March 20, 2018 01:53

this.playerReadyPromise_ >>> this.loadPromise(this.iframe_)

e570df0

nainar force-pushed the amp_bodymovin_actions branch from d9eaa24 to e570df0 Compare March 20, 2018 01:55

aghassemi approved these changes Mar 20, 2018

View reviewed changes

aghassemi merged commit b28c57e into ampproject:master Mar 20, 2018

Gregable pushed a commit that referenced this pull request Mar 22, 2018

Revision bump for #14040

48148dd

Gregable added a commit that referenced this pull request Mar 22, 2018

Validator Roll Up (#14202)

a0e7a22

* Allow meta http-equiv=x-dns-prefetch-control * Introduce excludes tagspec feature * Revision bump for #13782 * Revision bump for #14040 * Revision bump for #14157,#14127

aghassemi mentioned this pull request Apr 18, 2018

Status Update 2018-04-02 #14329

Closed


		Whether the animation should be looping or not. `true` by default. Values can be: `true`\|`false`\|`number`

		##### noautoplay (optional)

Conversation

nainar commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

Uh oh!

nainar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nainar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessepinho May 15, 2018 •

edited

Loading