Allow touch scroll to be default prevented by making the handlers passive #423

pradeepnschrodinger · 2019-04-03T05:46:56Z

Description

Made the touch handler passive through addEventListener (similar to #422)
Moved event.preventDefault calls to the top so that they are always executed.

Motivation and Context

Allows event to be default prevented, and thus (hopefully) fixes

what is best way to prevent whole page scrolling when table is scrolling up? #419 - what is best way to prevent whole page scrolling when table is scrolling up?
Scrolling horizontally using gestures causes browser to navigate through history instead of scrolling the table. #154 - Scrolling horizontally using gestures causes browser to navigate through history instead of scrolling the table.

How Has This Been Tested?

Tested on our touch scroll example using chrome device tools. With the fix the page doesn't get scrolled, even when the table is already scrolled to the start/end

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

…sive

pradeepnschrodinger · 2019-04-03T05:51:10Z

examples/TouchScrollExample.js

@@ -25,7 +25,7 @@ class TouchScrollExample extends React.Component {
    // scrolling of parent containers of FDT. This style is a work around to fix this. By applying 'none' to
    // touch-action, we are disabling touch events from propagating.
    const tableParentStyle = {
-      'touch-action': 'none'
+      'touchAction': 'none'


Typo.. It worked anyway, so I guess React was being lenient.

pradeepnschrodinger · 2019-04-03T05:53:17Z

src/ReactTouchHandler.js

@@ -148,6 +148,7 @@ class ReactTouchHandler {
  }

  onTouchMove(/*object*/ event) {
+    event.preventDefault();


if the delta is zero, preventDefault wasn't being called. This is why the parent container / page get's scrolled
when the table is at any start/end scroll position

Will this lead to the issue where you can't scroll the parent page with your pointer over the grid?

@wcjordan , Yes exactly. My reasoning was that I wouldn't want to scroll any container if the cursor is on the table. But now I agree, that it's better to have a FF control this instead (#419 (comment)). Will add

wcjordan · 2019-04-04T01:11:37Z

src/ReactTouchHandler.js

@@ -148,6 +148,7 @@ class ReactTouchHandler {
  }

  onTouchMove(/*object*/ event) {
+    event.preventDefault();


Will this lead to the issue where you can't scroll the parent page with your pointer over the grid?

wcjordan · 2019-04-04T01:11:56Z

src/vendor_upstream/dom/ReactWheelHandler.js

@@ -64,6 +64,8 @@ class ReactWheelHandler {
  }

  onWheel(/*object*/ event) {
+    event.preventDefault();


Will this lead to the issue where you can't scroll the parent page with your pointer over the grid?

Yep, same as above.

wcjordan · 2019-04-04T01:13:05Z

src/FixedDataTable.js

@@ -456,11 +456,18 @@ class FixedDataTable extends React.Component {
  }

  componentWillUnmount() {
+    // TODO (pradeep): Remove these and pass to our table component directly after


Here's the related issue if you want to reference it:
facebook/react#6436

thanks, will put.

pradeepnschrodinger

I'll have a FF control the behavior. And default it so that scroll handlers are only default prevented when scroll offsets have changed (existing behavior)

pradeepnschrodinger · 2019-04-04T08:50:43Z

src/FixedDataTable.js

@@ -456,11 +456,18 @@ class FixedDataTable extends React.Component {
  }

  componentWillUnmount() {
+    // TODO (pradeep): Remove these and pass to our table component directly after


thanks, will put.

pradeepnschrodinger · 2019-04-04T08:52:25Z

src/ReactTouchHandler.js

@@ -148,6 +148,7 @@ class ReactTouchHandler {
  }

  onTouchMove(/*object*/ event) {
+    event.preventDefault();


@wcjordan , Yes exactly. My reasoning was that I wouldn't want to scroll any container if the cursor is on the table. But now I agree, that it's better to have a FF control this instead (#419 (comment)). Will add

pradeepnschrodinger · 2019-04-04T08:52:32Z

src/vendor_upstream/dom/ReactWheelHandler.js

@@ -64,6 +64,8 @@ class ReactWheelHandler {
  }

  onWheel(/*object*/ event) {
+    event.preventDefault();


Yep, same as above.

…vior

pradeepnschrodinger · 2019-04-04T11:33:15Z

@wcjordan , added a flag to work around the behavior seen in #419.
I noticed that ReactWheelHandler and ReactTouchHandler accepts these flags as functions (see here and here), but I don't think we use them this way as the docs (from props) tell they must be booleans. Thoughts?

pradeepnschrodinger · 2019-04-04T11:34:05Z

examples/TouchScrollExample.js

-          />
-        </Table>
-      </div>
+      <Table


just removed the wrapper div since the parent element and style is no longer needed

pradeepnschrodinger · 2019-04-04T11:34:26Z

src/FixedDataTable.js

+     * doesn't lead to a change in scroll offsets, which is preferable if you like
+     * the page/container to scroll up when the table is already scrolled up max.
+     */
+    stopScrollDefaultHandling: PropTypes.bool,


Is the prop name and description fine?

Could you change the term "default handled" to "bubbled to the browser default handler"?
I'd also change "disabled/unspecified" to "disabled (default when unspecified)"

Otherwise looks good.

wcjordan

lgtm

wcjordan · 2019-04-04T15:39:51Z

src/FixedDataTable.js

+     * doesn't lead to a change in scroll offsets, which is preferable if you like
+     * the page/container to scroll up when the table is already scrolled up max.
+     */
+    stopScrollDefaultHandling: PropTypes.bool,


Could you change the term "default handled" to "bubbled to the browser default handler"?
I'd also change "disabled/unspecified" to "disabled (default when unspecified)"

Otherwise looks good.

wcjordan · 2019-04-04T15:49:33Z

src/ReactTouchHandler.js

@@ -78,7 +79,15 @@ class ReactTouchHandler {
        emptyFunction.thatReturnsFalse;
    }

+    // Can we just make this a boolean flag instead?
+    if (typeof preventDefault !== 'function') {


yep, let's not support passing a function. It doesn't take args, so I can't imagine what they would be using to decide...

pradeepnschrodinger added 2 commits April 3, 2019 05:35

Allow touch scroll to be default prevented by making the handlers pas…

0d3a5f6

…sive

Uncomment and remove lines

a86b63f

pradeepnschrodinger requested review from wcjordan and somavara April 3, 2019 05:47

Typo in componentWillUnmount

fb83c2b

pradeepnschrodinger commented Apr 3, 2019

View reviewed changes

wcjordan reviewed Apr 4, 2019

View reviewed changes

pradeepnschrodinger commented Apr 4, 2019

View reviewed changes

pradeepnschrodinger added 4 commits April 4, 2019 10:37

Added flag 'stopScrollDefaultHandling' to control preventDefault beha…

803faaf

…vior

Added issue reference

6db0796

Add and fix tests

4127040

Add prevent default calls in onTouchStart and onTouchEnd

07a376f

pradeepnschrodinger commented Apr 4, 2019

View reviewed changes

wcjordan approved these changes Apr 4, 2019

View reviewed changes

pradeepnschrodinger added 2 commits April 4, 2019 18:50

The flags now need to be booleans

44a1411

Fix tests: Remove test cases where the flags were functions

3499535

pradeepnschrodinger merged commit 6da172b into v1.0-beta Apr 4, 2019

pradeepnschrodinger deleted the Beta-Fix-Passive-Scrolls branch April 4, 2019 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow touch scroll to be default prevented by making the handlers passive #423

Allow touch scroll to be default prevented by making the handlers passive #423

pradeepnschrodinger commented Apr 3, 2019

pradeepnschrodinger Apr 3, 2019

pradeepnschrodinger Apr 3, 2019

wcjordan Apr 4, 2019

pradeepnschrodinger Apr 4, 2019

wcjordan Apr 4, 2019

wcjordan Apr 4, 2019

pradeepnschrodinger Apr 4, 2019

wcjordan Apr 4, 2019

pradeepnschrodinger Apr 4, 2019

pradeepnschrodinger left a comment

pradeepnschrodinger Apr 4, 2019

pradeepnschrodinger Apr 4, 2019

pradeepnschrodinger Apr 4, 2019

pradeepnschrodinger commented Apr 4, 2019

pradeepnschrodinger Apr 4, 2019

pradeepnschrodinger Apr 4, 2019

wcjordan Apr 4, 2019

wcjordan left a comment

wcjordan Apr 4, 2019

wcjordan Apr 4, 2019

Allow touch scroll to be default prevented by making the handlers passive #423

Allow touch scroll to be default prevented by making the handlers passive #423

Conversation

pradeepnschrodinger commented Apr 3, 2019

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

pradeepnschrodinger left a 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

pradeepnschrodinger commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcjordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment