ampproject / amphtml

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

amp-carousel / amp-img fallback failure on Safari #16615

Closed AcidRaZor closed 6 years ago

AcidRaZor commented 6 years ago

I have WebP images, with a fallback to JPG on the amp-img within a carousel.

Even though the images load fine the first time (falling back to JPG), when you scroll in the carousel while using Safari on iOS, the images start disappearing/appear broken

cathyxz commented 6 years ago

I think this is a dupe of https://github.com/ampproject/amphtml/issues/16588, which should be fixed by outstanding PR https://github.com/ampproject/amphtml/pull/16404, hence closing for now in favor of #16588 for tracking.

AcidRaZor commented 6 years ago

@cathyxz Sorry but this isn't a dupe. If I felt it was, I would have posted a comment on the other thread. The resize is never triggered. When someone scrolls on the amp-carousel, amp-img's (webP with fallback to JPG) shows up as "broken". I've tested multiple browsers now as well on mobile devices (as I don't see this behavior happen on desktop)

So unless the amp-carousel is triggering the srcset for some reason when the browser never resizes and height/width are set, I don't see how it's 100% correlated/related.

This is now the 2nd production bug introduced by the AMP initiative. We've spend 8 months converting and fine-tuning our site to utilize AMP. I'm pretty confident I can get the same render time/speed benefits by some different means, as it's just too risky implementing anything in AMP, go through QA/UAT and end up 2 months later with an inexplicable bug on production with 0 (zero) manageability from our side to target a previous version of AMP that's not broke AF and would not affect our UX so badly as it did.

I'm officially recommending to my company to switch to something we can control in an environment that won't cost us money, both in revenue and dev time (their suggestion was for our QA department and some select user/testers to test the site every day just in case, which is a waste of time and resources)

The project is a good idea, poorly executed and 3 years down the line still too much of an infant to be taken seriously.

I wonder how much the e-commerce sites are affected by each time you push a bug out. Why waste my time, my QA team and UAT process on end-users when it's just going to break for no reason 2 months down the line.

/vent

cathyxz commented 6 years ago

Hi @AcidRaZor, sorry for missing your comment earlier.

What I meant to say was that it is a manifestation of the same underlying issue (that a refactor to get rid of our srcset polyfill to propagate native srcset instead seems to have broken fallback images), not to say that you filed this issue without reading the other one. Sorry that my language there was imprecise.

when you scroll in the carousel while using Safari on iOS, the images start disappearing/appear broken

I created test case https://codepen.io/cathyxz/pen/ajNJqV with webp images inside <amp-carousel> and tested this on Safari with a real iPhone X and iPhone 6, 7 simulators via Saucelabs. The issue doesn't seem to reproduce for either <amp-carousel type="slides"> or <amp-carousel type="carousel">, on production or canary.

Do you by chance know how to reproduce this bug?

aghassemi commented 6 years ago

I am also not able to reproduce the carousel issue. Both https://codepen.io/cathyxz/pen/ajNJqV and a pure srcset version of it (https://codepen.io/aghassemi/details/QBEOoJ/) work fine for me in production (specially given the srcset fallback bug is still in production, I am not sure if this issue is a regression yet).

@AcidRaZor Do you have a Url to help us repro?

@AcidRaZor Regarding your comments, thanks for the feedback and I'm sorry that your experience wasn't to your expectations. There are lots of tradeoffs between auto-updates and fixed versioning when it comes to software and in AMP we have chosen the former believing its benefits for end users overweights its cons overall. This is the same approach ever-green browsers are taking.

Of course, one of the cons in the ever-green approach is introduction of bugs. For this issue in particular, let's hold judgement until we know the root cause. Regarding #16588, yes it was a regression however we understand that there are lots of sites using AMP and we knew it was somewhat a risky change but an important one to make. We did take a calculated approach and rolled the change slowly over more than a month. Starting with 1% of sites and slowing ramping it up to 100% over 4-5 releases. This ensured any high-impact breakage would have been caught early on. #16588 was reported once after we were in 100% for a week and is not impacting much of AMP pages, unfortunately you were in the small group that did get impacted and I sympathize with your frustration. The fix is ready and will be in production soon. Of course there are workarounds at the moment as well for the short term.

erwinmombay commented 6 years ago

This is a high priority issue but it hasn't been updated in awhile. @cathyxz Do you have any updates?

cathyxz commented 6 years ago

Hi @AcidRaZor, I'm closing this for now because we weren't able to reproduce the issue. Feel free to reopen if needed.

bohdan-berezhniy commented 4 years ago

Hi, I have reproduced the issue you should use <amp-carousel with type="slides" and layout="responsive" https://opt.demo.magefan.top/amp-img/amp-img.html