Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<amp-bodymovin-actions> Add actions play/pause/stop/seekTo and noautoplay attribute #14040

Merged
merged 40 commits into from Mar 20, 2018
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
c4d5c82
WIP - Initial implementation
nainar Mar 1, 2018
2377934
First draft of code
nainar Mar 5, 2018
7a46387
remove unused test
nainar Mar 5, 2018
730541d
WIP
nainar Mar 7, 2018
0c1b1f4
Fix aghassemi@ suggestions
nainar Mar 7, 2018
80300b9
remove WIP tests
nainar Mar 7, 2018
429add4
Merge branch 'master' of github.com:ampproject/amphtml into amp_bodym…
nainar Mar 7, 2018
b38b2a0
Rename
nainar Mar 7, 2018
9565eb5
Stop using data prefixed variables
nainar Mar 8, 2018
38b2153
testing
nainar Mar 8, 2018
3c5de97
Add tentative tests
nainar Mar 8, 2018
4e2fe75
Add no-autoplay
nainar Mar 13, 2018
ba88af7
Add play/pause/stop actions
nainar Mar 13, 2018
e23b8ae
@aghassemi suggestions
nainar Mar 14, 2018
4870c85
Test works
nainar Mar 14, 2018
52c3c5f
Rebase
nainar Mar 14, 2018
e5d57ef
Post rebase lint fixes
nainar Mar 15, 2018
c6b528a
Merge parent in
nainar Mar 15, 2018
7c413b9
Fix all the compile issues
nainar Mar 15, 2018
a5b2e1c
Validator fixes
nainar Mar 15, 2018
e610b6c
Validator changes suggested by honeybadgerdontcare@
nainar Mar 15, 2018
e54f90f
Merge branch 'amp_bodymovin' into amp_bodymovin_actions
nainar Mar 15, 2018
85c132b
Add seekTo
nainar Mar 15, 2018
4c33fc5
more validator changes
nainar Mar 15, 2018
d2f1ec8
TIL: no commas in validator protoascii
nainar Mar 15, 2018
ad9f99e
Merge branch 'amp_bodymovin' into amp_bodymovin_actions
nainar Mar 15, 2018
123585f
Please BUILD
nainar Mar 15, 2018
67110ee
Merge branch 'amp_bodymovin' into amp_bodymovin_actions
nainar Mar 15, 2018
4a2cb58
Merge branch 'master' of github.com:ampproject/amphtml into amp_bodym…
nainar Mar 15, 2018
4749181
Merge branch 'amp_bodymovin' into amp_bodymovin_actions
nainar Mar 15, 2018
18461dc
gulp pr-check fixes
nainar Mar 15, 2018
8b2bbdc
Validator and check-types clean up
nainar Mar 16, 2018
6d4ab99
Allow bodymovinanimation.js to use event-helper.js
nainar Mar 16, 2018
2fd6e28
Add description for seekTo
nainar Mar 16, 2018
4ac7cd6
Rewrite as per aghassemi@ suggestions
nainar Mar 19, 2018
08c34ae
Make sure only valid JSON objects are accepted
nainar Mar 19, 2018
efd7623
gulp lint fixes
nainar Mar 19, 2018
cc64ffe
aghassemi@ suggestions
nainar Mar 20, 2018
ffddbda
Remove redundant code and remove buttons
nainar Mar 20, 2018
e570df0
this.playerReadyPromise_ >>> this.loadPromise(this.iframe_)
nainar Mar 20, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 28 additions & 4 deletions 3p/bodymovinanimation.js
Expand Up @@ -14,6 +14,8 @@
* limitations under the License.
*/

import {dict} from '../src/utils/object';
import {getData} from '../src/event-helper';
import {loadScript} from './3p';
import {parseJson} from '../src/json';

Expand All @@ -23,28 +25,50 @@ import {parseJson} from '../src/json';
* @param {function(!Object)} cb
*/

let animationHandler;

function getBodymovinAnimationSdk(global, cb) {
loadScript(global, 'https://cdnjs.cloudflare.com/ajax/libs/bodymovin/4.13.0/bodymovin_light.min.js', function() {
cb(global.bodymovin);
});
}

function parseMessage(event) {
const eventMessage = parseJson(getData(event));
const action = eventMessage['action'];
if (action == 'play') {
animationHandler.play();
} else if (action == 'pause') {
animationHandler.pause();
} else if (action == 'stop') {
animationHandler.stop();
} else if (action == 'seekTo') {
animationHandler.goToAndStop(eventMessage['value'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice that Lottie already accepts percent as value!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eventMessage['valueType'] !== 'time');
}
}

export function bodymovinanimation(global) {
const dataReceived = parseJson(global.name)['attributes']._context;
const dataLoop = dataReceived['loop'];
const animationData = dataReceived['animationData'];
const animatingContainer = global.document.createElement('div');

global.document.getElementById('c').appendChild(animatingContainer);
const shouldLoop = dataLoop != 'false';
const loop = !isNaN(dataLoop) ? dataLoop : shouldLoop;
getBodymovinAnimationSdk(global, function(bodymovin) {
bodymovin.loadAnimation({
animationHandler = bodymovin.loadAnimation({
container: animatingContainer,
renderer: 'svg',
loop,
autoplay: true,
animationData,
autoplay: dataReceived['autoplay'],
animationData: dataReceived['animationData'],
});
const message = JSON.stringify(dict({
'action': 'ready',
}));
global.parent. /*OK*/postMessage(message, '*');
});
global.addEventListener('message', parseMessage, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

9 changes: 7 additions & 2 deletions build-system/amp.extern.js
Expand Up @@ -279,8 +279,13 @@ FB.init;
var gist;
gist.gistid;

var bodymovin
bodymovin.loadAnimation
var bodymovin;
bodymovin.loadAnimation;
var animationHandler;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

animationHandler.play;
animationHandler.pause;
animationHandler.stop;
animationHandler.goToAndStop;

// Validator
var amp;
Expand Down
1 change: 1 addition & 0 deletions build-system/dep-check-config.js
Expand Up @@ -110,6 +110,7 @@ exports.rules = [
'3p/polyfills.js->src/polyfills/math-sign.js',
'3p/polyfills.js->src/polyfills/object-assign.js',
'3p/messaging.js->src/event-helper.js',
'3p/bodymovinanimation.js->src/event-helper.js',
'3p/iframe-messaging-client.js->src/event-helper.js',
],
},
Expand Down
12 changes: 11 additions & 1 deletion examples/bodymovin-animation.amp.html
Expand Up @@ -6,7 +6,7 @@
<link rel="canonical" href="amps.html" >
<meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
<link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'>
<script async custom-element="amp-bodymovin-player" src="https://cdn.ampproject.org/v0/amp-bodymovin-animation-0.1.js"></script>
<script async custom-element="amp-bodymovin-animation" src="https://cdn.ampproject.org/v0/amp-bodymovin-animation-0.1.js"></script>
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<script async src="https://cdn.ampproject.org/v0.js"></script>
</head>
Expand All @@ -26,6 +26,16 @@ <h2>Example of an animation that does 2 iteration</h2>
<amp-bodymovin-animation layout="fixed" width="200" height="200" src="https://nainar.github.io/loader.json" loop="2">
</amp-bodymovin-animation>

<h2>Example of an animation that doesn't autoplay</h2>
<amp-bodymovin-animation id="anim" layout="fixed" width="200" height="200" src="https://nainar.github.io/loader.json" loop="true" name="ripple_1" noautoplay>
</amp-bodymovin-animation>
<!-- Testing only - will be removed prior to landing -->
<button on="tap:anim.play">PLAY</button>
<button on="tap:anim.pause">PAUSE</button>
<button on="tap:anim.stop">STOP</button>
<button on="tap:anim.seekTo(time=1000)">SEEK TIME</button>
<button on="tap:anim.seekTo(percent=0.2)">SEEK PERCENT</button>

<h2>Example of a responsive layout animation</h2>
<amp-bodymovin-animation layout="responsive" width="200" height="200" src="https://nainar.github.io/loader.json" loop="true">
</amp-bodymovin-animation>
Expand Down
115 changes: 113 additions & 2 deletions extensions/amp-bodymovin-animation/0.1/amp-bodymovin-animation.js
Expand Up @@ -14,12 +14,19 @@
* limitations under the License.
*/

import {ActionTrust} from '../../../src/action-trust';
import {Services} from '../../../src/services';
import {assertHttpsUrl} from '../../../src/url';
import {batchFetchJsonFor} from '../../../src/batched-json';
import {clamp} from '../../../src/utils/math';
import {dict} from '../../../src/utils/object';
import {getData, listen} from '../../../src/event-helper';
import {getIframe, preloadBootstrap} from '../../../src/3p-frame';
import {isFiniteNumber, isObject} from '../../../src/types';
import {isLayoutSizeDefined} from '../../../src/layout';
import {parseJson} from '../../../src/json';
import {removeElement} from '../../../src/dom';
import {startsWith} from '../../../src/string';
import {user} from '../../../src/log';

const TAG = 'amp-bodymovin-animation';
Expand All @@ -33,15 +40,26 @@ export class AmpBodymovinAnimation extends AMP.BaseElement {
/** @private @const */
this.ampdoc_ = Services.ampdoc(this.element);

/** @private {?HTMLIFrameElement} */
/** @private {?Element} */
this.iframe_ = null;

/** @private {?string} */
this.loop_ = null;

/** @private {?boolean} */
this.autoplay_ = null;

/** @private {?string} */
this.src_ = null;

/** @private {?Promise} */
this.playerReadyPromise_ = null;

/** @private {?Function} */
this.playerReadyResolver_ = null;

/** @private {?Function} */
this.unlistenMessage_ = null;
}

/** @override */
Expand All @@ -61,10 +79,25 @@ export class AmpBodymovinAnimation extends AMP.BaseElement {
/** @override */
buildCallback() {
this.loop_ = this.element.getAttribute('loop') || 'true';
this.autoplay_ = !this.element.hasAttribute('noautoplay');
user().assert(this.element.hasAttribute('src'),
'The src attribute must be specified for <amp-bodymovin-animation>');
assertHttpsUrl(this.element.getAttribute('src'), this.element);
this.src_ = this.element.getAttribute('src');
this.playerReadyPromise_ = new Promise(resolve => {
this.playerReadyResolver_ = resolve;
});

// Register relevant actions
this.registerAction('play', () => { this.play_(); }, ActionTrust.LOW);
this.registerAction('pause', () => { this.pause_(); }, ActionTrust.LOW);
this.registerAction('stop', () => { this.stop_(); }, ActionTrust.LOW);
this.registerAction('seekTo', invocation => {
const args = invocation.args;
if (args) {
this.seekTo_(args);
}
}, ActionTrust.LOW);
}

/** @override */
Expand All @@ -73,16 +106,24 @@ export class AmpBodymovinAnimation extends AMP.BaseElement {
return animData.then(data => {
const opt_context = {
loop: this.loop_,
autoplay: this.autoplay_,
animationData: data,
};
const iframe = getIframe(
this.win, this.element, 'bodymovinanimation', opt_context);
return Services.vsyncFor(this.win).mutatePromise(() => {
this.applyFillContent(iframe);
this.unlistenMessage_ = listen(
this.win,
'message',
this.handleBodymovinMessages_.bind(this)
);
this.element.appendChild(iframe);
this.iframe_ = iframe;
}).then(() => {
return this.loadPromise(this.iframe_);
const loaded = this.loadPromise(this.iframe_);
this.playerReadyResolver_(loaded);
Copy link
Contributor

@aghassemi aghassemi Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return loaded;
});
});
}
Expand All @@ -93,8 +134,78 @@ export class AmpBodymovinAnimation extends AMP.BaseElement {
removeElement(this.iframe_);
this.iframe_ = null;
}
this.playerReadyPromise_ = new Promise(resolve => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also needs a

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

this.playerReadyResolver_ = resolve;
});
return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: jsdoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

handleBodymovinMessages_(event) {
if (event.source != this.iframe_.contentWindow) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return;
}
if (!getData(event) || !(isObject(getData(event))
|| startsWith(/** @type {string} */ (getData(event)), '{'))) {
return; // Doesn't look like JSON.
}

/** @const {?JsonObject} */
const eventData = /** @type {?JsonObject} */ (isObject(getData(event))
? getData(event)
: parseJson(getData(event)));
if (eventData === undefined) {
return; // We only process valid JSON.
}
if (eventData['action'] == 'ready') {
this.playerReadyResolver_(this.iframe_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to resolve with this,iframe_, just resolve without any params.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
}

/**
* Sends a command to the player through postMessage.
* @param {string} action
* @param {string=} opt_valueType
* @param {number=} opt_value
* @private
* */
sendCommand_(action, opt_valueType, opt_value) {
this.playerReadyPromise_.then(function(iframe) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

this.iframe_ = iframe;
if (this.iframe_ && this.iframe_.contentWindow) {
const message = JSON.stringify(dict({
'action': action,
'valueType': opt_valueType || '',
'value': opt_value || '',
}));
this.iframe_.contentWindow. /*OK*/postMessage(message, '*');
}
});
}

play_() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

this.sendCommand_('play');
}

pause_() {
this.sendCommand_('pause');
}

stop_() {
this.sendCommand_('stop');
}

seekTo_(args) {
const time = parseFloat(args && args['time']);
// time based seek
if (isFiniteNumber(time)) {
this.sendCommand_('seekTo', 'time', time);
}
// percent based seek
const percent = parseFloat(args && args['percent']);
if (isFiniteNumber(percent)) {
this.sendCommand_('seekTo', 'percent', clamp(percent, 0, 1));
}
}
}

AMP.extension(TAG, '0.1', AMP => {
Expand Down
4 changes: 4 additions & 0 deletions extensions/amp-bodymovin-animation/amp-bodymovin-animation.md
Expand Up @@ -58,6 +58,10 @@ The path to the exported Bodymovin animation object

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

##### noautoplay (optional)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


By default, an animation autoplays. If this attribute is added the video waits for an action to start playing.

##### common attributes

This element includes [common attributes](https://www.ampproject.org/docs/reference/common_attributes) extended to AMP components.
Expand Down
Expand Up @@ -33,6 +33,10 @@ tags: { # <amp-bodymovin-animation>
name: "loop"
value_regex_casei: "(false|number|true)"
}
attrs: {
name: "noautoplay"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

value: ""
}
attrs: {
name: "src"
mandatory: true
Expand Down
18 changes: 18 additions & 0 deletions spec/amp-actions-and-events.md
Expand Up @@ -314,6 +314,24 @@ event.response</pre></td>
</tr>
</table>

### amp-bodymovin-animation
<table>
<tr>
<th>Action</th>
<th>Description</th>
</tr>
<tr>
<td><code>play</code></td>
<td>Plays the animation.</td>
<td><code>pause</code></td>
<td>Pauses the animation.</td>
<td><code>stop</code></td>
<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add seekTo(percent=[0,1])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

</table>

### amp-carousel[type="slides"]
<table>
<tr>
Expand Down