tailwindlabs / headlessui

Completely unstyled, fully accessible UI components, designed to integrate beautifully with Tailwind CSS.
https://headlessui.com
MIT License
25.81k stars 1.07k forks source link

Combobox overrides HTML styles with overflow:hidden when menu opened #3209

Closed tusmenko closed 4 months ago

tusmenko commented 4 months ago

What package within Headless UI are you using?

For example: @headlessui/react

What version of that package are you using?

For example: v2.0.3

What browser are you using?

For example: Arc (Chrome)

Reproduction URL

CodeSanbox

Describe your issue

The combobox component adds styling to the HTML root when the menu opened, which corrupts the "sticky" behaviour of other components.

This is what is added to the when combobox menu is open:

style=overflow: hidden; padding-right: 0px;

image
RobinMalfait commented 4 months ago

Hey!

This is because the components is modal by default which enables accessibility features like scroll locking, focus trapping, and making other elements inert. The scroll locking part is what adds the overflow: hidden to the html (the padding-right is added to offset the scrollbar if there was one).

Can you make your CodeSandbox link public so that I can take a look?

tusmenko commented 4 months ago

@RobinMalfait Hi!

Sure, I've updated a link: CodeSanbox

The idea is understandable, but shouldn't it be explicit to force such a lock or at least a way to disable it?

Thank you!

RobinMalfait commented 4 months ago

You can disable that behavior (by setting the modal={false} on the ComboboxOptions, see: https://headlessui.com/react/combobox#combobox-options) but wanted to see if we could fix the issue in another way since those features are pretty useful to get a better user experience. Do you mind updating the CodeSandbox link with an example of those sticky behaviors you are talking about?

It looks like the CodeSandbox only includes a Combobox to show that it renders the overflow: hidden on the html but ideally it shows an example of the actual bug that's happening.

tusmenko commented 4 months ago

Thanks! I've missed that option...

I've updated Sandbox to replicate the issue. Please pay attention to index.css, as I found which part causes a problem but can't state it's uncommon to do so.

To reproduce:

=> Header is gone

RobinMalfait commented 4 months ago

I see what you mean now. The problem here is indeed the overflow-x: hidden;. The funny thing is that the overflow-x influences the overflow-y behavior:

From the spec:

The visible/clip values of overflow compute to auto/hidden (respectively) if one of overflow-x or overflow-y is neither visible nor clip.

I think there are 2 solutions to this problem:

  1. Do not use overflow-x: hidden; at all — your comment explains that this might be useful if you are 100% sure your content shouldn't scroll horizontally. I personally think this is a bit of a bandaid solution and instead you should fix the issue if any of the elements does make the page scroll horizontally.
  2. Use overflow-x: clip — I totally get that the previous solution is not as easy if you have a big app or if fixing the underlying problem is hard. However, if you use overflow-x: clip then you get the same behavior as overflow-x: hidden (aka, no scrollbar) but it doesn't influence the overflow-y behavior.

I think that for your scenario, the overflow-x: clip is going to be the easiest with the expected result.

Since this issue can be solved with a non-hacky CSS solution, and since the overflow: hidden on the html that we inject has useful UI/UX and accessibility improvements, I don't think that we should fix or provide an option to not do that in Headless UI for now.

For that reason I'm going to close this issue for now. Again, I think that the overflow-x: clip should do the trick!

If you still run into issues with this feel free to re-open the issue. If you run into other issues feel free to create a new issue.

Hope this helps!

tusmenko commented 4 months ago

Thank you! overflow-x: hidden; might be considered a hack, agree But thanks for triaging this, so I have what to try next