Amphtml: AMP-IMG not loading when amp-lightbox[scrollable] > amp-list > amp-img

[ikselll]

What's the issue?

BUG - amp-img are not loaded when they are inside amp-list and amp-list is inside amp-lightbox and amp-lightbox has scrollable attribute.

How do we reproduce the issue?

Steps to reproduce:

1. Open https://gist.github.com/ikselll/d2c54f15ceb355f7636e699512c1f231
2. copy/paste in the amp-playground https://ampbyexample.com/playground/
3. Click "open lightbox 3" button
4. The titles are visible but images are not.

TIP: Removing scrollable attribute from amp-lightbox fix it.

What browsers are affected?

Firefox 60.0.1, 64bit, windows 10

Which AMP version is affected?

1527021682973

[mcmd]

Safari Version 11.1 (13605.1.33.1.4)
Chrome Version 66.0.3359.181 (Official Build) (64-bit)
Opera 53.0.2907.68

All on OSX 10.13.4 do not present this issue.

Firefox 61.0b7 (64-bit) (on the same OS) does present the same issue.

[ericlindley-g]

Strange that removing the "scrollable" attribute fixes this—FWIW I've seen another report with the same issue where they weren't using amp-list, so it seems independent of the amp-list issue (thought that may be adding additional delay).

[juliavallina]

I have a similar example that reproduces this issue: amp-img are not loaded inside an amp-lightbox with scrollable attribute. This example doesn't have an amp-list, it uses amp-bind and setState to manipulate element's visibility inside the lightbox.

The code is this jsfiddle https://jsfiddle.net/evg6h532/29/ and you can access the example using this url https://lightbox-amp-test.netlify.com

How to reproduce the issue

1. Go to this URL https://lightbox-amp-test.netlify.com
2. Click "Contáctanos" button
3. Click "Ver otros números" button. It calls setState and toggles the visibility of some divs
4. A list of countries and phones are shown, but the flag before each country is not loaded (for this example I just use the same german flag for all countries)
5. Once I scroll or interact with the page, the request for the images is triggered and they are loaded.

Also, removing scrollable attribute fix this issue.

What browsers are affected?

I have tested it in iOS desktop (Chrome, Safari, Opera y Firefox) and mobile (Chrome y Safari).

Hope it helps!

[cathyxz]

This bug is interesting. It actually works fine in Chrome desktop (the flags will load), but repros in Chrome mobile emulators. <amp-lightbox> has logic to take over resource scheduling of its children, and checks on
lightbox open and scroll to layout all of its visible children. My guess here is that something is getting miscalculated here:
https://github.com/ampproject/amphtml/blob/36fe72c96317991cd86886f8ddd970036d2aa229/extensions/amp-lightbox/0.1/amp-lightbox.js#L519-L568
with the result being that <amp-lightbox> doesn't think those children are visible on lightbox open, but updates correctly on scroll.

This isn't a trivial bug, but it's relatively isolated (pertains to the isVisible calculation in scrollable lightbox), and could possibly be manageable as a GFI for people with JS experience. It would involve looking into the code referenced above in a Chrome emulator for iPhone / Pixel 2, and checking where the isVisible logic fails and fixing that.

[ampprojectbot]

This issue hasn't been updated in awhile. @cathyxz Do you have any updates?

[danieltxok]

Hi, this is still pending. Any updates? Thanks

[cathyxz]

~I took a look at this and the original sample doesn't repro anymore after you fix the urls. This also no longer repros locally.~ Here's my minimal repro after fixing the urls since ampbyexample is now amp.dev: https://codepen.io/cathyxz/pen/ZZdeab

Nvm, this is a Windows issue, not on OSX. This issue isn't present on Safari/FF/Chrome on Mac OSX. I'll need to grab a Windows device.

[cathyxz]

Another update on this. So per my comment above, this used to repro in Chrome's mobile emulators on macOS I believe, but it currently no longer does. I tried reproducing this on Windows 10 via a Saucelabs VM, it does not repro in FF 66, Chrome 74, but repros in Edge 18. So @dbarcosb do you need this for Edge support on Windows 10?

If you have a better repro than https://codepen.io/cathyxz/pen/ZZdeab, let me know and I'd be happy to look at that too.

[cathyxz]

Okay, this much I'm understanding about the Edge situation. Basically the way this goes is:

1. You open the lightbox, lightbox scans for descendants that are AMP elements and lays them out.
2. amp-list gets laid out, calls fetch, adds all of its children to itself and calls DOM_UPDATE.
3. amp-lightbox gets the DOM_UPDATE, rescans, finds 1 amp-list + n amp-img children, and then calls scheduleLayout on all of them.

On Windows 10 + Edge, the scan that happens after the DOM_UPDATE sees 1 amp-list without the n amp-img children.

[danieltxok]

Hi @cathyxz, thanks so much for looking into this. No, Edge support is not such a big deal here.

However, I still see the unexpected behavior, at least in Chrome. Your copen example works perfectly but it is different than the one @juliavallina mentioned. If you follow her example, the flags are not loaded unless the user interacts.

To show you another example, I modified your codepen into this, by adding a new button inside the lightbox. The list should show only when clicking the button, not before (I do this by toggling visibility after a user action). You can see how in this case not even the image is rendered. Tested in Chrome.

[aghassemi]

same as https://github.com/ampproject/amphtml/issues/14642

[aghassemi]

since it is the same root cause as #14642, deduping