From 1de304b2655d5396f6dfd1f4d399895f9560eb80 Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Wed, 23 Oct 2024 10:22:19 +0400 Subject: [PATCH] fix: scroll to correct focused row on keyboard navigation (#8003) --- .../vaadin-grid-pro-inline-editing-mixin.js | 2 +- .../vaadin-grid-keyboard-navigation-mixin.js | 29 +++---- .../keyboard-navigation-row-focus.common.js | 45 ++++++++-- .../grid/test/keyboard-navigation.common.js | 82 +++++++++++++------ 4 files changed, 105 insertions(+), 53 deletions(-) diff --git a/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js b/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js index 484c1761f5..74dd571353 100644 --- a/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js +++ b/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js @@ -508,7 +508,7 @@ export const InlineEditingMixin = (superClass) => if (!this.singleCellEdit && nextCell !== cell) { this._startEdit(nextCell, nextColumn); } else { - this._ensureScrolledToIndex(nextIndex); + this.__ensureFlatIndexInViewport(nextIndex); nextCell.focus(); } } diff --git a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js index 8d1e9c5b84..a4db5efa09 100644 --- a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js +++ b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js @@ -275,10 +275,10 @@ export const KeyboardNavigationMixin = (superClass) => } /** @private */ - _ensureScrolledToIndex(index) { + __ensureFlatIndexInViewport(index) { const targetRowInDom = [...this.$.items.children].find((child) => child.index === index); if (!targetRowInDom) { - this.scrollToIndex(index); + this._scrollToFlatIndex(index); } else { this.__scrollIntoViewport(index); } @@ -499,7 +499,7 @@ export const KeyboardNavigationMixin = (superClass) => } // Ensure correct vertical scroll position, destination row is visible - this._ensureScrolledToIndex(dstRowIndex); + this.__ensureFlatIndexInViewport(dstRowIndex); // When scrolling with repeated keydown, sometimes FocusEvent listeners // are too late to update _focusedItemIndex. Ensure next keydown @@ -729,23 +729,14 @@ export const KeyboardNavigationMixin = (superClass) => // The focus is about to exit the grid to the bottom. this.$.focusexit.focus(); } else if (focusTarget === this._itemsFocusable) { - let itemsFocusTarget = focusTarget; - const targetRow = isRow(focusTarget) ? focusTarget : focusTarget.parentNode; - this._ensureScrolledToIndex(this._focusedItemIndex); - if (targetRow.index !== this._focusedItemIndex && isCell(focusTarget)) { - // The target row, which is about to be focused next, has been - // assigned with a new index since last focus, probably because of - // scrolling. Focus the row for the stored focused item index instead. - const columnIndex = Array.from(targetRow.children).indexOf(this._itemsFocusable); - const focusedItemRow = Array.from(this.$.items.children).find( - (row) => !row.hidden && row.index === this._focusedItemIndex, - ); - if (focusedItemRow) { - itemsFocusTarget = focusedItemRow.children[columnIndex]; - } - } + this.__ensureFlatIndexInViewport(this._focusedItemIndex); + + // Ensure the correct element is set as focusable after scrolling. + // The virtualizer may use a different element to render the item. + this.__updateItemsFocusable(); + e.preventDefault(); - itemsFocusTarget.focus(); + this._itemsFocusable.focus(); } else { e.preventDefault(); focusTarget.focus(); diff --git a/packages/grid/test/keyboard-navigation-row-focus.common.js b/packages/grid/test/keyboard-navigation-row-focus.common.js index f9cce1de1e..96adf14176 100644 --- a/packages/grid/test/keyboard-navigation-row-focus.common.js +++ b/packages/grid/test/keyboard-navigation-row-focus.common.js @@ -99,11 +99,8 @@ function getTabbableRows(root) { return root.querySelectorAll('tr[tabindex]:not([hidden]):not([tabindex="-1"])'); } -function hierarchicalDataProvider({ parentItem }, callback) { - // Let's use a count lower than pageSize so we can ignore page + pageSize for now - const itemsOnEachLevel = 5; - - const items = [...Array(itemsOnEachLevel)].map((_, i) => { +function hierarchicalDataProvider({ parentItem, page, pageSize }, callback) { + const items = Array.from({ length: 100 }, (_, i) => { return { name: `${parentItem ? `${parentItem.name}-` : ''}${i}`, // Let's only have child items on every second item @@ -111,7 +108,8 @@ function hierarchicalDataProvider({ parentItem }, callback) { }; }); - callback(items, itemsOnEachLevel); + const offset = page * pageSize; + callback(items.slice(offset, offset + pageSize), items.length); } describe('keyboard navigation - row focus', () => { @@ -145,6 +143,39 @@ describe('keyboard navigation - row focus', () => { await nextRender(grid); }); + describe('scrolling and navigating', () => { + it('should scroll focused nested row into view on arrow key', () => { + focusItem(0); + // Expand first row + right(); + // Focus first nested row + down(); + // Simulate real scrolling to get the virtualizer to render + // the focused item in a different element. + grid.$.table.scrollTop = grid.$.table.scrollHeight / 2; + flushGrid(grid); + down(); + expect(getFocusedRowIndex(grid)).to.equal(2); + }); + + it('should scroll focused nested row into view on Tab', () => { + focusItem(0); + // Expand first row + right(); + // Focus first nested row + down(); + // Move focus to header + shiftTab(); + // Simulate real scrolling to get the virtualizer to render + // the focused item in a different element. + grid.$.table.scrollTop = grid.$.table.scrollHeight / 2; + flushGrid(grid); + // Move focus back to items + tab(); + expect(getFocusedRowIndex(grid)).to.equal(1); + }); + }); + describe('navigating with tab', () => { it('should have single tabbable row in every section', () => { const tabbableElements = getTabbableElements(grid.shadowRoot); @@ -339,7 +370,7 @@ describe('keyboard navigation - row focus', () => { end(); - expect(getFocusedRowIndex(grid)).to.equal(4); + expect(getFocusedRowIndex(grid)).to.equal(99); }); }); }); diff --git a/packages/grid/test/keyboard-navigation.common.js b/packages/grid/test/keyboard-navigation.common.js index 96377c3d2d..f90f81c63f 100644 --- a/packages/grid/test/keyboard-navigation.common.js +++ b/packages/grid/test/keyboard-navigation.common.js @@ -1305,43 +1305,73 @@ describe('keyboard navigation', () => { up(); expect(grid.$.table.scrollTop).to.equal(scrollTop); }); + }); - describe('rotating focus indicator prevention', () => { - it('should hide navigation mode when a focused row goes off screen', () => { - focusItem(0); - right(); + describe('scrolling and navigating', () => { + beforeEach(() => { + grid.items = undefined; + grid.size = 200; + grid.dataProvider = infiniteDataProvider; + flushGrid(grid); + }); - expect(grid.hasAttribute('navigating')).to.be.true; + it('should hide navigation mode when a focused row goes off screen', () => { + focusItem(0); + right(); - grid.scrollToIndex(100); - flushGrid(grid); + expect(grid.hasAttribute('navigating')).to.be.true; - expect(grid.hasAttribute('navigating')).to.be.false; - }); + grid.scrollToIndex(100); + flushGrid(grid); - it('should reveal navigation mode when a focused row is back on screen', () => { - focusItem(0); - right(); - grid.scrollToIndex(100); - flushGrid(grid); + expect(grid.hasAttribute('navigating')).to.be.false; + }); - grid.scrollToIndex(0); - flushGrid(grid); + it('should reveal navigation mode when a focused row is back on screen', () => { + focusItem(0); + right(); + grid.scrollToIndex(100); + flushGrid(grid); - expect(grid.hasAttribute('navigating')).to.be.true; - }); + grid.scrollToIndex(0); + flushGrid(grid); - it('should not hide navigation mode if a header cell is focused', () => { - tabToHeader(); - right(); + expect(grid.hasAttribute('navigating')).to.be.true; + }); - expect(grid.hasAttribute('navigating')).to.be.true; + it('should not hide navigation mode if a header cell is focused', () => { + tabToHeader(); + right(); - grid.scrollToIndex(100); - flushGrid(grid); + expect(grid.hasAttribute('navigating')).to.be.true; - expect(grid.hasAttribute('navigating')).to.be.true; - }); + grid.scrollToIndex(100); + flushGrid(grid); + + expect(grid.hasAttribute('navigating')).to.be.true; + }); + + it('should scroll focused row into view on arrow key', () => { + focusItem(0); + // Simulate real scrolling to get the virtualizer to render + // the focused item in a different element. + grid.$.table.scrollTop = grid.$.table.scrollHeight / 2; + flushGrid(grid); + down(); + expect(getFocusedRowIndex(grid)).to.equal(1); + expect(getFocusedCellIndex(grid)).to.equal(0); + }); + + it('should scroll focused row into view on Tab', () => { + focusItem(0); + tabToHeader(); + // Simulate real scrolling to get the virtualizer to render + // the focused item in a different element. + grid.$.table.scrollTop = grid.$.table.scrollHeight / 2; + flushGrid(grid); + tab(); + expect(getFocusedRowIndex(grid)).to.equal(0); + expect(getFocusedCellIndex(grid)).to.equal(0); }); }); });