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

Scrolling timeout hover performance issue #697

Closed
zaunermax opened this issue Jun 6, 2017 · 15 comments
Closed

Scrolling timeout hover performance issue #697

zaunermax opened this issue Jun 6, 2017 · 15 comments

Comments

@zaunermax
Copy link

In my particular case I use the Table element and I have to set the hovered row index to the component state in order to activate special functionality in my sub-components that are rendered inside of a table row.

The hovered row index is set by using the provided onRowMouseOver prop of the Table component.

When scrolling sometimes the hover event takes several seconds to kick in (up to 2 to 3 seconds(!)) and prevents the table from being able to be hovered during that time span which is horrible for user experience as it can be seen in the following gif (note that I did not slow down, this is realtime)

hover_delay

The issue only occurs in chrome as far as i could test it, other browsers like Edge or Firefox do not have that issue. Another strange behavior of the problem is, that it seems to resolve itself. After a certain amount of time playing around with the table by scrolling and hovering over some rows the hover events behave as they should. When the page is reloaded the issue comes back though.

After research I came to the conclusion that this issue might relate to the issues #564 #322 and maybe #518 but I thought I have to open a new issue as there is no issue that exactly covers my use case.

@bvaughn
Copy link
Owner

bvaughn commented Jun 6, 2017

This is out of my control, as noted in those other issues you linked to. I'm not sure what more I can say that I've already said in those issues. If you don't want pointer events to be disabled while scrolling, you can override that behavior as noticed in the replies I linked to.

I'm closing this because it's not actionable. We can talk more here or on Slack if you have follow up questions or concerns though.

@bvaughn bvaughn closed this as completed Jun 6, 2017
@zaunermax
Copy link
Author

zaunermax commented Jun 6, 2017

Setting pointer-events to "auto" does not resolve the issue as the onRowMouseOver callback does not fire neither. It is not the pointer that is bad for user experience, it is the inability to get the visual feedback of the row being hovered. That is the reason why i opened an issue.

It is good that there is an issue though so people like me that search for that problem will find this.

@bvaughn
Copy link
Owner

bvaughn commented Jun 6, 2017

If you read my replies to the issues you mentioned above, including the one I linked to, you'll see this:

I believe this is the correct default approach for performance reasons. However if you disagree that it's appropriate for your app then you can override by passing in your own containerStyle that sets pointer-events: "auto".

@zaunermax
Copy link
Author

zaunermax commented Jun 7, 2017

I did in fact read them, but as I said, the pointer-event setting does not solve the issue nor is it a workaround.

I understand that this is maybe not actionable but it still needs to be documented or reported as this is more or less might be a deal-breaker for some people that need the hover functionality.

@bvaughn
Copy link
Owner

bvaughn commented Jun 7, 2017

If overriding the pointer-event style doesn't resolve the issue you've described, then I assume something else is going on in your application (or you're doing it incorrectly). You could provide a Plnkr, as the new-issue template requests for bug reports:

Are you reporting a bug or runtime error?

Please include either a failing unit test or a Plnkr demonstrating the bug you are reporting. You can start by forking this Plnk! https://plnkr.co/edit/6syKo8cx3RfoO96hXFT1

Your tone sounds a bit aggressive to me. Perhaps I'm interpreting it incorrectly. But please remember that I created this library and support it (by responding to issues like this) in my spare time, for free.

Here's a Plnkr that shows the workaround working: https://plnkr.co/edit/rzOpcO?p=preview

If it doesn't work for your application, you'll need to at least provide a repro case where I can see it not working before I'll look into it any further.

@zaunermax
Copy link
Author

Sorry, I definitely did not intend to sound aggressive, thank you very much for maintaining this awesome library! Once i have time i will look into it!

@MarcMagnin
Copy link

I suppose you've tried this already:

.ReactVirtualized__Grid__innerScrollContainer {
  pointer-events: auto !important;
}

This as an effect on chrome on my current UI

@MarcMagnin
Copy link

Surprisingly perf wise I can't tell the difference.

@zaunermax
Copy link
Author

zaunermax commented Jun 9, 2017

My schedule is very tight right now but at some point in time there will be enough time to fix this issue in the app. Once I find a solution I will post it here.

I suppose you've tried this already:

.ReactVirtualized__Grid__innerScrollContainer {
  pointer-events: auto !important;
}

yeah, the only effect is, that the cursor is a pointer all the time but the hover event does not get triggered...

@zaunermax
Copy link
Author

I've provided workarounds in the linked issues and in comments and a Plnkr
above. Did you try them? Did they not work?

@bvaughn I answer here as your answer on the other linked issue belongs here I think. No, the plnkr did not work because it is not the mouse hovering that does not work. Setting the pointer events will only cause the css selector hover to trigger. As i explained before i need the callback function onRowMouseOver callback function to trigger. Setting pointer-events: 'auto' sadly does not acomplish anything in that case. The problem is the browser which idles for a few seconds before triggering the next hover callback function. It could be indeed chrome's handling of the setTimeout functions that is causing the delays.

It's a terrible hack, but apparently it looks like some people have had luck listening for common global events (mouseover, mousemove, etc..), which are not artificially delayed, to see if the timer should have fired and fire it manually. I'll probably explore that too and see if it alleviates the delay.

I think i will try this and see if it accomplishes something but this is indeed a terrible hack.

@bvaughn
Copy link
Owner

bvaughn commented Jun 10, 2017

Thanks for elaborating @Maxincredible52! I didn't realize it was a callback issue. (I see now that you mentioned this before but I somehow overlooked it.) Also, I responded to your comment on the other issue via email so I didn't realize it was a continuation of this thread. 😄 Anyway, my apologies!

It's a terrible hack, but apparently it looks like some people have had luck listening for common global events (mouseover, mousemove, etc..)

React is actually already doing this for events it manages. It listens for things at the document level. Still I guess it's worth a shot to see if you get different results using a "vanilla" document event listener.

@gnbaron
Copy link
Contributor

gnbaron commented Jun 16, 2017

I'm having this same issue on my project that I'm using the Grid component. I need use the isScrolling value to know how my element is rendered, but the scroll callback is having 1.5s ~ 2s of delay to be called.

Like some people already said in this kind of issue, this is happening because the browser is delaying the setTimeout callback on _debounceScrollEnded in exchange of performance reasons.

There are plans to someone work on it ?

I tested an workarround on my project using requestAnimationFrame instead of setTimeout and works fine, but I don't know if this is the best way to do that. I was thinking in something like this:

// I just wrote it as draft, needs to be tested and refactored
_debounceScrollEnded() {
  const { scrollingResetTimeInterval } = this.props

  if (this._disablePointerEventsTimeoutId) {
    cancelAnimationFrame(this._disablePointerEventsTimeoutId)
    this._scrollDebounceStart = Date.now();
  }

  const delay = function() {
    if (Date.now() - this._scrollDebounceStart >= scrollingResetTimeInterval) {
      this._debounceScrollEndedCallback();
      this._scrollDebounceStart = Date.now();
    } else {
      this._disablePointerEventsTimeoutId = requestAnimationFrame(delay);
    }
  }

  delay.bind(this);

  this._disablePointerEventsTimeoutId = requestAnimationFrame(delay);
  this._scrollDebounceStart = Date.now();
}

@bvaughn
Copy link
Owner

bvaughn commented Jun 19, 2017

There are plans to someone work on it ?

@guilhermefloriani: I'm not planning on tackling it, no.

If you'd like to submit a PR, I'll review it though. If it seems like an improvement, I'll merge it in.

@gnbaron
Copy link
Contributor

gnbaron commented Jun 19, 2017

Ok @bvaughn. I'm little busy these days, but I'll try to work on it this week.

@zaunermax
Copy link
Author

For everybody that stumbles upon this issue, possible solution here #722

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

No branches or pull requests

4 participants