Use passive event listener for touchmove (to prevent scrolling while swiping) #2

xanido · 2019-05-31T02:10:01Z

This small patch restores react-slider's ability to disable vertical scrolling when swiping through a slider on iOS.

Seems the touchmove event listener that React adds (when using the React's event system via event props) is not configured as a non-passive listener. iOS (starting with iOS 11.3) has made all root document touch event listeners passive by default to improve scrolling performance. React's event system attaches event handlers to the document root, which will result in a passive touchmove listener on iOS, which in turn means that preventDefault() has no effect.

The proposed solution in facebook/react#2043 should fix the issue, but for now adding the event listener directly to the node also solves the problem.

Please note, the {passive: false} in the addEventListener call in this PR is not technically necessary as only document root event listeners are passive by default on iOS, but it does explicitly communicate the intent.

mattcolman · 2019-05-31T03:33:41Z

src/inner-slider.js

@@ -188,6 +195,13 @@ export class InnerSlider extends React.Component {
    //   this.props.onLazyLoad([leftMostSlide])
    // }
    this.adaptHeight();
+
+    if (prevProps.touchMove !== this.props.touchMove) {
+      const method = touchMove ? "add" : "remove";


is touchMove defined? Should be this.props.touchMove?

Sorry, fixed!

mattcolman · 2019-05-31T04:50:32Z

src/inner-slider.js

@@ -704,7 +714,6 @@ export class InnerSlider extends React.Component {
      onMouseUp: touchMove ? this.swipeEnd : null,
      onMouseLeave: this.state.dragging && touchMove ? this.swipeEnd : null,
      onTouchStart: touchMove ? this.swipeStart : null,
-      onTouchMove: this.state.dragging && touchMove ? this.swipeMove : null,


do you also need to preserve this logic around this.state.dragging?

I've added it back in but in a slightly different manner to work around another iOS Safari quirk (touch listeners registered by other touch listeners don't seem to be able to be made non-passive).

mattcolman · 2019-05-31T04:54:48Z

src/inner-slider.js

@@ -97,6 +97,11 @@ export class InnerSlider extends React.Component {
        slide.onblur = this.props.pauseOnFocus ? this.onSlideBlur : null;
      }
    );
+
+    if (this.props.touchMove) {
+      this.list.addEventListener("touchmove", this.swipeMove);


did you remove { passive: false } because it's the default? I'd probably keep it and add a comment to explain that we need passive set to false to allow preventDefault and link to https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener for more details.

I felt that it was unnecessary to leave it in, as false is the default. I can add it back in to guard against a future change to the default (although I'd guess it'd be very unlikely to happen)

Added it back in and added note

Previously touchmove handler was registered from within a touchstart event. Seems as though handlers registered from within touch handlers are always passive, regardless of the passive param. Instead we keep the touchmove handler around, and return early if not in a dragging state.

mattcolman

looks good to me! Awesome work @xanido !

mattcolman · 2019-05-31T06:18:18Z

oh one last thing, might need to clean up touchmove in componentWillUnmount?

jooj123 · 2019-05-31T09:02:26Z

src/inner-slider.js

+      const method = this.props.touchMove ? "add" : "remove";
+      // swipeMove calls preventDefault which only has an effect on non-passive events
+      // https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
+      this.list[`${method}EventListener`]("touchmove", this.swipeMove, {


isnt there a chance that there is a listener thats created but not removed here?

xanido · 2019-05-31T09:02:32Z

D’oh you’re right! Sorry thanks for catching that, will update

…

On Fri, 31 May 2019 at 1:33 pm, Matt Colman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/inner-slider.js <#2 (comment)> : > @@ -188,6 +195,13 @@ export class InnerSlider extends React.Component { // this.props.onLazyLoad([leftMostSlide]) // } this.adaptHeight(); + + if (prevProps.touchMove !== this.props.touchMove) { + const method = touchMove ? "add" : "remove"; is touchMove defined? Should be this.props.touchMove? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2?email_source=notifications&email_token=AAGUQWFSDKA6T3BQDOBZ2J3PYCMBLA5CNFSM4HRQOHTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2GP4WI#pullrequestreview-244121177>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGUQWCMC4B4NC3T3QTTC2LPYCMBLANCNFSM4HRQOHTA> .

jooj123 · 2019-05-31T09:04:51Z

src/inner-slider.js

@@ -118,6 +127,11 @@ export class InnerSlider extends React.Component {
      this.callbackTimers.forEach(timer => clearTimeout(timer));
      this.callbackTimers = [];
    }
+    if(this.props.touchMove) {
+      this.list.removeEventListener("touchmove", this.swipeMove, {
+        passive: false


good to check if animating is true, if its true you can have passive true which will improve performance
as you are promising the handler won't call preventDefault to disable scrolling.

jooj123 · 2019-05-31T09:04:55Z

src/inner-slider.js

+      // swipeMove calls preventDefault which only has an effect on non-passive events
+      // https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
+      this.list.addEventListener("touchmove", this.swipeMove, {
+        passive: false


good to check if animating is true, if its true you can have passive true which will improve performance
as you are promising the handler won't call preventDefault to disable scrolling.

jooj123 · 2019-06-02T11:58:20Z

why do you need to disable vertical scrolling when swiping ?

mattcolman · 2019-06-02T12:00:44Z

That's the expected behaviour, if you are trying to swipe horizontal it won't also allow you to scroll vertically.

mattcolman reviewed May 31, 2019

View reviewed changes

Use passive event listener for touchmove

adbd56e

xanido force-pushed the prevent-scroll-on-swipe-ios branch from 8b951b2 to adbd56e Compare May 31, 2019 03:44

mattcolman reviewed May 31, 2019

View reviewed changes

xanido added 2 commits May 31, 2019 16:04

Make touchmove listener non-passive explicitly

c824669

xanido force-pushed the prevent-scroll-on-swipe-ios branch from 82def76 to 878b61b Compare May 31, 2019 06:04

mattcolman approved these changes May 31, 2019

View reviewed changes

Cleanup touchmove handler on unmount

ae3b729

jooj123 reviewed May 31, 2019

View reviewed changes

jooj123 approved these changes Jun 3, 2019

View reviewed changes

jooj123 merged commit 668dfd6 into DomainGroupOSS:master Jun 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use passive event listener for touchmove (to prevent scrolling while swiping) #2

Use passive event listener for touchmove (to prevent scrolling while swiping) #2

xanido commented May 31, 2019 •

edited

Loading

mattcolman May 31, 2019

xanido May 31, 2019

mattcolman May 31, 2019

xanido May 31, 2019

mattcolman May 31, 2019

xanido May 31, 2019

xanido May 31, 2019

mattcolman left a comment

mattcolman commented May 31, 2019

jooj123 May 31, 2019

xanido commented May 31, 2019 via email

jooj123 May 31, 2019

jooj123 May 31, 2019

jooj123 commented Jun 2, 2019

mattcolman commented Jun 2, 2019

Use passive event listener for touchmove (to prevent scrolling while swiping) #2

Use passive event listener for touchmove (to prevent scrolling while swiping) #2

Conversation

xanido commented May 31, 2019 • edited Loading

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

mattcolman left a comment

Choose a reason for hiding this comment

mattcolman commented May 31, 2019

Choose a reason for hiding this comment

xanido commented May 31, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooj123 commented Jun 2, 2019

mattcolman commented Jun 2, 2019

xanido commented May 31, 2019 •

edited

Loading