From 02d091fd23f801b849caa1074c0bf9e6d520e774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Thu, 29 Aug 2024 16:04:13 +0200 Subject: [PATCH 1/3] fix: do not scroll when clicking cells --- 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 | 114 ++++++++++++++++++ 5 files changed, 127 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..278a1c2f9a --- /dev/null +++ b/packages/grid/test/scroll-into-view.common.js @@ -0,0 +1,114 @@ +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 } from './helpers.js'; + +describe('scroll into view', () => { + let grid, firstCell, secondCell; + + function verifyCellFullyVisible(grid, cell) { + const scrollerBounds = grid.$.scroller.getBoundingClientRect(); + const cellBounds = cell.getBoundingClientRect(); + expect(cellBounds.top).to.be.greaterThanOrEqual(scrollerBounds.top); + expect(cellBounds.bottom).to.be.lessThanOrEqual(scrollerBounds.bottom); + } + + function verifyCellNotFullyVisible(grid, cell) { + const scrollerBounds = grid.$.scroller.getBoundingClientRect(); + const cellBounds = cell.getBoundingClientRect(); + const isTopInvisible = cellBounds.top < scrollerBounds.top; + const isBottomInvisible = cellBounds.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.items = [1, 2]; + flushGrid(grid); + await nextFrame(); + + firstCell = getContainerCell(grid.$.items, 0, 0); + secondCell = getContainerCell(grid.$.items, 1, 0); + }); + + describe('body cells', () => { + it('should scroll cell into view when focusing programmatically', () => { + verifyCellNotFullyVisible(grid, secondCell); + + secondCell.focus(); + + expect(grid.$.table.scrollTop).to.be.above(0); + verifyCellFullyVisible(grid, secondCell); + }); + + it('should scroll cell into view when focusing with keyboard navigation', async () => { + verifyCellNotFullyVisible(grid, secondCell); + + firstCell.focus(); + await sendKeys({ press: 'ArrowDown' }); + + expect(grid.$.table.scrollTop).to.be.above(0); + verifyCellFullyVisible(grid, secondCell); + }); + + it('should not change scroll position when focusing with click', async () => { + verifyCellNotFullyVisible(grid, secondCell); + + const bounds = secondCell.getBoundingClientRect(); + await sendMouse({ type: 'click', position: [bounds.x + 10, bounds.y + 10] }); + + expect(grid.$.table.scrollTop).to.equal(0); + }); + }); + + describe('details cells', () => { + let detailsCell; + + beforeEach(async () => { + grid.rowDetailsRenderer = (root, _, model) => { + root.innerHTML = `
${model.item} details
`; + }; + grid.detailsOpenedItems = [2]; + flushGrid(grid); + await nextFrame(); + + detailsCell = getLastVisibleItem(grid).querySelector('[part~="details-cell"]'); + }); + + it('should scroll details cell into view when focusing row cell programmatically', () => { + verifyCellNotFullyVisible(grid, detailsCell); + + secondCell.focus(); + + expect(grid.$.table.scrollTop).to.be.above(0); + verifyCellFullyVisible(grid, detailsCell); + }); + + it('should scroll cell into view when focusing row cell with keyboard navigation', async () => { + verifyCellNotFullyVisible(grid, detailsCell); + + firstCell.focus(); + await sendKeys({ press: 'ArrowDown' }); + + expect(grid.$.table.scrollTop).to.be.above(0); + verifyCellFullyVisible(grid, detailsCell); + }); + + it('should not change scroll position when focusing details cell with click', async () => { + verifyCellNotFullyVisible(grid, detailsCell); + + const bounds = detailsCell.getBoundingClientRect(); + await sendMouse({ type: 'click', position: [bounds.x + 10, bounds.y + 10] }); + + expect(grid.$.table.scrollTop).to.equal(0); + }); + }); +}); From 15a3d8c3e6be75734942fa87d1984afd07cc8e74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sascha=20I=C3=9Fbr=C3=BCcker?= Date: Thu, 29 Aug 2024 19:37:37 +0200 Subject: [PATCH 2/3] restructure tests --- packages/grid/test/scroll-into-view.common.js | 112 +++++++----------- 1 file changed, 44 insertions(+), 68 deletions(-) diff --git a/packages/grid/test/scroll-into-view.common.js b/packages/grid/test/scroll-into-view.common.js index 278a1c2f9a..2a33f58542 100644 --- a/packages/grid/test/scroll-into-view.common.js +++ b/packages/grid/test/scroll-into-view.common.js @@ -1,23 +1,25 @@ 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 } from './helpers.js'; +import { flushGrid, getContainerCell, getLastVisibleItem, getPhysicalItems } from './helpers.js'; describe('scroll into view', () => { - let grid, firstCell, secondCell; + let grid, firstCell, secondCell, secondDetailsCell; - function verifyCellFullyVisible(grid, cell) { + function verifyRowFullyVisible(grid, rowIndex) { const scrollerBounds = grid.$.scroller.getBoundingClientRect(); - const cellBounds = cell.getBoundingClientRect(); - expect(cellBounds.top).to.be.greaterThanOrEqual(scrollerBounds.top); - expect(cellBounds.bottom).to.be.lessThanOrEqual(scrollerBounds.bottom); + 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 verifyCellNotFullyVisible(grid, cell) { + function verifyRowNotFullyVisible(grid, rowIndex) { const scrollerBounds = grid.$.scroller.getBoundingClientRect(); - const cellBounds = cell.getBoundingClientRect(); - const isTopInvisible = cellBounds.top < scrollerBounds.top; - const isBottomInvisible = cellBounds.bottom > scrollerBounds.bottom; + 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; } @@ -31,84 +33,58 @@ describe('scroll into view', () => { 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"]'); }); - describe('body cells', () => { - it('should scroll cell into view when focusing programmatically', () => { - verifyCellNotFullyVisible(grid, secondCell); - - secondCell.focus(); - - expect(grid.$.table.scrollTop).to.be.above(0); - verifyCellFullyVisible(grid, secondCell); - }); - - it('should scroll cell into view when focusing with keyboard navigation', async () => { - verifyCellNotFullyVisible(grid, secondCell); - - firstCell.focus(); - await sendKeys({ press: 'ArrowDown' }); - - expect(grid.$.table.scrollTop).to.be.above(0); - verifyCellFullyVisible(grid, secondCell); - }); + it('should scroll row into view when focusing programmatically', () => { + verifyRowNotFullyVisible(grid, 1); - it('should not change scroll position when focusing with click', async () => { - verifyCellNotFullyVisible(grid, secondCell); + secondCell.focus(); - const bounds = secondCell.getBoundingClientRect(); - await sendMouse({ type: 'click', position: [bounds.x + 10, bounds.y + 10] }); - - expect(grid.$.table.scrollTop).to.equal(0); - }); + expect(grid.$.table.scrollTop).to.be.above(0); + verifyRowFullyVisible(grid, 1); }); - describe('details cells', () => { - let detailsCell; - - beforeEach(async () => { - grid.rowDetailsRenderer = (root, _, model) => { - root.innerHTML = `
${model.item} details
`; - }; - grid.detailsOpenedItems = [2]; - flushGrid(grid); - await nextFrame(); - - detailsCell = getLastVisibleItem(grid).querySelector('[part~="details-cell"]'); - }); + it('should scroll row into view when focusing with keyboard navigation', async () => { + verifyRowNotFullyVisible(grid, 1); - it('should scroll details cell into view when focusing row cell programmatically', () => { - verifyCellNotFullyVisible(grid, detailsCell); + firstCell.focus(); + await sendKeys({ press: 'ArrowDown' }); - secondCell.focus(); + expect(grid.$.table.scrollTop).to.be.above(0); + verifyRowFullyVisible(grid, 1); + }); - expect(grid.$.table.scrollTop).to.be.above(0); - verifyCellFullyVisible(grid, detailsCell); - }); + it('should not change scroll position when focusing row cell with click', async () => { + verifyRowNotFullyVisible(grid, 1); - it('should scroll cell into view when focusing row cell with keyboard navigation', async () => { - verifyCellNotFullyVisible(grid, detailsCell); + const bounds = secondCell.getBoundingClientRect(); + await sendMouse({ type: 'click', position: [bounds.x + 5, bounds.y + 5] }); - firstCell.focus(); - await sendKeys({ press: 'ArrowDown' }); + expect(grid.$.table.scrollTop).to.equal(0); + }); - expect(grid.$.table.scrollTop).to.be.above(0); - verifyCellFullyVisible(grid, detailsCell); - }); + 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(); - it('should not change scroll position when focusing details cell with click', async () => { - verifyCellNotFullyVisible(grid, detailsCell); + verifyRowNotFullyVisible(grid, 1); - const bounds = detailsCell.getBoundingClientRect(); - await sendMouse({ type: 'click', position: [bounds.x + 10, bounds.y + 10] }); + const bounds = secondDetailsCell.getBoundingClientRect(); + await sendMouse({ type: 'click', position: [bounds.x + 5, bounds.y + 5] }); - expect(grid.$.table.scrollTop).to.equal(0); - }); + expect(grid.$.table.scrollTop).to.equal(0); }); }); From 3754c5631f48438f8bea378edbe4296e9a5a1a7a Mon Sep 17 00:00:00 2001 From: Serhii Kulykov Date: Fri, 30 Aug 2024 11:00:56 +0300 Subject: [PATCH 3/3] Update packages/grid/test/scroll-into-view.common.js --- packages/grid/test/scroll-into-view.common.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/grid/test/scroll-into-view.common.js b/packages/grid/test/scroll-into-view.common.js index 2a33f58542..74d9c5f35f 100644 --- a/packages/grid/test/scroll-into-view.common.js +++ b/packages/grid/test/scroll-into-view.common.js @@ -25,9 +25,9 @@ describe('scroll into view', () => { beforeEach(async () => { grid = fixtureSync(` - - - + + + `); const column = grid.querySelector('vaadin-grid-column'); column.renderer = (root, _, model) => {