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

combo-box-light opens wrong durection due to overlay contentHeight stuck at 200, ignores actual size #5757

Closed
karwosts opened this issue Apr 16, 2023 · 9 comments
Assignees
Labels
waiting for author Further information is requested

Comments

@karwosts
Copy link

Description

In HomeAssistant project, there is this combo-box-light here:

https://github.com/home-assistant/frontend/blob/dev/src/components/ha-combo-box.ts#L147

I see that when near the bottom of the screen, it opens downward, leading to the bottom of the overlay being cut off. When I looked at why it opened downwards instead of upwards, I see that in this code:

https://github.com/vaadin/web-components/blob/main/packages/overlay/src/vaadin-overlay-position-mixin.js#L278

    __shouldAlignStartVertically(targetRect) {
      // Using previous size to fix a case where window resize may cause the overlay to be squeezed
      // smaller than its current space before the fit-calculations.
      const contentHeight =
        this.requiredVerticalSpace || Math.max(this.__oldContentHeight || 0, this.$.overlay.offsetHeight);

The "requiredVerticalSpace" property in the overlay overrides the actual overlay size.

This property is set to fixed 200 in the combo-box overlay constructor, and I don't see any way for end user to override it, or anything that would clear it to allow the overlay to use its actual size.

https://github.com/vaadin/web-components/blob/main/packages/combo-box/src/vaadin-combo-box-overlay.js#L59

When I hack my usage to force the requiredVerticalSpace to 0, then the combo box opens to the correct direction, since overlay.offsetHeight has the correct size.

Maybe requiredVerticalSpace should be a term in the Math.max equation, instead of being prioritized over it? Otherwise I'm not sure how to make a combobox work correctly with this property set.

This code was added recently in #5432.

Expected outcome

Expect combo-box overlay's actual size to be used instead of fixed "200".

Minimal reproducible example

Sorry not sure how to set up something like this.

Steps to reproduce

N/A

Environment

Vaadin version(s): @vaadin/combo-box@npm:23.3.7
OS: Windows

Browsers

Chrome, Firefox, Issue is not browser related

@sissbruecker
Copy link
Contributor

I can't reproduce this with the following example:

<style>
  vaadin-combo-box-light {
    display: inline-block;
    margin-top: 400px;
  }
</style>

<vaadin-combo-box-light>
  <vaadin-text-field></vaadin-text-field>
</vaadin-combo-box-light>

<script type="module">
  import '@vaadin/combo-box/vaadin-combo-box-light.js';
  import '@vaadin/text-field/vaadin-text-field.js';

  const comboBox = document.querySelector('vaadin-combo-box-light');
  const res = await fetch('https://demo.vaadin.com/demo-data/1.0/filtered-countries?count=200');
  const arr = await res.json();
  comboBox.items = arr.result;
</script>

Unless there are less than 200px to the bottom of the window, the combo box opens downwards, and the overlay is not cut off.

Please provide a minimal reproduction, otherwise we can't rule out that the issue is caused by application-specific code.

@sissbruecker sissbruecker added the waiting for author Further information is requested label Apr 20, 2023
@karwosts
Copy link
Author

karwosts commented Apr 20, 2023

Is there any kind of scratch pad for where we can test/share these minimal reproductions?

I thought maybe I could do it here but I couldn't get yours working:
https://studio.webcomponents.dev/edit/b3jKlgU85FyqmqcsuyW8/www/index.html?p=website

Nevermind got it working with @web/dev-server

Anyway I thought what you said was interesting:

Unless there are less than 200px to the bottom of the window, the combo box opens downwards, and the overlay is not cut off.

So if there is 201px to the bottom of the window, is the overlay supposed to open downwards, but shrink to fit? And this 200px is not adjustable/customizable?

If that is the intended behavior, our error must be not that it is opening the wrong direction, but that the overlay is not shrinking to fit correctly.

@sissbruecker
Copy link
Contributor

sissbruecker commented Apr 20, 2023

Was able to get this working with the Lit playground: Link

So if there is 201px to the bottom of the window, is the overlay supposed to open downwards, but shrink to fit? And this 200px is not adjustable/customizable?

Yes, from what I can see that is how it is supposed to work. The combo box overlay is supposed to be scrollable anyway, so even if it ends up with more content than 200px, it seems reasonable to open downwards. I guess you expected that it would flip if it eventually ends up with more content than that? I can't remember all the details, but the current solution fixed an issue where the content was lazy loaded, so at the time of the measurement the overlay didn't have any height. Using a constant value also helps with the overlay not constantly flipping when filtering the combo box, which would be disorienting.

So if the actual overlay isn't cut off, as in the overlay expands beyond the size of the window, then this isn't a bug. In that case I would recommend to open a new issue as an enhancement to make the requiredVerticalSpace property configurable if 200px is too low for you. Or suggest a more advanced measurement mechanism that somehow deals with the overlay content possibly being lazy loaded and the overlay expanding and shrinking while filtering.

@karwosts
Copy link
Author

karwosts commented Apr 20, 2023

The combo box overlay is supposed to be scrollable anyway, so even if it ends up with more content than 200px, it seems reasonable to open downwards

The problem with that is that even if it is scrollable, the bottom list item might not be selectable, because if the overlay is 300px, 200px from the bottom of the screen, and it opens downwards and you can only see the top 200px, you can scroll to the bottom of the list, but you won't be able to scroll far enough to bring the bottom one or two list items onto the visible area.

The overlay is cut off in our current implementation, so we can't view/select the last list item at all, though I guess I misidentified what the actual bug is. Will have to look more to understand why the overlay is not shrinking correctly to fit.

@karwosts
Copy link
Author

Here's an example image. I've scrolled all the way to the bottom of the combo box, but the item "waterleak" you can see at the bottom there is cut off, and it's not even the last item in the list, there are two more after it which are unselectable.

image

@sissbruecker
Copy link
Contributor

Yes, that is not intended. But you'd have to provide a (minimal) reproduction for that, otherwise we can't rule out that application code is causing the behavior.

@karwosts
Copy link
Author

Ok I'll come back when I have something else. Thank you for your assistance.

@4orever
Copy link

4orever commented Apr 25, 2023

@karwosts Do you have any updates on that? I also met the same issue and I also use combobox inside dialog. Probably dialog is the key?

@karwosts
Copy link
Author

karwosts commented Apr 25, 2023

Nope haven't figured anything out yet. Somewhat on the backburner now, I'm not sure if or when I'll try to dig further into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants