Fixed column resize on Android. #280

davidhaimson · 2018-01-12T23:21:09Z

Description

In FixedDataTableCell.js, fixed event handling in columnResizerComponent. The ignored events must have event.preventDefault called as well as event.stopPropagation.
In vendor_upstream/dom/DOMMouseMoveTracker.js, removed handling for mouse out event.

Motivation and Context

Release 0.8.8 makes column resize work on iOS, and almost makes column resize work on Android. Our app requires that this feature work on Android.

Fixes #277

I have not modified the width of fixedDataTableCell_columnResizerKnob. Instead, I styled .public_fixedDataTableCell_columnResizerKnob in our app, increasing its width to 10px, but styling it with a linear gradient so it doesn't look twice as big.

How Has This Been Tested?

I tested this change, using our fork of fixed-data-table, in our app, on the following platforms:

Chrome/Android
Cordova/Android
Safari/iOS11
Cordova/iOS11
Chrome/Mac
Safari/Mac
Firefox/Mac
IE11/Win7
Edge/Win10
Both column resize and touch scroll are configured.

npm run test: all succeed.

npm run site-dev-server: all applicable demos work as expected. There really needs to be a demo with both column resize and touch scroll configured. I'm glad to add one, but I have not done so yet.

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. (I think)
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

1. In FixedDataTableCell: - in columnResizerComponent touch handlers, also preventDefault. - in _onColumnResizerMouseDown, also preventDefault.

1. In DOMMouseMoveTracker.js, eliminate handling the mouseout event. [The mouseout event is fired when a pointing device (usually a mouse) is moved off the element that has the listener attached or off one of its children.](https://developer.mozilla.org/en-US/docs/Web/Events/mouseout) Since the element that has the listener attached is document.body, this invariably does the wrong thing.

wcjordan · 2018-01-14T19:00:03Z

src/FixedDataTableCell.js

      columnResizerComponent = (
        <div
          className={cx('fixedDataTableCellLayout/columnResizerContainer')}
          style={columnResizerStyle}
          onMouseDown={this._onColumnResizerMouseDown}
          onTouchStart={this.props.touchEnabled ? this._onColumnResizerMouseDown : null}
-          onTouchEnd={this.props.touchEnabled ? e => e.stopPropagation() : null}


@SirGandal can you give this a quick look that it doesn't break your use case?

@wcjordan sorry for the late reply. That should be fine.
@davidhaimson out of curiosity, what's the reason for preventDefault in onTouchEnd and onTouchMove?

First thing I tried was adding event.preventDefault() in the _onColumnResizerMouseDown method. That made a big difference on the Android devices where I was testing this. Without it, trying to resize the columns scrolled the table body left/right. I made the corresponding change here for consistency.

Because of my injury, I am working from home, and I don't have the physical devices to test on. I tried removing all the preventDefault calls, and it still works, on all the platforms, except I had to test Android and iOS in emulators. Now I don't know what to think.

I was able to test the version with all preventDefault calls in the modified demo app on a physical iPhone and Android phone, and they worked fine. My intuition is that these calls are at the very least harmless, and the one in _onColumnResizerMouseDown is necessary. But I can't prove it until I get back to work on site. Not sure when that will be - a couple of weeks, probably.

What do you want to do here? Should we wait til you're back on site and can test fully before finalizing this?

We can do that, but as I say, it'll be a couple of weeks. I already tested it fully (with the harmless, but possibly superfluous preventDefault calls). My recommendation would be to merge it in its present form. When I am back on-site, we can do a PR trimming it down to the minimum necessary.

Sounds good. I'll merge and look to release over the weekend. Thanks

You're a prince! Thanks.

wcjordan · 2018-01-14T19:23:27Z

src/vendor_upstream/dom/DOMMouseMoveTracker.js

@@ -71,11 +70,6 @@ class DOMMouseMoveTracker {
        'mouseleave',
        this._onMouseEnd
      );
-      this._eventOutToken = EventListener.listen(


What's the reason for removing this rather than just fixing it?

It was added to fix #104, could you test out those cases on a few browsers? I tried in Chrome & Firefox and it seems the mouseleave event is enough for the fix to hold. @sisyphean2 do you remember why you added both mouseleave & mouseout handlers? Are there cases we should test?

Note that because the mouseout hander was misspelled, (this.onMouseEnd instead of this._onMouseEnd) it never did anything. Fixing the spelling makes it fail miserably. No mouseout handler is needed.

Not really any changes needed, just some research and see if anyone has a good memory of the related risks...

davidhaimson · 2018-01-19T16:09:30Z

@wcjordan: I had a bike accident, fractured a couple of bones, and I'm not going to be working again until next week. So I'm not going to be able to be as responsive as I'd like to be. Sorry! Regarding the change to FixedDataTableCell.js, I'd like to add a demo that allows column resizing and also turns on touch scrolling. I think if I start with the column resizing demo, the right thing should happen. Any advice is welcome. Regarding the change to DOMMouseMoveTracker.js, here's why: The handler is a capture handler on the body element. The mouseleave event happens when the mouse leaves any child node of the element the handler is configured on. So you start handling mousemove, and the moment the mouse leaves the 4px wide handle, _onMouseEnd is called. Lucky _onMouseEnd was misspelled there, because that could never have worked. I can't think of any reason we would want to handle mouseleave there, but let me know if there is something I'm not seeing.

wcjordan · 2018-01-20T04:26:00Z

Hey David, I'm sorry to hear that, and hope you have a swift recovery. Your explanation of the changes looks good and your idea for the example as well. It may be as simple as flipping a property on.

Let's give some time for the folks I tagged to weigh in if they want. Then we can move forward w/ merging.

Get well soon & thanks!

davidhaimson · 2018-01-22T16:08:23Z

I'm doing the work-from-home thing today, and I should be able to work on the demo later on. Thanks!

…

On Fri, Jan 19, 2018 at 9:26 PM, Chris Jordan ***@***.***> wrote: Hey David, I'm sorry to hear that, and hope you have a swift recovery. Your explanation of the changes looks good and your idea for the example as well. It may be as simple as flipping a property on. Let's give some time for the folks I tagged to weigh in if they want. Then we can move forward w/ merging. Get well soon & thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#280 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF7c-ozl2wT6kHn61_HaRm8nfslIULPCks5tMWrZgaJpZM4Rc-iI> .

davidhaimson · 2018-01-25T17:59:50Z

examples/ResizeExample.js

@@ -44,6 +44,7 @@ class ResizeExample extends React.Component {
        rowsCount={dataList.getSize()}
        onColumnResizeEndCallback={this._onColumnResizeEndCallback}
        isColumnResizing={false}
+        touchScrollEnabled={true}


I tested the Resizable Columns demo with this change on:

Chrome/Android

Safari/iOS11

Chrome/Mac (both dev device mode and desktop mode)

Safari/Mac

Firefox/Mac

IE11/Win7

Edge/Win10

Works correctly everywhere except IE11/Win7, where trying to load that demo gets the error:

SCRIPT1014: Invalid character File: main.js, Line: 16952, Column: 13

The character referred to is the first backquote in the following line in index.js:

type: `webpack${type}`,

davidhaimson · 2018-01-25T18:01:12Z

I updated the Resizable Columns demo.

wcjordan · 2018-01-30T23:35:15Z

src/FixedDataTableCell.js

      columnResizerComponent = (
        <div
          className={cx('fixedDataTableCellLayout/columnResizerContainer')}
          style={columnResizerStyle}
          onMouseDown={this._onColumnResizerMouseDown}
          onTouchStart={this.props.touchEnabled ? this._onColumnResizerMouseDown : null}
-          onTouchEnd={this.props.touchEnabled ? e => e.stopPropagation() : null}


Sounds good. I'll merge and look to release over the weekend. Thanks

wcjordan · 2018-02-07T16:46:25Z

Merged and released w/ 0.8.10

davidhaimson added 2 commits January 12, 2018 15:49

Made column resize work on all platforms.

8d62702

1. In FixedDataTableCell: - in columnResizerComponent touch handlers, also preventDefault. - in _onColumnResizerMouseDown, also preventDefault.

wcjordan previously requested changes Jan 14, 2018

View reviewed changes

Added touchScrollEnabled to ResizeExample.js.

5e8911a

davidhaimson commented Jan 25, 2018

View reviewed changes

wcjordan mentioned this pull request Jan 26, 2018

Column resize doesn't work on Android #277

Closed

wcjordan approved these changes Jan 30, 2018

View reviewed changes

wcjordan merged commit 839dc0f into schrodinger:master Feb 7, 2018

zuyetawarmatik mentioned this pull request Feb 9, 2018

Column resize problems #276

Closed

pradeepnschrodinger mentioned this pull request Jan 6, 2019

Changes for the merging range of commits [839dc0f, f3696c1] from master into v1.0-beta #377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed column resize on Android. #280

Fixed column resize on Android. #280

davidhaimson commented Jan 12, 2018

wcjordan Jan 14, 2018

SirGandal Jan 25, 2018

davidhaimson Jan 25, 2018

wcjordan Jan 30, 2018

davidhaimson Jan 30, 2018

wcjordan Jan 30, 2018

davidhaimson Jan 30, 2018

wcjordan Jan 14, 2018

davidhaimson Jan 25, 2018

davidhaimson commented Jan 19, 2018 via email

wcjordan commented Jan 20, 2018

davidhaimson commented Jan 22, 2018 via email

davidhaimson Jan 25, 2018

davidhaimson commented Jan 25, 2018

wcjordan Jan 30, 2018

wcjordan commented Feb 7, 2018

Fixed column resize on Android. #280

Fixed column resize on Android. #280

Conversation

davidhaimson commented Jan 12, 2018

Description

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

davidhaimson commented Jan 19, 2018 via email

wcjordan commented Jan 20, 2018

davidhaimson commented Jan 22, 2018 via email

Choose a reason for hiding this comment

davidhaimson commented Jan 25, 2018

Choose a reason for hiding this comment

wcjordan commented Feb 7, 2018