fix: add required vertical space for combo-box overlay

[tomivirkki]

Description

Since the <combo-box> overlay has no intrinsic height, PositionMixin occasionally had it misaligned on open.

A new property requiredVerticalSpace was added for the PositionMixin to cover this case (defining required vertical space for overlays that have no intrinsic height of their own). A default value of 200 (pixels) for the property was assigned to the <vaadin-combo-box-overlay> to fix the issue.

Fixes vaadin/flow-components#2820

Type of change

Bugfix

[tomivirkki]

PositionMixin is private so didn't label this as a feat.

[tomivirkki]

Not 100% sure about the property name. Any other suggestions? It's for internal use only so it doesn't matter all that much.

[sissbruecker]

Sounds fine. I would suggest to mention in the JsDoc that this effectively disables the content measurement in favor of using this fixed value for determining the open direction.

[tomivirkki]

Updated the description

[tomivirkki]

I wonder if 200px is a good default value for combo-box?

[sissbruecker]

Should be a good start. We can consider changing it, or making this configurable if this doesn't fit every use case.

[web-padawan]

We used to have a hardcoded number in V14 - IIRC, it originates from the 1.x Paper elements based version. It makes no sense already in V10 where we switched to Lumo, but apparently no one noticed it 🙂

Personally, I feel like 200px should be a reasonable default for the minumal overlay height.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[vaadin-bot]

Hi @tomivirkki , this commit cannot be picked to 23.3 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick 80c16b6
error: could not apply 80c16b6... fix: add required vertical space for combo-box overlay (#5432)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

[vaadin-bot]

Hi @tomivirkki , this commit cannot be picked to 22.0 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick 80c16b6
error: could not apply 80c16b6... fix: add required vertical space for combo-box overlay (#5432)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.0.0.beta1 and is also targeting the upcoming stable 24.0.0 version.

[karwosts]

I am thinking I am seeing problems with this change.

In HomeAssistant we have vaadin-combo-box-light, and this required-vertical-space seems to be throwing off the open direction calculation of the combo box.

If I force required-vertical-space to 0, the overlay.offsetHeight has the correct overlay size and the combo-box opens in the correct direction.

But since this required-vertical-space was set to 200, the overlay always acts as if it is 200px in contentHeight for calculating open direction, even if it is larger. Therefore a 400px overlay will still open downwards if it is 300px from the bottom of the screen, leading to the bottom of the overlay being cut off. If not for this required-vertical-space the contentHeight would correctly be 400px and it would open upward instead.

from vaadin/overlay/src/vaadin-overlay-position-mixin.js

    __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);

There seems to be no way to override the requiredVerticalSpace for a vaadin-combo-box-light. Am I misunderstanding something here?