ampproject / amphtml

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

amp-img side scroller does not load offscreen images #24929

Closed p2er closed 4 years ago

p2er commented 4 years ago

What's the issue?

The amp travel demo shows a teaser slider with images. Once in mobile viewport native scrolling is used instead of the usual amp-carousel for slidable content. That allows for different content dimensions due to flowing text on mobile compared to desktop. Unfortunately the images at least one viewport width sidewards in the scrollable container are no longer loaded. This did work in the initial version of the demo.

How do we reproduce the issue?

  1. Download the template: https://amp.dev/static/files/templates/travel.zip
  2. Open templates/travel.amp.html
  3. Reduce browser size to mobile viewport (320) and relaod
  4. Scroll down to "Top adventures near you"
  5. Scroll sidewards to last teaser
  6. Should show image but did not load resource nor render image tag

What browsers are affected?

All browsers. Tested on Chrome and Safari and Safari Simulator-

Which AMP version is affected?

Powered by AMP ⚑ HTML – Version 1909181902540

sparhami commented 4 years ago

If it worked before, I would guess that it was due to the layers experiment (which was at 100% on canary for a while), which was never launched. AMP has never really handled horizontal scrolling containers for loading amp elements.

One restriction for amp-carousel is that the height of the carousel must be known up front, which is enforced by the layouts it supports. I think that could be relaxed, as long as none of the slides is able to resize the carousel dynamically (e.g. has an amp-list that can load more content). Not sure how difficult implementing this would be.

As far as the specific issue of not loading images in a horizontal scrolling container, I don't think this is something we are planning on addressing at the moment.

/cc @jridgewell

jridgewell commented 4 years ago

We plan to fix Resources to detect scrolling and invalidate the position-coordinates of the scrolled elements. When the elements are remeasured, we'll be able to load them.

p2er commented 4 years ago

For now it might be a good idea to update the example templates. We used to focus on amp-carousel in our team until the templates where introduced with the new amp.dev at amp conf. Sparked a lot of discussion on how to solve flexible teaser content without restricting the authors in text length. Happy to hear that @jridgewell keeps this option alive. Any idea on when this option might work again?

jridgewell commented 4 years ago

It's low priority for me since this is the behavior it's had for years. Maybe before the new year?

jridgewell commented 4 years ago

What templates are you talking about?

p2er commented 4 years ago

The example templates: https://amp.dev/documentation/templates/ Especially travel, which we looked at for our own implementation before noticing that the behaviour no longer works.

jridgewell commented 4 years ago

Ah, these are using SVGs to get around the amp-img loading requirements.

p2er commented 4 years ago

The problem is further down at the Top Adventures near you section, where actual images are used in the teasers: https://drive.google.com/file/d/1lAC4lqIItWlANTIVhkuXYO7jdWMTGz2-/view

jridgewell commented 4 years ago

@choumx: Should we prioritize the Resources fix?

dreamofabear commented 4 years ago

How long would this take in your estimation?

jridgewell commented 4 years ago

Maybe a week to make sure tests pass and I don't break amp-carousel.

dreamofabear commented 4 years ago

One week sounds reasonable. πŸ‘

c0b41 commented 4 years ago

any update?

jridgewell commented 4 years ago

Last week was AMP Contributor summit, so no work was done. I have running code, but it still needs to be tested.

c0b41 commented 4 years ago

@jridgewell thanks for the update, i am following :)

jridgewell commented 4 years ago

Initial PR at https://github.com/ampproject/amphtml/pull/25172

sparhami commented 4 years ago

@jridgewell Is this actually fixed? The linked PR seems to depend on a flag, layoutbox-invalidate-on-scroll, that doesn't seem to exist in either the prod or canary config.

c0b41 commented 4 years ago

@sparhami yeah, already fixed and relased see https://github.com/ampproject/amphtml/issues/25319 and also i can confirm fixed my problem..

sparhami commented 4 years ago

From https://github.com/ampproject/amphtml/issues/25397#issuecomment-549680062, it seems like the issue may still be occurring, though I don't see the issue myself.

p2er commented 4 years ago

Does the container need overflow scroll to work? We have an example in place where an animation slides images across the screen that are initially off viewport and still won't load.

jridgewell commented 4 years ago

The linked PR seems to depend on a flag, layoutbox-invalidate-on-scroll, that doesn't seem to exist in either the prod or canary config.

I was going to manually test before setting the experiment percentage.

We have an example in place where an animation slides images across the screen that are initially off viewport and still won't load.

Animations will not work, this only affects overflow elements that emit a scroll event.

sparhami commented 4 years ago

The linked PR seems to depend on a flag, layoutbox-invalidate-on-scroll, that doesn't seem to exist in either the prod or canary config.

I was going to manually test before setting the experiment percentage.

Should we leave the issue open until then to better communicate the current status?

Gaitz commented 4 years ago

From #25397 (comment), it seems like the issue may still be occurring, though I don't see the issue myself.

Hi @sparhami and thanks to your response so quickly.

From https://github.com/ampproject/amphtml/issues/25397#issuecomment-549977803, I gather that the way to reproduce the issue was wrong.

I made a video to reproduce the issue on my demo page. ezgif-4-da1f15573c92

There are some more complex pages with similar issue, amp-img do not load on scroll horizontally, and I assume that my simple demo page have same issue.

Should I wait for pr "Enable layoutbox invalidate onscroll experiment in Canary" #25453 ?

sparhami commented 4 years ago

From #25397 (comment), it seems like the issue may still be occurring, though I don't see the issue myself.

Hi @sparhami and thanks to your response so quickly.

From #25397 (comment), I gather that the way to reproduce the issue was wrong.

I made a video to reproduce the issue on my demo page. ezgif-4-da1f15573c92

There are some more complex pages with similar issue, amp-img do not load on scroll horizontally, and I assume that my simple demo page have same issue.

Should I wait for pr "Enable layoutbox invalidate onscroll experiment in Canary" #25453 ?

Following those steps I can reproduce the issue. I tried enabling the experiment locally and it still doesn't load the images with the steps from the video. It seems like the experiment is working (it runs a pass after scroll), so I don't expect this to be fixed in Canary once #25453 lands.

jridgewell commented 4 years ago

Found the bug. I we have a quick check that runs before doing the more expensive remeasure checks. If the quick check says there's nothing to do, we don't ever check if something has scrolled.

Adding elementsThatScrolled to the quick check makes everything work as expected.

Gaitz commented 4 years ago

Hi @jridgewell,

I try on latest release 1911070201440, which contains πŸ› Check if any remeasures are necessary when nested scrollers scroll (#25479), but my demo page still not work.

Do we have some other bug fix still work in progress?

Many thanks

jridgewell commented 4 years ago

@Gaitz: it's only enabled in canary. You can opt-into the canary build by going to https://cdn.ampproject.org/experiments.html and turning "AMP Dev Channel" on.

Doing that, your demo page works fine for me.

Gaitz commented 4 years ago

Thanks a lot.

I test on version 1911121900560 "AMP Dev Channel", it works fine. Is this mean I can just wait for version 1911121900560 to be latest release, and all other online pages will work without any other setting?

jridgewell commented 4 years ago

No, the experiment is not enabled in production builds yet, only the canary. We'll need a new PR turning it on for production before it'll be available to all users.

jridgewell commented 4 years ago

Another use case at https://github.com/ampproject/amphtml/issues/25555.

FlorianSW commented 4 years ago

I found this issue when looking for a solution for my problem as well, and it seems my use case is hit by this issue as well (verified by enabling the dev channel and the images load perfectly fine):

I'm currently experimanting on how to support AMP in MediaWiki (https://www.mediawiki.org/wiki/Extension:AcceleratedMobilePages) and in order to have a working (and compliant) version of mediawiki pages, I've changed tables (which are used in MediaWiki a lot for content pages) to display as block and make it horizontally scrollable.

This leads to the problem that images, which are on a far right column, which is initially not visible, not to load once the user scrolls to them. Here's an example AMP page which shows this problem: https://www.droidwiki.org/wiki/Liste_von_Android-Versionen?action=amp

Enabling the dev channel, like I said, fixes this problem. Is there any timeline when this will be released (and enabled) in production? :)

jridgewell commented 4 years ago

Within the next few weeks

raju-jeelaga commented 4 years ago

Hi @jridgewell can you give me any solution for this how to fix this problem....? amp-img inside<img decode tag do not load on scroll horizontally when it is out of initial view port https://monosnap.com/file/r8CeGkNcmRFm8zVQ3DvrywIqqKTEb3

jridgewell commented 4 years ago

You have to wait until the experiment is enabled in production.

jaspersorrio commented 4 years ago

Hi all, facing the same issue here.

Steps to reproduce:

  1. Visit https://eezee.sg/m/brand/loctite on mobile
  2. Try to scroll the categories to the right
  3. Images will not load for cards outside viewport.

It’s been a month since December. So just wanted to check up on the progress of this :)

Thanks!

798F1CC7-5AAA-4DE8-975E-9ACACB96C152

josewhitetower commented 4 years ago

Can anybody try to zoom in out the images. I have the same problem in a vertical carrousel and doing it shows the images, only iOS.

c0b41 commented 4 years ago

@jridgewell any update for this feature?

jridgewell commented 4 years ago

Opened #26430 to launch

Gaitz commented 4 years ago

@jridgewell Thanks a lot, #26430 is released today and everything works fine. πŸŽ‰

awkaw commented 3 years ago

There is a problem again. Only in Firefox (version 83). AMP version 2011070101001

awkaw commented 3 years ago

There is a problem if the images are inside div that are scrolling. Since the window is not scrolling, images are not loaded.

jridgewell commented 3 years ago

@awkaw Can you open a new issue with a URL that demonstrates?