Disallowing resize of elements near top of viewport when viewport not scrolled #5774

tlong2 · 2016-10-24T10:22:45Z

Modifying resources-impl.js so that an attemptChangeSize request doesn't resize elements near the top of the viewport when the viewport has not been scrolled.

Currently if the bottom of something is within the viewport but less than 10% of the viewport height from the top it is treated as though it is above the viewport. This can have the bad consequence that if something very small at the top of the page gets resized it can push things at the top (like the nav-bar) above the viewport and out of view, which is currently a bad and confusing experience for users, since this can happen at initial page load.

jridgewell · 2016-10-24T20:21:40Z

Do you have a demo page we could see the issue on?

/to @dvoytenko

tlong2 · 2016-10-25T13:11:57Z

I don't have a demo page example. This was something I came across when experimenting with an extension I was playing around with.

Basically the scenario is:
There is a custom element (e.g. ) that is allowed to be placed near the top of the page. The implementation of it tries to increase its height using attemptChangeSize. If the were in the middle of the viewport then this would just not be allowed. But if the whole of the were within the top 10% of the viewport currently, then the resize would be allowed and the page would be scrolled, which would push the things above the (e.g. a nav bar) out of view.

dvoytenko · 2016-10-25T17:07:31Z

@tlong2 Thanks! Let's however delay it until we are a bit deeper and closer to the feature-set we'd like. I'm pretty confident we can quickly bridge the gap, including via this PR. But I'd first like to get a good demonstrable state.

dvoytenko · 2016-10-25T17:43:29Z

In other words, let's get the repro case merged first :)

tlong2 · 2017-02-22T15:03:23Z

@dvoytenko PTAL. The problem is now demonstrated by amp-auto-ads and this PR fixes the issue.

Problem can be demonstrated/solution seen on http://amp.autoplaced.com/

jridgewell · 2017-02-22T18:41:20Z

I think that's a bug with resetting the scrollTop, not the resize.

tlong2 · 2017-02-22T20:43:08Z

There are two problems:

Expanding things near the top of the page can push important things (like the menu) off the top. This is bad experience but is not the thing causing reflow.
When we push something off the top of the page before page load completes a reflow occurs because on page load complete the scroll top gets reset to 0 by something

This pull request fixes (1), which also has the effect of making (2) never be an issue for amp-auto-ads.

Regardless of (2), (1) is still bad and this PR fixes it.

jridgewell · 2017-02-22T20:52:16Z

src/service/resources-impl.js

@@ -1052,9 +1053,13 @@ export class Resources {
          // an element's boundary is not changed above the viewport after
          // resize.
          resize = true;
-        } else if (bottomDisplacedBoundary <= viewportRect.top + topOffset) {
+        } else if (scrollTop > 0 &&


For iOS, this might be set to 1.

jridgewell · 2017-02-22T20:52:53Z

Expanding things near the top of the page can push important things (like the menu) off the top. This is bad experience but is not the thing causing reflow.

Perfect explanation, thanks.

dvoytenko · 2017-02-24T05:21:14Z

@tlong2 Thanks! I'll take a look at the repro case.

dvoytenko · 2017-02-27T18:27:11Z

src/service/resources-impl.js

@@ -1052,9 +1053,13 @@ export class Resources {
          // an element's boundary is not changed above the viewport after
          // resize.
          resize = true;
-        } else if (bottomDisplacedBoundary <= viewportRect.top + topOffset) {
+        } else if (scrollTop > 1 &&


Just use viewportRect.top here instead of scrollTop

dvoytenko · 2017-02-27T18:27:36Z

src/service/resources-impl.js

          // 5. Elements above the viewport can only be resized if we are able
-          // to compensate the height change by setting scrollTop.
+          // to compensate the height change by setting scrollTop and only if


Is there a good demo code to play with to see the effect and fix? Does this only affect elements that grow in size in reality?

The effect can be seen on http://amp.autoplaced.com/ if you enable the experiment: AMP.toggleExperiment('amp-auto-ads').

It's not easy to capture in an example page, as amp-auto-ads requires a remote configuration, which seems hard to mock out in an example page.

googlebot · 2017-02-27T19:13:55Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-02-27T19:18:15Z

CLAs look good, thanks!

dvoytenko · 2017-02-27T19:56:43Z

LGTM from me.

jridgewell

Looks like test flake, please ping after it passes.

tlong2 · 2017-02-27T21:00:43Z

Looks like tests are all passing

… scrolled (#5774) * changes * Disallowing resize when vp not scrolled * preventing size changes at top of viewport before scrolling.

… scrolled (ampproject#5774) * changes * Disallowing resize when vp not scrolled * preventing size changes at top of viewport before scrolling.

tlong2 changed the title ~~Syncing commits~~ Disallowing resize of things near top of viewport when viewport not scrolled Oct 24, 2016

tlong2 changed the title ~~Disallowing resize of things near top of viewport when viewport not scrolled~~ Disallowing resize of elements near top of viewport when viewport not scrolled Oct 24, 2016

jridgewell added the Type: Discussion/Question label Oct 24, 2016

jridgewell assigned dvoytenko Oct 24, 2016

jridgewell reviewed Feb 22, 2017

View reviewed changes

dvoytenko suggested changes Feb 27, 2017

View reviewed changes

tlong2 and others added 3 commits February 27, 2017 19:15

changes

6353587

Disallowing resize when vp not scrolled

193bf99

preventing size changes at top of viewport before scrolling.

86b581b

dvoytenko approved these changes Feb 27, 2017

View reviewed changes

jridgewell approved these changes Feb 27, 2017

View reviewed changes

jridgewell merged commit 5a4c7e2 into ampproject:master Feb 27, 2017

adelinamart pushed a commit that referenced this pull request Mar 1, 2017

Disallowing resize of elements near top of viewport when viewport not…

2d68333

… scrolled (#5774) * changes * Disallowing resize when vp not scrolled * preventing size changes at top of viewport before scrolling.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallowing resize of elements near top of viewport when viewport not scrolled #5774

Disallowing resize of elements near top of viewport when viewport not scrolled #5774

tlong2 commented Oct 24, 2016 •

edited

jridgewell commented Oct 24, 2016

tlong2 commented Oct 25, 2016

dvoytenko commented Oct 25, 2016

dvoytenko commented Oct 25, 2016

tlong2 commented Feb 22, 2017

jridgewell commented Feb 22, 2017

tlong2 commented Feb 22, 2017

jridgewell Feb 22, 2017

tlong2 Feb 22, 2017

jridgewell commented Feb 22, 2017

dvoytenko commented Feb 24, 2017

dvoytenko Feb 27, 2017

tlong2 Feb 27, 2017

dvoytenko Feb 27, 2017

tlong2 Feb 27, 2017

googlebot commented Feb 27, 2017

googlebot commented Feb 27, 2017

dvoytenko commented Feb 27, 2017

jridgewell left a comment •

edited

tlong2 commented Feb 27, 2017

Disallowing resize of elements near top of viewport when viewport not scrolled #5774

Disallowing resize of elements near top of viewport when viewport not scrolled #5774

Conversation

tlong2 commented Oct 24, 2016 • edited

jridgewell commented Oct 24, 2016

tlong2 commented Oct 25, 2016

dvoytenko commented Oct 25, 2016

dvoytenko commented Oct 25, 2016

tlong2 commented Feb 22, 2017

jridgewell commented Feb 22, 2017

tlong2 commented Feb 22, 2017

jridgewell Feb 22, 2017

Choose a reason for hiding this comment

tlong2 Feb 22, 2017

Choose a reason for hiding this comment

jridgewell commented Feb 22, 2017

dvoytenko commented Feb 24, 2017

dvoytenko Feb 27, 2017

Choose a reason for hiding this comment

tlong2 Feb 27, 2017

Choose a reason for hiding this comment

dvoytenko Feb 27, 2017

Choose a reason for hiding this comment

tlong2 Feb 27, 2017

Choose a reason for hiding this comment

googlebot commented Feb 27, 2017

googlebot commented Feb 27, 2017

dvoytenko commented Feb 27, 2017

jridgewell left a comment • edited

Choose a reason for hiding this comment

tlong2 commented Feb 27, 2017

tlong2 commented Oct 24, 2016 •

edited

jridgewell left a comment •

edited