🐛 [amp-carousel 0.1] Fix race for setting slideWidth_ (#32866)

* init * fix race * Update extensions/amp-carousel/0.1/slidescroll.js Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
ampproject · Feb 25, 2021 · 25bb462 · 25bb462
1 parent c88fbbb
commit 25bb462
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 0 deletions.
diff --git a/extensions/amp-carousel/0.1/slidescroll.js b/extensions/amp-carousel/0.1/slidescroll.js
@@ -146,6 +146,9 @@ export class AmpSlideScroll extends BaseSlides {
       ? false
       : !isExperimentOn(this.win, 'amp-carousel-chrome-scroll-snap');
 
+    /** @private {boolean} */
+    this.hasFirstResizedOccured_ = false;
+
     this.onResized_ = this.onResized_.bind(this);
   }
 
@@ -331,6 +334,7 @@ export class AmpSlideScroll extends BaseSlides {
    */
   onResized_(size) {
     this.slideWidth_ = size.width;
+    this.hasFirstResizedOccured_ = true;
   }
 
   /** @override */
@@ -349,6 +353,12 @@ export class AmpSlideScroll extends BaseSlides {
       return Promise.resolve();
     }
 
+    // Account for race when onResized_ has not fired before layoutCallback,
+    // since we need slideWidth_ to proceed.
+    if (!this.hasFirstResizedOccured_) {
+      this.slideWidth_ = this.slidesContainer_./*OK*/ clientWidth;
+    }
+
     if (this.slideIndex_ === null) {
       this.showSlide_(this.initialSlideIndex_);
     } else {

diff --git a/extensions/amp-carousel/0.1/test/test-slidescroll.js b/extensions/amp-carousel/0.1/test/test-slidescroll.js
@@ -907,6 +907,19 @@ describes.realWin(
         expect(ampSlideScroll.hasAttribute('loop')).to.be.false;
       });
 
+      it('sets the correct scrollLeft for looping carousel', async () => {
+        const ampSlideScroll = await getAmpSlideScroll(true, 7, false, true);
+        doc.body.appendChild(ampSlideScroll);
+        await ampSlideScroll.buildInternal();
+        const impl = await ampSlideScroll.getImpl();
+        ampSlideScroll.layoutCallback();
+        expect(impl.slideWidth_).to.not.be.null;
+        expect(impl.slideWidth_).to.be.greaterThan(0);
+
+        // I.e. the scrollContainer is centered (not at 0)
+        expect(impl.slidesContainer_.scrollLeft).to.equal(impl.slideWidth_);
+      });
+
       // TODO(#17197): This test triggers sinonjs/sinon issues 1709 and 1321.
       it.skip('should hide unwanted slides when looping', async () => {
         const ampSlideScroll = await getAmpSlideScroll(true);