From dd0ccce14e39dc5fc589e53c9e8c7130680c3f9e Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Wed, 16 Oct 2024 13:59:25 +0400 Subject: [PATCH 1/7] fix: ensure no focus outline when item is scrolled out of view --- .../vaadin-grid-keyboard-navigation-mixin.js | 57 ++++++++++++------- packages/grid/src/vaadin-grid-styles.js | 4 ++ 2 files changed, 40 insertions(+), 21 deletions(-) diff --git a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js index e6cb59fdc42..f8197d30de7 100644 --- a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js +++ b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js @@ -3,7 +3,9 @@ * Copyright (c) 2016 - 2024 Vaadin Ltd. * 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 { isElementFocused, 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'; @@ -920,26 +922,39 @@ 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() { + this.__preventScrollerRotatingCellFocusDebouncer = Debouncer.debounce( + this.__preventScrollerRotatingCellFocusDebouncer, + animationFrame, + () => { + const isFocusedItemRendered = this._getRenderedRows().some((row) => row.index === this._focusedItemIndex); + if (isFocusedItemRendered) { + // Focused item was scrolled back to view, revert the focus outline + this.__updateItemsFocusable(); + + if (isElementFocused(this._itemsFocusable) && !this.__rowFocusMode) { + this._focusedCell = this._itemsFocusable; + } + + if (this._navigatingIsHidden) { + // Focused item is back, restore navigation mode + this._navigatingIsHidden = false; + this.toggleAttribute('navigating', true); + } + } else { + // Focused item was scrolled out of view, remove the focus outline + if (isElementFocused(this._itemsFocusable)) { + this._focusedCell = null; + } + + if (this.hasAttribute('navigating') && this._activeRowGroup) { + this._navigatingIsHidden = true; + this.toggleAttribute('navigating', false); + } + } + }, + ); } /** diff --git a/packages/grid/src/vaadin-grid-styles.js b/packages/grid/src/vaadin-grid-styles.js index 7fd91295034..0ea5f666fb8 100644 --- a/packages/grid/src/vaadin-grid-styles.js +++ b/packages/grid/src/vaadin-grid-styles.js @@ -145,6 +145,10 @@ export const gridStyles = css` white-space: nowrap; } + [part~='cell'] { + outline: none; + } + [part~='cell'] > [tabindex] { display: flex; align-items: inherit; From a41da5356ec1ffb0f3218d2b7d71e35abfe927d5 Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Wed, 16 Oct 2024 14:35:20 +0400 Subject: [PATCH 2/7] fix tests --- .../vaadin-grid-keyboard-navigation-mixin.js | 20 +++++++++++-------- .../grid/test/keyboard-navigation.common.js | 10 +++++++--- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js index f8197d30de7..18815313f08 100644 --- a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js +++ b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js @@ -3,7 +3,7 @@ * Copyright (c) 2016 - 2024 Vaadin Ltd. * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/ */ -import { isElementFocused, isKeyboardActive } from '@vaadin/a11y-base/src/focus-utils.js'; +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'; @@ -928,27 +928,31 @@ export const KeyboardNavigationMixin = (superClass) => this.__preventScrollerRotatingCellFocusDebouncer, animationFrame, () => { + const isItemsRowGroupActive = this._activeRowGroup === this.$.items; const isFocusedItemRendered = this._getRenderedRows().some((row) => row.index === this._focusedItemIndex); if (isFocusedItemRendered) { - // Focused item was scrolled back to view, revert the focus outline + // Ensure the correct element is focused, as the virtualizer + // may render items using different elements. this.__updateItemsFocusable(); - if (isElementFocused(this._itemsFocusable) && !this.__rowFocusMode) { + // Revert the cell focus outline if focus is still inside the body. + if (isItemsRowGroupActive && !this.__rowFocusMode) { this._focusedCell = this._itemsFocusable; } + // Restore navigation mode if (this._navigatingIsHidden) { - // Focused item is back, restore navigation mode - this._navigatingIsHidden = false; this.toggleAttribute('navigating', true); + this._navigatingIsHidden = false; } } else { - // Focused item was scrolled out of view, remove the focus outline - if (isElementFocused(this._itemsFocusable)) { + // Focused item was scrolled out of view, remove the cell focus outline + if (isItemsRowGroupActive) { this._focusedCell = null; } - if (this.hasAttribute('navigating') && this._activeRowGroup) { + // Hide navigation mode if focus is inside the body. + if (isItemsRowGroupActive && this.hasAttribute('navigating')) { this._navigatingIsHidden = true; this.toggleAttribute('navigating', false); } diff --git a/packages/grid/test/keyboard-navigation.common.js b/packages/grid/test/keyboard-navigation.common.js index 1786a18da7c..85f12b2c4fd 100644 --- a/packages/grid/test/keyboard-navigation.common.js +++ b/packages/grid/test/keyboard-navigation.common.js @@ -1325,34 +1325,38 @@ describe('keyboard navigation', () => { }); describe('rotating focus indicator prevention', () => { - it('should hide navigation mode when a focused row goes off screen', () => { + it('should hide navigation mode when a focused row goes off screen', async () => { focusItem(0); right(); expect(grid.hasAttribute('navigating')).to.be.true; grid.scrollToIndex(100); + await nextFrame(); expect(grid.hasAttribute('navigating')).to.be.false; }); - it('should reveal navigation mode when a focused row is back on screen', () => { + it('should reveal navigation mode when a focused row is back on screen', async () => { focusItem(0); right(); grid.scrollToIndex(100); + await nextFrame(); grid.scrollToIndex(0); + await nextFrame(); expect(grid.hasAttribute('navigating')).to.be.true; }); - it('should not hide navigation mode if a header cell is focused', () => { + it('should not hide navigation mode if a header cell is focused', async () => { tabToHeader(); right(); expect(grid.hasAttribute('navigating')).to.be.true; grid.scrollToIndex(100); + await nextFrame(); expect(grid.hasAttribute('navigating')).to.be.true; }); From 57f7036be2e4a4be021cabc680f983ecbfc6deaf Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Wed, 16 Oct 2024 15:46:04 +0400 Subject: [PATCH 3/7] wip --- .../vaadin-grid-keyboard-navigation-mixin.js | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js index 18815313f08..e7b6398eea0 100644 --- a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js +++ b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js @@ -932,27 +932,25 @@ export const KeyboardNavigationMixin = (superClass) => const isFocusedItemRendered = this._getRenderedRows().some((row) => row.index === this._focusedItemIndex); if (isFocusedItemRendered) { // Ensure the correct element is focused, as the virtualizer - // may render items using different elements. + // may have rendered the item using a different element. this.__updateItemsFocusable(); - // Revert the cell focus outline if focus is still inside the body. + // if the focused item is visible, restore the cell focus outline + // and navigation mode. if (isItemsRowGroupActive && !this.__rowFocusMode) { this._focusedCell = this._itemsFocusable; } - // Restore navigation mode if (this._navigatingIsHidden) { this.toggleAttribute('navigating', true); this._navigatingIsHidden = false; } - } else { - // Focused item was scrolled out of view, remove the cell focus outline - if (isItemsRowGroupActive) { - this._focusedCell = null; - } + } else if (isItemsRowGroupActive) { + // If the focused item was scrolled out of view and focus is still inside body, + // remove the cell focus outline and hide navigation mode. + this._focusedCell = null; - // Hide navigation mode if focus is inside the body. - if (isItemsRowGroupActive && this.hasAttribute('navigating')) { + if (this.hasAttribute('navigating')) { this._navigatingIsHidden = true; this.toggleAttribute('navigating', false); } From f4e4c5744a4672227bc73200c6a6ac5a8b6ba0a4 Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Wed, 16 Oct 2024 17:26:36 +0400 Subject: [PATCH 4/7] add tests --- .../vaadin-grid-keyboard-navigation-mixin.js | 4 + .../grid/test/keyboard-navigation.common.js | 74 +++++++++++++++++-- 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js index e7b6398eea0..76c50d89aeb 100644 --- a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js +++ b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js @@ -924,6 +924,10 @@ export const KeyboardNavigationMixin = (superClass) => /** @protected */ _preventScrollerRotatingCellFocus() { + if (this._activeRowGroup !== this.$.items) { + return; + } + this.__preventScrollerRotatingCellFocusDebouncer = Debouncer.debounce( this.__preventScrollerRotatingCellFocusDebouncer, animationFrame, diff --git a/packages/grid/test/keyboard-navigation.common.js b/packages/grid/test/keyboard-navigation.common.js index 85f12b2c4fd..7d57c7dfec3 100644 --- a/packages/grid/test/keyboard-navigation.common.js +++ b/packages/grid/test/keyboard-navigation.common.js @@ -26,6 +26,7 @@ import { getContainerCell, getFirstVisibleItem, getLastVisibleItem, + getPhysicalItems, getRowCells, getRows, infiniteDataProvider, @@ -1580,18 +1581,77 @@ 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', async () => { + focusItem(5); + grid.scrollToIndex(2); + await nextFrame(); + const cell = getPhysicalItems(grid)[5].firstChild; + expect(cell.getAttribute('part')).to.contain('focused-cell'); + }); - expect(getFirstHeaderCell().getAttribute('part')).to.not.contain('focused-cell'); + it('should remove the part when focused item is scrolled out of view', async () => { + focusItem(5); + grid.scrollToIndex(100); + await nextFrame(); + expect(grid.$.items.querySelector(':not([hidden]) [part~="focused-cell"')).to.be.null; + }); + + it('should restore the part when focused item is scrolled back to view', async () => { + focusItem(5); + grid.scrollToIndex(100); + await nextFrame(); + + // Simulate real scrolling to get the virtualizer to render + // the focused item in a different element. + grid.$.table.scrollTop = 0; + // Virtualizer scroll handler + await nextFrame(); + // preventScrollerRotatingCellFocusDebouncer + await nextFrame(); + + 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', async () => { + focusItem(5); + left(); + grid.scrollToIndex(100); + await nextFrame(); + grid.scrollToIndex(0); + await nextFrame(); + expect(grid.$.items.querySelector(':not([hidden]) [part~="focused-cell"')).to.be.null; + }); + + it('should not remove the part from header cell when scrolling items', async () => { + focusFirstHeaderCell(); + grid.scrollToIndex(100); + await nextFrame(); + expect(getFirstHeaderCell().getAttribute('part')).to.contain('focused-cell'); + + grid.scrollToIndex(0); + await nextFrame(); + expect(getFirstHeaderCell().getAttribute('part')).to.contain('focused-cell'); }); }); From e174d3e3065f8f56ad08f182e66cc8d847eec85c Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Wed, 16 Oct 2024 17:31:33 +0400 Subject: [PATCH 5/7] improve code comments --- .../grid/src/vaadin-grid-keyboard-navigation-mixin.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js index 76c50d89aeb..c916cf05551 100644 --- a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js +++ b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js @@ -936,10 +936,10 @@ export const KeyboardNavigationMixin = (superClass) => const isFocusedItemRendered = this._getRenderedRows().some((row) => row.index === this._focusedItemIndex); if (isFocusedItemRendered) { // Ensure the correct element is focused, as the virtualizer - // may have rendered the item using a different element. + // may use different elements when re-rendering visible items. this.__updateItemsFocusable(); - // if the focused item is visible, restore the cell focus outline + // The focused item is visible, so restore the cell focus outline // and navigation mode. if (isItemsRowGroupActive && !this.__rowFocusMode) { this._focusedCell = this._itemsFocusable; @@ -950,8 +950,8 @@ export const KeyboardNavigationMixin = (superClass) => this._navigatingIsHidden = false; } } else if (isItemsRowGroupActive) { - // If the focused item was scrolled out of view and focus is still inside body, - // remove the cell focus outline and hide navigation mode. + // 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')) { From 29857f5582e0db86731ece052deeb8528e329e41 Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Wed, 16 Oct 2024 17:46:22 +0400 Subject: [PATCH 6/7] update code comment to accurately describe code behavior --- packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js index c916cf05551..8d1e9c5b84c 100644 --- a/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js +++ b/packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js @@ -506,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 { From 0279e22d4f3cf6117f0e08235db24ed57b992aea Mon Sep 17 00:00:00 2001 From: Sergey Vinogradov Date: Thu, 17 Oct 2024 12:58:00 +0400 Subject: [PATCH 7/7] fix failing test in webkit --- packages/grid/test/helpers.js | 1 + .../grid/test/keyboard-navigation.common.js | 43 +++++++++---------- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/packages/grid/test/helpers.js b/packages/grid/test/helpers.js index d4ee0f3845a..c2dcddb0b32 100644 --- a/packages/grid/test/helpers.js +++ b/packages/grid/test/helpers.js @@ -16,6 +16,7 @@ export const flushGrid = (grid) => { ].forEach((debouncer) => debouncer?.flush()); grid.__virtualizer.flush(); + grid.__preventScrollerRotatingCellFocusDebouncer?.flush(); grid.performUpdate?.(); }; diff --git a/packages/grid/test/keyboard-navigation.common.js b/packages/grid/test/keyboard-navigation.common.js index 7d57c7dfec3..fe3c3f7be08 100644 --- a/packages/grid/test/keyboard-navigation.common.js +++ b/packages/grid/test/keyboard-navigation.common.js @@ -1326,38 +1326,38 @@ describe('keyboard navigation', () => { }); describe('rotating focus indicator prevention', () => { - it('should hide navigation mode when a focused row goes off screen', async () => { + it('should hide navigation mode when a focused row goes off screen', () => { focusItem(0); right(); expect(grid.hasAttribute('navigating')).to.be.true; grid.scrollToIndex(100); - await nextFrame(); + flushGrid(grid); expect(grid.hasAttribute('navigating')).to.be.false; }); - it('should reveal navigation mode when a focused row is back on screen', async () => { + it('should reveal navigation mode when a focused row is back on screen', () => { focusItem(0); right(); grid.scrollToIndex(100); - await nextFrame(); + flushGrid(grid); grid.scrollToIndex(0); - await nextFrame(); + flushGrid(grid); expect(grid.hasAttribute('navigating')).to.be.true; }); - it('should not hide navigation mode if a header cell is focused', async () => { + it('should not hide navigation mode if a header cell is focused', () => { tabToHeader(); right(); expect(grid.hasAttribute('navigating')).to.be.true; grid.scrollToIndex(100); - await nextFrame(); + flushGrid(grid); expect(grid.hasAttribute('navigating')).to.be.true; }); @@ -1601,56 +1601,53 @@ describe('keyboard navigation', () => { expect(cell.getAttribute('part')).to.not.contain('focused-cell'); }); - it('should keep the part when focused item is scrolled but still visible', async () => { + it('should keep the part when focused item is scrolled but still visible', () => { focusItem(5); grid.scrollToIndex(2); - await nextFrame(); + 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', async () => { + it('should remove the part when focused item is scrolled out of view', () => { focusItem(5); grid.scrollToIndex(100); - await nextFrame(); + flushGrid(grid); expect(grid.$.items.querySelector(':not([hidden]) [part~="focused-cell"')).to.be.null; }); - it('should restore the part when focused item is scrolled back to view', async () => { + it('should restore the part when focused item is scrolled back to view', () => { focusItem(5); grid.scrollToIndex(100); - await nextFrame(); + flushGrid(grid); // Simulate real scrolling to get the virtualizer to render // the focused item in a different element. grid.$.table.scrollTop = 0; - // Virtualizer scroll handler - await nextFrame(); - // preventScrollerRotatingCellFocusDebouncer - await nextFrame(); + 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', async () => { + 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); - await nextFrame(); + flushGrid(grid); grid.scrollToIndex(0); - await nextFrame(); + 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', async () => { + it('should not remove the part from header cell when scrolling items', () => { focusFirstHeaderCell(); grid.scrollToIndex(100); - await nextFrame(); + flushGrid(grid); expect(getFirstHeaderCell().getAttribute('part')).to.contain('focused-cell'); grid.scrollToIndex(0); - await nextFrame(); + flushGrid(grid); expect(getFirstHeaderCell().getAttribute('part')).to.contain('focused-cell'); }); });