Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Apr 4, 2019
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0d3a5f6
Allow touch scroll to be default prevented by making the handlers pas…
pradeepnschrodinger Apr 3, 2019
a86b63f
Uncomment and remove lines
pradeepnschrodinger Apr 3, 2019
fb83c2b
Typo in componentWillUnmount
pradeepnschrodinger Apr 3, 2019
803faaf
Added flag 'stopScrollDefaultHandling' to control preventDefault beha…
pradeepnschrodinger Apr 4, 2019
6db0796
Added issue reference
pradeepnschrodinger Apr 4, 2019
4127040
Add and fix tests
pradeepnschrodinger Apr 4, 2019
07a376f
Add prevent default calls in onTouchStart and onTouchEnd
pradeepnschrodinger Apr 4, 2019
44a1411
The flags now need to be booleans
pradeepnschrodinger Apr 4, 2019
3499535
Fix tests: Remove test cases where the flags were functions
pradeepnschrodinger Apr 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/TouchScrollExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

return (
Expand Down
13 changes: 12 additions & 1 deletion src/FixedDataTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,11 +456,18 @@ class FixedDataTable extends React.Component {
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will put.

// React provides an API where event handlers can be specified to be non-passive
this._divRef && this._divRef.removeEventListener(
'wheel',
this._wheelHandler.onWheel,
{ passive: false }
);
this._divRef && this._divRef.removeEventListener(
'touchmove',
this._touchHandler.onTouchMove,
{ passive: false }
);
this._wheelHandler = null;
this._touchHandler = null;

Expand Down Expand Up @@ -576,6 +583,11 @@ class FixedDataTable extends React.Component {
this._wheelHandler.onWheel,
{ passive: false }
);
this._divRef && this._divRef.addEventListener(
'touchmove',
this._touchHandler.onTouchMove,
{ passive: false }
);
this._reportContentHeight();
}

Expand Down Expand Up @@ -798,7 +810,6 @@ class FixedDataTable extends React.Component {
onKeyDown={this._onKeyDown}
onTouchStart={this._touchHandler.onTouchStart}
onTouchEnd={this._touchHandler.onTouchEnd}
onTouchMove={this._touchHandler.onTouchMove}
onTouchCancel={this._touchHandler.onTouchCancel}
ref={this._onRef}
style={{
Expand Down
3 changes: 1 addition & 2 deletions src/ReactTouchHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ class ReactTouchHandler {
}

onTouchMove(/*object*/ event) {
event.preventDefault();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


var moveX = event.touches[0].pageX;
var moveY = event.touches[0].pageY;
Expand Down Expand Up @@ -175,8 +176,6 @@ class ReactTouchHandler {
this._deltaY = 0;
}

event.preventDefault();

// Ensure minimum delta magnitude is met to avoid jitter
var changed = false;
if (Math.abs(this._deltaX) > 2 || Math.abs(this._deltaY) > 2) {
Expand Down
3 changes: 2 additions & 1 deletion src/vendor_upstream/dom/ReactWheelHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ class ReactWheelHandler {
}

onWheel(/*object*/ event) {
event.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, same as above.


var normalizedEvent = normalizeWheel(event);

// if shift is held, swap the axis of scrolling.
Expand All @@ -85,7 +87,6 @@ class ReactWheelHandler {

this._deltaX += handleScrollX ? normalizedEvent.pixelX : 0;
this._deltaY += handleScrollY ? normalizedEvent.pixelY : 0;
event.preventDefault();

var changed;
if (this._deltaX !== 0 || this._deltaY !== 0) {
Expand Down