From f4d05d834e4b5daa196bd4a7997a12d4629047a3 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 1 Feb 2023 14:10:23 +0200 Subject: [PATCH 1/9] fix: add required vertical space for combo-box overlay --- .../combo-box/src/vaadin-combo-box-overlay.js | 6 ++++ .../combo-box/test/overlay-position.test.js | 27 +++++++++++++++- .../src/vaadin-overlay-position-mixin.d.ts | 9 ++++++ .../src/vaadin-overlay-position-mixin.js | 20 ++++++++++-- packages/overlay/test/position-mixin.test.js | 32 +++++++++++++++++++ .../overlay/test/typings/overlay.types.ts | 1 + 6 files changed, 92 insertions(+), 3 deletions(-) diff --git a/packages/combo-box/src/vaadin-combo-box-overlay.js b/packages/combo-box/src/vaadin-combo-box-overlay.js index 8b0a5c01c3..37067f363e 100644 --- a/packages/combo-box/src/vaadin-combo-box-overlay.js +++ b/packages/combo-box/src/vaadin-combo-box-overlay.js @@ -52,6 +52,12 @@ export class ComboBoxOverlay extends ComboBoxOverlayMixin(Overlay) { return memoizedTemplate; } + + constructor() { + super(); + + this.requiredVerticalSpace = 200; + } } customElements.define(ComboBoxOverlay.is, ComboBoxOverlay); diff --git a/packages/combo-box/test/overlay-position.test.js b/packages/combo-box/test/overlay-position.test.js index 8fe5e86ba3..5920e57de2 100644 --- a/packages/combo-box/test/overlay-position.test.js +++ b/packages/combo-box/test/overlay-position.test.js @@ -1,6 +1,7 @@ import { expect } from '@esm-bundle/chai'; import { aTimeout, fixtureSync, isIOS } from '@vaadin/testing-helpers'; -import '../src/vaadin-combo-box.js'; +import '../vaadin-combo-box.js'; +import './not-animated-styles.js'; import { html, PolymerElement } from '@polymer/polymer/polymer-element.js'; import { makeItems, setInputValue } from './helpers.js'; @@ -33,6 +34,13 @@ describe('overlay position', () => { } beforeEach(async () => { + fixtureSync(` + `); + comboBox = fixtureSync(``); const comboBoxRect = comboBox.getBoundingClientRect(); comboBox.items = makeItems(20); @@ -159,6 +167,23 @@ describe('overlay position', () => { expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); }); + it('should be above input when near bottom', async () => { + moveComboBox(xCenter, yBottom - 200, 300); + + comboBox.open(); + await aTimeout(1); + expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); + }); + + it('should be below input when far from bottom', async () => { + moveComboBox(xCenter, yBottom - 220, 300); + + await aTimeout(1); + comboBox.open(); + await aTimeout(1); + expect(overlayPart.getBoundingClientRect().top).to.closeTo(inputField.getBoundingClientRect().bottom, 1); + }); + it('should reposition after filtering', async () => { moveComboBox(xCenter, yBottom, 300); diff --git a/packages/overlay/src/vaadin-overlay-position-mixin.d.ts b/packages/overlay/src/vaadin-overlay-position-mixin.d.ts index e8afc1bcbc..bd65467b92 100644 --- a/packages/overlay/src/vaadin-overlay-position-mixin.d.ts +++ b/packages/overlay/src/vaadin-overlay-position-mixin.d.ts @@ -54,4 +54,13 @@ export declare class PositionMixinClass { * @attr {boolean} no-vertical-overlap */ noVerticalOverlap: boolean; + + /** + * If the overlay content has no intrinsic height, this property can be used to set + * the minimum vertical space required by the overlay. If there is not enough space + * available on the primary side, the overlay will be positioned on the opposite side. + * + * @attr {number} required-vertical-space + **/ + requiredVerticalSpace: number; } diff --git a/packages/overlay/src/vaadin-overlay-position-mixin.js b/packages/overlay/src/vaadin-overlay-position-mixin.js index d14e7b015f..afe6c6899e 100644 --- a/packages/overlay/src/vaadin-overlay-position-mixin.js +++ b/packages/overlay/src/vaadin-overlay-position-mixin.js @@ -93,12 +93,24 @@ export const PositionMixin = (superClass) => type: Boolean, value: false, }, + + /** + * If the overlay content has no intrinsic height, this property can be used to set + * the minimum vertical space required by the overlay. If there is not enough space + * available on the primary side, the overlay will be positioned on the opposite side. + * + * @attr {number} required-vertical-space + **/ + requiredVerticalSpace: { + type: Number, + value: 0, + }, }; } static get observers() { return [ - '__positionSettingsChanged(horizontalAlign, verticalAlign, noHorizontalOverlap, noVerticalOverlap)', + '__positionSettingsChanged(horizontalAlign, verticalAlign, noHorizontalOverlap, noVerticalOverlap, requiredVerticalSpace)', '__overlayOpenedChanged(opened, positionTarget)', ]; } @@ -262,7 +274,11 @@ export const PositionMixin = (superClass) => __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 = Math.max(this.__oldContentHeight || 0, this.$.overlay.offsetHeight); + const contentHeight = Math.max( + this.__oldContentHeight || 0, + this.$.overlay.offsetHeight, + this.requiredVerticalSpace, + ); this.__oldContentHeight = this.$.overlay.offsetHeight; const viewportHeight = Math.min(window.innerHeight, document.documentElement.clientHeight); diff --git a/packages/overlay/test/position-mixin.test.js b/packages/overlay/test/position-mixin.test.js index 91829b451b..2908dd80ef 100644 --- a/packages/overlay/test/position-mixin.test.js +++ b/packages/overlay/test/position-mixin.test.js @@ -171,6 +171,22 @@ describe('position mixin', () => { expectEdgesAligned(TOP, TOP); }); + it('should flip when out of required vertical space', () => { + overlay.requiredVerticalSpace = 200; + target.style.top = `${targetPositionToFlipOverlay - 100}px`; + updatePosition(); + expectEdgesAligned(BOTTOM, BOTTOM); + }); + + it('should flip back when enough required vertical space', () => { + overlay.requiredVerticalSpace = 200; + target.style.top = `${targetPositionToFlipOverlay - 100}px`; + updatePosition(); + target.style.top = `${targetPositionToFlipOverlay - 200}px`; + updatePosition(); + expectEdgesAligned(TOP, TOP); + }); + it('should choose the bigger side when it fits neither', () => { overlayContent.style.height = `${document.documentElement.clientHeight}px`; @@ -262,6 +278,22 @@ describe('position mixin', () => { expectEdgesAligned(BOTTOM, BOTTOM); }); + it('should flip when out of required vertical space', () => { + overlay.requiredVerticalSpace = 200; + target.style.top = `${targetPositionToFlipOverlay + 100}px`; + updatePosition(); + expectEdgesAligned(TOP, TOP); + }); + + it('should flip back when enough required vertical space', () => { + overlay.requiredVerticalSpace = 200; + target.style.top = `${targetPositionToFlipOverlay + 100}px`; + updatePosition(); + target.style.top = `${targetPositionToFlipOverlay + 200}px`; + updatePosition(); + expectEdgesAligned(BOTTOM, BOTTOM); + }); + it('should choose the bigger side when it fits neither', () => { overlayContent.style.height = `${document.documentElement.clientHeight}px`; diff --git a/packages/overlay/test/typings/overlay.types.ts b/packages/overlay/test/typings/overlay.types.ts index e15701b643..32b1072e40 100644 --- a/packages/overlay/test/typings/overlay.types.ts +++ b/packages/overlay/test/typings/overlay.types.ts @@ -66,3 +66,4 @@ assertType(customOverlay.noVerticalOverlap); assertType<'end' | 'start'>(customOverlay.horizontalAlign); assertType<'bottom' | 'top'>(customOverlay.verticalAlign); assertType(customOverlay.positionTarget); +assertType(customOverlay.requiredVerticalSpace); From 2bc7481b402e7cb84d0407f5a4a9a8cceca481b0 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 1 Feb 2023 16:44:07 +0200 Subject: [PATCH 2/9] test: cleanup --- packages/combo-box/test/overlay-position.test.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/combo-box/test/overlay-position.test.js b/packages/combo-box/test/overlay-position.test.js index 5920e57de2..96ea1adca9 100644 --- a/packages/combo-box/test/overlay-position.test.js +++ b/packages/combo-box/test/overlay-position.test.js @@ -2,21 +2,8 @@ import { expect } from '@esm-bundle/chai'; import { aTimeout, fixtureSync, isIOS } from '@vaadin/testing-helpers'; import '../vaadin-combo-box.js'; import './not-animated-styles.js'; -import { html, PolymerElement } from '@polymer/polymer/polymer-element.js'; import { makeItems, setInputValue } from './helpers.js'; -class XFixed extends PolymerElement { - static get template() { - return html` -
- -
- `; - } -} - -customElements.define('x-fixed', XFixed); - describe('overlay position', () => { let comboBox, overlayPart, inputField; From b372cbae234e0994a7d368098136966f02b6801b Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 1 Feb 2023 16:56:36 +0200 Subject: [PATCH 3/9] test: fix the tests to use a lazy data provider --- .../combo-box/test/overlay-position.test.js | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/packages/combo-box/test/overlay-position.test.js b/packages/combo-box/test/overlay-position.test.js index 96ea1adca9..b37277531f 100644 --- a/packages/combo-box/test/overlay-position.test.js +++ b/packages/combo-box/test/overlay-position.test.js @@ -154,31 +154,48 @@ describe('overlay position', () => { expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); }); - it('should be above input when near bottom', async () => { - moveComboBox(xCenter, yBottom - 200, 300); + it('should reposition after filtering', async () => { + moveComboBox(xCenter, yBottom, 300); + + setInputValue(comboBox, 'item 1'); comboBox.open(); - await aTimeout(1); + await aTimeout(0); expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); }); - it('should be below input when far from bottom', async () => { - moveComboBox(xCenter, yBottom - 220, 300); + describe('lazy data provider', () => { + beforeEach(() => { + comboBox.items = undefined; + + comboBox.dataProvider = (params, callback) => { + const index = params.page * params.pageSize; + const size = 20; + const result = [...Array(size).keys()].map((i) => { + return { + label: `Item ${index + i}`, + value: `item-${index + i}`, + }; + }); + setTimeout(() => callback(result, size), 100); + }; + }); - await aTimeout(1); - comboBox.open(); - await aTimeout(1); - expect(overlayPart.getBoundingClientRect().top).to.closeTo(inputField.getBoundingClientRect().bottom, 1); - }); + it('should be above input when near bottom', async () => { + moveComboBox(xCenter, yBottom - 200, 300); - it('should reposition after filtering', async () => { - moveComboBox(xCenter, yBottom, 300); + comboBox.open(); + await aTimeout(1); + expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); + }); - setInputValue(comboBox, 'item 1'); + it('should be below input when far from bottom', async () => { + moveComboBox(xCenter, yBottom - 220, 300); - comboBox.open(); - await aTimeout(0); - expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); + comboBox.open(); + await aTimeout(1); + expect(overlayPart.getBoundingClientRect().top).to.closeTo(inputField.getBoundingClientRect().bottom, 1); + }); }); }); From 8d53297090adfa4da2f47b8b836291e265b2f91a Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 1 Feb 2023 17:10:29 +0200 Subject: [PATCH 4/9] fix: ignore calculated overlay height if the prop is used --- packages/combo-box/test/overlay-position.test.js | 16 ++++++++++++++++ .../overlay/src/vaadin-overlay-position-mixin.js | 7 ++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/combo-box/test/overlay-position.test.js b/packages/combo-box/test/overlay-position.test.js index b37277531f..394e5d8435 100644 --- a/packages/combo-box/test/overlay-position.test.js +++ b/packages/combo-box/test/overlay-position.test.js @@ -164,6 +164,22 @@ describe('overlay position', () => { expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); }); + it('should be above input when near bottom', async () => { + moveComboBox(xCenter, yBottom - 200, 300); + + comboBox.open(); + await aTimeout(1); + expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1); + }); + + it('should be below input when far from bottom', async () => { + moveComboBox(xCenter, yBottom - 220, 300); + + comboBox.open(); + await aTimeout(1); + expect(overlayPart.getBoundingClientRect().top).to.closeTo(inputField.getBoundingClientRect().bottom, 1); + }); + describe('lazy data provider', () => { beforeEach(() => { comboBox.items = undefined; diff --git a/packages/overlay/src/vaadin-overlay-position-mixin.js b/packages/overlay/src/vaadin-overlay-position-mixin.js index afe6c6899e..a09b53e0e7 100644 --- a/packages/overlay/src/vaadin-overlay-position-mixin.js +++ b/packages/overlay/src/vaadin-overlay-position-mixin.js @@ -274,11 +274,8 @@ export const PositionMixin = (superClass) => __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 = Math.max( - this.__oldContentHeight || 0, - this.$.overlay.offsetHeight, - this.requiredVerticalSpace, - ); + const contentHeight = + this.requiredVerticalSpace || Math.max(this.__oldContentHeight || 0, this.$.overlay.offsetHeight); this.__oldContentHeight = this.$.overlay.offsetHeight; const viewportHeight = Math.min(window.innerHeight, document.documentElement.clientHeight); From b9b1d1fbbe5993d1fbd51f7dd92e82ccc606512d Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Thu, 2 Feb 2023 11:26:53 +0200 Subject: [PATCH 5/9] docs: update property description. --- packages/overlay/src/vaadin-overlay-position-mixin.d.ts | 5 +++-- packages/overlay/src/vaadin-overlay-position-mixin.js | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/overlay/src/vaadin-overlay-position-mixin.d.ts b/packages/overlay/src/vaadin-overlay-position-mixin.d.ts index bd65467b92..a98fde53a0 100644 --- a/packages/overlay/src/vaadin-overlay-position-mixin.d.ts +++ b/packages/overlay/src/vaadin-overlay-position-mixin.d.ts @@ -57,8 +57,9 @@ export declare class PositionMixinClass { /** * If the overlay content has no intrinsic height, this property can be used to set - * the minimum vertical space required by the overlay. If there is not enough space - * available on the primary side, the overlay will be positioned on the opposite side. + * the minimum vertical space (in pixels) required by the overlay. Setting a value to + * the property effectively disables the content measurement in favor of using this + * fixed value for determining the open direction. * * @attr {number} required-vertical-space **/ diff --git a/packages/overlay/src/vaadin-overlay-position-mixin.js b/packages/overlay/src/vaadin-overlay-position-mixin.js index a09b53e0e7..9c066d6535 100644 --- a/packages/overlay/src/vaadin-overlay-position-mixin.js +++ b/packages/overlay/src/vaadin-overlay-position-mixin.js @@ -96,8 +96,9 @@ export const PositionMixin = (superClass) => /** * If the overlay content has no intrinsic height, this property can be used to set - * the minimum vertical space required by the overlay. If there is not enough space - * available on the primary side, the overlay will be positioned on the opposite side. + * the minimum vertical space (in pixels) required by the overlay. Setting a value to + * the property effectively disables the content measurement in favor of using this + * fixed value for determining the open direction. * * @attr {number} required-vertical-space **/ From bf657945366c378f3bd150369a250713e5e7287f Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Thu, 2 Feb 2023 17:13:42 +0200 Subject: [PATCH 6/9] Update packages/combo-box/test/overlay-position.test.js Co-authored-by: Serhii Kulykov --- packages/combo-box/test/overlay-position.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/combo-box/test/overlay-position.test.js b/packages/combo-box/test/overlay-position.test.js index 394e5d8435..d7d402529d 100644 --- a/packages/combo-box/test/overlay-position.test.js +++ b/packages/combo-box/test/overlay-position.test.js @@ -26,7 +26,8 @@ describe('overlay position', () => { vaadin-combo-box-overlay::part(overlay) { margin: 0 !important; } - `); + + `); comboBox = fixtureSync(``); const comboBoxRect = comboBox.getBoundingClientRect(); From 529a1834a136c00095183752c26f07f69c74bac8 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Thu, 2 Feb 2023 17:13:54 +0200 Subject: [PATCH 7/9] Update packages/combo-box/test/overlay-position.test.js Co-authored-by: Serhii Kulykov --- packages/combo-box/test/overlay-position.test.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/combo-box/test/overlay-position.test.js b/packages/combo-box/test/overlay-position.test.js index d7d402529d..ff6e2879c0 100644 --- a/packages/combo-box/test/overlay-position.test.js +++ b/packages/combo-box/test/overlay-position.test.js @@ -188,12 +188,10 @@ describe('overlay position', () => { comboBox.dataProvider = (params, callback) => { const index = params.page * params.pageSize; const size = 20; - const result = [...Array(size).keys()].map((i) => { - return { - label: `Item ${index + i}`, - value: `item-${index + i}`, - }; - }); + const result = [...Array(size).keys()].map((i) => ({ + label: `Item ${index + i}`, + value: `item-${index + i}`, + })); setTimeout(() => callback(result, size), 100); }; }); From 0a0c8f728d2772b8524d911b6082a98f3c0bd92c Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Thu, 2 Feb 2023 17:14:00 +0200 Subject: [PATCH 8/9] Update packages/overlay/src/vaadin-overlay-position-mixin.js Co-authored-by: Serhii Kulykov --- packages/overlay/src/vaadin-overlay-position-mixin.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/overlay/src/vaadin-overlay-position-mixin.js b/packages/overlay/src/vaadin-overlay-position-mixin.js index 9c066d6535..ae78c06005 100644 --- a/packages/overlay/src/vaadin-overlay-position-mixin.js +++ b/packages/overlay/src/vaadin-overlay-position-mixin.js @@ -101,7 +101,7 @@ export const PositionMixin = (superClass) => * fixed value for determining the open direction. * * @attr {number} required-vertical-space - **/ + */ requiredVerticalSpace: { type: Number, value: 0, From 35c1b6d13cc28d4394879111c37491050b298423 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Thu, 2 Feb 2023 17:14:05 +0200 Subject: [PATCH 9/9] Update packages/overlay/src/vaadin-overlay-position-mixin.d.ts Co-authored-by: Serhii Kulykov --- packages/overlay/src/vaadin-overlay-position-mixin.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/overlay/src/vaadin-overlay-position-mixin.d.ts b/packages/overlay/src/vaadin-overlay-position-mixin.d.ts index a98fde53a0..6895d06f8e 100644 --- a/packages/overlay/src/vaadin-overlay-position-mixin.d.ts +++ b/packages/overlay/src/vaadin-overlay-position-mixin.d.ts @@ -62,6 +62,6 @@ export declare class PositionMixinClass { * fixed value for determining the open direction. * * @attr {number} required-vertical-space - **/ + */ requiredVerticalSpace: number; }