Skip to content

Commit

Permalink
fix: do not scroll when focusing a cell with click (#7717) (#7722)
Browse files Browse the repository at this point in the history
Co-authored-by: Sascha Ißbrücker <[email protected]>
  • Loading branch information
vaadin-bot and sissbruecker authored Aug 30, 2024
1 parent 941a8e2 commit 10a1537
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 5 deletions.
8 changes: 4 additions & 4 deletions packages/grid/src/vaadin-grid-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -600,8 +600,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);
Expand All @@ -624,7 +624,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);
};
Expand All @@ -634,7 +634,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 });
}
});
}
Expand Down
5 changes: 4 additions & 1 deletion packages/grid/src/vaadin-grid-scroll-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions packages/grid/test/scroll-into-view-lit.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import '../theme/lumo/lit-all-imports.js';
import '../src/lit-all-imports.js';
import './scroll-into-view.common.js';
2 changes: 2 additions & 0 deletions packages/grid/test/scroll-into-view-polymer.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import '../vaadin-grid.js';
import './scroll-into-view.common.js';
90 changes: 90 additions & 0 deletions packages/grid/test/scroll-into-view.common.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import { expect } from '@esm-bundle/chai';
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(`
<vaadin-grid style="height: 150px">
<vaadin-grid-column></vaadin-grid-column>
</vaadin-grid>
`);
const column = grid.querySelector('vaadin-grid-column');
column.renderer = (root, _, model) => {
root.innerHTML = `<div style="height: 100px">${model.item}</div>`;
};
grid.rowDetailsRenderer = (root, _, model) => {
root.innerHTML = `<div style="height: 30px">${model.item} details</div>`;
};
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);
});
});

0 comments on commit 10a1537

Please sign in to comment.