Amphtml: amp-sticky-ad fails to load sometimes on Safari 10

[jasti]

Fails about 1 in 3 times.
Sample URL: https://fresnobee.relaymedia.com/amp/news/local/education/article61889207.html
Note that browser emulation works fine - only when tried from an iPhone using Safari 10.

CC @zhouyx

[jpettitt]

Additional info: If you scroll down and reload the page from the scrolled position the sticky ad appears 100% of the time. If you reload from top of page and then scroll some % of the time it doesn't appear. It smells like a race condition becasue it's very unpredictable - sometimes it will fail 5 in a row then work for 10.

[zhouyx]

Looking into it. @jasti does this happen only on ios10?

[jasti]

Tested only on 10. Didn't have an older device.

[zhouyx]

Something I found here is that though sticky-ad take owner of the child ad, it will sometimes be laid out before sticky-ad schedule layout for them. @dvoytenko any ideas why?

[zhouyx]

OK it seems this happens because #getResourcesInViewport run before sticky-ad build. During that time we haven't declare ownership for the ad yet, and the ad will be laid out. However sticky-ad assumes its child ad can only be laid out with it manually call #scheduleLayout. It never receive load:end event in this case.

This explains

If you scroll down and reload the page from the scrolled position the sticky ad appears 100% of the time. If you reload from top of page and then scroll some % of the time it doesn't appear.

[jridgewell]

#getResourcesInViewport doesn't layout any elements, though?

[zhouyx]

This exact function won't layout any elements, but those elements will be laid out since they are in viewport. And if the ad makes to this list, we will layout it eventually w/o sticky-ad #scheduleLayout for it

[mhchu]

Not sure if this information helps, but, in my testing of amp-sticky-ad, I am also getting the situation where the ad does not show when scrolling down. (However, reloading the page after scrolling down, does reveal the sticky ad.) This occurs both on Safari with iOS 9 (did not upgrade to 10 yet) and in the Chrome emulator. In the emulator/inspector I saw that an ad did load in the amp-sticky-ad tag but .amp-sticky-ad-layout still has a visibility:hidden !important which was preventing it from being seen. In the OP's sample link, I was able to see the same behavior in their amp-sticky-ad (they also have a style on their amp-sticky-ad which sets opacity:0 which I don't have on my development page). Disabling the visibility:hidden (and opacity:0) revealed the ad. I'm assuming whatever triggers the ad to be shown isn't properly setting the visibility of the amp-sticky-ad. (While scrolling down, I do see that body styles are modified to include border-bottom: 50px solid transparent; so I believe the scroll trigger is working as it adds the buffer space for the ad unit - it just doesn't make the ad visible.)

[mhchu]

Upon further testing, I see that the visible attribute is never added to amp-sticky-ad after the scroll is triggered and the border-bottom: 50px is added to body. In addition, the class amp-sticky-ad-loaded is not added to amp-sticky-ad. (Both the visible attribute and the class are properly added if the page is reloaded after scrolling down.)

[zhouyx]

Thanks @mhchu for the info. This is caused by sticky-ad never receive ad loaded event and thus never apply the amp-sticky-ad-loaded class. We will have a fix for it soon.

[jridgewell]

Hm, I wonder if it's due to https://github.com/ampproject/amphtml/pull/3595...

[dvoytenko]

@zhouyx It's somewhat unlikely that an amp-ad is loaded independently since it's shown my the amp-sticky-ad. There reason is most likely something else.

That being said, I don't see why an independent loading would cause a risk condition. The state of an amp-ad is always known.

[zhouyx]

Well another fix I think would be check the amp-ad laid out state before I call #scheduleLayout for it.

[dvoytenko]

We probably just need to add whenFirstLayout() promise-based API to custom-element.

[dvoytenko]

But first we need to confirm the reason why this bug is happening. Even if the reasoning here is right, this is most likely a symptom and not the underlying reason.

[zhouyx]

Sure, I'll investigate more

[zhouyx]

I researched more and start to agree with @jridgewell. #3595 seems to be a reason.
Here's what I found, after amp-sticky-ad #buildCallback, we remove amp-sticky-ad display:none and set ownership of amp-ad. Then in our resources-impl discoverWork it schedule layout for amp-sticky-ad and succeed. Because amp-ad inside also get display:block now, our resources move on to try to schedule layout for amp-ad, and is not blocked by the #hasOwner().

[dvoytenko]

Why do we remove display:none in build callback when it's really not visible until layout?

[zhouyx]

Oh! that's a long time ago issue. I compromise with remove display:none in buildCallback to solve the ios blink issue.

[zhouyx]

#getResourcesInViewport is kind of related to this issue because that is where #hasOwner() is first called for amp-ad before sticky-ad #buildCallback.
https://github.com/ampproject/amphtml/blob/master/src/service/resources-impl.js#L248
we call hasOwner() before isDisplayed() here