Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure no focus outline when item is scrolled out of view #7980

Merged
merged 7 commits into from
Oct 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 42 additions & 21 deletions packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
*/
import { isKeyboardActive } from '@vaadin/a11y-base/src/focus-utils.js';
import { animationFrame } from '@vaadin/component-base/src/async.js';
import { Debouncer } from '@vaadin/component-base/src/debounce.js';
import { addValueToAttribute, removeValueFromAttribute } from '@vaadin/component-base/src/dom-utils.js';
import { get } from '@vaadin/component-base/src/path-utils.js';

Expand Down Expand Up @@ -504,8 +506,8 @@ export const KeyboardNavigationMixin = (superClass) =>
// listener invocation gets updated _focusedItemIndex value.
this._focusedItemIndex = dstRowIndex;

// This has to be set after scrolling, otherwise it can be removed by
// `_preventScrollerRotatingCellFocus(row, index)` during scrolling.
// Reapply navigating state in case it was removed due to previous item
// being focused with the mouse.
this.toggleAttribute('navigating', true);

return {
Expand Down Expand Up @@ -920,26 +922,45 @@ export const KeyboardNavigationMixin = (superClass) =>
focusTarget.tabIndex = isInteractingWithinActiveSection ? -1 : 0;
}

/**
* @param {!HTMLTableRowElement} row
* @param {number} index
* @protected
*/
_preventScrollerRotatingCellFocus(row, index) {
if (
row.index === this._focusedItemIndex &&
this.hasAttribute('navigating') &&
this._activeRowGroup === this.$.items
) {
// Focused item has went, hide navigation mode
this._navigatingIsHidden = true;
this.toggleAttribute('navigating', false);
}
if (index === this._focusedItemIndex && this._navigatingIsHidden) {
// Focused item is back, restore navigation mode
this._navigatingIsHidden = false;
this.toggleAttribute('navigating', true);
/** @protected */
_preventScrollerRotatingCellFocus() {
if (this._activeRowGroup !== this.$.items) {
Copy link
Contributor Author

@vursen vursen Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: This is an optimization to avoid triggering the debouncer when there is no focus within the items container.

return;
}

this.__preventScrollerRotatingCellFocusDebouncer = Debouncer.debounce(
this.__preventScrollerRotatingCellFocusDebouncer,
animationFrame,
() => {
const isItemsRowGroupActive = this._activeRowGroup === this.$.items;
const isFocusedItemRendered = this._getRenderedRows().some((row) => row.index === this._focusedItemIndex);
if (isFocusedItemRendered) {
// Ensure the correct element is focused, as the virtualizer
// may use different elements when re-rendering visible items.
this.__updateItemsFocusable();
Copy link
Contributor Author

@vursen vursen Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Debouncer + __updateItemsFocusable fixes #7659 (comment). It also fixes cases where scrolling back to the focused item is triggered by Tab or an arrow key.


// The focused item is visible, so restore the cell focus outline
// and navigation mode.
if (isItemsRowGroupActive && !this.__rowFocusMode) {
this._focusedCell = this._itemsFocusable;
}

if (this._navigatingIsHidden) {
this.toggleAttribute('navigating', true);
this._navigatingIsHidden = false;
}
} else if (isItemsRowGroupActive) {
// The focused item was scrolled out of view and focus is still inside body,
// so remove the cell focus outline and hide navigation mode.
this._focusedCell = null;

if (this.hasAttribute('navigating')) {
this._navigatingIsHidden = true;
this.toggleAttribute('navigating', false);
}
}
},
);
}

/**
Expand Down
4 changes: 4 additions & 0 deletions packages/grid/src/vaadin-grid-styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ export const gridStyles = css`
white-space: nowrap;
}

[part~='cell'] {
outline: none;
}
Comment on lines +148 to +150
Copy link
Contributor Author

@vursen vursen Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: This hides the default browser outline that otherwise appears when the focused-cell part is removed. It appears because the virtualizer element remains focused even after the focused item that was previously rendered in that element is no longer visible.

image


[part~='cell'] > [tabindex] {
display: flex;
align-items: inherit;
Expand Down
1 change: 1 addition & 0 deletions packages/grid/test/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const flushGrid = (grid) => {
].forEach((debouncer) => debouncer?.flush());

grid.__virtualizer.flush();
grid.__preventScrollerRotatingCellFocusDebouncer?.flush();
grid.performUpdate?.();
};

Expand Down
75 changes: 68 additions & 7 deletions packages/grid/test/keyboard-navigation.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
getContainerCell,
getFirstVisibleItem,
getLastVisibleItem,
getPhysicalItems,
getRowCells,
getRows,
infiniteDataProvider,
Expand Down Expand Up @@ -1332,6 +1333,7 @@ describe('keyboard navigation', () => {
expect(grid.hasAttribute('navigating')).to.be.true;

grid.scrollToIndex(100);
flushGrid(grid);

expect(grid.hasAttribute('navigating')).to.be.false;
});
Expand All @@ -1340,8 +1342,10 @@ describe('keyboard navigation', () => {
focusItem(0);
right();
grid.scrollToIndex(100);
flushGrid(grid);

grid.scrollToIndex(0);
flushGrid(grid);

expect(grid.hasAttribute('navigating')).to.be.true;
});
Expand All @@ -1353,6 +1357,7 @@ describe('keyboard navigation', () => {
expect(grid.hasAttribute('navigating')).to.be.true;

grid.scrollToIndex(100);
flushGrid(grid);

expect(grid.hasAttribute('navigating')).to.be.true;
});
Expand Down Expand Up @@ -1576,18 +1581,74 @@ describe('keyboard navigation', () => {
});

describe('focused-cell part', () => {
it('should add focused-cell to cell part when focused', () => {
focusFirstHeaderCell();

expect(getFirstHeaderCell().getAttribute('part')).to.contain('focused-cell');
beforeEach(() => {
grid.items = undefined;
grid.size = 200;
grid.dataProvider = infiniteDataProvider;
flushGrid(grid);
});

it('should remove focused-cell from cell part when blurred', () => {
focusFirstHeaderCell();
it('should add the part to cell when focused', () => {
focusItem(5);
const cell = getPhysicalItems(grid)[5].firstChild;
expect(cell.getAttribute('part')).to.contain('focused-cell');
});

it('should remove the part from cell when blurred', () => {
focusItem(5);
focusable.focus();
const cell = getPhysicalItems(grid)[5].firstChild;
expect(cell.getAttribute('part')).to.not.contain('focused-cell');
});

it('should keep the part when focused item is scrolled but still visible', () => {
focusItem(5);
grid.scrollToIndex(2);
flushGrid(grid);
const cell = getPhysicalItems(grid)[5].firstChild;
expect(cell.getAttribute('part')).to.contain('focused-cell');
});

it('should remove the part when focused item is scrolled out of view', () => {
focusItem(5);
grid.scrollToIndex(100);
flushGrid(grid);
expect(grid.$.items.querySelector(':not([hidden]) [part~="focused-cell"')).to.be.null;
});

expect(getFirstHeaderCell().getAttribute('part')).to.not.contain('focused-cell');
it('should restore the part when focused item is scrolled back to view', () => {
focusItem(5);
grid.scrollToIndex(100);
flushGrid(grid);

// Simulate real scrolling to get the virtualizer to render
// the focused item in a different element.
grid.$.table.scrollTop = 0;
flushGrid(grid);

const cell = getPhysicalItems(grid)[5].firstChild;
expect(cell.getAttribute('part')).to.contain('focused-cell');
});

it('should not add the part to any element when focused item is scrolled back to view - row focus mode', () => {
focusItem(5);
left();
grid.scrollToIndex(100);
flushGrid(grid);
grid.scrollToIndex(0);
flushGrid(grid);
expect(grid.$.items.querySelector(':not([hidden]) [part~="focused-cell"')).to.be.null;
});

it('should not remove the part from header cell when scrolling items', () => {
focusFirstHeaderCell();
grid.scrollToIndex(100);
flushGrid(grid);
expect(getFirstHeaderCell().getAttribute('part')).to.contain('focused-cell');

grid.scrollToIndex(0);
flushGrid(grid);
expect(getFirstHeaderCell().getAttribute('part')).to.contain('focused-cell');
});
});

Expand Down