Fix touch handlers and make them passive #706

pradeepnschrodinger · 2023-11-10T19:44:14Z

Description

Touch support in FDT v2 is wonky.
There's multiple issues here:

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

Motivation and Context

Fixes #705

How Has This Been Tested?

Tested with local examples, and also on codesandbox: https://codesandbox.io/s/fixed-data-table-2-touch-resizing-s22nn7
TODO: Test within LiveDesign

Screenshots (if appropriate):

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.

pradeepnschrodinger · 2023-11-10T19:53:37Z

examples/ColumnGroupsResizeReorderExample.js

@@ -156,6 +156,7 @@ class ColumnGroupsExample extends React.Component {
        rowsCount={dataList.getSize()}
        width={1000}
        height={500}
+        touchScrollEnabled={true}


Updated resize-reorder column group example to work on touch devices

pradeepnschrodinger · 2023-11-10T19:53:49Z

examples/ReorderExample.js

@@ -90,6 +90,7 @@ class ReorderExample extends React.Component {
        rowsCount={dataList.getSize()}
        width={1000}
        height={500}
+        touchScrollEnabled={true}


Updated basic reorder example to work on touch devices

pradeepnschrodinger · 2023-11-10T19:54:20Z

src/FixedDataTable.js

@@ -794,6 +794,7 @@ class FixedDataTable extends React.Component {
          fixedRightColumns={fixedRightColumnGroups}
          scrollableColumns={scrollableColumnGroups}
          visible={true}
+          touchEnabled={touchScrollEnabled}


We forgot to pass touchEnabled to the group header...

pradeepnschrodinger · 2023-11-10T19:54:52Z

src/FixedDataTableCell.js

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

    let cellProps = {
+      touchEnabled,


This passes touchEnabled to users' cell renderers.

pradeepnschrodinger · 2023-11-10T19:55:11Z

src/FixedDataTableCellDefault.js

@@ -91,6 +91,7 @@ class FixedDataTableCellDefault extends React.Component {
      isGroupHeader,
      maxWidth,
      minWidth,
+      touchEnabled,


Avoid passing touchEnabled to the div.

does it cause an issue?

pradeepnschrodinger · 2023-11-10T19:58:00Z

src/plugins/ResizeReorder/ReorderCell.js

@@ -115,11 +115,10 @@ class ReorderCell extends React.PureComponent {
      onColumnReorderStart,
      onColumnReorderEnd,
      reorderStartEvent,
+      children,


This takes out children outside of props.

Without this change, ReorderCell calls up children(props) which'll in turn result in children being duplicated twice, cause props contains props.children 🤦 .
I saw this live on ColumnGroupsResizeReorderExample.

pradeepnschrodinger · 2023-11-10T19:59:56Z

src/plugins/ResizeReorder/ReorderCell.js

-        onMouseDown={this.onMouseDown}
-        onTouchStart={this.onTouchStart}


We need these to be defined as non-passive so that FDT can default-prevent or stop-propagate on them if required.
So I'm registering these manually by using addEventListener.

pradeepnschrodinger · 2023-11-10T20:02:22Z

src/plugins/ResizeReorder/ResizerKnob.js

-        onMouseDown={this.onMouseDown}
-        onTouchStart={this.props.touchEnabled ? this.onMouseDown : null}
-        onTouchEnd={this.props.touchEnabled ? this.suppressEvent : null}
-        onTouchMove={this.props.touchEnabled ? this.suppressEvent : null}


Same reasoning as above.
I moved the registration to setupHandlers() and deregistration to cleanupHandlers.

pradeepnschrodinger · 2023-11-10T20:04:32Z

src/vendor_upstream/dom/DOMMouseMoveTracker.js

+        event.target,
        'touchmove',
-        this._onMouseMove
+        this._onMouseMove,
+        { passive: false }
      );
      this._eventTouchEndToken = EventListener.listen(
-        this._domNode,
+        event.target,
        'touchend',
-        this._onMouseUp
+        this._onMouseUp,
+        { passive: false }


I'm registering these at the source element that triggered the touch (i.e, event.target) so we don't abruptly loose the touch handling just because the source element got removed.

TODO: Add a comment explaining this and link to moz docs.

pradeepnschrodinger · 2023-11-10T20:04:44Z

src/vendor_upstream/stubs/EventListener.js

   * @return {object} Object with a `remove` method.
   */
-  listen(target, eventType, callback) {
+  listen(target, eventType, callback, options = {}) {


Added options param to let the caller customize the listener.

karry08

LGTM

pradeepnschrodinger · 2023-11-16T07:31:34Z

Released with v2.0.3.

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.

pradeepnschrodinger added 2 commits November 10, 2023 23:33

Fix touch handlers and make them passive

4d87da2

Remove touchEnabled from apiData

4425222

pradeepnschrodinger requested review from abhisheksingh2410, karry08 and AmanGupta2708 November 10, 2023 19:44

pradeepnschrodinger mentioned this pull request Nov 10, 2023

Column resize doesn't properly work on mobile #705

Closed

pradeepnschrodinger added bug regression issue that was "fixed" but got reopened later on labels Nov 10, 2023

pradeepnschrodinger commented Nov 10, 2023

View reviewed changes

pradeepnschrodinger assigned karry08, AmanGupta2708 and abhisheksingh2410 Nov 10, 2023

Add todo

c42228b

karry08 approved these changes Nov 14, 2023

View reviewed changes

pradeepnschrodinger merged commit 11b82dd into master Nov 16, 2023

pradeepnschrodinger deleted the fix-touch-handlers branch November 16, 2023 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix touch handlers and make them passive #706

Fix touch handlers and make them passive #706

pradeepnschrodinger commented Nov 10, 2023 •

edited

Loading

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

karry08 Nov 14, 2023

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

pradeepnschrodinger Nov 10, 2023

karry08 left a comment

pradeepnschrodinger commented Nov 16, 2023 •

edited

Loading

		onMouseDown={this.onMouseDown}
		onTouchStart={this.onTouchStart}

Fix touch handlers and make them passive #706

Fix touch handlers and make them passive #706

Conversation

pradeepnschrodinger commented Nov 10, 2023 • edited Loading

Description

problem 1

problem 2

problem 3

problem 4

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karry08 left a comment

Choose a reason for hiding this comment

pradeepnschrodinger commented Nov 16, 2023 • edited Loading

pradeepnschrodinger commented Nov 10, 2023 •

edited

Loading

pradeepnschrodinger commented Nov 16, 2023 •

edited

Loading