Implements scrollTo, focus and blur standard actions #10398

wassgha · 2017-07-13T02:34:20Z

This adds an action to dynamically focus and scroll to elements as detailed in #7626

Changes

Added position parameter to animateScrollIntoView in viewport-impl specifying the final position of the element in the viewport after the scroll animation is over
Implemented scrollTo action on all elements with configurable duration of animation and position inside the viewport
Implemented focus action on all elements
Implemented blur action on all elements
Added example that implements the changes
Made changes to standard action spec
Added unit tests

Closes #7626

wassgha · 2017-07-13T23:14:02Z

Ready for review @aghassemi

aghassemi · 2017-07-14T18:45:01Z

spec/amp-actions-and-events.md

    <td>Toggles the visibility of the target element.</td>
  </tr>
+  <tr>
+    <td>`scrollTo(duration=INTEGER, position=STRING)`</td>
+    <td>Scrolls the element into view with a smooth animation. If defined,


mentioned the defaults for these params.

/cc @bpaduch

aghassemi · 2017-07-14T18:47:38Z

spec/amp-actions-and-events.md

+    <td>Makes the target element gain focus.</td>
+  </tr>
+  <tr>
+    <td>`blur`</td>


blur is not good to expose because it breaks A11Y. It moves the focus back to body which is almost never a desired behaviour. If author wants something to lose focus, they need to decide what makes sense to get the focus and use focus action instead.

Was there a particular use-case you had in mind for this?

aghassemi · 2017-07-14T18:49:54Z

src/service/standard-actions-impl.js

+    const permittedPosVals = ['top','bottom','center'];
+    const pos = invocation.args
+                && invocation.args['position']
+                && (permittedPosVals.indexOf(invocation.args['position']) + 1) ?


we polyfill array.includes, so you can use it here

aghassemi · 2017-07-14T18:51:41Z

src/service/standard-actions-impl.js

+                     invocation.args['duration'] : 500;
+
+    // Position in the viewport at the end
+    const permittedPosVals = ['top','bottom','center'];


move outside as a module level constant.

aghassemi · 2017-07-14T18:59:39Z

src/service/standard-actions-impl.js

+    const duration = invocation.args
+                     && invocation.args['duration']
+                     && invocation.args['duration'] >= 0 ?
+                     invocation.args['duration'] : 500;


please create an issue to make the default be based on some formula that account for the distance between source and target. Ideally whatever Chrome is using for their native implementation (https://bugs.chromium.org/p/chromium/issues/detail?id=243871)

ghost · 2017-07-17T12:01:08Z

spec/amp-actions-and-events.md

@@ -89,7 +89,7 @@ For example, the following is possible in AMP.
    <th>Description</th>
  </tr>
  <tr>
-    <td>tap</td>
+    <td>`tap`</td>


Did you preview this file? I ask because you'll notice that the back ticks don't work as code formatting if it's in HTML code. Instead, please replace all back ticks with <code></code>.

My bad! Thanks :)

ghost

Approval of .md file changes

aghassemi · 2017-07-17T22:47:30Z

examples/standard-actions.amp.html

+<div class="spacer">
+</div>
+
+<h4>Subscribe to our weekly Newsletter Form</h4>


I think this is too much and makes the example cluttered, let's just do a simple <input type="text" id="input-element"> for focus and a simple div for scrollTo, no need for unrelated stuff like amp-form, CLIENT_ID, etc...

aghassemi · 2017-07-17T22:47:34Z

examples/standard-actions.amp.html

+  <button on="tap:form.scrollTo('position' = 'center')">ScrollTo Center</button>
+  <button on="tap:form.scrollTo('duration' = 5000)">ScrollTo Slowly</button>
+  <button on="tap:name1.focus">Focus</button>
+  <button on="tap:name1.blur">Lose Focus</button>


aghassemi · 2017-07-17T22:48:41Z

src/service/standard-actions-impl.js

+        'scrollTo', this.handleScrollTo.bind(this));
+    actionService.addGlobalMethodHandler(
+        'focus', this.handleFocus.bind(this));
+    actionService.addGlobalMethodHandler('blur', this.handleBlur.bind(this));


aghassemi · 2017-07-17T22:48:53Z

src/service/standard-actions-impl.js

+   * Handles the `blur` action where given an element, we make it lose its focus
+   * @param {!./action-impl.ActionInvocation} invocation
+   */
+  handleBlur(invocation) {


aghassemi · 2017-07-17T22:51:12Z

src/service/viewport-impl.js

@@ -430,8 +447,8 @@ export class Viewport {
    // TODO(erwinm): the duration should not be a constant and should
    // be done in steps for better transition experience when things
    // are closer vs farther.
-    return Animation.animate(this.ampdoc.getRootNode(), pos => {
-      this.binding_.setScrollTop(interpolate(pos));
+    return Animation.animate(this.ampdoc.getRootNode(), position => {


update the comment above to point to #10463 (assign to me)

aghassemi · 2017-07-18T00:45:48Z

examples/standard-actions.amp.html

@@ -7,6 +7,7 @@
    <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
    <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>
+    <script async custom-element="amp-form" src="https://cdn.ampproject.org/v0/amp-form-0.1.js"></script>


aghassemi · 2017-07-18T00:47:41Z

src/service/viewport-impl.js

-    // are closer vs farther.
-    return Animation.animate(this.ampdoc.getRootNode(), pos => {
-      this.binding_.setScrollTop(interpolate(pos));
+    // TODO(aghassemi): the duration should not be a constant and should


TODO(aghassemi, #10463) (Dima has a parser that finds TODO issues of this format)

pulitz · 2017-07-27T07:53:39Z

Example does not work. Is anything special needed?

Action Error: Target element does not support provided action in [tap:normal-element2.scrollTo] on [[object HTMLDivElement]]

wassgha · 2017-07-27T08:17:27Z

That is odd. @pulitz I would make sure that your build is up to date and that the example page isn't being served production AMP (this feature is not in production yet)
@aghassemi can you test on your end? It works for me.

pulitz · 2017-07-27T15:14:14Z

Oh. Sorry, I guess it was my mistake. I thought this feature was in production seeing that it was already merged. Hopefully it really does go on production soon.

aghassemi · 2017-07-27T19:38:13Z

@pulitz sorry currently it is in Dev Channel, you can try it by enabling Dev Channel (https://cdn.ampproject.org/experiments.html) will be in production next week.

googlebot added the cla: yes label Jul 13, 2017

wassgha requested a review from aghassemi July 13, 2017 02:34

wassgha force-pushed the git branch 4 times, most recently from 2a47957 to f4efc1d Compare July 13, 2017 05:13

wassgha changed the title ~~Implements scrollTo and blur standard actions~~ Implements scrollTo, focus and blur standard actions Jul 13, 2017

wassgha force-pushed the git branch 4 times, most recently from 7e8af82 to 916bd58 Compare July 13, 2017 23:13

wassgha added this to the Fixit Week EOQ2 milestone Jul 13, 2017

wassgha force-pushed the git branch 2 times, most recently from e8c9cb4 to 0ef99ef Compare July 14, 2017 01:23

aghassemi suggested changes Jul 14, 2017

View reviewed changes

wassgha force-pushed the git branch from 0ef99ef to 793a51c Compare July 15, 2017 06:04

wassgha mentioned this pull request Jul 15, 2017

Default animateScrollIntoView() speed to a dynamically calculated speed #10463

Closed

wassgha force-pushed the git branch from 793a51c to 24a0b32 Compare July 15, 2017 17:07

ghost suggested changes Jul 17, 2017

View reviewed changes

wassgha force-pushed the git branch from 24a0b32 to 37cb6fb Compare July 17, 2017 16:00

ghost approved these changes Jul 17, 2017

View reviewed changes

aghassemi reviewed Jul 17, 2017

View reviewed changes

wassgha added 4 commits July 17, 2017 16:30

Added scrollTo, focus and blur standard actions

b237856

Added unit tests

f6471f4

Added top, center, bottom to animateScrollIntoView and scrollTo

f2e2806

Requested changes

c3b68a2

wassgha force-pushed the git branch from b48556a to c3b68a2 Compare July 17, 2017 23:31

Requested changes

45011d8

wassgha force-pushed the git branch from 4299279 to 45011d8 Compare July 18, 2017 00:16

aghassemi approved these changes Jul 18, 2017

View reviewed changes

Nit requested changes

3d7940c

aghassemi merged commit e589398 into ampproject:master Jul 18, 2017

wassgha deleted the git branch July 18, 2017 16:40

ampprojectbot added the PR use: In Beta / Experimental label Jul 24, 2017

aghassemi mentioned this pull request Jul 27, 2017

Allow the new scrollTo action to also optionally focus #10670

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements scrollTo, focus and blur standard actions #10398

Implements scrollTo, focus and blur standard actions #10398

wassgha commented Jul 13, 2017 •

edited

wassgha commented Jul 13, 2017

aghassemi Jul 14, 2017

aghassemi Jul 14, 2017

aghassemi Jul 14, 2017

aghassemi Jul 14, 2017

aghassemi Jul 14, 2017

ghost Jul 17, 2017

wassgha Jul 17, 2017

ghost left a comment

aghassemi Jul 17, 2017

aghassemi Jul 17, 2017

aghassemi Jul 17, 2017

aghassemi Jul 17, 2017

aghassemi Jul 17, 2017

aghassemi Jul 18, 2017

aghassemi Jul 18, 2017 •

edited

wassgha Jul 18, 2017

pulitz commented Jul 27, 2017

wassgha commented Jul 27, 2017

pulitz commented Jul 27, 2017

aghassemi commented Jul 27, 2017

Implements scrollTo, focus and blur standard actions #10398

Implements scrollTo, focus and blur standard actions #10398

Conversation

wassgha commented Jul 13, 2017 • edited

Changes

wassgha commented Jul 13, 2017

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

ghost 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

aghassemi Jul 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulitz commented Jul 27, 2017

wassgha commented Jul 27, 2017

pulitz commented Jul 27, 2017

aghassemi commented Jul 27, 2017

wassgha commented Jul 13, 2017 •

edited

aghassemi Jul 18, 2017 •

edited