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
Conversation
@@ -33,6 +33,9 @@ tags: { # <amp-bodymovin-animation> | |||
name: "loop" | |||
value_regex_casei: "(false|number|true)" | |||
} | |||
attrs: { | |||
name: "noautoplay" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validation changes look good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aghassemi could you TAL?
3p/bodymovinanimation.js
Outdated
}); | ||
}); | ||
} | ||
|
||
window.addEventListener('message', parseMessage, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should move inside bodymovinanimation
and use global
instead of window
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
import {getIframe, preloadBootstrap} from '../../../src/3p-frame'; | ||
import {isLayoutSizeDefined} from '../../../src/layout'; | ||
import {removeElement} from '../../../src/dom'; | ||
import {user} from '../../../src/log'; | ||
|
||
const TAG = 'amp-bodymovin-animation'; | ||
|
||
/** @enum {number} */ | ||
export const PLAYING_STATE = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const message = JSON.stringify(dict({ | ||
'action': 'play', | ||
})); | ||
this.iframe_.contentWindow./*OK*/postMessage(message, '*'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
seekTo_(timeVal) { | ||
const message = JSON.stringify(dict({ | ||
'action': 'goToAndStop', | ||
'value': timeVal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
3p/bodymovinanimation.js
Outdated
function parseMessage(event) { | ||
const eventMessage = parseJson(getData(event)); | ||
const action = eventMessage['action']; | ||
if (animationHandler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a comment below that if done, means animationHandler can never be null when you get messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
seekTo_(timeVal) { | ||
const message = JSON.stringify(dict({ | ||
'action': 'goToAndStop', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also the pause()
call was redundant since I was accidentally calling lottie.goToAndPlay
instead of lottie.goToAndPause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aghassemi could you TAL?
3p/bodymovinanimation.js
Outdated
function parseMessage(event) { | ||
const eventMessage = parseJson(getData(event)); | ||
const action = eventMessage['action']; | ||
if (animationHandler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
3p/bodymovinanimation.js
Outdated
}); | ||
}); | ||
} | ||
|
||
window.addEventListener('message', parseMessage, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -33,6 +33,9 @@ tags: { # <amp-bodymovin-animation> | |||
name: "loop" | |||
value_regex_casei: "(false|number|true)" | |||
} | |||
attrs: { | |||
name: "noautoplay" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
import {getIframe, preloadBootstrap} from '../../../src/3p-frame'; | ||
import {isLayoutSizeDefined} from '../../../src/layout'; | ||
import {removeElement} from '../../../src/dom'; | ||
import {user} from '../../../src/log'; | ||
|
||
const TAG = 'amp-bodymovin-animation'; | ||
|
||
/** @enum {number} */ | ||
export const PLAYING_STATE = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
seekTo_(timeVal) { | ||
const message = JSON.stringify(dict({ | ||
'action': 'goToAndStop', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also the pause()
call was redundant since I was accidentally calling lottie.goToAndPlay
instead of lottie.goToAndPause
const message = JSON.stringify(dict({ | ||
'action': 'play', | ||
})); | ||
this.iframe_.contentWindow./*OK*/postMessage(message, '*'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
seekTo_(timeVal) { | ||
const message = JSON.stringify(dict({ | ||
'action': 'goToAndStop', | ||
'value': timeVal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bodymovin.loadAnimation | ||
var bodymovin; | ||
bodymovin.loadAnimation; | ||
var animationHandler; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
spec/amp-actions-and-events.md
Outdated
<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> |
There was a problem hiding this comment.
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])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}); | ||
} | ||
|
||
play_() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* @private | ||
* */ | ||
sendCommand_(action, opt_valueType, opt_value) { | ||
this.playerReadyPromise_.then(function(iframe) { |
There was a problem hiding this comment.
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_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: jsdoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return true; | ||
} | ||
|
||
handleBodymovinMessages_(event) { | ||
if (event.source != this.iframe_.contentWindow) { |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -93,8 +134,78 @@ export class AmpBodymovinAnimation extends AMP.BaseElement { | |||
removeElement(this.iframe_); | |||
this.iframe_ = null; | |||
} | |||
this.playerReadyPromise_ = new Promise(resolve => { |
There was a problem hiding this comment.
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_();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} else if (action == 'stop') { | ||
animationHandler.stop(); | ||
} else if (action == 'seekTo') { | ||
animationHandler.goToAndStop(eventMessage['value'], |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3p/bodymovinanimation.js
Outdated
}); | ||
global.addEventListener('message', parseMessage, false); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aghassemi could you TAL?
} else if (action == 'stop') { | ||
animationHandler.stop(); | ||
} else if (action == 'seekTo') { | ||
animationHandler.goToAndStop(eventMessage['value'], |
There was a problem hiding this comment.
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
3p/bodymovinanimation.js
Outdated
}); | ||
global.addEventListener('message', parseMessage, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -93,8 +134,78 @@ export class AmpBodymovinAnimation extends AMP.BaseElement { | |||
removeElement(this.iframe_); | |||
this.iframe_ = null; | |||
} | |||
this.playerReadyPromise_ = new Promise(resolve => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return true; | ||
} | ||
|
||
handleBodymovinMessages_(event) { | ||
if (event.source != this.iframe_.contentWindow) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* @private | ||
* */ | ||
sendCommand_(action, opt_valueType, opt_value) { | ||
this.playerReadyPromise_.then(function(iframe) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
}); | ||
} | ||
|
||
play_() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
spec/amp-actions-and-events.md
Outdated
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bodymovin.loadAnimation | ||
var bodymovin; | ||
bodymovin.loadAnimation; | ||
var animationHandler; |
There was a problem hiding this comment.
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.
this.element.appendChild(iframe); | ||
this.iframe_ = iframe; | ||
}).then(() => { | ||
return this.loadPromise(this.iframe_); | ||
const loaded = this.loadPromise(this.iframe_); | ||
this.playerReadyResolver_(loaded); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@aghassemi Made the change you asked for - also removed the temp buttons in |
d6b37d2
to
d9eaa24
Compare
d9eaa24
to
e570df0
Compare
@aghassemi could you merge this in? |
These validator changes will be live everywhere within an hour. |
Future work: (low priority can potentially file as GFIs)