Fix non-carousel scrolling on iOS 13 reverting to first slide. (#24771)

One effect of these changes is that on slow swipes, the carousel takes longer to switch over to the snapped slide, since the scroll momentum needs to stop before the snap instead of snapping shortly after the touch is released. - Change how the snapping/scroll end is handled with respect to touch events and scroll. - As long as the user is touching, do not snap the position. - Do not snap after swipe is released like before, wait for scrolling to stop. - Increase the timeout after the last scroll event to be more robust.
ampproject · Sep 27, 2019 · c230379 · c230379
1 parent 4bbfe99
commit c230379
Show file tree

Hide file tree

Showing 3 changed files with 244 additions and 66 deletions.
diff --git a/extensions/amp-carousel/0.1/slidescroll.js b/extensions/amp-carousel/0.1/slidescroll.js
@@ -35,7 +35,7 @@ import {triggerAnalyticsEvent} from '../../../src/analytics';
 const SHOWN_CSS_CLASS = 'i-amphtml-slide-item-show';
 
 /** @const {number} */
-const NATIVE_SNAP_TIMEOUT = 135;
+const NATIVE_SNAP_TIMEOUT = 200;
 
 /** @const {number} */
 const IOS_CUSTOM_SNAP_TIMEOUT = 45;
@@ -80,11 +80,8 @@ export class AmpSlideScroll extends BaseSlides {
     /** @private {?number} */
     this.scrollTimeout_ = null;
 
-    /** @private {?number} */
-    this.touchEndTimeout_ = null;
-
     /** @private {boolean} */
-    this.hasTouchMoved_ = false;
+    this.isTouching_ = false;
 
     /**
      * 0 - not in an elastic state.
@@ -254,41 +251,48 @@ export class AmpSlideScroll extends BaseSlides {
    */
   touchMoveHandler_() {
     this.clearAutoplay();
-    if (!this.hasNativeSnapPoints_) {
-      return;
-    }
-    this.hasTouchMoved_ = true;
-    if (this.touchEndTimeout_) {
-      Services.timerFor(this.win).cancel(this.touchEndTimeout_);
+    this.isTouching_ = true;
+  }
+
+  /**
+   *
+   * @param {number} timeout The timeout to wait for before considering scroll
+   *    settled, unless this method is called again.
+   */
+  waitForScrollSettled_(timeout) {
+    if (this.scrollTimeout_) {
+      Services.timerFor(this.win).cancel(this.scrollTimeout_);
     }
+
+    this.scrollTimeout_ = /** @type {number} */ (Services.timerFor(
+      this.win
+    ).delay(() => {
+      this.scrollTimeout_ = null;
+
+      if (this.snappingInProgress_ || this.isTouching_) {
+        return;
+      }
+
+      const currentScrollLeft = this.slidesContainer_./*OK*/ scrollLeft;
+
+      if (this.hasNativeSnapPoints_) {
+        this.updateOnScroll_(currentScrollLeft);
+      } else {
+        this.customSnap_(currentScrollLeft);
+      }
+    }, timeout));
   }
 
   /**
    * Handles touchend event.
    * @private
    */
   touchEndHandler_() {
-    if (this.hasTouchMoved_) {
-      if (this.scrollTimeout_) {
-        Services.timerFor(this.win).cancel(this.scrollTimeout_);
-      }
-      const timeout = this.shouldDisableCssSnap_
-        ? IOS_TOUCH_TIMEOUT
-        : NATIVE_TOUCH_TIMEOUT;
-      // Timer that detects scroll end and/or end of snap scroll.
-      this.touchEndTimeout_ = /** @type {number} */ (Services.timerFor(
-        this.win
-      ).delay(() => {
-        const currentScrollLeft = this.slidesContainer_./*OK*/ scrollLeft;
-
-        if (this.snappingInProgress_) {
-          return;
-        }
-        this.updateOnScroll_(currentScrollLeft);
-        this.touchEndTimeout_ = null;
-      }, timeout));
-    }
-    this.hasTouchMoved_ = false;
+    const timeout = this.shouldDisableCssSnap_
+      ? IOS_TOUCH_TIMEOUT
+      : NATIVE_TOUCH_TIMEOUT;
+    this.isTouching_ = false;
+    this.waitForScrollSettled_(timeout);
   }
 
   /** @override */
@@ -389,36 +393,20 @@ export class AmpSlideScroll extends BaseSlides {
    * @private
    */
   scrollHandler_(unusedEvent) {
-    if (this.scrollTimeout_) {
-      Services.timerFor(this.win).cancel(this.scrollTimeout_);
-    }
-
     const currentScrollLeft = this.slidesContainer_./*OK*/ scrollLeft;
 
     if (!this.isIos_) {
       this.handleCustomElasticScroll_(currentScrollLeft);
     }
 
-    if (!this.touchEndTimeout_) {
-      const timeout = this.hasNativeSnapPoints_
-        ? NATIVE_SNAP_TIMEOUT
-        : this.isIos_
-        ? IOS_CUSTOM_SNAP_TIMEOUT
-        : CUSTOM_SNAP_TIMEOUT;
-      // Timer that detects scroll end and/or end of snap scroll.
-      this.scrollTimeout_ = /** @type {number} */ (Services.timerFor(
-        this.win
-      ).delay(() => {
-        if (this.snappingInProgress_) {
-          return;
-        }
-        if (this.hasNativeSnapPoints_) {
-          this.updateOnScroll_(currentScrollLeft);
-        } else {
-          this.customSnap_(currentScrollLeft);
-        }
-      }, timeout));
-    }
+    const timeout = this.hasNativeSnapPoints_
+      ? NATIVE_SNAP_TIMEOUT
+      : this.isIos_
+      ? IOS_CUSTOM_SNAP_TIMEOUT
+      : CUSTOM_SNAP_TIMEOUT;
+    // Timer that detects scroll end and/or end of snap scroll.
+    this.waitForScrollSettled_(timeout);
+
     this.previousScrollLeft_ = currentScrollLeft;
   }
 
@@ -590,19 +578,9 @@ export class AmpSlideScroll extends BaseSlides {
     this.snappingInProgress_ = true;
     const newIndex = this.getNextSlideIndex_(currentScrollLeft);
     this.vsync_.mutate(() => {
-      // TODO(amphtml): Identify more platforms that require
-      // i-amphtml-no-scroll.
-      if (this.isIos_) {
-        // Make the container non scrollable to stop scroll events.
-        this.slidesContainer_.classList.add('i-amphtml-no-scroll');
-      }
       // Scroll to new slide and update scrollLeft to the correct slide.
       this.showSlideAndTriggerAction_(newIndex);
       this.vsync_.mutate(() => {
-        if (this.isIos_) {
-          // Make the container scrollable again to enable user swiping.
-          this.slidesContainer_.classList.remove('i-amphtml-no-scroll');
-        }
         this.snappingInProgress_ = false;
       });
     });

diff --git a/test/manual/amp-carousel/0.1/slides.amp.html b/test/manual/amp-carousel/0.1/slides.amp.html
@@ -0,0 +1,100 @@
+<!doctype html>
+<html ⚡>
+<head>
+  <meta charset="utf-8">
+  <title>AMP #0</title>
+  <link rel="canonical" href="amps.html" >
+  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+  <link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'>
+  <style amp-custom>
+    body {
+      font-family: 'Questrial', Arial;
+    }
+  </style>
+
+  <script async custom-element="amp-vine" src="https://cdn.ampproject.org/v0/amp-vine-0.1.js"></script>
+  <script async custom-element="amp-vimeo" src="https://cdn.ampproject.org/v0/amp-vimeo-0.1.js"></script>
+  <script async custom-element="amp-dailymotion" src="https://cdn.ampproject.org/v0/amp-dailymotion-0.1.js"></script>
+  <script async custom-element="amp-brightcove" src="https://cdn.ampproject.org/v0/amp-brightcove-0.1.js"></script>
+  <script async custom-element="amp-audio" src="https://cdn.ampproject.org/v0/amp-audio-0.1.js"></script>
+  <script async custom-element="amp-soundcloud" src="https://cdn.ampproject.org/v0/amp-soundcloud-0.1.js"></script>
+  <script async custom-element="amp-anim" src="https://cdn.ampproject.org/v0/amp-anim-0.1.js"></script>
+  <script async custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.1.js"></script>
+  <script async custom-element="amp-youtube" src="https://cdn.ampproject.org/v0/amp-youtube-0.1.js"></script>
+  <script async custom-element="amp-video" src="https://cdn.ampproject.org/v0/amp-video-0.1.js"></script>
+  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+</head>
+<body>
+
+  <amp-carousel width=400 height=300 layout=responsive type=slides>
+
+    <div>hello world</div>
+
+    <amp-img src="https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n" layout=fill>
+    </amp-img>
+
+    <amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width=400 height=300 layout=responsive>
+    </amp-img>
+
+    <amp-img src="https://lh3.googleusercontent.com/Z4gtm5Bkxyv21Z2PtbTf95Clb9AE4VTR6olbBKYrenM=w400-h300-no-n" width=400 height=300 layout=responsive>
+    </amp-img>
+
+    <amp-soundcloud height=300
+        layout="fixed-height"
+        data-trackid="243169232">
+    </amp-soundcloud>
+
+    <amp-youtube
+        data-videoid="mGENRKrdoGY"
+        width="400" height="300">
+    </amp-youtube>
+
+    <amp-anim
+        src="https://lh3.googleusercontent.com/qNn8GDz8Jfd-s9lt3Nc4lJeLjVyEaqGJTk1vuCUWazCmAeOBVjSWDD0SMTU7x0zhVe5UzOTKR0n-kN4SXx7yElvpKYvCMaRyS_g-jydhJ_cEVYmYPiZ_j1Y9de43mlKxU6s06uK1NAlpbSkL_046amEKOdgIACICkuWfOBwlw2hUDfjPOWskeyMrcTu8XOEerCLuVqXugG31QC345hz3lUyOlkdT9fMYVUynSERGNzHba7bXMOxKRe3izS5DIWUgJs3oeKYqA-V8iqgCvneD1jj0Ff68V_ajm4BDchQubBJU0ytXVkoWh27ngeEHubpnApOS6fcGsjPxeuMjnzAUtoTsiXz2FZi1mMrxrblJ-kZoAq1DJ95cnoqoa2CYq3BTgq2E8BRe2paNxLJ5GXBCTpNdXMpVJc6eD7ceijQyn-2qanilX-iK3ChbOq0uBHMvsdoC_LsFOu5KzbbNH71vM3DPkvDGmHJmF67Vj8sQ6uBrLnzpYlCyN4-Y9frR8zugDcqX5Q=w400-h300-no"
+        width="400" height="300">
+      <amp-img placeholder
+          src="https://lh3.googleusercontent.com/qNn8GDz8Jfd-s9lt3Nc4lJeLjVyEaqGJTk1vuCUWazCmAeOBVjSWDD0SMTU7x0zhVe5UzOTKR0n-kN4SXx7yElvpKYvCMaRyS_g-jydhJ_cEVYmYPiZ_j1Y9de43mlKxU6s06uK1NAlpbSkL_046amEKOdgIACICkuWfOBwlw2hUDfjPOWskeyMrcTu8XOEerCLuVqXugG31QC345hz3lUyOlkdT9fMYVUynSERGNzHba7bXMOxKRe3izS5DIWUgJs3oeKYqA-V8iqgCvneD1jj0Ff68V_ajm4BDchQubBJU0ytXVkoWh27ngeEHubpnApOS6fcGsjPxeuMjnzAUtoTsiXz2FZi1mMrxrblJ-kZoAq1DJ95cnoqoa2CYq3BTgq2E8BRe2paNxLJ5GXBCTpNdXMpVJc6eD7ceijQyn-2qanilX-iK3ChbOq0uBHMvsdoC_LsFOu5KzbbNH71vM3DPkvDGmHJmF67Vj8sQ6uBrLnzpYlCyN4-Y9frR8zugDcqX5Q=w400-h300-no-k"
+          width="400" height="300">
+        </amp-img>
+    </amp-anim>
+
+    <amp-audio src="/examples/av/audio.mp3">
+    </amp-audio>
+
+    <amp-brightcove
+        data-account="1290862519001"
+        data-video-id="ref:amp-docs-sample"
+        data-player="SyIOV8yWM"
+        layout="responsive" width="480" height="270">
+    </amp-brightcove>
+
+    <amp-vimeo
+        data-videoid="27246366"
+        width="500"
+        height="281">
+    </amp-vimeo>
+
+    <amp-dailymotion
+        data-videoid="x3rdtfy"
+        width="500"
+        height="281">
+    </amp-dailymotion>
+
+    <amp-vine
+        data-vineid="MdKjXez002d"
+        width="381"
+        height="381"
+        layout="responsive">
+    </amp-vine>
+
+    <amp-video
+        src="https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerJoyrides.mp4"
+        width="358"
+        height="204"
+        layout="responsive"
+        controls>
+    </amp-video>
+
+  </amp-carousel>
+</html>
diff --git a/test/manual/amp-carousel/0.2/slides.amp.html b/test/manual/amp-carousel/0.2/slides.amp.html
@@ -0,0 +1,100 @@
+<!doctype html>
+<html ⚡>
+<head>
+  <meta charset="utf-8">
+  <title>AMP #0</title>
+  <link rel="canonical" href="amps.html" >
+  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+  <link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'>
+  <style amp-custom>
+    body {
+      font-family: 'Questrial', Arial;
+    }
+  </style>
+
+  <script async custom-element="amp-vine" src="https://cdn.ampproject.org/v0/amp-vine-0.1.js"></script>
+  <script async custom-element="amp-vimeo" src="https://cdn.ampproject.org/v0/amp-vimeo-0.1.js"></script>
+  <script async custom-element="amp-dailymotion" src="https://cdn.ampproject.org/v0/amp-dailymotion-0.1.js"></script>
+  <script async custom-element="amp-brightcove" src="https://cdn.ampproject.org/v0/amp-brightcove-0.1.js"></script>
+  <script async custom-element="amp-audio" src="https://cdn.ampproject.org/v0/amp-audio-0.1.js"></script>
+  <script async custom-element="amp-soundcloud" src="https://cdn.ampproject.org/v0/amp-soundcloud-0.1.js"></script>
+  <script async custom-element="amp-anim" src="https://cdn.ampproject.org/v0/amp-anim-0.1.js"></script>
+  <script async custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.2.js"></script>
+  <script async custom-element="amp-youtube" src="https://cdn.ampproject.org/v0/amp-youtube-0.1.js"></script>
+  <script async custom-element="amp-video" src="https://cdn.ampproject.org/v0/amp-video-0.1.js"></script>
+  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+</head>
+<body>
+
+  <amp-carousel width=400 height=300 layout=responsive type=slides>
+
+    <div>hello world</div>
+
+    <amp-img src="https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n" layout=fill>
+    </amp-img>
+
+    <amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width=400 height=300 layout=responsive>
+    </amp-img>
+
+    <amp-img src="https://lh3.googleusercontent.com/Z4gtm5Bkxyv21Z2PtbTf95Clb9AE4VTR6olbBKYrenM=w400-h300-no-n" width=400 height=300 layout=responsive>
+    </amp-img>
+
+    <amp-soundcloud height=300
+        layout="fixed-height"
+        data-trackid="243169232">
+    </amp-soundcloud>
+
+    <amp-youtube
+        data-videoid="mGENRKrdoGY"
+        width="400" height="300">
+    </amp-youtube>
+
+    <amp-anim
+        src="https://lh3.googleusercontent.com/qNn8GDz8Jfd-s9lt3Nc4lJeLjVyEaqGJTk1vuCUWazCmAeOBVjSWDD0SMTU7x0zhVe5UzOTKR0n-kN4SXx7yElvpKYvCMaRyS_g-jydhJ_cEVYmYPiZ_j1Y9de43mlKxU6s06uK1NAlpbSkL_046amEKOdgIACICkuWfOBwlw2hUDfjPOWskeyMrcTu8XOEerCLuVqXugG31QC345hz3lUyOlkdT9fMYVUynSERGNzHba7bXMOxKRe3izS5DIWUgJs3oeKYqA-V8iqgCvneD1jj0Ff68V_ajm4BDchQubBJU0ytXVkoWh27ngeEHubpnApOS6fcGsjPxeuMjnzAUtoTsiXz2FZi1mMrxrblJ-kZoAq1DJ95cnoqoa2CYq3BTgq2E8BRe2paNxLJ5GXBCTpNdXMpVJc6eD7ceijQyn-2qanilX-iK3ChbOq0uBHMvsdoC_LsFOu5KzbbNH71vM3DPkvDGmHJmF67Vj8sQ6uBrLnzpYlCyN4-Y9frR8zugDcqX5Q=w400-h300-no"
+        width="400" height="300">
+      <amp-img placeholder
+          src="https://lh3.googleusercontent.com/qNn8GDz8Jfd-s9lt3Nc4lJeLjVyEaqGJTk1vuCUWazCmAeOBVjSWDD0SMTU7x0zhVe5UzOTKR0n-kN4SXx7yElvpKYvCMaRyS_g-jydhJ_cEVYmYPiZ_j1Y9de43mlKxU6s06uK1NAlpbSkL_046amEKOdgIACICkuWfOBwlw2hUDfjPOWskeyMrcTu8XOEerCLuVqXugG31QC345hz3lUyOlkdT9fMYVUynSERGNzHba7bXMOxKRe3izS5DIWUgJs3oeKYqA-V8iqgCvneD1jj0Ff68V_ajm4BDchQubBJU0ytXVkoWh27ngeEHubpnApOS6fcGsjPxeuMjnzAUtoTsiXz2FZi1mMrxrblJ-kZoAq1DJ95cnoqoa2CYq3BTgq2E8BRe2paNxLJ5GXBCTpNdXMpVJc6eD7ceijQyn-2qanilX-iK3ChbOq0uBHMvsdoC_LsFOu5KzbbNH71vM3DPkvDGmHJmF67Vj8sQ6uBrLnzpYlCyN4-Y9frR8zugDcqX5Q=w400-h300-no-k"
+          width="400" height="300">
+        </amp-img>
+    </amp-anim>
+
+    <amp-audio src="/examples/av/audio.mp3">
+    </amp-audio>
+
+    <amp-brightcove
+        data-account="1290862519001"
+        data-video-id="ref:amp-docs-sample"
+        data-player="SyIOV8yWM"
+        layout="responsive" width="480" height="270">
+    </amp-brightcove>
+
+    <amp-vimeo
+        data-videoid="27246366"
+        width="500"
+        height="281">
+    </amp-vimeo>
+
+    <amp-dailymotion
+        data-videoid="x3rdtfy"
+        width="500"
+        height="281">
+    </amp-dailymotion>
+
+    <amp-vine
+        data-vineid="MdKjXez002d"
+        width="381"
+        height="381"
+        layout="responsive">
+    </amp-vine>
+
+    <amp-video
+        src="https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerJoyrides.mp4"
+        width="358"
+        height="204"
+        layout="responsive"
+        controls>
+    </amp-video>
+
+  </amp-carousel>
+</html>