From fde6eddcee09d5cc0fe3eb4ab850328c2f4e6fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Fri, 30 Aug 2024 10:35:32 +0200 Subject: [PATCH] fix: do not scroll when focusing a cell with click (#7717) --- packages/grid/src/vaadin-grid-mixin.js | 8 +- packages/grid/src/vaadin-grid-scroll-mixin.js | 5 +- .../grid/test/scroll-into-view-lit.test.js | 3 + .../test/scroll-into-view-polymer.test.js | 2 + packages/grid/test/scroll-into-view.common.js | 90 +++++++++++++++++++ 5 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 packages/grid/test/scroll-into-view-lit.test.js create mode 100644 packages/grid/test/scroll-into-view-polymer.test.js create mode 100644 packages/grid/test/scroll-into-view.common.js diff --git a/packages/grid/src/vaadin-grid-mixin.js b/packages/grid/src/vaadin-grid-mixin.js index ebe1cfd26e..48e2775e09 100644 --- a/packages/grid/src/vaadin-grid-mixin.js +++ b/packages/grid/src/vaadin-grid-mixin.js @@ -618,8 +618,8 @@ export const GridMixin = (superClass) => // Patch `focus()` to use the button cell._focusButton = div; - cell.focus = function () { - cell._focusButton.focus(); + cell.focus = function (options) { + cell._focusButton.focus(options); }; div.appendChild(slot); @@ -642,7 +642,7 @@ export const GridMixin = (superClass) => // Only focus if mouse is released on cell content itself const mouseUpWithinCell = event.composedPath().includes(cellContent); if (!contentContainsFocusedElement && mouseUpWithinCell) { - cell.focus(); + cell.focus({ preventScroll: true }); } document.removeEventListener('mouseup', mouseUpListener, true); }; @@ -652,7 +652,7 @@ export const GridMixin = (superClass) => // Watch out sync focus removal issue, only async focus works here. setTimeout(() => { if (!cellContent.contains(this.getRootNode().activeElement)) { - cell.focus(); + cell.focus({ preventScroll: true }); } }); } diff --git a/packages/grid/src/vaadin-grid-scroll-mixin.js b/packages/grid/src/vaadin-grid-scroll-mixin.js index 01b01f4ce4..5725f6305e 100644 --- a/packages/grid/src/vaadin-grid-scroll-mixin.js +++ b/packages/grid/src/vaadin-grid-scroll-mixin.js @@ -126,7 +126,10 @@ export const ScrollMixin = (superClass) => if (this._rowWithFocusedElement) { // Make sure the row with the focused element is fully inside the visible viewport - this.__scrollIntoViewport(this._rowWithFocusedElement.index); + // Don't change scroll position if the user is interacting with the mouse + if (!this._isMousedown) { + this.__scrollIntoViewport(this._rowWithFocusedElement.index); + } if (!this.$.table.contains(e.relatedTarget)) { // Virtualizer can't catch the event because if orginates from the light DOM. diff --git a/packages/grid/test/scroll-into-view-lit.test.js b/packages/grid/test/scroll-into-view-lit.test.js new file mode 100644 index 0000000000..3d10732035 --- /dev/null +++ b/packages/grid/test/scroll-into-view-lit.test.js @@ -0,0 +1,3 @@ +import '../theme/lumo/lit-all-imports.js'; +import '../src/lit-all-imports.js'; +import './scroll-into-view.common.js'; diff --git a/packages/grid/test/scroll-into-view-polymer.test.js b/packages/grid/test/scroll-into-view-polymer.test.js new file mode 100644 index 0000000000..7866627fed --- /dev/null +++ b/packages/grid/test/scroll-into-view-polymer.test.js @@ -0,0 +1,2 @@ +import '../vaadin-grid.js'; +import './scroll-into-view.common.js'; diff --git a/packages/grid/test/scroll-into-view.common.js b/packages/grid/test/scroll-into-view.common.js new file mode 100644 index 0000000000..74d9c5f35f --- /dev/null +++ b/packages/grid/test/scroll-into-view.common.js @@ -0,0 +1,90 @@ +import { expect } from '@vaadin/chai-plugins'; +import { fixtureSync, nextFrame } from '@vaadin/testing-helpers'; +import { sendKeys, sendMouse } from '@web/test-runner-commands'; +import { flushGrid, getContainerCell, getLastVisibleItem, getPhysicalItems } from './helpers.js'; + +describe('scroll into view', () => { + let grid, firstCell, secondCell, secondDetailsCell; + + function verifyRowFullyVisible(grid, rowIndex) { + const scrollerBounds = grid.$.scroller.getBoundingClientRect(); + const row = getPhysicalItems(grid)[rowIndex]; + const rowBounds = row.getBoundingClientRect(); + expect(rowBounds.top).to.be.greaterThanOrEqual(scrollerBounds.top); + expect(rowBounds.bottom).to.be.lessThanOrEqual(scrollerBounds.bottom); + } + + function verifyRowNotFullyVisible(grid, rowIndex) { + const scrollerBounds = grid.$.scroller.getBoundingClientRect(); + const row = getPhysicalItems(grid)[rowIndex]; + const rowBounds = row.getBoundingClientRect(); + const isTopInvisible = rowBounds.top < scrollerBounds.top; + const isBottomInvisible = rowBounds.bottom > scrollerBounds.bottom; + expect(isTopInvisible || isBottomInvisible).to.be.true; + } + + beforeEach(async () => { + grid = fixtureSync(` + + + + `); + const column = grid.querySelector('vaadin-grid-column'); + column.renderer = (root, _, model) => { + root.innerHTML = `
${model.item}
`; + }; + grid.rowDetailsRenderer = (root, _, model) => { + root.innerHTML = `
${model.item} details
`; + }; + grid.items = [1, 2]; + grid.detailsOpenedItems = [2]; + + flushGrid(grid); + await nextFrame(); + + firstCell = getContainerCell(grid.$.items, 0, 0); + secondCell = getContainerCell(grid.$.items, 1, 0); + secondDetailsCell = getLastVisibleItem(grid).querySelector('[part~="details-cell"]'); + }); + + it('should scroll row into view when focusing programmatically', () => { + verifyRowNotFullyVisible(grid, 1); + + secondCell.focus(); + + expect(grid.$.table.scrollTop).to.be.above(0); + verifyRowFullyVisible(grid, 1); + }); + + it('should scroll row into view when focusing with keyboard navigation', async () => { + verifyRowNotFullyVisible(grid, 1); + + firstCell.focus(); + await sendKeys({ press: 'ArrowDown' }); + + expect(grid.$.table.scrollTop).to.be.above(0); + verifyRowFullyVisible(grid, 1); + }); + + it('should not change scroll position when focusing row cell with click', async () => { + verifyRowNotFullyVisible(grid, 1); + + const bounds = secondCell.getBoundingClientRect(); + await sendMouse({ type: 'click', position: [bounds.x + 5, bounds.y + 5] }); + + expect(grid.$.table.scrollTop).to.equal(0); + }); + + it('should not change scroll position when focusing details cell with click', async () => { + // Make details cell partially visible for clicking + grid.style.height = '240px'; + await nextFrame(); + + verifyRowNotFullyVisible(grid, 1); + + const bounds = secondDetailsCell.getBoundingClientRect(); + await sendMouse({ type: 'click', position: [bounds.x + 5, bounds.y + 5] }); + + expect(grid.$.table.scrollTop).to.equal(0); + }); +});