🐛 [amp-carousel 0.1] Fix snapping and closing race #32695
Conversation
micajuine-ho
force-pushed
the
nanSlide
branch
from
4fc3ab5
to
c255e0e
Compare
| const impl = await ampSlideScroll.getImpl(); | ||
| const devErrorSpy = env.sandbox.spy(dev(), 'error'); | ||
|
|
||
| impl.showSlideAndTriggerAction_(NaN) |
There was a problem hiding this comment.
Might be extra safe to add another unit test that also passes Infinity or similar.
micajuine-ho
force-pushed
the
nanSlide
branch
from
c255e0e
to
9e77182
Compare
micajuine-ho
force-pushed
the
nanSlide
branch
from
545e810
to
bbb4999
Compare
micajuine-ho
force-pushed
the
nanSlide
branch
from
bbb4999
to
7682f17
Compare
| @@ -686,7 +686,10 @@ export class AmpSlideScroll extends BaseSlides { | |||
| */ | |||
| showSlide_(newIndex) { | |||
| const {noOfSlides_} = this; | |||
| newIndex = dev().assertNumber(newIndex); | |||
| if (isNaN(newIndex)) { | |||
There was a problem hiding this comment.
From the stack trace, this looks like we're fixing the issue in the wrong place. We're getting a NaN from getNextSlideIndex_ (called from updateOnScroll_).
There was a problem hiding this comment.
Thanks for the catch. I'll add the check there as well.
micajuine-ho
force-pushed
the
nanSlide
branch
2 times, most recently
from
6dccb24
to
49d5468
Compare
micajuine-ho
changed the title
| @@ -521,6 +521,11 @@ export class AmpSlideScroll extends BaseSlides { | |||
| * @return {number} a number representing the next slide index. | |||
| */ | |||
| getNextSlideIndex_(currentScrollLeft) { | |||
| // Addresses race where slideWidth is 0, due to being closed | |||
There was a problem hiding this comment.
I'm not sure I understand what it means for the carousel to be closed?
There was a problem hiding this comment.
Within the lightbox gallery sense, you open the carousel by clicking on the lightbox image and then close it by clicking the image in the carousel.
There was a problem hiding this comment.
Oh I see. In that case perhaps we can update this to hidden?
micajuine-ho
force-pushed
the
nanSlide
branch
from
49d5468
to
c6a17c4
Compare
Closes #28050
For
amp-carousel's in a lightbox image gallery, a race occurs when the user drags the slide slightly (but not enough to move to the next slide), and then clicks the image, which closes the carousel.Dragging the slide causes the snapping feature to kick in:
customSnap_()which eventually callsupdateOnScroll_()=>getNextSlideIndex_().At this point, we
NaNdue to0/0, since thethis.slideWidth_has been updated to be 0, b/c we're closing the carousel.