window.context.requestResize() does not set corresponding height on i-amp-sizer

[e2corporation]

For scenarios where requestResize() is being used to satisfy multi-size/responsive Ads, shouldn't the i-amp-sizer element's height (padding-top) be set to match the value of the requested height?

If this does not occur, the i-amp-sizer inherits whatever height is set by the heights="" media query set, which may be responsible for scrolling (and clicking) issues within the iframe when there are differences between these values.

Is it by design that i-amp-sizer is not adjusted with the resize call?

[e2corporation]

In addition to this, what are the rules surrounding when a resize request is honored? I am also seeing another issue where a resize attempt gets denied 90% more than it's allowed.

requestResize() is being called on initial load, and during orientation changes the request is still denied frequently (with or without timeouts)

[jridgewell]

When an element requests a resize, it become fully responsible for managing it's size from there on out. https://github.com/ampproject/amphtml/blob/master/src/custom-element.js#L803-L809

Resizes are honored when the resize will not adjust the content the user is currently reading. Ie, if the ad is above the viewport's contents, it'll resize. Same if it's below. If it's in the viewport, we ignore it.

[e2corporation]

"Resizes are honored when the resize will not adjust the content the user is currently reading. Ie, if the ad is above the viewport's contents, it'll resize. Same if it's below. If it's in the viewport, we ignore it."

That logic does not seem appropriate for multi-size ads at all, For an example if we have an ad-fill of 1 column and 6 rows, the content will always exceed viewport height (100vh). This means that our resize request will always get denied.

The crucial time when resizing is most needed is when the Ad is in the viewport...

[jridgewell]

Viewport height doesn't affect the resizes, just wether the element is in the viewport. If you want to resize, do it earlier in the request before the user has had the chance to scroll to your (not fully rendered) ad.

Additionally, you can show an "overflow" button asking the user to allow resize. If the user interacts with it, any resize you issue next will be fulfilled.

[e2corporation]

Thanks for the feedback Justin.

How can resizing be accomplished beforehand when the amp-ad has not yet intersected the viewport (this is when the amp-ad starts to render?)? Unless the ad starts filling creative, with headline, image and related margins, accurate height of the rendered amp-iframe's loaded content cannot be detected until it has finished loading images, this information is certainly not calculated ahead of time.

If one tries to detect or set this value too soon it will most likely be zero, or some other unreliable amount.

Here are some excerpts on how resizing is being handled for this example, along with what the resizing operation looks like.

Custom Resize Handler

....
var frameHeight = document.body.scrollHeight; 
// use a mixture of reliable values for the height, this is for reference only.

/*
In landscape orientations, the height of the panel is usually smaller than vewport
so we can't use body height it will be too large, 
fallback to the element's content height.
*/
if(self.widgetEl.offsetHeight > 0 && frameHeight > self.widgetEl.offsetHeight) {
   frameHeight = self.widgetEl.offsetHeight;
} 

// Request Resize via API
window.context.requestResize(document.clientWidth, Math.max(50, providerHeight + frameHeight));
.....

The above call is fired once during init, again after a 3second delay. Afterwards, intersection observer is used to maintain accurate heights:

1) During Intersection - Headline height varies constantly based on viewport width, so resizing during intersection ensures the panel is not clipped at the bottom because of a previous stale height value.

...
        self.stopObservingIntersection = window.context.observeIntersection(function(changes) {
            changes.forEach(function(c) {
                clearTimeout(self.ioTimeout);
                self.ioTimeout = setTimeout(function(){
                    self.adjustHeight();
                }, 125);
            });
        });
...

2) During Orientation Change - Switching from portrait to landscape needs height adjustment as well.

            window.addEventListener('orientationchage', function (event) {
                var orientatioHandler = function(e){
                    self.adjustHeight();
                    window.removeEventListener('resize', orientationHandler);
                }
                window.addEventListener('resize', orientationHandler);
            });

The "overflow" button seems like a nice alternative, though it contradicts the rule that resize requests are denied if the ad is within view. I think the we need the ability to override and force resize on the tag especially when in viewport, regardless of what the user is doing.

What's the purpose of having an API resize helper method that has a goal of improving rendering yet prefers to deny requests a majority of the time?

[e2corporation]

If we have to go a deny-first route, could it have another mode "less-aggressive", where when enabled has the following constraints.

1. Deny if scroll user scroll too fast, $velocity > [some-value-considered-too-fast]
2. Deny if timestamp difference of previous request is too soon, $eventTime below [optimal-resize-wait]ms

otherwise, allow resize...

[e2corporation]

Will try another route in the mean time, as requests are only honored when panel is outside viewport scope, that means event.intersectionRatio == 0. I'll also stop the intersection observer on a successful resize event, and restart it again on resize and/or orientation change.

[e2corporation]

@jridgewell Note the behavior of iPhone class devices with the following example, ad placement is at the bottom of the page.

https://shogun-amp.herokuapp.com/examples/revcontent-heavy.html

In the example above, the resize requests are only submitted when our panel is outside viewport scope.

On iPhone the amp-ad tag resize request is never honored in portrait orientation, resulting in an amp-ad tag with no inline-height set, which then produces clipping and inside scrolling within the tag, the only visible portion is the value of i-amp-sizer's padding.

The BG is painted green upon a successful resize (works on Android devices, Galaxy S7).

Is there something missing in order to correct the behavior with iPhone?

...
...
self.stopObservingIntersection = window.context.observeIntersection(function(changes) {
            changes.forEach(function(c) {
                    if(c.intersectionRatio == 0){
                        self.adjustHeight();
                    }
            });
});
...
...

...
window.context.onResizeSuccess(function() {
        document.body.style.background = "green";
        document.body.classList.remove("resize-denied");
        document.body.classList.add("resize-success");
        console.log("SUCCESS... Stop Observing..");
        self.stopObservingIntersection();
        self.isObserving = false;
}); 
...

[dvoytenko]

/cc @lannka

This is a pretty interesting case. Let me confirm a couple of things. It's an ad that uses an AMP creative, right? Is this intentional or just a happenstance?

Given AMP's current core use cases, AMP attempts to make itself scrollable. This is definitely not intended here and relatively easy to fix. I'll send the fix for it shortly. One consequence of this is that iframe sometimes struggles to correctly calculate scrolling height. So, I expect my fix would resolve resize as well.

[e2corporation]

@dvoytenko This is intentional, as an interim solution until we can roll-out full A4A support, we are using an internal API call and generating a native AMPHTML document (3p-iframe customized) with amp-img tags for each creative. Will this method produce improved load time with the use of amp-img tags as opposed to the traditional HTML document normally present inside the iframe?

[dvoytenko]

@e2corporation In a 3p case, it's possible that you will get performance benefit as well. But, of course, a4a would give you the most benefit so this is useful from that point of view.

[e2corporation]

@dvoytenko Thanks for the updates to master branch, I pulled in the changes and I noticed iOS is responding better to resize requests.

There is also a related issue of the inability to scroll up once scrolling down past the top of the ad panel on iOS, is this something that's also controlled by the ios-embed logic?

If touchmove is used on the left and right edges of the viewport to scroll up (outside amp-ad/iframe area) then the device will scroll up however.

[e2corporation]

@dvoytenko @lannka I noticed if we remove the i-amp-sizer element the problem goes away, it renders with a padding-top of 80%, so it is intercepting touch events.

Is it possible to have the i-amp-sizer's padding set to 0 and/or display:none when context.requestResize is used? Maybe even another helper method to re-enable the sizer manually if needed, assuming we no longer want to manually maintain heights within code and fallback to the inherited heights media queries.

[lannka]

i-amp-sizer is introduced for responsive layout. after a successful "resize", we should change it to fixed layout.

I have a PR to fix another issue with a similar cause: #6208 As @e2corporation has mentioned, probably we should also remove i-amp-sizer?

@dvoytenko @jridgewell thoughts?

[dvoytenko]

@lannka This is an interesting idea for sure. But to confirm, we already always set sizer's padding to 0. Why is not helping here?

RE:#6208 PR: it looks good, but we'll probably also need to apply display: none on sizer for all non-responsive layouts?

[e2corporation]

@dvoytenko do you guys have a rough eta on when the potential fixes would hit amphtml:master? We have a few things queued for our release that are centered around resizing. Thanks for all the help.

• Implement renderStart()
• Fix "broken" status on examples
• Responsive Revcontent amp-ad Enhancements
• Native API Support

[dvoytenko]

@e2corporation slotting some early next week.

[dvoytenko]

@e2corporation I lost the repro case. I keep hitting the https://shogun-amp.herokuapp.com/examples/revcontent-heavy.html but seeing no embed content.

[dvoytenko]

@e2corporation It actually looks you've made bunch of changes and maybe have a pull request for me to debug? As it stands, it's pretty hard to debug with minified files and source maps are directed to localhost. Alternatively, I maybe able to debug if you give me commit hash for GitHub repo...

[dvoytenko]

@e2corporation Also, what time zone are you in? Can we connect on our slack and run through debug steps together?

[e2corporation]

@dvoytenko Thanks again for jumping on this, the ad service code running on heroku is unminified, and maybe accessed here for reference:

https://shogun-amp.herokuapp.com/showcase-labs/js/revcontent.amp.js

The Github PR for amphtml:master has minor changes, mainly to add renderStartImplemented.

Steps to reproduce:

1. Load Test URL on iPhone https://shogun-amp.herokuapp.com/examples/revcontent-heavy.html
2. Begin scrolling down to reach the Advertisement panel
3. Scroll down past the panel till the end of the page
4. Attempt to scroll back up the document with touch focus on the center of the screen
5. Notice that only the inside iframe scrolls up not the parent document.
6. Inspect i-amp-sizer element post-render, notice value at 80%
7. Workaround: deletingi-amp-sizer element allows scrolling up and down without being trapped inside the frame.

Custom 3P frame.max.html https://shogun-amp.herokuapp.com/dist.3p/current/frame.max.html

[e2corporation]

@dvoytenko Also if you are receiving a blank fill, it could be a security block. You should see a message in the XHR log for our API call if this happens.

[dvoytenko]

So, there's a bit of confluence of things:

First, we disabled scrolling in an AMP creative when it's an embed. This should go to PROD next Thursday. Without this, even with correct sizing, the UX will be unideal b/c the iframe will be periodically intercepting scrolling. Once it's on PROD, it should be no longer an issue.

Second, aspect ratio calculations were overriding resize back to previous value. This is fixed in #6313.

Third: a minor thing, but when an embed requests the same size it already has, the request is denied. I believe this was in mistake. And #6313 changes this behavior to report success in this case, since, indeed, the embed will have the desired size.

Fourth, I still believe the needed height calculations in the revcontent.amp.js are incorrect. AMP doesn't have a strong opinion on how an embed would get this size. But, in particular, this code looks suspicious:

if(self.widgetEl.offsetHeight > 0 && frameHeight > self.widgetEl.offsetHeight) {
  frameHeight = self.widgetEl.offsetHeight;
} 

a. It'd seem it should be self.widgetEl.offsetHeight > frameHeight in the if above. b. Most of time, the desired height is simply self.widgetEl.scrollHeight. There might be some intricacies with this with position:absolute elements and such, but usually these are still straightforward to incorporate.

[e2corporation]

@dvoytenko Thanks again for the feedback and digging into this, I will pull in the changes from master once those fixes are merged and run some additional tests.

In regards to the height calculation you mentioned, that logic is useful for when a device rotates from portrait to landscape. If we have a1(column)x4(rows)in portrait, then switch to landscape it becomes a 4(column)x1(rows). Element height is preferred as document or scroll height ended up being closer to viewport height which will be too tall creating excess space/scrolling below the ad panel while in landscape orientation.

Since the widget will be in view at that time, any resize requests will be denied until the event intersection falls back to 0 (ad panel outside viewport).

I will retest with scrollHeight as you suggested to see if there any other improvements to be made. Were you able to test resizing and scrolling while swapping from portrait to landscape?

[dvoytenko]

@e2corporation

I will pull in the changes from master once those fixes are merged and run some additional tests

One thing I noticed, the iframe itself in your examples uses the CDN scripts, not local. If you want to test against CDN (which is only relevant to "we disabled scrolling in an AMP creative when it's an embed" statement above), you will have to wait until the scrolling fix is in canary, which should be next Thursday.

I will retest with scrollHeight as you suggested to see if there any other improvements to be made. Were you able to test resizing and scrolling while swapping from portrait to landscape?

This really depends on what the page/widget look like inside. But it's a very typical pattern. E.g. Twitter and most of other embeds do something similar. You'd just need to listen to resize events on your window and issue size change requests based on your widget's scrollHeight. In my experience, though, scrollHeight is the best way to do it, as long as you do it on your widget element and not on document's body or documentElement or similar - those follow viewport.

Since the widget will be in view at that time, any resize requests will be denied until the event intersection falls back to 0 (ad panel outside viewport).

This shouldn't happen. Changes in orientation are considered user actions and we allow size changes for this. Let me know if that's not what you see.