Disable native snap point in slidescroll #7966

muxin · 2017-03-03T22:04:16Z

Tested in ios 10.3 beta and 10.2

camelburrito · 2017-03-03T22:06:01Z

tested on other devices? platforms?

camelburrito · 2017-03-03T22:07:58Z

Also this is not a permanent fix , this is just a stop gap. So just flip the flag in the JS dont cleanup the code yet. We would need a permanant fix (or may be identify the issue and file a webkit bug that would be fixed soon)

muxin · 2017-03-03T22:48:16Z

I just tested the code (disable native snap point) on real ios device and the autoplay iterating rapidly bug is fixed but there are other misbehaviors showing up. @camelburrito

PS, I tried to debug what's wrong with the native snap point but didn't have a clue.

muxin · 2017-03-03T23:32:26Z

I'm going to debug slidescroll custom snap and hopefully find a fix for the newly found misbehaviour: if user scrolls too much and reach the end of the next slide, the carousel will jump one more slide suddenly.

muxin · 2017-03-04T00:20:15Z

Thanks to @camelburrito I figured out the fix for the sudden slide jump in ios. I will keep testing and commit coming.

muxin · 2017-03-10T20:19:34Z

@camelburrito I removed handleCustomElasticScroll_ and the problem (jumping to one more slide suddenly) is solved. I removed it in both android and ios cases and confirmed the slidescroll still functional in both platforms. Do you think it's ok to remove it?

amphtml-team · 2017-03-11T00:28:24Z

No :(

On Fri, Mar 10, 2017 at 3:19 PM Yuxi Chen ***@***.***> wrote: @camelburrito <https://github.com/camelburrito> I removed handleCustomElasticScroll_ and the problem (jumping to one more slide suddenly) is solved. I removed it in both android and ios cases and confirmed the slidescroll still functional in both platforms. Do you think it's ok to remove it? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7966 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANd2kCs_UbZ2E_wm8TJPNNgrOm_eK9cyks5rkbBfgaJpZM4MSzHT> . -- You received this message because you are subscribed to the Google Groups "amphtml-eng-github" group. To unsubscribe from this group and stop receiving emails from it, send an email to ***@***.*** To post to this group, send email to ***@***.*** To view this discussion on the web visit https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/7966/c285774256%40github.com <https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/7966/c285774256%40github.com?utm_medium=email&utm_source=footer> .

-- -- Sriram

muxin · 2017-03-11T00:31:34Z

@camelburrito I updated the heroku code with current PR code: http://yuxichen2.herokuapp.com/examples/test.amp.max.html (I added a test.amp.html in the heroku deployment), please check the swiping behavior. It seems great, I wonder why can't we remove handleCustomElasticScroll_

ampprojectbot · 2017-03-14T17:51:54Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko

extensions/amp-carousel/0.1/amp-carousel.css
extensions/amp-carousel/0.1/slidescroll.js
extensions/amp-carousel/0.1/test/test-slidescroll.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

jridgewell · 2017-03-14T19:36:45Z

Ping @camelburrito @muxin, this is a P0.

camelburrito · 2017-03-14T19:39:27Z

@jridgewell - we are working on it we have a solution and we are testing it out on lots of devices - taking a lot of time. @muxin Let me know when the device testing is done.

ampprojectbot · 2017-03-15T17:50:44Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to camelburrito dvoytenko

extensions/amp-carousel/0.1/amp-carousel.css
extensions/amp-carousel/0.1/slidescroll.js
extensions/amp-carousel/0.1/test/test-slidescroll.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

ampprojectbot · 2017-03-16T20:43:34Z

Hi, ampproject bot here! Here are a list of the owners that can approve your files.

You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status

/to cramforce dvoytenko jridgewell

tools/experiments/experiments.js

/to camelburrito dvoytenko

extensions/amp-carousel/0.1/amp-carousel.css
extensions/amp-carousel/0.1/slidescroll.js
extensions/amp-carousel/0.1/test/test-slidescroll.js

For any issues please file a bug at https://github.com/google/github-owners-bot/issues

muxin · 2017-03-16T20:52:33Z

@camelburrito Testing on devices. But PTAL

muxin · 2017-03-16T21:28:42Z

Tested, the android and ios non-beta behaves the same as before, and the ios beta uses custom snap without crazy slide switching. @camelburrito PTAL

camelburrito · 2017-03-16T23:34:33Z

extensions/amp-carousel/0.1/slidescroll.js

+
+    /** @private {boolean} */
+    this.isIosBeta_ = startsWith(platformFor(this.win).getIosVersionString(),
+        '10.3');


you should check if the version is > 10.2

Right now the 10.3 is the only problematic version, I will create a followup issue for comparing IOS minor version.

Created #8206

camelburrito · 2017-03-16T23:36:50Z

extensions/amp-carousel/0.1/slidescroll.js

+    /** @private {boolean} */
+    this.isIosBetaExperimentOn_ = isExperimentOn(this.win,
+        'slidescroll-ios-beta');
+


disable-css-snap

camelburrito · 2017-03-16T23:37:09Z

extensions/amp-carousel/0.1/slidescroll.js

+        'slidescroll-ios-beta');
+
+    /** @private {boolean} */
+    this.isIosBeta_ = startsWith(platformFor(this.win).getIosVersionString(),


isCssSnapDisabled_

camelburrito · 2017-03-16T23:37:46Z

extensions/amp-carousel/0.1/slidescroll.js

@@ -139,6 +155,11 @@ export class AmpSlideScroll extends BaseSlides {
    // user.
    this.slidesContainer_.setAttribute('aria-live', 'polite');

+    // Snap point is buggy in IOS 10.3 (beta), so it is disabled in beta.
+    if (this.isIosBetaExperimentOn_ && this.isIosBeta_) {
+      this.slidesContainer_.classList.add('-amp-disable-snap-type');


-amp-slidescroll-dsable-snap

camelburrito · 2017-03-16T23:37:54Z

extensions/amp-carousel/0.1/slidescroll.js

+
+      // Snap point is buggy in IOS 10.3 (beta), so it is disabled in beta.
+      if (this.isIosBetaExperimentOn_ && this.isIosBeta_) {
+        slideWrapper.classList.add('-amp-disable-snap-coordinate');


use only one class

camelburrito · 2017-03-16T23:41:51Z

extensions/amp-carousel/0.1/amp-carousel.css

@@ -180,4 +180,12 @@ amp-carousel .amp-carousel-button.amp-disabled {
  overflow-y: hidden !important;
  -webkit-overflow-scrolling: touch !important;
 }
+
+.-amp-disable-snap-type {


.-amp-slidescroll-no-snap..-amp-slides-container { scroll-snap-type: none !important; } .-amp-slidescroll-no-snap.-amp-slide-item { scroll-snap-coordinate: none !important; }

camelburrito · 2017-03-16T23:42:15Z

tools/experiments/experiments.js

+  {
+    id: 'slidescroll-ios-beta',
+    name: 'Slidescroll IOS beta',
+    cleanupIssue: 'https://github.com/ampproject/amphtml/issues/8195',


add a spec and link the issue.

camelburrito · 2017-03-16T23:42:24Z

tools/experiments/experiments.js

@@ -266,6 +266,11 @@ const EXPERIMENTS = [
    spec: 'https://github.com/ampproject/amphtml/blob/master/extensions/' +
        'amp-sortable-table/amp-sortable-table.md',
  },
+  {
+    id: 'slidescroll-ios-beta',


change the expt name.

camelburrito · 2017-03-17T00:30:48Z

extensions/amp-carousel/0.1/slidescroll.js

+        'slidescroll-disable-css-snap');
+
+    /** @private {boolean} */
+    this.shouldDisableCssSnap_ = startsWith(


bring in the experiment here and use one boolean.

muxin requested review from camelburrito and aghassemi March 3, 2017 22:04

muxin force-pushed the slidescroll-disable-native-snap branch from d6e8332 to aaf8b63 Compare March 3, 2017 22:06

muxin added the PR: Work in Progress label Mar 3, 2017

muxin force-pushed the slidescroll-disable-native-snap branch from aaf8b63 to f3b62cd Compare March 10, 2017 19:48

muxin force-pushed the slidescroll-disable-native-snap branch from f3b62cd to 76a8a6b Compare March 10, 2017 20:26

muxin removed the PR: Work in Progress label Mar 10, 2017

muxin force-pushed the slidescroll-disable-native-snap branch from 76a8a6b to 9f7dbac Compare March 14, 2017 17:51

muxin force-pushed the slidescroll-disable-native-snap branch from 9f7dbac to 09776d2 Compare March 14, 2017 17:53

muxin force-pushed the slidescroll-disable-native-snap branch from 09776d2 to 30cf942 Compare March 14, 2017 20:16

muxin added 3 commits March 16, 2017 13:21

Disable native snap point in slidescroll

2a9e73c

Keep the old code, should refactor later

195d017

Revert everything

f427233

muxin force-pushed the slidescroll-disable-native-snap branch from 30cf942 to 686021c Compare March 16, 2017 20:43

muxin force-pushed the slidescroll-disable-native-snap branch from 686021c to e337a6c Compare March 16, 2017 20:46

Disable snap point only in ios 10.3

69c50ae

muxin force-pushed the slidescroll-disable-native-snap branch from e337a6c to 69c50ae Compare March 16, 2017 20:49

camelburrito suggested changes Mar 16, 2017

View reviewed changes

muxin removed the request for review from aghassemi March 16, 2017 23:43

Rename css class and experiment flag

12ce405

camelburrito reviewed Mar 17, 2017

View reviewed changes

muxin mentioned this pull request Mar 17, 2017

Be able to compare ios minor version #8206

Closed

Use only one boolean

755861b

camelburrito approved these changes Mar 17, 2017

View reviewed changes

muxin merged commit 6a4b67b into ampproject:master Mar 17, 2017

muxin deleted the slidescroll-disable-native-snap branch March 17, 2017 01:15

aghassemi mentioned this pull request Mar 29, 2017

Momentum scroll stopped by "-amp-no-scroll" on iOS 10 Safari #8476

Closed

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Disable native snap point in slidescroll (ampproject#7966)

cd9cc51

micajuine-ho mentioned this pull request Apr 9, 2021

🐛 [amp-carousel 0.1] Stop jittery slides on scroll #33661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable native snap point in slidescroll #7966

Disable native snap point in slidescroll #7966

muxin commented Mar 3, 2017

camelburrito commented Mar 3, 2017

camelburrito commented Mar 3, 2017

muxin commented Mar 3, 2017 •

edited

muxin commented Mar 3, 2017

muxin commented Mar 4, 2017

muxin commented Mar 10, 2017

amphtml-team commented Mar 11, 2017 via email

muxin commented Mar 11, 2017 •

edited

ampprojectbot commented Mar 14, 2017

jridgewell commented Mar 14, 2017

camelburrito commented Mar 14, 2017

ampprojectbot commented Mar 15, 2017

ampprojectbot commented Mar 16, 2017

muxin commented Mar 16, 2017

muxin commented Mar 16, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

muxin Mar 17, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

camelburrito Mar 16, 2017

muxin Mar 17, 2017

camelburrito Mar 17, 2017

muxin Mar 17, 2017

Disable native snap point in slidescroll #7966

Disable native snap point in slidescroll #7966

Conversation

muxin commented Mar 3, 2017

camelburrito commented Mar 3, 2017

camelburrito commented Mar 3, 2017

muxin commented Mar 3, 2017 • edited

muxin commented Mar 3, 2017

muxin commented Mar 4, 2017

muxin commented Mar 10, 2017

amphtml-team commented Mar 11, 2017 via email

muxin commented Mar 11, 2017 • edited

ampprojectbot commented Mar 14, 2017

jridgewell commented Mar 14, 2017

camelburrito commented Mar 14, 2017

ampprojectbot commented Mar 15, 2017

ampprojectbot commented Mar 16, 2017

muxin commented Mar 16, 2017

muxin commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muxin commented Mar 3, 2017 •

edited

muxin commented Mar 11, 2017 •

edited