Skip to content

Commit

Permalink
fix: scroll to correct focused row on keyboard navigation (#8003)
Browse files Browse the repository at this point in the history
  • Loading branch information
@vursen
vursen authored Oct 23, 2024
1 parent 0cb7c09 commit 1de304b
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 53 deletions.
2 changes: 1 addition & 1 deletion packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Expand Down
29 changes: 10 additions & 19 deletions packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
45 changes: 38 additions & 7 deletions packages/grid/test/keyboard-navigation-row-focus.common.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,17 @@ 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
children: i % 2 === 0,
};
});

callback(items, itemsOnEachLevel);
const offset = page * pageSize;
callback(items.slice(offset, offset + pageSize), items.length);
}

describe('keyboard navigation - row focus', () => {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -339,7 +370,7 @@ describe('keyboard navigation - row focus', () => {

end();

expect(getFocusedRowIndex(grid)).to.equal(4);
expect(getFocusedRowIndex(grid)).to.equal(99);
});
});
});
Expand Down
82 changes: 56 additions & 26 deletions packages/grid/test/keyboard-navigation.common.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
Expand Down

0 comments on commit 1de304b

Please sign in to comment.