Skip to content

Commit 31ed76d

Browse files
committed
Fix touch handlers and make them passive (#706)
Touch support in FDT v2 is wonky. There's multiple issues here, all of which are fixed with this commit: ## problem 1 We're no longer passing down the `touchEnabled` prop to the cell renderers within FDT. But the resizer plugin `<ResizeCell />` still expects it, making it not work for touch devices. ## problem 2 The touch/mouse related event listeners were treated as passive. This is a problem with React not yet having an API for clients to specify event listener options. FDT relies on stopping propagation or preventing default behavior of these events in various places, and they did not work as expected. I'm fixing this by manually registering the handlers via `addEventListener` with the `passive` property turned OFF. See facebook/react#6436 ## problem 3 Sometimes, the first touch events don't seem to work unless the user does a follow-up click. One particular case is when the user clicks on the reorder knob; FDT will render a "drag proxy" which replaces the original cell thus making the original touch event (which relies on the original cell to be in the document tree) to not work properly. The docs explain this nicely: https://developer.mozilla.org/en-US/docs/Web/API/Touch/target > The read-only target property of the Touch interface returns the `EventTarget` on which the touch contact started when it was first placed on the surface, even if the touch point has since moved outside the interactive area of that element or even been removed from the document. **Note that if the target element is removed from the document, events will still be targeted at it, and hence won't necessarily bubble up to the window or document anymore.** If there is any risk of an element being removed while it is being touched, the best practice is to attach the touch listeners directly to the target. ## problem 4 `<ReorderCell />` accidentally renders the children twice in some cases because we accidentally passed its' children to its child renderer... Check this comment -- schrodinger/fixed-data-table-2#706 (comment) -- for details.
1 parent fa8a11e commit 31ed76d

10 files changed

+98
-20
lines changed

examples/ColumnGroupsResizeReorderExample.js

+1
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ class ColumnGroupsExample extends React.Component {
156156
rowsCount={dataList.getSize()}
157157
width={1000}
158158
height={500}
159+
touchScrollEnabled={true}
159160
{...this.props}
160161
>
161162
{this.state.columnGroupOrder.map(function (columnGroupKey, i) {

examples/ReorderExample.js

+1
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ class ReorderExample extends React.Component {
9090
rowsCount={dataList.getSize()}
9191
width={1000}
9292
height={500}
93+
touchScrollEnabled={true}
9394
{...this.props}
9495
>
9596
{this.state.columnOrder.map(function (columnKey, i) {

src/FixedDataTable.js

+1
Original file line numberDiff line numberDiff line change
@@ -794,6 +794,7 @@ class FixedDataTable extends React.Component {
794794
fixedRightColumns={fixedRightColumnGroups}
795795
scrollableColumns={scrollableColumnGroups}
796796
visible={true}
797+
touchEnabled={touchScrollEnabled}
797798
onColumnResizeEndCallback={onColumnResizeEndCallback}
798799
onColumnReorderEndCallback={onColumnReorderEndCallback}
799800
showScrollbarY={scrollEnabledY}

src/FixedDataTableCell.js

+2
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ class FixedDataTableCell extends React.Component {
177177
isVisible,
178178
columnKey,
179179
isHeaderOrFooter,
180+
touchEnabled,
180181
...props
181182
} = this.props;
182183

@@ -205,6 +206,7 @@ class FixedDataTableCell extends React.Component {
205206
);
206207

207208
let cellProps = {
209+
touchEnabled,
208210
isVisible,
209211
isHeader: this.props.isHeader,
210212
isGroupHeader: this.props.isGroupHeader,

src/FixedDataTableCellDefault.js

+1
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ class FixedDataTableCellDefault extends React.Component {
9191
isGroupHeader,
9292
maxWidth,
9393
minWidth,
94+
touchEnabled,
9495
...props
9596
} = this.props;
9697

src/FixedDataTableCellDefaultDeprecated.js

+1
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ class FixedDataTableCellDefault extends React.Component {
9696
isGroupHeader,
9797
maxWidth,
9898
minWidth,
99+
touchEnabled,
99100
...props
100101
} = this.props;
101102

src/plugins/ResizeReorder/ReorderCell.js

+13-4
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,10 @@ class ReorderCell extends React.PureComponent {
115115
onColumnReorderStart,
116116
onColumnReorderEnd,
117117
reorderStartEvent,
118+
children,
118119
...props
119120
} = this.props;
120121

121-
const { children, left } = props;
122-
123122
let className = joinClasses(
124123
cx({
125124
'public/fixedDataTableCell/resizeReorderCellContainer': true,
@@ -167,17 +166,27 @@ class ReorderCell extends React.PureComponent {
167166

168167
return (
169168
<div
169+
ref={this.setReorderHandle}
170170
className={cx({
171171
'fixedDataTableCellLayout/columnReorderContainer': true,
172172
'fixedDataTableCellLayout/columnReorderContainer/active': false,
173173
})}
174-
onMouseDown={this.onMouseDown}
175-
onTouchStart={this.onTouchStart}
176174
style={style}
177175
/>
178176
);
179177
}
180178

179+
setReorderHandle = (element) => {
180+
if (element) {
181+
element.addEventListener('mousedown', this.onMouseDown, {
182+
passive: false,
183+
});
184+
element.addEventListener('touchstart', this.onTouchStart, {
185+
passive: false,
186+
});
187+
}
188+
};
189+
181190
onTouchStart = (ev) => {
182191
if (!this.props.touchEnabled) {
183192
return;

src/plugins/ResizeReorder/ResizerKnob.js

+66-8
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class ResizerKnob extends React.PureComponent {
4848
* Ref to ResizerKnob
4949
* @type {HTMLDivElement}
5050
*/
51-
curRef = null;
51+
resizerKnobRef = null;
5252

5353
/**
5454
*
@@ -62,8 +62,14 @@ class ResizerKnob extends React.PureComponent {
6262

6363
componentDidMount() {
6464
this.setState({
65-
top: this.curRef.getBoundingClientRect().top,
65+
top: this.resizerKnobRef.getBoundingClientRect().top,
6666
});
67+
68+
this.setupHandlers();
69+
}
70+
71+
componentWillUnmount() {
72+
this.cleanupHandlers();
6773
}
6874

6975
render() {
@@ -82,18 +88,58 @@ class ResizerKnob extends React.PureComponent {
8288
return (
8389
<div
8490
className={cx('fixedDataTableCellLayout/columnResizerContainer')}
85-
ref={(element) => (this.curRef = element)}
91+
ref={this.setResizerKnobRef}
8692
style={resizerKnobStyle}
87-
onMouseDown={this.onMouseDown}
88-
onTouchStart={this.props.touchEnabled ? this.onMouseDown : null}
89-
onTouchEnd={this.props.touchEnabled ? this.suppressEvent : null}
90-
onTouchMove={this.props.touchEnabled ? this.suppressEvent : null}
9193
>
9294
{resizerLine}
9395
</div>
9496
);
9597
}
9698

99+
setResizerKnobRef = (element) => {
100+
this.resizerKnobRef = element;
101+
};
102+
103+
setupHandlers() {
104+
// TODO (pradeep): Remove these and pass to our knob component directly after React
105+
// provides an API where event handlers can be specified to be non-passive (facebook/react#6436).
106+
this.resizerKnobRef.addEventListener('mousedown', this.onMouseDown, {
107+
passive: false,
108+
});
109+
this.resizerKnobRef.addEventListener('touchstart', this.onTouchStart, {
110+
passive: false,
111+
});
112+
this.resizerKnobRef.addEventListener(
113+
'touchmove',
114+
this.suppressEventIfInTouchMode,
115+
{ passive: false }
116+
);
117+
this.resizerKnobRef.addEventListener(
118+
'touchend',
119+
this.suppressEventIfInTouchMode,
120+
{ passive: false }
121+
);
122+
}
123+
124+
cleanupHandlers() {
125+
this.resizerKnobRef.removeEventListener('mousedown', this.onMouseDown, {
126+
passive: false,
127+
});
128+
this.resizerKnobRef.removeEventListener('touchstart', this.onTouchStart, {
129+
passive: false,
130+
});
131+
this.resizerKnobRef.removeEventListener(
132+
'touchmove',
133+
this.suppressEventIfInTouchMode,
134+
{ passive: false }
135+
);
136+
this.resizerKnobRef.removeEventListener(
137+
'touchend',
138+
this.suppressEventIfInTouchMode,
139+
{ passive: false }
140+
);
141+
}
142+
97143
/**
98144
* Registers event listeners for mouse tracking
99145
* @param {MouseEvent} event
@@ -108,6 +154,15 @@ class ResizerKnob extends React.PureComponent {
108154
this.mouseMoveTracker.captureMouseMoves(event);
109155
};
110156

157+
/**
158+
* @param {TouchEvent} event The touch start event
159+
*/
160+
onTouchStart = (event) => {
161+
if (this.props.touchEnabled) {
162+
this.onMouseDown(event);
163+
}
164+
};
165+
111166
/**
112167
* @param {MouseEvent} ev Mouse down event
113168
*/
@@ -185,7 +240,10 @@ class ResizerKnob extends React.PureComponent {
185240
/**
186241
* @param {Object} event
187242
*/
188-
suppressEvent = (event) => {
243+
suppressEventIfInTouchMode = (event) => {
244+
if (!this.props.touchEnabled) {
245+
return;
246+
}
189247
event.preventDefault();
190248
event.stopPropagation();
191249
};

src/vendor_upstream/dom/DOMMouseMoveTracker.js

+8-5
Original file line numberDiff line numberDiff line change
@@ -86,17 +86,20 @@ class DOMMouseMoveTracker {
8686
this._eventTouchStartToken = EventListener.listen(
8787
this._domNode,
8888
'touchstart',
89-
this._onMouseMove
89+
this._onMouseMove,
90+
{ passive: false }
9091
);
9192
this._eventTouchMoveToken = EventListener.listen(
92-
this._domNode,
93+
event.target,
9394
'touchmove',
94-
this._onMouseMove
95+
this._onMouseMove,
96+
{ passive: false }
9597
);
9698
this._eventTouchEndToken = EventListener.listen(
97-
this._domNode,
99+
event.target,
98100
'touchend',
99-
this._onMouseUp
101+
this._onMouseUp,
102+
{ passive: false }
100103
);
101104
}
102105

src/vendor_upstream/stubs/EventListener.js

+4-3
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,15 @@ const EventListener = {
2323
* @param {DOMEventTarget} target DOM element to register listener on.
2424
* @param {string} eventType Event type, e.g. 'click' or 'mouseover'.
2525
* @param {function} callback Callback function.
26+
* @param {object} options Extra options to customize the listener
2627
* @return {object} Object with a `remove` method.
2728
*/
28-
listen(target, eventType, callback) {
29+
listen(target, eventType, callback, options = {}) {
2930
if (target.addEventListener) {
30-
target.addEventListener(eventType, callback, false);
31+
target.addEventListener(eventType, callback, options || false);
3132
return {
3233
remove() {
33-
target.removeEventListener(eventType, callback, false);
34+
target.removeEventListener(eventType, callback, options || false);
3435
},
3536
};
3637
} else if (target.attachEvent) {

0 commit comments

Comments
 (0)