amp-lightbox-gallery - unexpected behavior with thumbnails

[SimonHogstromRonbol]

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]

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

[kristoferbaxter]

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

[caroqliu]

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:

amphtml/extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js

Line 798 in f9df1c0

.then((carousel) => carousel.goToSlide(this.currentElemId_));

(3) Carousel is detached from the dom and resets its internal slide value:

amphtml/extensions/amp-carousel/0.1/slidescroll.js

Line 393 in f9df1c0

this.slideIndex_ = null;

(4) Carousel is re-attached to the dom, and lays out to the first slide by default:

amphtml/extensions/amp-carousel/0.1/slidescroll.js

Lines 366 to 368 in f9df1c0

	if (this.slideIndex_ === null) {
	this.showSlide_(this.initialSlideIndex_);
	} else {

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:

amphtml/extensions/amp-carousel/0.2/amp-carousel.js

Lines 248 to 251 in f9df1c0

	unlayoutCallback() {
	this.childLayoutManager_.wasUnlaidOut();
	return true;
	}

amphtml/extensions/amp-base-carousel/0.1/child-layout-manager.js

Lines 432 to 440 in f9df1c0

	wasUnlaidOut() {
	this.laidOut_ = false;
	this.monitorChildren_(this.laidOut_);
	for (let i = 0; i < this.children_.length; i++) {
	this.triggerLayout_(this.children_[i], false);
	this.triggerVisibility_(this.children_[i], false);
	}
	}

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]

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.