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

Click events not always registering for Rows when scrolling fast #128

Closed
johot opened this issue Feb 3, 2019 · 11 comments
Closed

Click events not always registering for Rows when scrolling fast #128

johot opened this issue Feb 3, 2019 · 11 comments

Comments

@johot
Copy link

johot commented Feb 3, 2019

So I have created a basic ListBox using react-window and everything is rendering great!

One problem I have however is that if I scroll the list and right after scrolling click on an item it's click handler (a div in this case) is not always invoked.

My guess is that I click before the item has been properly rendered and placed on the screen. Does this sound plausible?

Maybe if the FixedSizeList could have it's own click handler that could calculate which items should have been clicked in case it has not been rendered fully yet?

Is there any strategy or workaround for these kinds of problems? Is it a known issue?

Thanks for a great library!

@bvaughn
Copy link
Owner

bvaughn commented Feb 4, 2019

This library intentionally disables pointer-events while a user is scrolling to improve performance. This is a common technique in windowing libraries (see react-virtualized, react-infinite's Infiniteand fixed-data-table's FixedDataTableBufferedRows).

To my knowledge, this is still commonly done to improve performance (although admittedly it has been a while since I've looked into this so it's possible things have changed– in which case, I'd welcome any updated resources you may have).

For the time being though, I'm going to close this issue as I don't plan on changing the current behavior. We can continue to chat here though if you have more ideas or questions. 😄

@bvaughn bvaughn closed this as completed Feb 4, 2019
@bvaughn
Copy link
Owner

bvaughn commented Feb 4, 2019

I should add that I currently reset the is-scrolling state after a 150ms delay so the window of time in which a click event is not handled should be small.

@johot
Copy link
Author

johot commented Feb 4, 2019

Thank you @bvaughn!

I actually found the same info yesterday looking through the react-virtualized issues for hints. I fixed the problem by adding pointerEvents: 'auto' to my Rows so this was definitely the case.

Could this maybe be documented somewhere? Or even better the ability to disable the behavior using a prop on the FixedSizeList etc?

Also regarding performance, in my case I did not really notice any decrease in performance after overriding pointerEvents so the fix is working great. I do occasionally still hit it if the item is slow to render, in that case a virtualized onClickItem or something on the list could present a possible fix since it should in theory be able to calculate which item should be clicked based on offset and row height right?

Thanks again!

@bvaughn
Copy link
Owner

bvaughn commented Feb 4, 2019

I don't think I want to add a prop to override this behavior, because those one-off props bloat the library and make maintenance more difficult.

Have a Code Sandbox or something showing your workaround? Maybe I could add it to the FAQs or something.

in that case a virtualized onClickItem or something on the list could present a possible fix since it should in theory be able to calculate which item should be clicked based on offset and row height right?

That's sort of an interesting idea. If you'd like to put up a PR, I'd be happy to review it. (No promises it would land– depends on complexity and size.)

@johot
Copy link
Author

johot commented Feb 4, 2019

The workaround is basically just this:

export function Row(props) {
  return (
    <div onClick={() => alert("Clicked")} style={{ pointerEvents: "auto" }}>
      Click me
    </div>
  );
}

A first step would be to just add it to the FAQ I guess (in Readme.md?). Another thing I would love to see in the FAQ is something about AutoSizer, had to dig around the issues to find that info also :)

@bvaughn
Copy link
Owner

bvaughn commented Feb 4, 2019

Cool, thanks. :) PRs to the README are always welcome too~

@johot
Copy link
Author

johot commented Feb 4, 2019

Sure thing, will look into it :D

@heyimalex
Copy link

heyimalex commented Feb 4, 2019

@johot Here's a codesandbox that kinda does what you were talking about with the virtualized onClick but outside of the library. I didn't think too hard about it but it seems to work? It tracks scroll position through the onScroll callback, and attaches an onClick handler to a container div that calculates the item under the cursor.

Edit: I'm dumb, this needs to take into account the container position, so an actual working impl also needs a ref on the container and to use getBoundingClientRect.

@chrisnojima
Copy link

Just want to chime in that I just got a bug report about this in our app and am now applying this as a workaround

@Amaranthusss
Copy link

It's now the end of 2023, and I've encountered the exact same issue. I've spent many hours trying to optimize my code, and now I know that simply setting pointerEvents to 'auto' was all that was needed :). If I had pushed the changes with ignoring events during fast scrolling, someone would surely have reported it as a bug.

@johot Thanks for the workaround :)

@johot
Copy link
Author

johot commented Nov 9, 2023

Glad to have helped! 😊

sebmarkbage added a commit to facebook/react that referenced this issue Aug 1, 2024
[`react-window` disables `pointerEvents` while scrolling meaning you
can't click anything while
scrolling.](bvaughn/react-window#128).

This means that the first click when you stop the scroll with inertial
scrolling doesn't get registered. This is suuuper annoying. This might
make sense when you click to stop on a more intentional UI but it
doesn't makes sense in a list like this because we eagerly click things
even on mousedown.

This PR just override that to re-enable pointer events.

Supposedly this is done for performance but that might be outdated
knowledge. I haven't observed any difference so far.

If we discover that it's a perf problem, there's another technique we
can use where we call `ownerDocument.elementFromPoint(e.pageX, e.pageY)`
and then dispatch the event against that element. But let's try the
simplest approach first?
github-actions bot pushed a commit to surenpoghosian/react that referenced this issue Aug 1, 2024
[`react-window` disables `pointerEvents` while scrolling meaning you
can't click anything while
scrolling.](bvaughn/react-window#128).

This means that the first click when you stop the scroll with inertial
scrolling doesn't get registered. This is suuuper annoying. This might
make sense when you click to stop on a more intentional UI but it
doesn't makes sense in a list like this because we eagerly click things
even on mousedown.

This PR just override that to re-enable pointer events.

Supposedly this is done for performance but that might be outdated
knowledge. I haven't observed any difference so far.

If we discover that it's a perf problem, there's another technique we
can use where we call `ownerDocument.elementFromPoint(e.pageX, e.pageY)`
and then dispatch the event against that element. But let's try the
simplest approach first?

DiffTrain build for commit facebook@06d0b89.
github-actions bot pushed a commit to surenpoghosian/react that referenced this issue Aug 1, 2024
[`react-window` disables `pointerEvents` while scrolling meaning you
can't click anything while
scrolling.](bvaughn/react-window#128).

This means that the first click when you stop the scroll with inertial
scrolling doesn't get registered. This is suuuper annoying. This might
make sense when you click to stop on a more intentional UI but it
doesn't makes sense in a list like this because we eagerly click things
even on mousedown.

This PR just override that to re-enable pointer events.

Supposedly this is done for performance but that might be outdated
knowledge. I haven't observed any difference so far.

If we discover that it's a perf problem, there's another technique we
can use where we call `ownerDocument.elementFromPoint(e.pageX, e.pageY)`
and then dispatch the event against that element. But let's try the
simplest approach first?

DiffTrain build for [06d0b89](facebook@06d0b89)
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

5 participants