amp-position-observer and redesign of scrollbound/visibility-based behaviour #10818
Conversation
aghassemi
changed the title
|
/to @dvoytenko This is ready for review. (thanks!). It should be a fairly complete PR with docs, integration and functional tests. Remaining items:
|
| # limitations under the license. | ||
| # | ||
|
|
||
| tags: { # amp-position-observer script |
There was a problem hiding this comment.
Two spaces between { and # and omit word script.
There was a problem hiding this comment.
all done, if nothing is specified, defaults kick in (0 and 0 for both margin and ratios)
| attrs: { | ||
| name: "intersection-ratios" | ||
| # "0", "0.1" , "1", "0 1", "0 0.5", "0.5 1", "1 1", etc... | ||
| value_regex: "^([0]*?\.\d*$|1$|0$)|([0]*?\.\d*|1|0)\s{1}([0]*?\.\d*$|1$|0$)" |
There was a problem hiding this comment.
Note that value_regex needs \ to be escaped to work properly. For instance \d should be \\d. To handle this, we usually include the unescaped version above in a comment. For example see amp-timeago.
| value_regex: "^([0]*?\.\d*$|1$|0$)|([0]*?\.\d*|1|0)\s{1}([0]*?\.\d*$|1$|0$)" | ||
| } | ||
| attrs: { | ||
| name: "exclusion-margins" |
There was a problem hiding this comment.
Please alphabetize the attributes. E.g. this would go above intersection-ratios.
| attrs: { | ||
| name: "exclusion-margins" | ||
| # "100", "100px", "100vh", "100 100px", "100vh 100", "100px 100vh", etc.. | ||
| value_regex: "^(\d+$|\d+px$|\d+vh$)|((\d+|\d+px|\d+vh)\s{1}(\d+$|\d+px$|\d+vh$))" |
There was a problem hiding this comment.
Same as above, needs escaping and comment with unescaped.
| <amp-position-observer exclusion-margins="100vh 200px" layout="nodisplay"></amp-position-observer> | ||
| <amp-position-observer exclusion-margins="100 200vh" layout="nodisplay"></amp-position-observer> | ||
|
|
||
| <!-- Invalid --> |
There was a problem hiding this comment.
We usually group invalid ones with comments stating why they're invalid.
E.g. <!-- Invalid: missing required layout value -->, <!-- Invalid: interesection-ratios not valid entries -->, etc.
| } | ||
| this.maybeCreateRunner_().then(() => { | ||
| if (this.visible_ && this.triggered_) { | ||
| this.runner_.resume(); |
There was a problem hiding this comment.
Reads strange. maybeCreateRunner_ reads as "runner may end up created and maybe not", but inside the runner_ is used w/o check. Is this because promise won't be resolved or will be rejected if runner is not created?
| } | ||
| // percent based seek | ||
| const percent = parseFloat(invocation.args && invocation.args['percent']); | ||
| if (isFiniteNumber(percent) && percent <= 1 && percent >= 0) { |
There was a problem hiding this comment.
A better way, i think, would be to just do minmax[percent, 0, 1]
There was a problem hiding this comment.
added clamp to math.js
| @@ -262,7 +289,7 @@ export class AmpAnimation extends AMP.BaseElement { | |||
| setVisible_(visible) { | |||
| if (this.visible_ != visible) { | |||
| this.visible_ = visible; | |||
| if (this.triggered_) { | |||
| if (this.triggered_ && this.triggerOnVisibility_) { | |||
There was a problem hiding this comment.
Hmm. Why would it be triggered then? Would it be better to reset triggered_ to false instead?
| * @private | ||
| */ | ||
| maybeCreateRunner_(opt_args) { | ||
| if (!this.runnerPromise_ || !this.runner_) { |
There was a problem hiding this comment.
Not sure how to read this: why would one exist but the other would not? Further, if runner_ exists but promise doesn't, why do we recreate the runner? Shouldn't the previous runner be destroyed/stopped/etc? It will continue playing otherwise. This is from reading the code, so maybe just some asserts missing?
There was a problem hiding this comment.
All done as discussed. Re-did the actions integrations tests so they mock less
| * @return {!Promise} | ||
| * @private | ||
| */ | ||
| maybeCreateRunner_(opt_args) { |
There was a problem hiding this comment.
What are potential consequences of this? Is it strictly necessary? Is it to be able to seek right away? Maybe it's better to animate() right before the first seek? Feels like a biggish change from before, but maybe it's not.
| } | ||
| } | ||
|
|
||
| this.targetSelector_ = this.element.getAttribute('target-selector'); |
There was a problem hiding this comment.
FWIW, I think the target="id" form is better for this use case. Selector kind of implies behavior for multiple targets. This also clarifies typical confusion around the direction of DOM search, e.g. it's not a closest thing, etc. Asking folk to add an ID for this use case is very reasonable. Especially since it's optional anyway.
| the target is entering/exiting from top (0 will be used) or bottom (1 will be used). | ||
|
|
||
|
|
||
| #### exclusion-margins (optional) |
There was a problem hiding this comment.
A bit bikeshed on name. Would it be better to rename to root margins or viewport margins to get it closer to InOb terminology which we use elsewhere?
There was a problem hiding this comment.
viewport-margins it is
| const totalDuration = adjustedViewportRect.height + | ||
| positionRect.height - totalDurationOffset; | ||
|
|
||
| const topOffset = Math.abs( |
There was a problem hiding this comment.
Why isn't the topRatio used in this formula?
There was a problem hiding this comment.
The formula aligns everything to create an equivalent case where top ratio is 1.
| * @private | ||
| */ | ||
| discoverScene_() { | ||
| if (this.targetSelector_) { |
There was a problem hiding this comment.
See above about target=id. But if selector is used and specified by the doc - it could be a bad ID and needs to be defined as such via try{}catch(){user.error("bad selector")}.
| discoverScene_() { | ||
| if (this.targetSelector_) { | ||
| this.scene_ = user().assertElement( | ||
| this.getAmpDoc().getRootNode().querySelector(this.targetSelector_), |
There was a problem hiding this comment.
Follow up question: is this code guaranteed to run after the DOM has been fully parsed? Typically we wrap query selectors into ampdoc.whenReady().then(...)
|
@dvoytenko all done, PTAL |
| * @param {number} max1 the upper bound of the source range | ||
| * @param {number} min2 the lower bound of the target range | ||
| * @param {number} max2 the upper bound of the target range | ||
| * @param {!number} val the value in the source range |
There was a problem hiding this comment.
nit: numbers and other primitives are non-nullable by default, so no need for !
Closes #8411 #9301 #10880
New Approach to scrollbound and visibility-based behaviour
amp-animation<>amp-position-observer(scrollbound animations and visibility-based animation scenes)amp-animation<>amp-video-player-x(Not implemented in this PR, syncing animation and videos)amp-video-player-x<>amp-position-observer(Not implemented in this PR, scrollbound behaviour for videos)enterexitprogressevents (e.g. range input, carousel, XHR call, etc..)amp-position-observer
enter,exit,scroll(percent)LOW-TRUST events. (i.e. has "timeline")ratiosandmargins(similar to IntersectionObserver)Examples
Play my animation scene when 50% visible and pause when 50% invisible
Play my animation using scrolling instead of time.
Animation starts when scene is fully visible and pauses as soon as partially invisible.
Play a video when a heading is scrolled into view.
Imagine a position fixed video that would play automatically when user scrolls to a related heading somewhere in the page.
Sync my animation timeline with this video (Not implemented in this PR)
Animation starts when video starts, pauses when video is paused, seeking the video also seeks the animation, animation ends when video ends.
Play my video using scrolling instead of time (Not implemented in this PR)
Similar to scrollbound animation, but with a video instead