From 11b82dd35ce59ebb0db16b6ba0001462ceaeae58 Mon Sep 17 00:00:00 2001
From: Niranjan Pradeep <41563608+pradeepnschrodinger@users.noreply.github.com>
Date: Thu, 16 Nov 2023 12:52:42 +0530
Subject: [PATCH] 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 `` 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
`` accidentally renders the children twice in some cases because we accidentally passed its' children to its child renderer...
Check this comment -- https://github.com/schrodinger/fixed-data-table-2/pull/706#discussion_r1389857673 -- for details.
---
examples/ColumnGroupsResizeReorderExample.js | 1 +
examples/ReorderExample.js | 1 +
src/FixedDataTable.js | 1 +
src/FixedDataTableCell.js | 2 +
src/FixedDataTableCellDefault.js | 1 +
src/FixedDataTableCellDefaultDeprecated.js | 1 +
src/plugins/ResizeReorder/ReorderCell.js | 17 ++++-
src/plugins/ResizeReorder/ResizerKnob.js | 74 +++++++++++++++++--
.../dom/DOMMouseMoveTracker.js | 13 ++--
src/vendor_upstream/stubs/EventListener.js | 7 +-
10 files changed, 98 insertions(+), 20 deletions(-)
diff --git a/examples/ColumnGroupsResizeReorderExample.js b/examples/ColumnGroupsResizeReorderExample.js
index b22134f7..125172b8 100644
--- a/examples/ColumnGroupsResizeReorderExample.js
+++ b/examples/ColumnGroupsResizeReorderExample.js
@@ -156,6 +156,7 @@ class ColumnGroupsExample extends React.Component {
rowsCount={dataList.getSize()}
width={1000}
height={500}
+ touchScrollEnabled={true}
{...this.props}
>
{this.state.columnGroupOrder.map(function (columnGroupKey, i) {
diff --git a/examples/ReorderExample.js b/examples/ReorderExample.js
index a3b73dda..a3c18f67 100644
--- a/examples/ReorderExample.js
+++ b/examples/ReorderExample.js
@@ -90,6 +90,7 @@ class ReorderExample extends React.Component {
rowsCount={dataList.getSize()}
width={1000}
height={500}
+ touchScrollEnabled={true}
{...this.props}
>
{this.state.columnOrder.map(function (columnKey, i) {
diff --git a/src/FixedDataTable.js b/src/FixedDataTable.js
index c7e7a964..f3cb1e79 100644
--- a/src/FixedDataTable.js
+++ b/src/FixedDataTable.js
@@ -794,6 +794,7 @@ class FixedDataTable extends React.Component {
fixedRightColumns={fixedRightColumnGroups}
scrollableColumns={scrollableColumnGroups}
visible={true}
+ touchEnabled={touchScrollEnabled}
onColumnResizeEndCallback={onColumnResizeEndCallback}
onColumnReorderEndCallback={onColumnReorderEndCallback}
showScrollbarY={scrollEnabledY}
diff --git a/src/FixedDataTableCell.js b/src/FixedDataTableCell.js
index 0420dabb..1a666481 100644
--- a/src/FixedDataTableCell.js
+++ b/src/FixedDataTableCell.js
@@ -177,6 +177,7 @@ class FixedDataTableCell extends React.Component {
isVisible,
columnKey,
isHeaderOrFooter,
+ touchEnabled,
...props
} = this.props;
@@ -205,6 +206,7 @@ class FixedDataTableCell extends React.Component {
);
let cellProps = {
+ touchEnabled,
isVisible,
isHeader: this.props.isHeader,
isGroupHeader: this.props.isGroupHeader,
diff --git a/src/FixedDataTableCellDefault.js b/src/FixedDataTableCellDefault.js
index 1bc4b307..fbcfe76e 100644
--- a/src/FixedDataTableCellDefault.js
+++ b/src/FixedDataTableCellDefault.js
@@ -91,6 +91,7 @@ class FixedDataTableCellDefault extends React.Component {
isGroupHeader,
maxWidth,
minWidth,
+ touchEnabled,
...props
} = this.props;
diff --git a/src/FixedDataTableCellDefaultDeprecated.js b/src/FixedDataTableCellDefaultDeprecated.js
index 9ed4ed7d..7808de87 100644
--- a/src/FixedDataTableCellDefaultDeprecated.js
+++ b/src/FixedDataTableCellDefaultDeprecated.js
@@ -96,6 +96,7 @@ class FixedDataTableCellDefault extends React.Component {
isGroupHeader,
maxWidth,
minWidth,
+ touchEnabled,
...props
} = this.props;
diff --git a/src/plugins/ResizeReorder/ReorderCell.js b/src/plugins/ResizeReorder/ReorderCell.js
index 2f308238..3ab49e00 100644
--- a/src/plugins/ResizeReorder/ReorderCell.js
+++ b/src/plugins/ResizeReorder/ReorderCell.js
@@ -115,11 +115,10 @@ class ReorderCell extends React.PureComponent {
onColumnReorderStart,
onColumnReorderEnd,
reorderStartEvent,
+ children,
...props
} = this.props;
- const { children, left } = props;
-
let className = joinClasses(
cx({
'public/fixedDataTableCell/resizeReorderCellContainer': true,
@@ -167,17 +166,27 @@ class ReorderCell extends React.PureComponent {
return (
);
}
+ setReorderHandle = (element) => {
+ if (element) {
+ element.addEventListener('mousedown', this.onMouseDown, {
+ passive: false,
+ });
+ element.addEventListener('touchstart', this.onTouchStart, {
+ passive: false,
+ });
+ }
+ };
+
onTouchStart = (ev) => {
if (!this.props.touchEnabled) {
return;
diff --git a/src/plugins/ResizeReorder/ResizerKnob.js b/src/plugins/ResizeReorder/ResizerKnob.js
index 6e5163db..5cb23821 100644
--- a/src/plugins/ResizeReorder/ResizerKnob.js
+++ b/src/plugins/ResizeReorder/ResizerKnob.js
@@ -48,7 +48,7 @@ class ResizerKnob extends React.PureComponent {
* Ref to ResizerKnob
* @type {HTMLDivElement}
*/
- curRef = null;
+ resizerKnobRef = null;
/**
*
@@ -62,8 +62,14 @@ class ResizerKnob extends React.PureComponent {
componentDidMount() {
this.setState({
- top: this.curRef.getBoundingClientRect().top,
+ top: this.resizerKnobRef.getBoundingClientRect().top,
});
+
+ this.setupHandlers();
+ }
+
+ componentWillUnmount() {
+ this.cleanupHandlers();
}
render() {
@@ -82,18 +88,58 @@ class ResizerKnob extends React.PureComponent {
return (
(this.curRef = element)}
+ ref={this.setResizerKnobRef}
style={resizerKnobStyle}
- onMouseDown={this.onMouseDown}
- onTouchStart={this.props.touchEnabled ? this.onMouseDown : null}
- onTouchEnd={this.props.touchEnabled ? this.suppressEvent : null}
- onTouchMove={this.props.touchEnabled ? this.suppressEvent : null}
>
{resizerLine}
);
}
+ setResizerKnobRef = (element) => {
+ this.resizerKnobRef = element;
+ };
+
+ setupHandlers() {
+ // TODO (pradeep): Remove these and pass to our knob component directly after React
+ // provides an API where event handlers can be specified to be non-passive (facebook/react#6436).
+ this.resizerKnobRef.addEventListener('mousedown', this.onMouseDown, {
+ passive: false,
+ });
+ this.resizerKnobRef.addEventListener('touchstart', this.onTouchStart, {
+ passive: false,
+ });
+ this.resizerKnobRef.addEventListener(
+ 'touchmove',
+ this.suppressEventIfInTouchMode,
+ { passive: false }
+ );
+ this.resizerKnobRef.addEventListener(
+ 'touchend',
+ this.suppressEventIfInTouchMode,
+ { passive: false }
+ );
+ }
+
+ cleanupHandlers() {
+ this.resizerKnobRef.removeEventListener('mousedown', this.onMouseDown, {
+ passive: false,
+ });
+ this.resizerKnobRef.removeEventListener('touchstart', this.onTouchStart, {
+ passive: false,
+ });
+ this.resizerKnobRef.removeEventListener(
+ 'touchmove',
+ this.suppressEventIfInTouchMode,
+ { passive: false }
+ );
+ this.resizerKnobRef.removeEventListener(
+ 'touchend',
+ this.suppressEventIfInTouchMode,
+ { passive: false }
+ );
+ }
+
/**
* Registers event listeners for mouse tracking
* @param {MouseEvent} event
@@ -108,6 +154,15 @@ class ResizerKnob extends React.PureComponent {
this.mouseMoveTracker.captureMouseMoves(event);
};
+ /**
+ * @param {TouchEvent} event The touch start event
+ */
+ onTouchStart = (event) => {
+ if (this.props.touchEnabled) {
+ this.onMouseDown(event);
+ }
+ };
+
/**
* @param {MouseEvent} ev Mouse down event
*/
@@ -185,7 +240,10 @@ class ResizerKnob extends React.PureComponent {
/**
* @param {Object} event
*/
- suppressEvent = (event) => {
+ suppressEventIfInTouchMode = (event) => {
+ if (!this.props.touchEnabled) {
+ return;
+ }
event.preventDefault();
event.stopPropagation();
};
diff --git a/src/vendor_upstream/dom/DOMMouseMoveTracker.js b/src/vendor_upstream/dom/DOMMouseMoveTracker.js
index d46b9374..8cd936f5 100644
--- a/src/vendor_upstream/dom/DOMMouseMoveTracker.js
+++ b/src/vendor_upstream/dom/DOMMouseMoveTracker.js
@@ -86,17 +86,20 @@ class DOMMouseMoveTracker {
this._eventTouchStartToken = EventListener.listen(
this._domNode,
'touchstart',
- this._onMouseMove
+ this._onMouseMove,
+ { passive: false }
);
this._eventTouchMoveToken = EventListener.listen(
- this._domNode,
+ event.target,
'touchmove',
- this._onMouseMove
+ this._onMouseMove,
+ { passive: false }
);
this._eventTouchEndToken = EventListener.listen(
- this._domNode,
+ event.target,
'touchend',
- this._onMouseUp
+ this._onMouseUp,
+ { passive: false }
);
}
diff --git a/src/vendor_upstream/stubs/EventListener.js b/src/vendor_upstream/stubs/EventListener.js
index fc3c02fd..3612ebe8 100644
--- a/src/vendor_upstream/stubs/EventListener.js
+++ b/src/vendor_upstream/stubs/EventListener.js
@@ -23,14 +23,15 @@ const EventListener = {
* @param {DOMEventTarget} target DOM element to register listener on.
* @param {string} eventType Event type, e.g. 'click' or 'mouseover'.
* @param {function} callback Callback function.
+ * @param {object} options Extra options to customize the listener
* @return {object} Object with a `remove` method.
*/
- listen(target, eventType, callback) {
+ listen(target, eventType, callback, options = {}) {
if (target.addEventListener) {
- target.addEventListener(eventType, callback, false);
+ target.addEventListener(eventType, callback, options || false);
return {
remove() {
- target.removeEventListener(eventType, callback, false);
+ target.removeEventListener(eventType, callback, options || false);
},
};
} else if (target.attachEvent) {