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

Fix scrolling issues in RTL in Chrome, Firefox and Edge #215

Closed
wants to merge 8 commits into from

Conversation

MarkFalconbridge
Copy link
Contributor

@MarkFalconbridge MarkFalconbridge commented Apr 17, 2019

Hopefully this fixes #159 and fixes #198.

The code is essentially now keeping an internal normalized representation of scrollLeft which is used as scrollLeft used to be to determine the indices to be rendered. Anywhere that scrollLeft is reported out or received in, e.g. onScroll and scrollTo it reports and expects scrollLeft as the browser would represent it.

I've used the normalize-scroll-left library to determine what type of scrolling the current browser implements.

I've changed the ScrollToItem file to make it easier to see the changes in action hopefully proving the validity of the code changes.

@MarkFalconbridge
Copy link
Contributor Author

@bvaughn Any chance you could look at this?

@bvaughn
Copy link
Owner

bvaughn commented Apr 24, 2019

This is a side project of mine. I will look when I get the time and have the energy 😅

RTL issuea are particularly unpleasant to deal with and I'm not really looking forward to sorting through the prs and issues about it.

@MarkFalconbridge
Copy link
Contributor Author

I completely understand. The fix I've written wasn't pleasant to write at all but I've tested it in all the scenarios I can think of and think I've got it covered.

An alternative solution would be to write some sort of shim to patch how browsers get and set scrollLeft, i.e. patching the HTMLElement prototype. The upside of this would be that the behavior would then be normalized and react-window's code could remain largely as is apart from removing any rtl specific code. The downside would be that anyone already trying to handle the various browser implementations of scrollLeft would see different behavior.

@bvaughn
Copy link
Owner

bvaughn commented May 27, 2019

Thanks for this PR!

I think I've found a simpler solution via PR #251 after looking at this and the related #159. Going to close this in favor of that.

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