search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

amp-next-page bugs - fallback/placeholder rendering #30641

Closed andyblackwell closed 3 years ago

andyblackwell commented 4 years ago

What's the issue?

We first noticed yesterday that pages pulled in via amp-next-page (both v0.1 and v1.0) are rendering very buggy. Ad placeholders are behind and stretched across the entire length of the article, and image fallback/placeholders are not being removed and stacked above the image. In one case a modal is also auto-popping (which shouldn't), and isn't dismissable. This all only seems to happen when hosted on google's cdn.

How do we reproduce the issue?

emulate mobile and load these urls and scroll down until the first amp-next-page loads to reproduce:

https://www.google.com/amp/s/amp.travelawaits.com/2547955/The-Best-Greek-Islands-To-Visit/ v0.1 ^ can see the ad/image issue here

https://www.google.com/amp/s/www.eastmojo.com/nagaland/2020/10/05/manipur-covid-19-death-count-climbs-to-74-with-3-more-cases v1.0 ^ found in the wild, also can see the ad/image issue

https://www.google.com/amp/s/amp.cinemablend.com/news/2556691/why-one-matrix-4-actor-thinks-lana-wachowski-will-change-the-industry v0.1 ^ ad/image issues, and this one also oddly auto-pops a modal on next-page (which shouldn't happen) that you also can't dismiss

What browsers are affected?

All browsers

Which AMP version is affected?

We first noticed yesterday, unsure if it was happening prior to yesterday. current AMP version: 2009252320001

andyblackwell commented 4 years ago

You can see below the grey ad placeholder expands outside the ad div to the top and bottom of the article, rendering behind it.

Screen Shot 2020-10-13 at 12 17 25 PM

zhouyx commented 4 years ago

@andyblackwell Thanks for reporting the issue.

I'm having trouble reproducing this issue. The ad placeholder all has size zero when I investigated the element. And I am investigating the page within the SERP viewer, under google.com I'm also not able to see the auto pops up modal you mentioned.

More information on how to reproduce is appreciated. In addition, we're only supporting amp-next-page 1.0, as amp-next-page 0.1 is under experiment and not officially launched.

andyblackwell commented 4 years ago

Looks like the amp version I'm getting is different now: 2010010034001 And things are looking normal again for me as well

Any chance the buggy version is just running at a percent of traffic and will come back? Like it's being tested and may be merged in later? Or does this newer version # mean we're good moving forward? Is there a way to see if 2009252320001 is still being used on live traffic?

zhouyx commented 4 years ago

I'm not aware of any changes to <amp-next-page> or ad placeholder between 2009252320001 and 2010010034001

While 2010010034001 is the new Stable version, 2009252320001 will be promoted to our LTS version soon, so it will be used on live traffic by sites that choose to use LTS.

Let me test with 2009252320001 version

zhouyx commented 4 years ago

@andyblackwell I tried again with 2009252320001 version, still couldn't reproduce the issue.

As shown, the ad size is correct, and the fallback placeholder has size 0.

image

zhouyx commented 4 years ago

FYI: I tested using the 1.0 url you provide https://www.google.com/amp/s/www.eastmojo.com/nagaland/2020/10/05/manipur-covid-19-death-count-climbs-to-74-with-3-more-cases

If possible, can you opt in to 012009252320001 version via the Advanced tab in https://www-eastmojo-com.cdn.ampproject.org/experiments.html And verify if that issue still exist. Thank you

andyblackwell commented 4 years ago

Getting the same as you, is working fine now on both v0.1 and v1.0. Is strange that it was broken for a day, but fixed itself within a couple hours of my report 🤷 We had several reports of the broken behaviour, so it wasn't just me. Maybe there was a bad cached version out there or something that happened to get cleared?

Guess we can close this out

zhouyx commented 4 years ago

Glad to hear the issue is gone. Agreed this very strange. I checked our release history, only 2009190410000 was out for only one day (from 9/29-9/30), but the time didn't match.

Definitely let us know if we're seeing this again. Thanks

Cauchon commented 3 years ago

@zhouyx We are seeing this pop up on CNET, ZDNet and Gamespot sites.

For example on https://www.google.com/amp/s/www.cnet.com/google-amp/news/new-airpods-max-apple-549-dollar-over-ear-headphones-all-the-things-you-could-buy-instead/

Screen Shot 2020-12-10 at 9 12 21 AM
zhouyx commented 3 years ago

Hi @Cauchon it looks like the placeholder has the correct size in the screenshot. Could you clarify the issue. Thanks

Cauchon commented 3 years ago

Thanks for the reply, Yuxuan. @andyblackwell is probably best to explain the issue we're both seeing with amp-next-page. He put more details in #23983 - basically all ad placeholders are the entire height of the amp-next-page doc.

zhouyx commented 3 years ago

@andyblackwell do you see the issue consistently? I couldn't reproduce the issue, the ad size seems correct to me. Could you provide a url or the steps to reproduce. Thanks

andyblackwell commented 3 years ago

It looks to be fixed for me now and seeing it fixed on CNET, is there any chance there was an amp deploy in the last few hours? That's what fixed it last time, and I'm suspicious there's an underlying caching issue that a fresh deploy fixes.

zhouyx commented 3 years ago

@estherkim any thoughts?

Cauchon commented 3 years ago

For me, it does seem to be mostly fixed on CNET but some of my coworkers are still seeing the issue.

I am able to reproduce on ZDNet inconsistently: https://www.google.com/amp/s/www.zdnet.com/google-amp/article/safer-networks-at-home-working-remotely-in-2021/

estherkim commented 3 years ago

Re: fresh deploys - we didn't release anything today.

The CNET screenshot shows cdn.ampproject.org/rtv/012011252111002 which matches the stable version that was deployed yesterday, 2011252111002

edit - we just promoted an opt-in beta 2012050452002 to 1%... so technically we had a deployment today.

zhouyx commented 3 years ago

Hmm this is really weird.

I'm suspicious there's an underlying caching issue that a fresh deploy fixes.

I agree this could be the reason. Another thing I realize is that the amp-loader script was loaded lazily, wonder if there could be a race condition between the two scripts. Have we tried disabling cache in browser and can we reproduce the issue?

Cauchon commented 3 years ago

@zhouyx Confirming that if I do still see the issue occurring if I disable cache via the Chrome DevTools --> Network tab --> Disable cache.

Screen Shot 2020-12-11 at 1 11 27 PM

When this issue occurs, it also seems to cause amp-analytics to not get fired on the amp-next-page.

andyblackwell commented 3 years ago

we started seeing this again today, FYI

Cauchon commented 3 years ago

Seeing it again on CNET as well.

andyblackwell commented 3 years ago

Same, happening again for us as well

amclarenRV commented 3 years ago

hey @andyblackwell I read this related/ connected post and wanted to check if its a possible cause in our case

https://github.com/ampproject/amphtml/pull/25629

colintdf commented 3 years ago

We have had big problems with AMP Next Page on our sites as we've rolled out starting at the end of last year. The Google Cached version of the pages seem to be ignoring the next-page-hide tags in some instances and removing/changing styles on next pages causing lead images to break outside of the site rails.

The functionality is fine on our own hosted versions of the pages so something in the Google Caching is breaking the functionality. The problem also doesn't appear viewing the pages via desktop browsers but does affect iPhone Safari and Android Chrome in the same way.

We're on version 1.0 and noticed the problems mostly yesterday (5th January) as we rolled it out to our biggest site. The problems were also present on our initial launch site and we're unsure as to when they started happening.

We have now disabled amp-next-page while this is investigated however here are a couple of screen recordings of the issue.

Hosted: https://user-images.githubusercontent.com/23098228/103785056-a9a9da80-5032-11eb-8a5b-5e31a35cef36.mp4

Google Cached: https://user-images.githubusercontent.com/23098228/103785081-b0385200-5032-11eb-9aab-de9fb20166e3.mp4

andyblackwell commented 3 years ago

@amclarenRV since I first saw this in early/mid October, I'd guess those later changes aren't related. I'm still leaning more toward caching issues, since the bugs only happen on the google cdn and seem to disappear for a while after a new amp deploy.

kristoferbaxter commented 3 years ago

Closing issue in favor of #31951.

The Google team has temporarily rolled back a release of module scripts from the Google AMP Cache, we'll use #31951 to track fixing the underlying issue.