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.

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

[FlexTable] onRowClick and onRowDoubleClick not working always #322

Closed
levrik opened this issue Jul 23, 2016 · 19 comments
Closed

[FlexTable] onRowClick and onRowDoubleClick not working always #322

levrik opened this issue Jul 23, 2016 · 19 comments

Comments

@levrik
Copy link

levrik commented Jul 23, 2016

Hey there. Me again.
If I scroll a bit down in a FlexTable and try to click or double click on a row the handler does not get called. If I try again the handler does get called.
Here's a plunker: https://plnkr.co/edit/gAEv8xgCjCJ3IZtJTlx5
And a GIF demonstrating the problem:
2016-07-23_10-49-31

@levrik
Copy link
Author

levrik commented Jul 23, 2016

@bvaughn Here a another GIF demonstrating the source of this issue:
2016-07-23_10-55-04
There's sometimes a delay when switching back from pointer-events: none to pointer-events: auto

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2016

pointer-events are toggled off when scrolling for performance reasons. They're re-enabled a short time (150ms) after the final scroll event is received.

While pointer events are disabled, clicks are ignored, as you've observed. Not sure there's much I can do about this though to be honest. Perhaps I could lower the interval slightly before they're reenabled...

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2016

This is a common optimization technique. For example, fixed-data-table does this too (condition, timeout defaults to 200ms) as does react-infinite (condition, timeout defaults to 150ms)

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2016

Even if I lowered the interval slightly- something I'm reluctant to do, for fear of turning off prematurely- there's still going to be some time when the browser is performing the scroll animation when users would not be able to click on rows.

I think the best thing to do in this case is to use the isScrolling parameter passed to the cell renderer to change the style of cells slightly to make it visually clear when they're clickable vs not clickable.

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2016

My guess is that it's fairly unlikely that users are scrolling and clicking at the same time. It's kind of an awkward interaction. (Human brains need a bit of time to process what they're seeing when data has changed.) I suspect this may be something that we do as engineers that real users don't actually do?

@levrik
Copy link
Author

levrik commented Jul 23, 2016

@bvaughn As you can see on the GIF there is much more time between the final scroll event and the re-enabling. About 800-1000ms. I also experienced in my application a delay by about 2 seconds. I will analyze this in production if it's a problem or not :)

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2016

The gif doesn't show much, to be honest. Because it doesn't show when you're clicking. (There's no indication of ignored clicks.)

The code for this is pretty straight forward. Each time Grid receives a scroll event, it registers a timeout to re-enable scroll events This is the only place this method is called, and it's called on every scroll.

Browsers manage a smooth scrolling animation that has kind of a long tail as it slows down and stops. Maybe you're perceiving the end of scrolling before it's actually done. (Maybe the browser fires a last few micro-adjustments.) I'm fairly confident this is what's happening. The last ~second of scrolling is just the tail end of the animation, not very noticeable.

Also keep in mind that setTimeout isn't guaranteed to invoke the callback at exactly the specified time. It's only guaranteed not to invoke it before the specified time. If a lot of heavy crunching is going on (not likely from the look of your Plunkr) that can cause timeouts to be invoked later. Nothing I can do about that if that's the case.

@levrik
Copy link
Author

levrik commented Jul 23, 2016

Yeah I unterstand. It looked at first like an issue with the library to me. I will close this now and open a new issue if this is really a problem. Like you said: It's maybe something a real user don't actually do.

@levrik levrik closed this as completed Jul 23, 2016
@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2016

😅 Sounds good to me.

@mikhail-eremin
Copy link

mikhail-eremin commented Oct 6, 2016

Have the same issue with react-virtulized 7.16.0
Sometimes, after scroll, "pointer-events:none" isn't removed from Grid__innerScrollContainer, so items are unavailable for a while after scroll.
May be it's connected somehow with macOs scroll inertia

@levrik
Copy link
Author

levrik commented Oct 6, 2016

@bvaughn I will reopen this. Experienced this issue now in production. Users don't see this as a problem with the application, but they are trying a second time the double click action. This is not the ideal situation.

@levrik levrik reopened this Oct 6, 2016
@bvaughn
Copy link
Owner

bvaughn commented Oct 6, 2016

Sometimes, after scroll, "pointer-events:none" isn't removed from Grid__innerScrollContainer, so items are unavailable for a while after scroll.

@mikhail-eremin I have never seen this happen and I've used react-virtualized a lot. If you could provide a repro case then I'll be happy to take a look. Else I'm a little skeptical because the code for this is fairly straight forward (see here, here, and here).

Have you specified a custom/override value for scrollingResetTimeInterval?

You aren't, by chance, using WindowScroller are you? (That logic is a bit more complicated.)

@levrik Disabling pointer events while scrolling is something all of the windowing libraries do (see fixed-data-table, react-infinite, etc). However if you want to prevent (or change) this behavior, you have 2 options:

  1. Set pointerEvents: 'auto' on your cell/row/column renderer. (This overrides the parent value.)
  2. Try reducing the scrollingResetTimeInterval interval. It's 150ms by default which more or less aligns with the others (eg fixed-data-table uses 200ms, react-infinite uses 150ms) but you could try reducing it if you think users are clicking "too soon".

In general though, I don't plan on removing this feature as the default behavior.

@levrik
Copy link
Author

levrik commented Oct 6, 2016

@bvaughn Yeah. I understand why react-virtualizedand the other libs do this.

I investigated this issue a bit more precisely now. It seems that the issue is something with Chrome (at least in my case).
I remove items on double click. The pointer-evens changes from auto to none and back to auto. Now if I double click on the next item which moved up under the cursor, Chome doesn't regnonize the double click. I need to move the mouse before the double click, which some users don't do.
I will close this again. The initial problem seems not to cause this issue anymore and this is definitly an Chrome issue.

@levrik levrik closed this as completed Oct 6, 2016
@bvaughn
Copy link
Owner

bvaughn commented Oct 6, 2016

Ah. Interesting. Are you saying that unless you move your mouse a little bit before the click- the browser doesn't pick up on it?

@levrik
Copy link
Author

levrik commented Oct 6, 2016

@bvaughn Yep. Exactly.

@bvaughn
Copy link
Owner

bvaughn commented Oct 6, 2016

Hmm... that sounds like a browser security feature. Maybe intended to prevent malicious sites from tricking a user into clicking something he didn't intend to by changing what was under a cursor.

@levrik
Copy link
Author

levrik commented Oct 6, 2016

@bvaughn Makes sense. I will investigate this a bit more some day :)

@bvaughn
Copy link
Owner

bvaughn commented Oct 6, 2016

Sounds good. 😄

@mikhail-eremin
Copy link

@bvaughn I will try you suggestions thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants