diff --git a/packages/combo-box/src/vaadin-combo-box-base-mixin.js b/packages/combo-box/src/vaadin-combo-box-base-mixin.js index f98c8271f56..6c59d002c8a 100644 --- a/packages/combo-box/src/vaadin-combo-box-base-mixin.js +++ b/packages/combo-box/src/vaadin-combo-box-base-mixin.js @@ -696,11 +696,11 @@ export const ComboBoxBaseMixin = (superClass) => } /** @protected */ - _scrollIntoView(index) { + _scrollIntoView(index, alignToCenter = false) { if (!this._scroller) { return; } - this._scroller.scrollIntoView(index); + this._scroller.scrollIntoView(index, alignToCenter); } /** @private */ diff --git a/packages/combo-box/src/vaadin-combo-box-focus-index-mixin.js b/packages/combo-box/src/vaadin-combo-box-focus-index-mixin.js index 876a7ce5e05..af09e5dfbce 100644 --- a/packages/combo-box/src/vaadin-combo-box-focus-index-mixin.js +++ b/packages/combo-box/src/vaadin-combo-box-focus-index-mixin.js @@ -36,11 +36,14 @@ export const ComboBoxFocusIndexMixin = (superClass) => return; } - // Move the viewport and set the focused row. Rendering rows - // around `index` lets placeholder rows fire `index-requested`, - // which loads any missing pages via the data-provider chain. - this._scrollIntoView(index); + // Set the focused row first: this synchronously runs the scroller's + // `__focusedIndexChanged` observer, which scrolls the row into view + // (bottom-aligned). Then scroll again to center it so the final position + // wins. Rendering rows around `index` also lets placeholder rows fire + // `index-requested`, loading any missing pages via the data-provider + // chain. this._focusedIndex = index; + this._scrollIntoView(index, true); // A page-load may have kicked in during the scroll (placeholder // rows in the new viewport requested their pages). Re-fire after diff --git a/packages/combo-box/src/vaadin-combo-box-scroller-mixin.d.ts b/packages/combo-box/src/vaadin-combo-box-scroller-mixin.d.ts index e0fc969aacc..ebd3015dca5 100644 --- a/packages/combo-box/src/vaadin-combo-box-scroller-mixin.d.ts +++ b/packages/combo-box/src/vaadin-combo-box-scroller-mixin.d.ts @@ -70,10 +70,13 @@ export declare class ComboBoxScrollerMixinClass { requestContentUpdate(): void; /** - * Scrolls an item at given index into view and adjusts `scrollTop` - * so that the element gets fully visible on Arrow Down key press. + * Scrolls an item at given index into view. By default the item is made + * fully visible at the bottom of the viewport (the behavior wanted on + * Arrow Down key press). When `alignToCenter` is `true`, the item is placed + * in the middle of the viewport instead, as close to center as the list + * allows near its start and end. */ - scrollIntoView(index: number): void; + scrollIntoView(index: number, alignToCenter?: boolean): void; protected _isItemSelected(item: TItem, selectedItem: TItem, itemIdPath: string | null | undefined): void; diff --git a/packages/combo-box/src/vaadin-combo-box-scroller-mixin.js b/packages/combo-box/src/vaadin-combo-box-scroller-mixin.js index 417b0090b0e..7cf12ee181f 100644 --- a/packages/combo-box/src/vaadin-combo-box-scroller-mixin.js +++ b/packages/combo-box/src/vaadin-combo-box-scroller-mixin.js @@ -166,29 +166,53 @@ export const ComboBoxScrollerMixin = (superClass) => } /** - * Scrolls an item at given index into view and adjusts `scrollTop` - * so that the element gets fully visible on Arrow Down key press. + * Scrolls an item at given index into view. By default the item is made + * fully visible at the bottom of the viewport (the behavior wanted on + * Arrow Down key press). When `alignToCenter` is `true`, the item is placed + * in the middle of the viewport instead, as close to center as the list + * allows near its start and end. * @param {number} index + * @param {boolean} [alignToCenter=false] */ - scrollIntoView(index) { + scrollIntoView(index, alignToCenter = false) { if (!this.__virtualizer || !(this.opened && index >= 0)) { return; } - const visibleItemsCount = this._visibleItemsCount(); + // If the target row is already fully visible, leave the scroll untouched. + // Re-running scrollToIndex would snap an unaligned scrollTop to a row + // boundary — a visible jump when only the focus ring should move (arrow + // navigation, clicks). Centering always repositions, so it skips this. + const renderedTarget = [...this.children].find((el) => !el.hidden && el.index === index); + if (!alignToCenter && renderedTarget) { + const targetRect = renderedTarget.getBoundingClientRect(); + const scrollerRect = this.getBoundingClientRect(); + if ( + targetRect.top >= scrollerRect.top && + targetRect.bottom + this._viewportTotalPaddingBottom <= scrollerRect.bottom + ) { + return; + } + } + // When centering, scrolling straight to the target index renders it and + // puts it at the top of the scroller; the rect-based correction below + // then moves it to the middle. No index recalculation is needed. let targetIndex = index; - if (index > this.__virtualizer.lastVisibleIndex - 1) { - // Index is below the bottom, scrolling down. Make the item appear at the bottom. - // First scroll to target (will be at the top of the scroller) to make sure it's rendered. - this.__virtualizer.scrollToIndex(index); - // Then calculate the index for the following scroll (to get the target to bottom of the scroller). - targetIndex = index - visibleItemsCount + 1; - } else if (index > this.__virtualizer.firstVisibleIndex) { - // The item is already visible, scrolling is unnecessary per se. But we need to trigger iron-list to set - // the correct scrollTop on the scrollTarget. Scrolling to firstVisibleIndex. - targetIndex = this.__virtualizer.firstVisibleIndex; + if (!alignToCenter) { + const visibleItemsCount = this._visibleItemsCount(); + if (index > this.__virtualizer.lastVisibleIndex - 1) { + // Index is below the bottom, scrolling down. Make the item appear at the bottom. + // First scroll to target (will be at the top of the scroller) to make sure it's rendered. + this.__virtualizer.scrollToIndex(index); + // Then calculate the index for the following scroll (to get the target to bottom of the scroller). + targetIndex = index - visibleItemsCount + 1; + } else if (index > this.__virtualizer.firstVisibleIndex) { + // The item is already visible, scrolling is unnecessary per se. But we need to trigger iron-list to set + // the correct scrollTop on the scrollTarget. Scrolling to firstVisibleIndex. + targetIndex = this.__virtualizer.firstVisibleIndex; + } } this.__virtualizer.scrollToIndex(Math.max(0, targetIndex)); @@ -201,6 +225,15 @@ export const ComboBoxScrollerMixin = (superClass) => } const targetRect = target.getBoundingClientRect(); const scrollerRect = this.getBoundingClientRect(); + if (alignToCenter) { + // Center the target in the viewport. Clamp to 0 at the top; the browser + // clamps to the maximum scroll position at the bottom, so near the start + // and end the item sits as close to center as the list allows. + const targetCenter = targetRect.top + targetRect.height / 2; + const scrollerCenter = scrollerRect.top + scrollerRect.height / 2; + this.scrollTop = Math.max(0, this.scrollTop + (targetCenter - scrollerCenter)); + return; + } const targetBottom = targetRect.bottom + this._viewportTotalPaddingBottom; if (targetBottom > scrollerRect.bottom) { this.scrollTop += targetBottom - scrollerRect.bottom; diff --git a/packages/combo-box/test/focus-index.test.js b/packages/combo-box/test/focus-index.test.js index 198c946105a..cf13f8f8866 100644 --- a/packages/combo-box/test/focus-index.test.js +++ b/packages/combo-box/test/focus-index.test.js @@ -1,5 +1,5 @@ import { expect } from '@vaadin/chai-plugins'; -import { arrowUpKeyDown, fixtureSync, nextFrame, nextRender } from '@vaadin/testing-helpers'; +import { arrowDownKeyDown, arrowUpKeyDown, fixtureSync, nextFrame, nextRender } from '@vaadin/testing-helpers'; import '../src/vaadin-combo-box.js'; import { ComboBoxPlaceholder } from '../src/vaadin-combo-box-placeholder.js'; import { flushComboBox, getViewportItems, makeItems } from './helpers.js'; @@ -34,6 +34,39 @@ describe('__focusIndex', () => { expect(comboBox._focusedIndex).to.equal(100); }); + it('should place the target in the middle of the viewport when opened', async () => { + comboBox.opened = true; + comboBox.__focusIndex(100); + flushComboBox(comboBox); + await nextFrame(); + + const findItem = (index) => [...comboBox._scroller.children].find((el) => !el.hidden && el.index === index); + const scrollerRect = comboBox._scroller.getBoundingClientRect(); + const targetRect = findItem(100).getBoundingClientRect(); + // The target is fully visible and centered in the viewport. + expect(targetRect.top).to.be.at.least(scrollerRect.top - 1); + expect(targetRect.bottom).to.be.at.most(scrollerRect.bottom + 1); + const targetCenter = targetRect.top + targetRect.height / 2; + const scrollerCenter = scrollerRect.top + scrollerRect.height / 2; + expect(targetCenter).to.be.closeTo(scrollerCenter, targetRect.height / 2 + 1); + }); + + it('should not shift the viewport when arrow-navigating among visible items', async () => { + comboBox.opened = true; + comboBox.__focusIndex(100); + flushComboBox(comboBox); + await nextFrame(); + + // Centering leaves the scroll position unaligned. Arrow navigation among + // already-visible rows must move only the focus ring, not the viewport. + const scrollTopBefore = comboBox._scroller.scrollTop; + arrowDownKeyDown(comboBox.inputElement); + await nextFrame(); + + expect(comboBox._focusedIndex).to.equal(101); + expect(comboBox._scroller.scrollTop).to.be.closeTo(scrollTopBefore, 1); + }); + it('should queue the scroll when called before opening', async () => { comboBox.__focusIndex(100); expect(comboBox.__pendingFocusIndex).to.equal(100); @@ -121,6 +154,17 @@ describe('__focusIndex', () => { return [...comboBox._scroller.children].find((el) => !el.hidden && el.index === index); } + // Asserts the target is fully visible and centered in the viewport. + function expectCenteredInViewport(index) { + const scrollerRect = getScrollerRect(); + const targetRect = findRenderedItem(index).getBoundingClientRect(); + expect(targetRect.top).to.be.at.least(scrollerRect.top - 1); + expect(targetRect.bottom).to.be.at.most(scrollerRect.bottom + 1); + const targetCenter = targetRect.top + targetRect.height / 2; + const scrollerCenter = scrollerRect.top + scrollerRect.height / 2; + expect(targetCenter).to.be.closeTo(scrollerCenter, 2.5); + } + beforeEach(async () => { comboBox = fixtureSync(` { comboBox.items = items; }); - it('should fully reveal target below the viewport with variable heights', async () => { + it('should center a target below the viewport with variable heights', async () => { comboBox.opened = true; flushComboBox(comboBox); @@ -140,35 +184,27 @@ describe('__focusIndex', () => { flushComboBox(comboBox); await nextFrame(); - const target = findRenderedItem(targetIndex); - const targetRect = target.getBoundingClientRect(); - const scrollerRect = getScrollerRect(); - expect(Math.round(targetRect.bottom)).to.be.at.most(Math.round(scrollerRect.bottom) + 1); - expect(Math.round(targetRect.top)).to.be.at.least(Math.round(scrollerRect.top) - 1); + expectCenteredInViewport(targetIndex); }); - it('should fully reveal target above the viewport with variable heights', async () => { + it('should center a target above the viewport with variable heights', async () => { comboBox.opened = true; flushComboBox(comboBox); - // Pre-scroll well past the target, then jump back to a low index. + // Pre-scroll well past the target, then jump back to a centerable index. comboBox.__focusIndex(150); flushComboBox(comboBox); await nextFrame(); - const targetIndex = 5; + const targetIndex = 30; comboBox.__focusIndex(targetIndex); flushComboBox(comboBox); await nextFrame(); - const target = findRenderedItem(targetIndex); - const targetRect = target.getBoundingClientRect(); - const scrollerRect = getScrollerRect(); - expect(Math.round(targetRect.top)).to.be.at.least(Math.round(scrollerRect.top) - 1); - expect(Math.round(targetRect.bottom)).to.be.at.most(Math.round(scrollerRect.bottom) + 1); + expectCenteredInViewport(targetIndex); }); - it('should not shift the viewport when target is already visible', async () => { + it('should re-center an already-visible target with variable heights', async () => { comboBox.opened = true; flushComboBox(comboBox); await nextFrame(); @@ -177,13 +213,30 @@ describe('__focusIndex', () => { const lastVisible = comboBox._scroller.__virtualizer.lastVisibleIndex; const targetIndex = firstVisible + Math.floor((lastVisible - firstVisible) / 2); - const scrollTopBefore = comboBox._scroller.scrollTop; comboBox.__focusIndex(targetIndex); flushComboBox(comboBox); await nextFrame(); - // Scrolling to an already-visible item should not shift the viewport. - expect(Math.abs(comboBox._scroller.scrollTop - scrollTopBefore)).to.be.below(2); + expectCenteredInViewport(targetIndex); + }); + + it('should clamp to the top for a near-start target that cannot be centered', async () => { + comboBox.opened = true; + flushComboBox(comboBox); + await nextFrame(); + + const targetIndex = 2; + comboBox.__focusIndex(targetIndex); + flushComboBox(comboBox); + await nextFrame(); + + // Not enough rows above to center; the list stays scrolled to the top + // and the target is fully visible. + expect(comboBox._scroller.scrollTop).to.be.closeTo(0, 1); + const scrollerRect = getScrollerRect(); + const targetRect = findRenderedItem(targetIndex).getBoundingClientRect(); + expect(targetRect.top).to.be.at.least(scrollerRect.top - 1); + expect(targetRect.bottom).to.be.at.most(scrollerRect.bottom + 1); }); });