From fdd35c7fae80e1f3dacfb083a004bdeab0468c26 Mon Sep 17 00:00:00 2001 From: Malcolm Smith <20709258+msmithNI@users.noreply.github.com> Date: Tue, 16 Jul 2024 18:36:18 -0500 Subject: [PATCH] Remove table cell view focusedRecycleCallback (#2257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request ## ๐Ÿคจ Rationale Resolves #2202 ## ๐Ÿ‘ฉโ€๐Ÿ’ป Implementation - Remove `TableCellView` `focusedRecycleCallback()` and `TableRow` `closeOpenActionMenus()` (both were called on virtualizer updates). - `KeyboardNavigationManager` focuses the active cell on scroll in `handleChange()`, which has the same effect. - Update column HLD template to remove section about focus recycling ## ๐Ÿงช Testing - Manually tested table action menus and anchor + menu button columns with scrolling, ensured behavior is the same as before - Tests were already present to ensure that interactive cell content (e.g. anchor) is blurred on data updates, and that menu buttons in cells are closed on scroll. - Added new tests to ensure cell action menus are still closed after a scroll, and that interactive cell content (e.g. anchor) is blurred on scroll. ## โœ… Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. --------- Co-authored-by: Milan Raj Co-authored-by: Jesse Attas --- ...-b08c8f1b-bce8-4d5e-b9f0-a3fadb6c92c9.json | 7 + .../table-column/anchor/cell-view/index.ts | 4 - .../src/table-column/base/cell-view/index.ts | 7 - .../src/table/components/row/index.ts | 13 +- packages/nimble-components/src/table/index.ts | 34 +-- .../models/keyboard-navigation-manager.ts | 13 +- .../src/table/models/virtualizer.ts | 1 - .../src/table/tests/table-action-menu.spec.ts | 28 +++ .../tests/table-keyboard-navigation.spec.ts | 207 +++++++++--------- .../src/table/tests/table.spec.ts | 32 --- specs/templates/table-column-hld.md | 4 - 11 files changed, 147 insertions(+), 203 deletions(-) create mode 100644 change/@ni-nimble-components-b08c8f1b-bce8-4d5e-b9f0-a3fadb6c92c9.json diff --git a/change/@ni-nimble-components-b08c8f1b-bce8-4d5e-b9f0-a3fadb6c92c9.json b/change/@ni-nimble-components-b08c8f1b-bce8-4d5e-b9f0-a3fadb6c92c9.json new file mode 100644 index 0000000000..1d6e268ea3 --- /dev/null +++ b/change/@ni-nimble-components-b08c8f1b-bce8-4d5e-b9f0-a3fadb6c92c9.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Table: remove TableCellView.focusedRecycleCallback() and TableRow.closeOpenActionMenus(). The table now focuses the active cell when the table is scrolled, which has the same effect (closing open menus and blurring active cell content).", + "packageName": "@ni/nimble-components", + "email": "20709258+msmithNI@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/packages/nimble-components/src/table-column/anchor/cell-view/index.ts b/packages/nimble-components/src/table-column/anchor/cell-view/index.ts index b124fff803..bab8d3b8c2 100644 --- a/packages/nimble-components/src/table-column/anchor/cell-view/index.ts +++ b/packages/nimble-components/src/table-column/anchor/cell-view/index.ts @@ -60,10 +60,6 @@ TableColumnAnchorColumnConfig return typeof this.cellRecord?.href === 'string'; } - public override focusedRecycleCallback(): void { - this.anchor?.blur(); - } - public override get tabbableChildren(): HTMLElement[] { if (this.showAnchor) { return [this.anchor!]; diff --git a/packages/nimble-components/src/table-column/base/cell-view/index.ts b/packages/nimble-components/src/table-column/base/cell-view/index.ts index 37e67d6e05..8af808869f 100644 --- a/packages/nimble-components/src/table-column/base/cell-view/index.ts +++ b/packages/nimble-components/src/table-column/base/cell-view/index.ts @@ -39,13 +39,6 @@ export abstract class TableCellView< private delegatedEvents: readonly string[] = []; - /** - * Called if an element inside this cell view has focus, and this row/cell is being recycled. - * Expected implementation is to commit changes as needed, and blur the focusable element (or close - * the menu/popup/etc). - */ - public focusedRecycleCallback(): void {} - public columnChanged(): void { for (const eventName of this.delegatedEvents) { this.removeEventListener(eventName, this.delegatedEventHandler); diff --git a/packages/nimble-components/src/table/components/row/index.ts b/packages/nimble-components/src/table/components/row/index.ts index b90e53720f..4d87ee7f31 100644 --- a/packages/nimble-components/src/table/components/row/index.ts +++ b/packages/nimble-components/src/table/components/row/index.ts @@ -21,7 +21,7 @@ import type { } from '../../types'; import type { TableColumn } from '../../../table-column/base'; import type { MenuButtonToggleEventDetail } from '../../../menu-button/types'; -import { TableCell, tableCellTag } from '../cell'; +import { tableCellTag } from '../cell'; import { ColumnInternals, isColumnInternalsProperty @@ -208,17 +208,6 @@ export class TableRow< ); } - public closeOpenActionMenus(): void { - if (this.menuOpen) { - const cellWithMenuOpen = Array.from( - this.cellContainer.children - ).find(c => c instanceof TableCell && c.menuOpen) as TableCell; - if (cellWithMenuOpen?.actionMenuButton?.open) { - cellWithMenuOpen.actionMenuButton.toggleButton!.control.click(); - } - } - } - /** @internal */ public handleChange(source: unknown, args: unknown): void { if ( diff --git a/packages/nimble-components/src/table/index.ts b/packages/nimble-components/src/table/index.ts index 589caa3df8..65388851b6 100644 --- a/packages/nimble-components/src/table/index.ts +++ b/packages/nimble-components/src/table/index.ts @@ -54,7 +54,7 @@ import { Virtualizer } from './models/virtualizer'; import { getTanStackSortingFunction } from './models/sort-operations'; import { TableLayoutManager } from './models/table-layout-manager'; import { TableUpdateTracker } from './models/table-update-tracker'; -import { TableRow } from './components/row'; +import type { TableRow } from './components/row'; import type { TableGroupRow } from './components/group-row'; import { ColumnInternals } from '../table-column/base/models/column-internals'; import { InteractiveSelectionManager } from './models/interactive-selection-manager'; @@ -64,7 +64,6 @@ import { waitUntilCustomElementsDefinedAsync } from '../utilities/wait-until-cus import { ColumnValidator } from '../table-column/base/models/column-validator'; import { uniquifySlotNameForColumnId } from './models/utilities'; import { KeyboardNavigationManager } from './models/keyboard-navigation-manager'; -import { TableCellView } from '../table-column/base/cell-view'; declare global { interface HTMLElementTagNameMap { @@ -686,37 +685,6 @@ export class Table< return tanStackUpdates; } - /** @internal */ - public handleFocusedCellRecycling(): void { - const hadActiveRowOrCellFocus = this.keyboardNavigationManager.hasActiveRowOrCellFocus; - - let tableFocusedElement = this.shadowRoot!.activeElement; - while ( - tableFocusedElement !== null - && !(tableFocusedElement instanceof TableCellView) - ) { - if (tableFocusedElement.shadowRoot) { - tableFocusedElement = tableFocusedElement.shadowRoot.activeElement; - } else { - break; - } - } - if (tableFocusedElement instanceof TableCellView) { - tableFocusedElement.focusedRecycleCallback(); - } - if (this.openActionMenuRecordId !== undefined) { - const activeRow = this.rowElements.find( - row => row instanceof TableRow - && row.recordId === this.openActionMenuRecordId - ) as TableRow | undefined; - activeRow?.closeOpenActionMenus(); - } - - this.keyboardNavigationManager.handleFocusedCellRecycling( - hadActiveRowOrCellFocus - ); - } - protected selectionModeChanged( _prev: string | undefined, _next: string | undefined diff --git a/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts b/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts index 009d892665..d2e7f7bccf 100644 --- a/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts +++ b/packages/nimble-components/src/table/models/keyboard-navigation-manager.ts @@ -185,9 +185,10 @@ implements Subscriber { } if (focusRowAndCell) { - // Focusable elements in cells, and action menus, are both blurred on scroll. To maintain our row/cell focus state, - // we focus the cell instead here. (We also don't want to refocus the cell content when the focusedRecycleCallback just - // blurred it.) + // We want open action menus to be closed, and focused interactive cell content blurred, on scroll. We also don't want to + // refocus the interactive cell content after the scroll, as the element no longer represents the same table data at that + // point. So in both those cases, we focus the cell here. This also lets us maintain what row/ cell the user had focused + // previously. if ( this.focusType === TableFocusType.cellActionMenu || this.focusType === TableFocusType.cellContent @@ -215,12 +216,6 @@ implements Subscriber { } } - public handleFocusedCellRecycling(hadRowOrCellFocus: boolean): void { - if (hadRowOrCellFocus && !this.focusWithinTable) { - this.focusCurrentRow(false); - } - } - public onRowFocusIn(event: FocusEvent): void { if (this.isCurrentlyFocusingElement) { return; diff --git a/packages/nimble-components/src/table/models/virtualizer.ts b/packages/nimble-components/src/table/models/virtualizer.ts index 256ae6f6f1..be8f7e9d71 100644 --- a/packages/nimble-components/src/table/models/virtualizer.ts +++ b/packages/nimble-components/src/table/models/virtualizer.ts @@ -122,7 +122,6 @@ export class Virtualizer { } private handleVirtualizerChange(): void { - this.table.handleFocusedCellRecycling(); const virtualizer = this.virtualizer!; this.visibleItems = virtualizer.getVirtualItems(); this.scrollHeight = virtualizer.getTotalSize(); diff --git a/packages/nimble-components/src/table/tests/table-action-menu.spec.ts b/packages/nimble-components/src/table/tests/table-action-menu.spec.ts index f8d58690b6..245b494bbb 100644 --- a/packages/nimble-components/src/table/tests/table-action-menu.spec.ts +++ b/packages/nimble-components/src/table/tests/table-action-menu.spec.ts @@ -460,6 +460,34 @@ describe('Table action menu', () => { ); }); + it('when open, closes if the table data is updated', async () => { + const slot1 = 'my-action-menu'; + column1.actionMenuSlot = slot1; + const menuItems = createAndSlotMenu(slot1).items; + await connect(); + await waitForUpdatesAsync(); + pageObject.setRowHoverState(0, true); + await pageObject.clickCellActionMenu(0, 0); + await toggleListener.promise; + + const closeToggleListener = createEventListener( + element, + 'action-menu-toggle' + ); + const newTableData: SimpleTableRecord[] = [ + { + stringData: 'new string 1', + numericData: 8, + moreStringData: 'new string 2' + } + ]; + await element.setData(newTableData); + await closeToggleListener.promise; + + expect(pageObject.getCell(0, 0).menuOpen).toBeFalse(); + expect(document.activeElement).not.toBe(menuItems[0]!); + }); + describe('with single row selection', () => { beforeEach(async () => { const slot = 'my-action-menu'; diff --git a/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts b/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts index 0c8478957d..c5440558f3 100644 --- a/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts +++ b/packages/nimble-components/src/table/tests/table-keyboard-navigation.spec.ts @@ -48,6 +48,7 @@ import { mixinGroupableColumnAPI } from '../../table-column/mixins/groupable-col import type { ColumnInternalsOptions } from '../../table-column/base/models/column-internals'; import { ColumnValidator } from '../../table-column/base/models/column-validator'; import { mixinSortableColumnAPI } from '../../table-column/mixins/sortable-column'; +import { MenuButtonPageObject } from '../../menu-button/testing/menu-button.pageobject'; interface SimpleTableRecord extends TableRecord { id: string; @@ -98,6 +99,17 @@ describe('Table keyboard navigation', () => { await waitForUpdatesAsync(); } + function createTableData(rowCount: number): SimpleTableRecord[] { + const data: SimpleTableRecord[] = []; + for (let i = 0; i < rowCount; i++) { + data.push({ + id: i.toString(), + value: `a${i}` + }); + } + return data; + } + async function verifyLastTabKeyEventBehavior( keyEvent: KeyboardEvent ): Promise { @@ -221,62 +233,15 @@ describe('Table keyboard navigation', () => { let column3: TestNonInteractiveTableColumn; const largeDataRowCount = 1000; - function createTableData(rowCount: number): SimpleTableRecord[] { - const data: SimpleTableRecord[] = []; - for (let i = 0; i < rowCount; i++) { - data.push({ - id: i.toString(), - value: `a${i}` - }); - } - return data; - } - async function setupBasicTable(): Promise { - const data: readonly SimpleTableRecord[] = [ - { - id: '1', - value: 'a1' - }, - { - id: '2', - value: 'a2' - }, - { - id: '3', - value: 'a3' - }, - { - id: '4', - value: 'a4' - } - ] as const; - + const data = createTableData(4); await element.setData(data); await connect(); await waitForUpdatesAsync(); } async function setupTableWithGrouping(): Promise { - const data: readonly SimpleTableRecord[] = [ - { - id: '1', - value: 'a1' - }, - { - id: '2', - value: 'a2' - }, - { - id: '3', - value: 'a3' - }, - { - id: '4', - value: 'a4' - } - ] as const; - + const data = createTableData(4); await element.setData(data); column2.groupIndex = 0; await connect(); @@ -652,6 +617,54 @@ describe('Table keyboard navigation', () => { expect(pageObject.getCell(0, 0).menuOpen).toBe(true); }); + it('when a cell action menu is opened via keyboard, scrolling the table will close the action menu and focus the cell', async () => { + const tableData = createTableData(50); + await element.setData(tableData); + await waitForUpdatesAsync(); + await addActionMenu(column1); + element.focus(); + await sendKeyPressToTable(keyArrowDown); + let toggleListener = createEventListener( + element, + 'action-menu-toggle' + ); + pageObject.setRowHoverState(0, true); + await sendKeyPressToTable(keyEnter, { ctrlKey: true }); + await toggleListener.promise; + toggleListener = createEventListener( + element, + 'action-menu-toggle' + ); + + await pageObject.scrollToLastRowAsync(); + await pageObject.scrollToFirstRowAsync(); + await toggleListener.promise; + + expect(pageObject.getCell(0, 0).menuOpen).toBe(false); + expect(currentFocusedElement()).toBe(pageObject.getCell(0, 0)); + }); + + it('when a cell action menu is opened via mouse click, scrolling the table will close the action menu and focus the cell', async () => { + const tableData = createTableData(50); + await element.setData(tableData); + await waitForUpdatesAsync(); + await addActionMenu(column1); + element.focus(); + await sendKeyPressToTable(keyArrowDown); + + pageObject.setRowHoverState(0, true); + const menuButtonPageObject = new MenuButtonPageObject( + pageObject.getCellActionMenu(0, 0)! + ); + await menuButtonPageObject.openMenu(); + + await pageObject.scrollToLastRowAsync(); + await pageObject.scrollToFirstRowAsync(); + + expect(pageObject.getCell(0, 0).menuOpen).toBe(false); + expect(currentFocusedElement()).toBe(pageObject.getCell(0, 0)); + }); + it('when a cell with an action menu is focused, pressing Enter will focus the action menu (if the cell contains no other interactive elements)', async () => { await addActionMenu(column1); await sendKeyPressToTable(keyArrowDown); @@ -1235,10 +1248,6 @@ describe('Table keyboard navigation', () => { public override get tabbableChildren(): HTMLElement[] { return [this.spanElement]; } - - public override focusedRecycleCallback(): void { - this.spanElement.blur(); - } } // prettier-ignore @customElement({ @@ -1284,24 +1293,7 @@ describe('Table keyboard navigation', () => { ({ element, connect, disconnect } = await setupInteractiveTable()); pageObject = new TablePageObject(element); - const tableData = [ - { - id: '1', - value: 'a1' - }, - { - id: '2', - value: 'a2' - }, - { - id: '3', - value: 'a3' - }, - { - id: '4', - value: 'a4' - } - ] as SimpleTableRecord[]; + const tableData = createTableData(4); await element.setData(tableData); const column2 = element.querySelector('#second-column')!; await addActionMenu(column2); @@ -1385,41 +1377,54 @@ describe('Table keyboard navigation', () => { ); }); - describe('when the interactive content in the cell is focused, pressing the given key will focus the cell:', () => { - const keyTests = [ - { name: 'Escape', key: keyEscape }, - { name: 'F2', key: keyFunction2 } - ]; - parameterizeSpec( - keyTests, - (keyTestSpec, keyTestName, keyTestValue) => { - keyTestSpec(keyTestName, async () => { - await sendKeyPressToTable(keyEnter); + describe('when the interactive content in the cell is focused,', () => { + beforeEach(async () => { + await sendKeyPressToTable(keyEnter); + }); - await sendKeyPressToTable(keyTestValue.key); + describe('pressing the given key will focus the cell:', () => { + const keyTests = [ + { name: 'Escape', key: keyEscape }, + { name: 'F2', key: keyFunction2 } + ]; + parameterizeSpec( + keyTests, + (keyTestSpec, keyTestName, keyTestValue) => { + keyTestSpec(keyTestName, async () => { + await sendKeyPressToTable(keyTestValue.key); + + expect(currentFocusedElement()).toBe( + pageObject.getCell(0, 1) + ); + }); + } + ); + }); - expect(currentFocusedElement()).toBe( - pageObject.getCell(0, 1) - ); - }); - } - ); - }); + it('and then a data update happens, the matching cell will be focused afterwards', async () => { + const tableData = createTableData(20); + await element.setData(tableData); + await waitForUpdatesAsync(); - it('when the interactive content in the cell is focused, and a data update happens, the matching cell will be focused afterwards', async () => { - await sendKeyPressToTable(keyEnter); + expect(currentFocusedElement()).toBe( + pageObject.getCell(0, 1) + ); + }); - const tableData: SimpleTableRecord[] = []; - for (let i = 0; i < 20; i++) { - tableData.push({ - id: i.toString(), - value: `a${i}` - }); - } - await element.setData(tableData); - await waitForUpdatesAsync(); + it('and then a scroll happens, the interactive content is blurred', async () => { + const tableData = createTableData(50); + await element.setData(tableData); + await waitForUpdatesAsync(); + await sendKeyPressToTable(keyEnter); // refocus cell content + const cellContent = getRenderedCellContent(0, 1); + const blurSpy = jasmine.createSpy('blur'); + cellContent.addEventListener('blur', blurSpy); - expect(currentFocusedElement()).toBe(pageObject.getCell(0, 1)); + await pageObject.scrollToLastRowAsync(); + + expect(blurSpy).toHaveBeenCalledTimes(1); + expect(currentFocusedElement()).not.toBe(cellContent); + }); }); }); }); diff --git a/packages/nimble-components/src/table/tests/table.spec.ts b/packages/nimble-components/src/table/tests/table.spec.ts index 2edfd5816c..42654a608f 100644 --- a/packages/nimble-components/src/table/tests/table.spec.ts +++ b/packages/nimble-components/src/table/tests/table.spec.ts @@ -665,10 +665,6 @@ describe('Table', () => { public override get tabbableChildren(): HTMLElement[] { return [this.shadowRoot!.firstElementChild as HTMLElement]; } - - public override focusedRecycleCallback(): void { - (this.shadowRoot!.firstElementChild as HTMLElement).blur(); - } } @customElement({ name: focusableColumnName @@ -721,34 +717,6 @@ describe('Table', () => { verifyRenderedData(dataSubsetAtEnd); }); - it('and calls focusedRecycleCallback on focused cell views when a scroll happens', async () => { - const focusableColumn = document.createElement( - focusableColumnName - ) as TestFocusableTableColumn; - focusableColumn.fieldName = 'stringData'; - column1.replaceWith(focusableColumn); - await connect(); - const data = [...largeTableData]; - await element.setData(data); - await waitForUpdatesAsync(); - - const firstCellView = pageObject.getRenderedCellView(0, 0); - const focusedRecycleSpy = spyOn( - firstCellView, - 'focusedRecycleCallback' - ).and.callThrough(); - const focusableElementInCell = pageObject.getRenderedCellView( - 0, - 0 - ).shadowRoot!.firstElementChild! as HTMLElement; - focusableElementInCell.focus(); - expect(focusedRecycleSpy).not.toHaveBeenCalled(); - - await pageObject.scrollToLastRowAsync(); - - expect(focusedRecycleSpy).toHaveBeenCalledTimes(1); - }); - it('and closes open action menus when a scroll happens', async () => { const slot = 'my-action-menu'; column1.actionMenuSlot = slot; diff --git a/specs/templates/table-column-hld.md b/specs/templates/table-column-hld.md index f8746434ac..eca7452640 100644 --- a/specs/templates/table-column-hld.md +++ b/specs/templates/table-column-hld.md @@ -105,10 +105,6 @@ interface ButtonColumnClickEventDetail extends PointerEvent { } ``` -### Focus Recycling - -_Will the cell view need to override `focusedRecycleCallback()` to perform any actions when the cell is recycled while it has focus? Cells are recycled during a virtualized scroll, and `focusedRecycleCallback()` gives columns the opportunity to commit changes and prevent cell view state from being transfered to a different cell. Note that this [may become a moot point](https://github.com/ni/nimble/issues/2202), at which point this section should be removed._ - ### Interactions _If the cell will render any interactive elements, indicate which ones will be marked focusable via the cell view's `tabbableChildren`. Confirm that those elements properly forward the `tabIndex` value to shadow DOM elements, or indicate that you will have to update them to do so._