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 5 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
93 changes: 42 additions & 51 deletions examples/TouchScrollExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,58 +20,49 @@ class TouchScrollExample extends React.Component {
render() {
const { dataList } = this.state;

// Recent browser versions are making touch events passive by default. Unfortunately, React doesn't allow us
// to specify the event handlers as passive/active (see #6436 on facebook/react). This can lead to unneeded
// 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'
};

return (
<div style={tableParentStyle}>
<Table
rowHeight={50}
rowsCount={dataList.getSize()}
headerHeight={50}
touchScrollEnabled={true}
width={1000}
height={500}
{...this.props}>
<Column
columnKey="firstName"
header={<Cell>First Name</Cell>}
cell={<TextCell data={dataList} />}
fixed={true}
width={100}
/>
<Column
columnKey="lastName"
header={<Cell>Last Name</Cell>}
cell={<TextCell data={dataList} />}
fixed={true}
width={100}
/>
<Column
columnKey="city"
header={<Cell>City</Cell>}
cell={<TextCell data={dataList} />}
width={100}
/>
<Column
columnKey="street"
header={<Cell>Street</Cell>}
cell={<TextCell data={dataList} />}
width={200}
/>
<Column
columnKey="zipCode"
header={<Cell>Zip Code</Cell>}
cell={<TextCell data={dataList} />}
width={200}
/>
</Table>
</div>
<Table
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

rowHeight={50}
rowsCount={dataList.getSize()}
headerHeight={50}
touchScrollEnabled={true}
width={1000}
height={500}
{...this.props}
>
<Column
columnKey="firstName"
header={<Cell>First Name</Cell>}
cell={<TextCell data={dataList} />}
fixed={true}
width={100}
/>
<Column
columnKey="lastName"
header={<Cell>Last Name</Cell>}
cell={<TextCell data={dataList} />}
fixed={true}
width={100}
/>
<Column
columnKey="city"
header={<Cell>City</Cell>}
cell={<TextCell data={dataList} />}
width={100}
/>
<Column
columnKey="street"
header={<Cell>Street</Cell>}
cell={<TextCell data={dataList} />}
width={200}
/>
<Column
columnKey="zipCode"
header={<Cell>Zip Code</Cell>}
cell={<TextCell data={dataList} />}
width={200}
/>
</Table>
);
}
}
Expand Down
23 changes: 22 additions & 1 deletion src/FixedDataTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,14 @@ class FixedDataTable extends React.Component {
*/
stopReactWheelPropagation: PropTypes.bool,

/**
* If enabled scroll events will never be default handled.
* If disabled/unspecified, scroll events will be default handled if the scroll
* 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,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the prop name and description fine?

Copy link
Member

Choose a reason for hiding this comment

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

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.


/**
* If enabled scroll events will not be propagated outside of the table.
*/
Expand Down Expand Up @@ -444,23 +452,32 @@ class FixedDataTable extends React.Component {
this._onScroll,
this._shouldHandleWheelX,
this._shouldHandleWheelY,
this.props.stopScrollDefaultHandling,
this.props.stopScrollPropagation
);

this._touchHandler = new ReactTouchHandler(
this._onScroll,
this._shouldHandleTouchX,
this._shouldHandleTouchY,
this.props.stopScrollDefaultHandling,
this.props.stopScrollPropagation
);
}

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 (facebook/react#6436)
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 +593,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 +820,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
18 changes: 17 additions & 1 deletion src/ReactTouchHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class ReactTouchHandler {
/*function*/ onTouchScroll,
/*boolean|function*/ handleScrollX,
/*boolean|function*/ handleScrollY,
/*?boolean|?function*/ preventDefault,
/*?boolean|?function*/ stopPropagation
) {

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

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

Choose a reason for hiding this comment

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

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...

preventDefault = preventDefault ?
emptyFunction.thatReturnsTrue :
emptyFunction.thatReturnsFalse;
}

// TODO (jordan) Is configuring this necessary
// I don't think so. We are only just passing a boolean anyway.
if (typeof stopPropagation !== 'function') {
stopPropagation = stopPropagation ?
emptyFunction.thatReturnsTrue :
Expand All @@ -87,6 +96,7 @@ class ReactTouchHandler {

this._handleScrollX = handleScrollX;
this._handleScrollY = handleScrollY;
this._preventDefault = preventDefault;
this._stopPropagation = stopPropagation;
this._onTouchScrollCallback = onTouchScroll;

Expand Down Expand Up @@ -148,6 +158,9 @@ class ReactTouchHandler {
}

onTouchMove(/*object*/ event) {
if (this._preventDefault()) {
event.preventDefault();
}

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

event.preventDefault();
// The event will result in a scroll to the table, so there's no need to also let the parent containers scroll
if (!event.defaultPrevented) {
event.preventDefault();
}

// Ensure minimum delta magnitude is met to avoid jitter
var changed = false;
Expand Down
21 changes: 20 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 @@ -30,6 +30,7 @@ class ReactWheelHandler {
/*function*/ onWheel,
/*boolean|function*/ handleScrollX,
/*boolean|function*/ handleScrollY,
/*?boolean|?function*/ preventDefault,
/*?boolean|?function*/ stopPropagation
) {
this._animationFrameID = null;
Expand All @@ -50,6 +51,15 @@ class ReactWheelHandler {
emptyFunction.thatReturnsFalse;
}

// Can we just make this a boolean flag instead?
if (typeof preventDefault !== 'function') {
preventDefault = preventDefault ?
emptyFunction.thatReturnsTrue :
emptyFunction.thatReturnsFalse;
}

// TODO (jordan) Is configuring this necessary
// I don't think so. We are only just passing a boolean anyway.
if (typeof stopPropagation !== 'function') {
stopPropagation = stopPropagation ?
emptyFunction.thatReturnsTrue :
Expand All @@ -58,12 +68,17 @@ class ReactWheelHandler {

this._handleScrollX = handleScrollX;
this._handleScrollY = handleScrollY;
this._preventDefault = preventDefault;
this._stopPropagation = stopPropagation;
this._onWheelCallback = onWheel;
this.onWheel = this.onWheel.bind(this);
}

onWheel(/*object*/ event) {
if (this._preventDefault()) {
event.preventDefault();
}

var normalizedEvent = normalizeWheel(event);

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

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

// This will result in a scroll to the table, so there's no need to let the parent containers scroll
if (!event.defaultPrevented) {
event.preventDefault();
}

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