New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AMP Image loading issue on long form story page. #9397

Closed
gregdevs opened this Issue May 17, 2017 · 30 comments

Comments

@gregdevs

gregdevs commented May 17, 2017

What's the issue?

AMP Images don't seem to load after the first few hundred pixels from the top of the browser as you scroll a long form story. Reference > https://www.google.com/amp/abcnews.go.com/amp/International/International/deepdive/syrian-refugee-children-photos-this-is-home-42066342

How do we reproduce the issue?

  1. Google search ABC News This is home
  2. Click First link
  3. As you start scrolling past the first couple of divs, images either show as blank, white space or take a very long time to display. It seems the img src value is showing the correct url to the image. The page validates successfully.

Attached screenshots.

step1
step2
step3

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi May 17, 2017

Member

Some investigations:

1- This only happens if the page is prereneded first via SERP. Refreshing the page or hitting the viewer link directly does not repro the issue.
2- Similar long image pages under same condition do not repro the issue: Search for "top 25 news photos", the Atlantic AMP blue link is prerendered similar to this one but the issue is not producible on that page, there is probably something on that ABC page that triggers the issue.

/to @dvoytenko @jridgewell

Member

aghassemi commented May 17, 2017

Some investigations:

1- This only happens if the page is prereneded first via SERP. Refreshing the page or hitting the viewer link directly does not repro the issue.
2- Similar long image pages under same condition do not repro the issue: Search for "top 25 news photos", the Atlantic AMP blue link is prerendered similar to this one but the issue is not producible on that page, there is probably something on that ABC page that triggers the issue.

/to @dvoytenko @jridgewell

@gregdevs

This comment has been minimized.

Show comment
Hide comment
@gregdevs

gregdevs May 17, 2017

Interesting. Ok, i'll continue to dig in and try to find the cause of the issue. Thanks.

gregdevs commented May 17, 2017

Interesting. Ok, i'll continue to dig in and try to find the cause of the issue. Thanks.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi May 17, 2017

Member

@gregdevs Still an AMP bug regardless, we are just hitting a race condition on that page that we are not hitting on some similar pages.

Member

aghassemi commented May 17, 2017

@gregdevs Still an AMP bug regardless, we are just hitting a race condition on that page that we are not hitting on some similar pages.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi May 17, 2017

Member

/cc @rsimha-amp Something to consider for image diffing infrastructure, how one lands on an AMP page (considering SRP, cache transformations, viewer pre-rendering, all play a role) matters on the final rendered result. Ideally we can account for these (or at minimum start with the common case which is SRP + cache + viewer rather than direct canonical link hit)

Member

aghassemi commented May 17, 2017

/cc @rsimha-amp Something to consider for image diffing infrastructure, how one lands on an AMP page (considering SRP, cache transformations, viewer pre-rendering, all play a role) matters on the final rendered result. Ideally we can account for these (or at minimum start with the common case which is SRP + cache + viewer rather than direct canonical link hit)

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell May 17, 2017

Member

I'm debugging it now. Very close to finding the root cause.

Member

jridgewell commented May 17, 2017

I'm debugging it now. Very close to finding the root cause.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell May 17, 2017

Member

The issue is the images think they're wayyyyyyy further down the page (some 10000px more) than they actually are. If I manually request a remeasure on the images, they get their correct positions, and rendering works properly (they're either inViewport or renderOutsideViewport eligible).

This is caused by an <amp-carousel> element with a ton of elements. Particularly, there are:

  1. An <amp-img> which has 0x0 size
  2. A caption, which has implicit height.

This caption element pushes all the images down. By the time the carousel is "attached", its images have already had their first measure. Then the carousel attaches, and causes all it's children to display: none (like it should, it hasn't built yet). And it also set overflow: hidden, which takes this huge list of images/captions and collapses hides them inside the carousel. Now everything beneath the carousel gets moved up. But they never get told to remeasure.

Member

jridgewell commented May 17, 2017

The issue is the images think they're wayyyyyyy further down the page (some 10000px more) than they actually are. If I manually request a remeasure on the images, they get their correct positions, and rendering works properly (they're either inViewport or renderOutsideViewport eligible).

This is caused by an <amp-carousel> element with a ton of elements. Particularly, there are:

  1. An <amp-img> which has 0x0 size
  2. A caption, which has implicit height.

This caption element pushes all the images down. By the time the carousel is "attached", its images have already had their first measure. Then the carousel attaches, and causes all it's children to display: none (like it should, it hasn't built yet). And it also set overflow: hidden, which takes this huge list of images/captions and collapses hides them inside the carousel. Now everything beneath the carousel gets moved up. But they never get told to remeasure.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi May 17, 2017

Member

Interesting, .i-amphtml-layout-size-defined should have protected against this with its overflow:hidden and applying fixed size before the extension even loads. Is the problem that applyLayout_ is happening too late? I'd have expected no measuring to happen until applyLayout_ is done for all elements.

Member

aghassemi commented May 17, 2017

Interesting, .i-amphtml-layout-size-defined should have protected against this with its overflow:hidden and applying fixed size before the extension even loads. Is the problem that applyLayout_ is happening too late? I'd have expected no measuring to happen until applyLayout_ is done for all elements.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell May 17, 2017

Member

Right, the carousel has not been stubbed yet, so it doesn't have i-amphtml-notbuilt nor .i-amphtml-layout-size-defined yet.

Member

jridgewell commented May 17, 2017

Right, the carousel has not been stubbed yet, so it doesn't have i-amphtml-notbuilt nor .i-amphtml-layout-size-defined yet.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell May 17, 2017

Member

This is another that would be fixed by #9279. @dvoytenko I think we should merge it while we work on Layers.

Member

jridgewell commented May 17, 2017

This is another that would be fixed by #9279. @dvoytenko I think we should merge it while we work on Layers.

@gregdevs

This comment has been minimized.

Show comment
Hide comment
@gregdevs

gregdevs May 17, 2017

Whoa, interesting. I checked another page that is similar content type without the carousel and yes, the images load as expected. https://www.google.com/amp/abcnews.go.com/amp/US/deepdive/disappearing-beaches-sea-level-rise-39427567

gregdevs commented May 17, 2017

Whoa, interesting. I checked another page that is similar content type without the carousel and yes, the images load as expected. https://www.google.com/amp/abcnews.go.com/amp/US/deepdive/disappearing-beaches-sea-level-rise-39427567

@dvoytenko dvoytenko assigned jridgewell and unassigned dvoytenko May 23, 2017

@dvoytenko

This comment has been minimized.

Show comment
Hide comment
@dvoytenko

dvoytenko May 23, 2017

Collaborator

@jridgewell I'd prefer if at this time we did something like this: remeasure everything once stubbing is done. Again, I don't think the #9279 would really address enough of issues here, b/c this not only affects AMP element's children, but it affects siblings. Actually, it looks like it affects siblings more.

I think we already have a relayout-all signals in resources - we may need to trigger it again.

Collaborator

dvoytenko commented May 23, 2017

@jridgewell I'd prefer if at this time we did something like this: remeasure everything once stubbing is done. Again, I don't think the #9279 would really address enough of issues here, b/c this not only affects AMP element's children, but it affects siblings. Actually, it looks like it affects siblings more.

I think we already have a relayout-all signals in resources - we may need to trigger it again.

@adelinamart

This comment has been minimized.

Show comment
Hide comment
@adelinamart

adelinamart Jun 7, 2017

Collaborator

@jridgewell any updates on the comment above? Thanks.

Collaborator

adelinamart commented Jun 7, 2017

@jridgewell any updates on the comment above? Thanks.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Jun 7, 2017

Member

This has to wait for Layers to fix.

Member

jridgewell commented Jun 7, 2017

This has to wait for Layers to fix.

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Jul 6, 2017

Member

We need a faster fix for this.

Could this be the same issue as #10267. Did this repro in Chrome? Would the workaround for #10267 fix this?

Member

cramforce commented Jul 6, 2017

We need a faster fix for this.

Could this be the same issue as #10267. Did this repro in Chrome? Would the workaround for #10267 fix this?

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Jul 6, 2017

Member

Still repros. We need to fix this immediately. I don't see how this can be related to #9279

Member

cramforce commented Jul 6, 2017

Still repros. We need to fix this immediately. I don't see how this can be related to #9279

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Jul 6, 2017

Member

Working on it now. We can do a really dirty fix by requesting remeasures of everything after build/layout. Layers code will do this, though it'll be a bit smarter.

Member

jridgewell commented Jul 6, 2017

Working on it now. We can do a really dirty fix by requesting remeasures of everything after build/layout. Layers code will do this, though it'll be a bit smarter.

@dvoytenko

This comment has been minimized.

Show comment
Hide comment
@dvoytenko

dvoytenko Jul 6, 2017

Collaborator

I don't think remeasures after build/layout would work - no guarantee that siblings are fully stubbed correctly. I think we need to figure out a way to estimate completion of stubbing for everything or by groups and request remeasures based on that. We now do something similar for font, but we need to find a way to do it for stubbing as well, unfortunately.

Collaborator

dvoytenko commented Jul 6, 2017

I don't think remeasures after build/layout would work - no guarantee that siblings are fully stubbed correctly. I think we need to figure out a way to estimate completion of stubbing for everything or by groups and request remeasures based on that. We now do something similar for font, but we need to find a way to do it for stubbing as well, unfortunately.

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Jul 6, 2017

Member

Can't we just debounce a remeasure after calling buildCallback?

Member

cramforce commented Jul 6, 2017

Can't we just debounce a remeasure after calling buildCallback?

@dvoytenko

This comment has been minimized.

Show comment
Hide comment
@dvoytenko

dvoytenko Jul 6, 2017

Collaborator

buildCallback is too late. The right one is applyLayout. And, yes, we can debounce it after each - still a lot of debounces for the initial stage with full remeasure though. But would fix the problem.

Collaborator

dvoytenko commented Jul 6, 2017

buildCallback is too late. The right one is applyLayout. And, yes, we can debounce it after each - still a lot of debounces for the initial stage with full remeasure though. But would fix the problem.

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Jul 6, 2017

Member

Was thinking something like debounce over 1 or 2 seconds.

Member

cramforce commented Jul 6, 2017

Was thinking something like debounce over 1 or 2 seconds.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Jul 6, 2017

Member

buildCallback is too late. The right one is applyLayout.

#buildCallback is called immediately if the document has already been parsed, though. And if it's not parsed, it's likely ok?

Member

jridgewell commented Jul 6, 2017

buildCallback is too late. The right one is applyLayout.

#buildCallback is called immediately if the document has already been parsed, though. And if it's not parsed, it's likely ok?

@dvoytenko

This comment has been minimized.

Show comment
Hide comment
@dvoytenko

dvoytenko Jul 6, 2017

Collaborator

With @cramforce 's change, buildCallback has two problems:

  1. It's highly throttled in prerender.
  2. It waits for the script to download. That means that we are missing out on valuable time to remeasure many elements, especially builtins such as images, before something can get the right measures.

I think we just need, for now, to signal resources manager to debounce remeasures some time after this.layout_ = applyLayout_(this);.

Collaborator

dvoytenko commented Jul 6, 2017

With @cramforce 's change, buildCallback has two problems:

  1. It's highly throttled in prerender.
  2. It waits for the script to download. That means that we are missing out on valuable time to remeasure many elements, especially builtins such as images, before something can get the right measures.

I think we just need, for now, to signal resources manager to debounce remeasures some time after this.layout_ = applyLayout_(this);.

cramforce added a commit to cramforce/amphtml that referenced this issue Jul 7, 2017

Ensure that children of responsive elements cannot overflow element u…
…ntil build time.

Fixes #9397

This change is dangerous and might break things, but I think it is the right thing to do.

Children of `layout=responsive` elements should not be allowed to effect the height of the element. This should probably be extended to `fixed-height`.

@cramforce cramforce self-assigned this Jul 7, 2017

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Jul 7, 2017

Member

This newly raised issue: #10300 is related but for layout=fill. The i-amphtml-layout-fill is applied too late for runtime to understand that the item's size is not 0 in the first layout pass. Interestingly if I copy the i-amphtml-layout-fill CSS from core to <style amp-custom> for that page, everything works; so it may not be that i-amphtml-layout-fill class is added too late, but rather might be that "the injected core CSS is not parsed yet by the time layout starts"...

Member

aghassemi commented Jul 7, 2017

This newly raised issue: #10300 is related but for layout=fill. The i-amphtml-layout-fill is applied too late for runtime to understand that the item's size is not 0 in the first layout pass. Interestingly if I copy the i-amphtml-layout-fill CSS from core to <style amp-custom> for that page, everything works; so it may not be that i-amphtml-layout-fill class is added too late, but rather might be that "the injected core CSS is not parsed yet by the time layout starts"...

@gregdevs

This comment has been minimized.

Show comment
Hide comment
@gregdevs

gregdevs Jul 17, 2017

Thanks everyone. Looks like the issue has been resolved. Much appreciated!

gregdevs commented Jul 17, 2017

Thanks everyone. Looks like the issue has been resolved. Much appreciated!

@choumx choumx added this to the Fixit Week EOQ2 milestone Jul 17, 2017

@choumx choumx removed this from the Pending Triage milestone Jul 17, 2017

@diniscorreia

This comment has been minimized.

Show comment
Hide comment
@diniscorreia

diniscorreia Aug 2, 2017

I understand this might have been fixed in the meantime, but just leaving this here: I'm having what I think is the same issue, except this happens with <amp-img> nested on a <amp-list> (and not <amp-carousel>).

It only happens if the page is prereneded.

diniscorreia commented Aug 2, 2017

I understand this might have been fixed in the meantime, but just leaving this here: I'm having what I think is the same issue, except this happens with <amp-img> nested on a <amp-list> (and not <amp-carousel>).

It only happens if the page is prereneded.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Aug 2, 2017

Member

Link?

Member

jridgewell commented Aug 2, 2017

Link?

@diniscorreia

This comment has been minimized.

Show comment
Hide comment
@diniscorreia

diniscorreia Aug 2, 2017

@jridgewell https://www.google.pt/amp/s/www.publico.pt/2017/08/01/sociedade/noticia/agencia-do-medicamento-porto-concorre-com-mais-18-cidades-1780977/amp Scroll down to the bottom of the page.

Weirdly, it only fails when clicking from a search results page. It loads when opening the prerendered URL directly.

diniscorreia commented Aug 2, 2017

@jridgewell https://www.google.pt/amp/s/www.publico.pt/2017/08/01/sociedade/noticia/agencia-do-medicamento-porto-concorre-com-mais-18-cidades-1780977/amp Scroll down to the bottom of the page.

Weirdly, it only fails when clicking from a search results page. It loads when opening the prerendered URL directly.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Aug 2, 2017

Member

It's not the same issue exactly (the element's cached top offset is correct). I'll look into this to figure it out.

Member

jridgewell commented Aug 2, 2017

It's not the same issue exactly (the element's cached top offset is correct). I'll look into this to figure it out.

@jridgewell

This comment has been minimized.

Show comment
Hide comment
@jridgewell

jridgewell Aug 7, 2017

Member

@diniscorreia: This was an issue with prerendering in a viewer context, and has been fixed.

Member

jridgewell commented Aug 7, 2017

@diniscorreia: This was an issue with prerendering in a viewer context, and has been fixed.

@diniscorreia

This comment has been minimized.

Show comment
Hide comment
@diniscorreia

diniscorreia commented Aug 8, 2017

@jridgewell awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment