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 scrolling menu into view does not work well inside scroll container #3473

Open
IanVS opened this issue Mar 13, 2019 · 8 comments
Open
Labels
issue/bug-confirmed Issues about a bug that has been confirmed by a maintainer issue/has-pr Issue has a PR attached to it that may solve the issue issue/reviewed Issue has recently been reviewed (mid-2020) menu-bug Addresses menu positioning, scrolling, or general interactions

Comments

@IanVS
Copy link
Contributor

IanVS commented Mar 13, 2019

Are you reporting a bug or runtime error?

A bug, here is a codesandbox: https://codesandbox.io/s/w0ymoxq827

The issue is that viewHeight is calculated from the window, which causes problems for select elements inside of other scroll containers. This can be seen in the provided codesandbox, by changing the height of the embedded browser window, and seeing that the scroll behavior changes depending on that height.

I've played around in the code enough to find what seems to be a solution, causing the scroll container to scroll when needed, and also constraining the height of the menu to the total visible height of the scroll container, so the entire menu is shown, and it's not necessary to scroll the container outside the menu to see the entire menu. I'll work on cleaning up my code and will submit a PR, but I wanted to at least open an issue to explain the problem.

Oh and one other question while I'm at it, why does the scroll container need to be position: relative; to be detected as a scroll parent? It's not immediately obvious that it's necessary, and it was only by digging through the
source that I figured out why my container was not scrolling.
I guess this is because the menu is absolutely positioned, so it needs a relative parent.

I believe this is similar to the case reported back in #1267, but that plnkr is broken, so I can't tell for sure.

@IanVS IanVS changed the title menuShouldScrollIntoView does not work well inside scroll container Scrolling scrolling menu into view does not work well inside scroll container Mar 13, 2019
@Rall3n
Copy link
Collaborator

Rall3n commented Mar 15, 2019

@IanVS I know it does not address your issue entirely, but a solution provided by this library is the usage of Portals. There is an extra menuPortalTarget prop which accepts a dom node. It renders the menu outside of the container.

<Select 
    { ... }
    menuPortalTarget={document.body}
/>

@IanVS
Copy link
Contributor Author

IanVS commented Mar 27, 2019

Thanks @Rall3n, I'm familiar with portals, but I'd really like to render inside the scroll container. I believe I've got a good fix for it, I'm just struggling a bit with some of the e2e testing framework before I push up a PR.

@satyamatwork
Copy link

Hi @IanVS,
I'm struggling with this too, may I know your fix ad it might resolve my problem?
Thanks

@IanVS
Copy link
Contributor Author

IanVS commented Apr 4, 2019

It's possible! As soon as I get a little time, I'll try to at least push up a branch/pr, and you can test it out. I'll @-mention you on the pr.

@IanVS
Copy link
Contributor Author

IanVS commented Apr 6, 2019

@satyamatwork I still haven't gotten any good tests written for it, but I've pushed up a branch to my fork and tested it in my own application, and it seems to work well, as long as the scroll container is position: relative;. Try it out and see if it solves your issues. And if so, I'll finish up the tests and make a PR.

@mmarchuk-jama
Copy link

Thanks for your comment @IanVS . Setting the scroll container to position: relative; fixed my issue related to menu focus and likely saved me a great deal of time!

@ranihorev
Copy link

Is there an open PR for that?

@IanVS
Copy link
Contributor Author

IanVS commented Jul 6, 2019

@ranihorev There is now. I finished up the e2e test that I was a bit stuck on, and pushed one up. I hope that it's valuable and that the maintainers will be able to merge it. 🤞

@bladey bladey added the issue/bug-unconfirmed Issues that describe a bug that hasn't been confirmed by a maintainer yet label May 29, 2020
@bladey bladey added the issue/reviewed Issue has recently been reviewed (mid-2020) label Jun 17, 2020
@bladey bladey added the issue/has-pr Issue has a PR attached to it that may solve the issue label Jul 6, 2020
@ebonow ebonow added the menu-bug Addresses menu positioning, scrolling, or general interactions label Jun 10, 2021
@Methuselah96 Methuselah96 added issue/bug-confirmed Issues about a bug that has been confirmed by a maintainer and removed issue/bug-unconfirmed Issues that describe a bug that hasn't been confirmed by a maintainer yet labels Nov 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/bug-confirmed Issues about a bug that has been confirmed by a maintainer issue/has-pr Issue has a PR attached to it that may solve the issue issue/reviewed Issue has recently been reviewed (mid-2020) menu-bug Addresses menu positioning, scrolling, or general interactions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants