search

icd2k3 / react-scroll-into-view-if-needed

A thin react component wrapper bundled with scroll-into-view-if-needed
https://codesandbox.io/s/3j4qjp7yp
MIT License
34 stars 8 forks source link

Window doesn't scroll upward when scrollMode is 'if-needed' #9

Closed petegleeson closed 6 years ago

petegleeson commented 6 years ago

Hello 👋 I am having some difficulty getting the window to scroll upward to a component when the scroll mode is 'if-needed'. If I set the scroll mode to 'always' it seems to work the way I expect.

I have created a codesandbox which demos this. Try clicking on the 'prev' button https://codesandbox.io/s/4w50o96p9w

Does this look like a bug to you? Thanks!

icd2k3 commented 6 years ago

Hey @petegleeson! This does indeed seem to be a bug.

My guess is that there is a problem with the logic in the scroll-into-view-if-needed package that might be incorrectly be determining if a node "needs" to be scrolled into view.

I'll do some investigation in the near future and see if I can narrow it down. Thanks for reporting and setting up that sandbox!

petegleeson commented 6 years ago

Thanks @icd2k3. I had a bit more of an explore last night and created a non-react version of that example. https://codepen.io/anon/pen/QxKKwg?editors=1010

This example appears to work fine so maybe this is a problem specific to usage in React?

icd2k3 commented 6 years ago

Thanks @petegleeson for testing! That's exactly what I was going to try when I got home from work, but you beat me to it! 😛

That is interesting. This component passes the React ref for the node and options to the scroll-into-view-if-needed module. So, at first glance, I'm not sure what the problem might be. Maybe some discrepancy with the way React handles refs or something... but I'll do some digging soon and see if I can narrow it down.

Thanks again for the help debugging

icd2k3 commented 6 years ago

Hey @petegleeson, after some investigation, this does look to be an issue with scroll-into-voew-if-needed ponyfill.

Your last example is using the native scrollIntoView MDN (ex: elem.scrollIntoView) that some browsers support (to varying degrees). This component uses a polyfill to cover a wider range of browsers.

I set up this demo of the issue using only the raw scroll-into-voew-if-needed method: https://codesandbox.io/s/38887pvyp6

I'll file an issue on their end soon and link it here. If I end up finding the problem in that codebase I'll submit a PR there as well.

OT: Unrelated to the bug - I could also update this package to use the native browser method for those that don't mind the spotty browser support if that might be valuable :)

petegleeson commented 6 years ago

Ah crap, sorry for the bum steer mate. I fixed up the example to use the library's scrollIntoView function now.

https://codepen.io/anon/pen/QxKKwg?editors=1011

I am observing the same behaviour now as the React example so looks to me as well like a problem inside the ponyfill library.

icd2k3 commented 6 years ago

👆 Added an issue to the other module! I'll post back here with updates.

icd2k3 commented 6 years ago

hey @petegleeson the issue should be resolves as of version 2.1.3. Thanks again for reporting this!