Skip to content
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-carousel : images load intermittently #27036

Closed
anuragphadke opened this issue Feb 29, 2020 · 10 comments · Fixed by #30426
Closed

amp-carousel : images load intermittently #27036

anuragphadke opened this issue Feb 29, 2020 · 10 comments · Fixed by #30426

Comments

@anuragphadke
Copy link

anuragphadke commented Feb 29, 2020

What's the issue?

I am using amp-carousel-0.2.js, however, images in the tag fail to load 1/5 attempts.

How do we reproduce the issue?

  1. Visit: https://beta.webfast.co/amp/image-carousel.html
  2. Press cmd-shift-r (hard refresh) and you will notice that the image in the carousel only loads intermittently.

Another way to produce, start Chrome on iOS in incognito mode:

  1. Visit: https://beta.webfast.co/amp/image-carousel.html, images don't load at first try.
  2. reload the page.
  3. images will now load.
    The above is repeatable everytime u close all incognito windows on Chrome on iOS.

What browsers are affected?

All browsers on mobile.

Which AMP version is affected?

AMP version: 2002192257490
AMP carousel: https://cdn.ampproject.org/v0/amp-carousel-0.2.js

Is this a new issue? Or was it always broken? Paste the version of AMP where you saw this issue.
Experiencing for first time related to the above page.

@anuragphadke
Copy link
Author

@alanorozco : any possible workarounds that I can use until this gets fixed?

@rsmith4321
Copy link

rsmith4321 commented Mar 5, 2020

I seem to be having the exact same issue. I'm very consistently getting missing images in the carousel. After scrolling through the entire gallery using the loop attribute and repeating the image will appear. This doesn't happen with v0.1. It seems to happen randomly I think it's an image lazyloading issue. Watching the network tab in chrome I see the image load, but it's blank in the carousel. Also I thought this was a Chrome issue only, but I've seen it happen once on iOS as well when opening a private tab. I'm going to switch back to v0.1 for now, but if anyone wants to test this live I can make a test page.

@rsmith4321
Copy link

I'm done some more testing on this issue. It seems to be related to the amp-carousel that is generated by Jetpack. Can someone do some testing with amp-carousel v2 and the jetpack gallery? I've switched to using the code generated from a standard wordpress gallery and just adding the loop and autoplay attributes to get the same functionality as the Jetpack gallery. Adding the code manually I no longer get missing images in the slideshow.

@westonruter
Copy link
Member

westonruter commented Sep 17, 2020

I can reliably regularly (if yet sporadically) reproduce this issue on: https://www.ryansmithphotography.com/

Here's a screen capture showing the problem after several reload attempts: https://youtu.be/nBtjBtai8s0

This may be a similar issue to #26952.

@jridgewell @choumx Is this Resources bug? Will it be fixed by IntersectionObserver?

@rsmith4321
Copy link

rsmith4321 commented Sep 17, 2020 via email

@jridgewell
Copy link
Contributor

Hi @rsmith4321, thanks for the report!

After investigating, I'm convinced this is caused by a bug in newly launched experiment. The bug fix was merged in #30188, and will rollout to production tomorrow.

@rsmith4321
Copy link

I'm still able to recreate the issue by doing an empty cache reload. I've also noticed this issue for many months when I use the jetpack carousel so I don't think it's anything recent. I've never seen the issue adding an amp-carousel in an html block.  Thanks.
https://drive.google.com/file/d/1Fr7InOAbm5Ut_BLqZhLcEfpNB25ql7mg/view?usp=sharing

@jridgewell
Copy link
Contributor

jridgewell commented Sep 28, 2020

Wow, this is strange. The amp-img failed to upgrade to the AmpImg BaseElement. We're still using the ElementStub. In fact, it's already "laid out":

Screen Shot 2020-09-28 at 4 56 22 PM

Reopening, setting to a higher priority. /cc @dvoytenko

@jridgewell jridgewell reopened this Sep 28, 2020
@jridgewell jridgewell assigned jridgewell and unassigned alanorozco Sep 28, 2020
jridgewell added a commit to jridgewell/amphtml that referenced this issue Sep 29, 2020
When an unlayout (eg, when carousel swipes away) occurs before an unlayout, we can get into a broken state where we _think_ we're fully laid out but in reality we've performed the unlayout.

There's still race conditions in each component, since they need to track their own layout/unlayout state. We should quickly migrate to Bento where this isn't an issue.

Fixes ampproject#27036
Partial ampproject#13838
jridgewell added a commit to jridgewell/amphtml that referenced this issue Oct 2, 2020
When an unlayout (eg, when carousel swipes away) occurs before an unlayout, we can get into a broken state where we _think_ we're fully laid out but in reality we've performed the unlayout.

There's still race conditions in each component, since they need to track their own layout/unlayout state. We should quickly migrate to Bento where this isn't an issue.

Fixes ampproject#27036
Partial ampproject#13838
jridgewell added a commit that referenced this issue Oct 9, 2020
* Fix race condition with layout -> unlayout

When an unlayout (eg, when carousel swipes away) occurs before an unlayout, we can get into a broken state where we _think_ we're fully laid out but in reality we've performed the unlayout.

There's still race conditions in each component, since they need to track their own layout/unlayout state. We should quickly migrate to Bento where this isn't an issue.

Fixes #27036
Partial #13838

* Rename variable

* Throw cancellation errors where appropriate

* Review fixes

* Fix dep requirement

* Fix sourcemap check

* Fix tests

* Only allow missing signal param in test mode

* Do not report missing abortController

Since we're clearing it out after layout completes, it won't exist when we eventually unlayout

* Toggle amp-analytics visibility _after_ layout

* Fix method call

* Fix test

* lint

* Fix test
@rsmith4321
Copy link

rsmith4321 commented Oct 12, 2020

Is there is way to know when this update will be live? Do we have to wait for a version 0.3 of the amp-carousel? Thanks!

@jridgewell
Copy link
Contributor

It will be live in beta (1% of users) tomorrow afternoon, and production (100%) next Tuesday.

ed-bird pushed a commit to ed-bird/amphtml that referenced this issue Dec 10, 2020
* Fix race condition with layout -> unlayout

When an unlayout (eg, when carousel swipes away) occurs before an unlayout, we can get into a broken state where we _think_ we're fully laid out but in reality we've performed the unlayout.

There's still race conditions in each component, since they need to track their own layout/unlayout state. We should quickly migrate to Bento where this isn't an issue.

Fixes ampproject#27036
Partial ampproject#13838

* Rename variable

* Throw cancellation errors where appropriate

* Review fixes

* Fix dep requirement

* Fix sourcemap check

* Fix tests

* Only allow missing signal param in test mode

* Do not report missing abortController

Since we're clearing it out after layout completes, it won't exist when we eventually unlayout

* Toggle amp-analytics visibility _after_ layout

* Fix method call

* Fix test

* lint

* Fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants