amp-lightbox-gallery - unexpected behavior with thumbnails

SimonHogstromRonbol commented 3 years ago

Description

When you click a thumbnail to open an amp-lightbox-gallery, the first image in the gallery will show up, rather then the image associated with the clicked thumbnail.

Note that this was something we only recently discovered on one of our sites using amp-lightbox-gallery, and all our sites using amp-lightbox-galleries are affected.

This was discovered by a team member on the 21st of july, and have not been a problem in the past.

This issue is only present in v0.1 of the component, and seems to be fixed in v1.0

Reproduction Steps

https://amp.dev/documentation/examples/e-commerce/hotel/preview/?format=websites

This is reproduceable in the hotel demo on amp.dev

if you click any image, it will scale up, and then switch to the first image in the gallery.

Relevant Logs

No response

Browser(s) Affected

Chrome, Firefox, Safari, Edge

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

No response

SimonHogstromRonbol commented 3 years ago

@kristoferbaxter @caroqliu As promised on slack, I've filed an issue about the unexpected behavior we experienced using amp-lightbox-gallery.

kristoferbaxter commented 3 years ago

Adding @caroqliu to the issue. We might want to map this over to @alanorozco instead though.

caroqliu commented 3 years ago

What you can do for now

As a bandaid fix, I would recommend importing amp-carousel-0.2.js instead of 0.1 (the default version used by amp-lightbox-gallery) for now. Here is a jsbin of the hotel demo with the 0.2 script included.

One downside of this approach is that this will cause startLayout called but not LAYOUT_SCHEDULED currently: 2 errors in the console due to an unrelated bug with installing the amp-image-viewer extension. If this is tolerable, I'd recommend it.

(As a side note: I wonder if the above errors could be related to #30616, so linking that issue as well.)

Investigation notes

As far as this issue goes, it looks like the carousel is being detached and re-attached by the lightbox gallery somehow. The rough flow of logic is: (1) Open the lightbox to the clicked target (2) Lightbox advances the carousel to the correct target: https://github.com/ampproject/amphtml/blob/f9df1c02adfbead3707f818c94ceadfb61e27d7b/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js#L798 (3) Carousel is detached from the dom and resets its internal slide value: https://github.com/ampproject/amphtml/blob/f9df1c02adfbead3707f818c94ceadfb61e27d7b/extensions/amp-carousel/0.1/slidescroll.js#L393 (4) Carousel is re-attached to the dom, and lays out to the first slide by default: https://github.com/ampproject/amphtml/blob/f9df1c02adfbead3707f818c94ceadfb61e27d7b/extensions/amp-carousel/0.1/slidescroll.js#L366-L368

I think our best bet for resolving this issue is eliminating 3 & 4. The reason the amp-carousel:0.2 works is that, while it is also being unlaid out and relaid out, for better or worse it does not reset its state in the unlayout step: https://github.com/ampproject/amphtml/blob/f9df1c02adfbead3707f818c94ceadfb61e27d7b/extensions/amp-carousel/0.2/amp-carousel.js#L248-L251 https://github.com/ampproject/amphtml/blob/f9df1c02adfbead3707f818c94ceadfb61e27d7b/extensions/amp-base-carousel/0.1/child-layout-manager.js#L432-L440

Two additional options that would prevent this problem from occurring without solving it at its root:

We could prevent the state from being reset in the unlayoutCallback described in (3), however that may have unpredictable consequences.
It's worth noting that amp-lightbox-gallery has an experiment in place to install amp-carousel:0.2 by default instead of 0.1. We could consider ramping up that experiment and safely launching it to avoid having this one component integrate with two versions of carousel, though that would not solve this problem at its root.

I'll dig further into why the carousel is being detached and update this issue accordingly.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

ampproject / amphtml