Scroll AMP doc to center highlighted elements in the viewport #15791

yunabe · 2018-06-04T09:37:45Z

This pull request should be merged after #15337. There is only one new commit in this PR (2674709)

alanorozco · 2018-06-04T20:55:38Z

yunabe · 2018-06-05T00:23:32Z

Thanks. Rebased the change to amphtml/master.

alanorozco · 2018-06-07T23:22:49Z

extensions/amp-viewer-integration/0.1/highlight-handler.js

    } else {
+      if (scrollTop > SCROLL_ANIMATION_HIGHT_LIMIT) {


nit: HEIGHT

Oops, good catch!

alanorozco · 2018-06-07T23:24:17Z

extensions/amp-viewer-integration/0.1/highlight-handler.js

+    if (!nodes) {
+      return 0;
+    }
+    const viewport = Services.viewportForDoc(this.ampdoc_);


Is Viewport.animateScrollIntoView(element, duration, curve, 'center'); not good enough for your use case?

There are two reasons I implemented a new logic:

I want to center multiple elements because discontinuous sentences can be highlighted.

Want to center elements when the header is shown (height = win.height - viewport.getPaddingTop).

In that case, I propose you do the following:

Use the same math to calculate a center rectangle.

Said rectangle is used to create an invisible element with pointer-events: none so that it won't block interaction.

Use animateScrollIntoView(invisibleElement, duration, curve, 'center').

Remove invisible element afterwards.

This has two purposes:

keep the viewport API stable.

Aid in the AMP build-size reduction effort. We have a bundle size restriction in place, which would be triggered by adding a method to the viewport service. We can relax this limit, but measures like this help keep it smaller in the first place! :)

I'm not sure I understand your suggestion correctly.

To implement your suggestion, I need to add new code to implement

Said rectangle is used to create an invisible element with pointer-events: none so that it won't block interaction.

Remove invisible element afterwards.

This is not very trivial and it will increase amp-viewer-extension size a little bit. However, I can not remove any code from the current implementation except for the interface of animateScrollToTop in viewport-impl.js.

Which part of code do you think we can remove by your suggestion?

Thanks,
yunabe

Increasing the size of amp-viewer-integeration is fine. However, by modifying Viewport, the size of the core AMP binary increases.

I'm suggesting that you revert the changes to Viewport and use the method described above.

That's true. But the additional size of dist/v0.js is just 7 bytes after gzip (78597 --> 78604, 107 bytes before gzip 247319 --> 247426).
Do 7 bytes (+0.01%) really matter in the core AMP binary size?

If 7 bytes really matter, I will remove default parameters from animateScrollToTop. Then, the size diff is only 1 byte after gzip (78597 --> 78598).

Do we really want to implement non-trivial logic in amp-viewer-integration just to save 7 bytes (1 byte w/o default parameters) in the core runtime?

Sorry if I'm missing something, but how is it non-trivial? It seems like you could calculate easily and left/right aren't an issue since we're only scrolling vertically. So it would only be a matter of doing something like:

const sentinel = document.createElement('i-amphtml-sentinel'); setStyles(sentinel, { 'position': absolute, 'left: '0', 'right: '0', 'top': topAsUsuallyCalculated, 'bottom': newLogicToCalculateBottom(), }); this.getAmpDoc().getBody().appendChild(sentinel); return viewport.animateScrollIntoView(// .. etc

Unfortunately we're only counting size before gzip.

But you're right, the difference is negligible. It's fine to bump size. Just one nit: rename to
animateScrollTo.

I meant I think it's less trivial to inject the sentinal node than to export animateScrollTo in viewerport, especially considering setScrollTop is already exported in viewport and the additional size to v0.js is limited.

But I do not have a strong preference. I reimplemented this based on your suggestion.

alanorozco · 2018-06-08T16:51:04Z

extensions/amp-viewer-integration/0.1/highlight-handler.js

+    let minTop = Number.MAX_VALUE;
+    let maxBottom = 0;
+    for (let i = 0; i < nodes.length; i++) {
+      const rect = viewport.getLayoutRect(nodes[i]);


nit: Use destructuring:

const {top, bottom} = viewport.getLayoutRect(nodes[i]);

alanorozco · 2018-06-09T20:02:26Z

extensions/amp-viewer-integration/0.1/highlight-handler.js

+        .animateScrollIntoView(sentinel).then(() => {
+          this.sendHighlightState_('shown');
+        });
+    body.removeChild(sentinel);


This should occur once animateScrollIntoView is resolved.

…-handler.js.

yunabe

Thank you for the review. Could you take another look?

yunabe · 2018-06-11T01:39:23Z

extensions/amp-viewer-integration/0.1/highlight-handler.js

+    let minTop = Number.MAX_VALUE;
+    let maxBottom = 0;
+    for (let i = 0; i < nodes.length; i++) {
+      const rect = viewport.getLayoutRect(nodes[i]);


yunabe · 2018-06-11T01:52:44Z

extensions/amp-viewer-integration/0.1/highlight-handler.js

+    if (!nodes) {
+      return 0;
+    }
+    const viewport = Services.viewportForDoc(this.ampdoc_);


I meant I think it's less trivial to inject the sentinal node than to export animateScrollTo in viewerport, especially considering setScrollTop is already exported in viewport and the additional size to v0.js is limited.

But I do not have a strong preference. I reimplemented this based on your suggestion.

googlebot added the cla: yes label Jun 4, 2018

aghassemi requested a review from alanorozco June 4, 2018 20:55

yunabe force-pushed the scrolldev branch 2 times, most recently from e4b6024 to d110ce2 Compare June 5, 2018 00:21

Scroll to highlighted elements so that they are centered.

ebd5cf1

yunabe force-pushed the scrolldev branch from d110ce2 to ebd5cf1 Compare June 5, 2018 01:08

alanorozco requested changes Jun 7, 2018

View reviewed changes

yunabe added 2 commits June 8, 2018 01:18

Merge branch 'master' into scrolldev

d7cf486

Fix a typoe (s/HIGHT_LIMIT/HEIGHT_LIMIT/) in highlight-handler.js.

b9f7b6b

alanorozco approved these changes Jun 8, 2018

View reviewed changes

alanorozco reviewed Jun 9, 2018

View reviewed changes

Specify the position to scroll using a temporary element in highlight…

aa01065

…-handler.js.

yunabe force-pushed the scrolldev branch from 138d070 to aa01065 Compare June 11, 2018 01:31

yunabe commented Jun 11, 2018

View reviewed changes

Use object destructuring to get top/bottom from viewport.getLayoutRect.

246d42d

yunabe force-pushed the scrolldev branch from 1f8cfc8 to 246d42d Compare June 11, 2018 04:32

Merge branch 'master' into scrolldev

eda8c7d

yunabe mentioned this pull request Jun 11, 2018

Support "skip rendering" mode in amp-viewer-integration/highlight-handler.js #15950

Merged

alanorozco merged commit 289a745 into ampproject:master Jun 11, 2018

yunabe deleted the scrolldev branch June 19, 2019 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scroll AMP doc to center highlighted elements in the viewport #15791

Scroll AMP doc to center highlighted elements in the viewport #15791

yunabe commented Jun 4, 2018 •

edited

alanorozco commented Jun 4, 2018

yunabe commented Jun 5, 2018

alanorozco Jun 7, 2018

yunabe Jun 8, 2018

alanorozco Jun 7, 2018

yunabe Jun 8, 2018

alanorozco Jun 8, 2018 •

edited

yunabe Jun 8, 2018 •

edited

alanorozco Jun 8, 2018 •

edited

yunabe Jun 8, 2018

alanorozco Jun 8, 2018 •

edited

yunabe Jun 11, 2018

alanorozco Jun 8, 2018

yunabe Jun 11, 2018

alanorozco Jun 9, 2018

yunabe left a comment

yunabe Jun 11, 2018

yunabe Jun 11, 2018

Scroll AMP doc to center highlighted elements in the viewport #15791

Scroll AMP doc to center highlighted elements in the viewport #15791

Conversation

yunabe commented Jun 4, 2018 • edited

alanorozco commented Jun 4, 2018

yunabe commented Jun 5, 2018

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

alanorozco Jun 8, 2018 • edited

Choose a reason for hiding this comment

yunabe Jun 8, 2018 • edited

Choose a reason for hiding this comment

alanorozco Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanorozco Jun 8, 2018 • edited

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

yunabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunabe commented Jun 4, 2018 •

edited

alanorozco Jun 8, 2018 •

edited

yunabe Jun 8, 2018 •

edited

alanorozco Jun 8, 2018 •

edited

alanorozco Jun 8, 2018 •

edited