Repository navigation
Grids: fix kbn typing gaps and clean up its API - #36063
Conversation
|
|
||
| const rowIndex = this._columnsController.getRowIndex(focusedColumn.index ?? -1, true); | ||
| const newVisibleIndex = this.getVisibleIndex(focusedColumn); | ||
| if (focusedColumn) { |
There was a problem hiding this comment.
Shift+Alt+G in a filter row editor, with no header cell focused, threw after clearGrouping() and inside beginUpdate(). The columns controller stayed locked and the grid didn't re-render. Update the focus position only when a header column is focused. Now fixed
| const column = this._getColumnByCellElement($cell, rowIndex); | ||
|
|
||
| if (this.canReorderColumn(column, direction, rowIndex)) { | ||
| if (column && this.canReorderColumn(column, direction, rowIndex)) { |
There was a problem hiding this comment.
Headers Ctrl+Left and DataGrid Ctrl+G: ignore a key target outside a header cell. groupColumn takes Column | undefined, like ungroupColumn.
| } | ||
|
|
||
| if (visibleColumnCount > columnIndex && isNavigableColumnCell($cell, column)) { | ||
| if (column && isNavigableColumnCell($cell, column)) { |
There was a problem hiding this comment.
column && replaces visibleColumnCount > columnIndex.
Both read the same index from the same array, so the check now bounds the index from both sides. A detached spanning cell returns false instead of throwing on column.command.
2e5f907 to
58ca59b
Compare
| columnIndex: newVisibleIndex, | ||
| }); | ||
| } else { | ||
| this.updateFocusPosition(); |
There was a problem hiding this comment.
With no focused header column (Shift+Alt+G in a filter row editor) reset the headers focus position, so focus goes to the first header cell, as the group panel's Shift+Alt+G does.
| } | ||
|
|
||
| public _focus( | ||
| public applyFocus( |
There was a problem hiding this comment.
applyFocus is reached only through this - from the class itself and from the m_focus.ts and scrollable_a11y.ts extenders - so could it be protected, like _getFocusedCell became in this PR?
| const $row = this._focusedView?.getRow(rowIndex); | ||
| const $cell = this._getFocusedCell(); | ||
| const allowCollapsing: Grouping['allowCollapsing'] = this.option('grouping.allowCollapsing'); | ||
| const masterDetailEnabled: MasterDetail['enabled'] = this.option('masterDetail.enabled'); |
There was a problem hiding this comment.
TreeList registers the masterDetail module as well (tree_list/m_master_detail.ts), so with masterDetail: { enabled: true } set there, Enter on the expand cell no longer toggles the detail row - since the option isn't in tree_list.d.ts, is it fine to leave this case behind?
There was a problem hiding this comment.
TreeList uses the module's rendering of detail-type rows and its nested-grid resizing, not the feature itself. Its data extender in tree_list/m_master_detail.ts sends isRowExpanded, _processItems and _processDataItem past master detail's implementation, through the …Hack methods. So TreeList never keeps master-detail expand state and never inserts detail rows. With masterDetail: { enabled: true }, all it gets is an extra empty dx-command-expand column.
Enter on that cell, before and after this PR:
- Before: core called this._dataController.changeRowExpand(key). In TreeList that's the tree's own changeRowExpand, so Enter expanded or collapsed the tree node, not a detail row. A click on the same cell did nothing, because TreeList's _rowClick only toggles on the tree's expand icon. So keyboard and mouse already disagreed.
- After: Enter does nothing there, the same as the click. No detail row appears through a click, Enter or masterDetail.autoExpandAll: true. This PR only changes the Enter path.
Why the master-detail part sits in grouping:
- Master-detail expansion already depends on DataGrid grouping. DataMasterDetailExtender overrides changeRowExpandCore, which only grouping's data controller defines and calls. It's called from grouping's changeRowExpand, which also fires onRowExpanding/onRowExpanded. The public expandRow/collapseRow/isRowExpanded come from grouping too.
- The mouse counterpart is already there. Grouping's _rowClick → _changeGroupRowState handles both group rows and the data rows of master detail. So the Enter handler now sits next to the click handler, in the one grid where master detail works.
- A cleaner split is a separate refactor. That would give master detail its own expand entry point, so its data and keyboard parts could move into the master-detail module. I'd keep it out of this PR.
There was a problem hiding this comment.
so Enter now just matches the click on that cell. Ok, let's leave it be
There was a problem hiding this comment.
🟢 Approval recommended
The review identifies no active issues with the keyboard-navigation typing and controller refactor.
No open issues 🎉
What changed in this PR
This PR strengthens grid keyboard-navigation typing and cleans up its internal controller API. It also moves DataGrid-specific row expansion out of shared grid code.
Changes:
- Type focused-cell state and handle missing columns safely.
- Encapsulate editing focus state, rename controller methods, and extract Enter-key expansion into a grouping extender.
- Add Jest regression coverage and update existing QUnit references.
| File | Description |
|---|---|
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/masterDetail.integration.tests.js |
Updates cell-validation method reference. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/keyboardNavigation.rowsView.tests.js |
Updates focus and validation calls and mocks. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/keyboardNavigation.realControllers.tests.js |
Renames cell-validation mock. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/keyboardNavigation.keyboardKeys.tests.js |
Updates focus calls and assertions. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/keyboardNavigation.keyboardController.tests.js |
Updates focus calls and mocks. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/keyboardNavigation.customization.tests.js |
Updates fast-editing state checks. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/focus.tests.js |
Updates cell-validation assertion. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/editing.tests.js |
Renames focused-row notification stub. |
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/columnFixing.integration.tests.js |
Updates command-cell validation call. |
packages/devextreme/js/__internal/grids/grid_core/views/columns_view.ts |
Accepts partial cell positions with explicit checks. |
packages/devextreme/js/__internal/grids/grid_core/row_dragging/row_dragging.ts |
Uses renamed focus-reset method. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/utils.ts |
Uses renamed virtual-scrolling method. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/types.ts |
Removes duplicate position type and grouping dependency. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/scrollable_a11y.ts |
Updates focus override and tab-index call. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/m_keyboard_navigation.ts |
Types positions, encapsulates focus state, and delegates expansion. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/keyboard_navigation_core.ts |
Types protected focus state and optional column lookup. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/headers_keyboard_navigation.ts |
Guards missing columns during reordering. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/column_keyboard_navigation_core.ts |
Reuses shared cell-position type. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/__tests__/keyboard_navigation.integration.test.ts |
Adds invalid-cell and editing-focus coverage. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/__tests__/helpers/utils.ts |
Shares key-event processing and adds header helpers. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/__tests__/headers_keyboard_navigation.integration.test.ts |
Covers reordering outside header cells. |
packages/devextreme/js/__internal/grids/grid_core/focus/m_focus.ts |
Updates keyboard-controller API calls. |
packages/devextreme/js/__internal/grids/grid_core/adaptivity/extenders/adaptivity_keyboard_navigation_view_controller.ts |
Renames cell-validation and detection overrides. |
packages/devextreme/js/__internal/grids/data_grid/keyboard_navigation/headers_keyboard_navigation.ts |
Handles missing columns during grouping and ungrouping. |
packages/devextreme/js/__internal/grids/data_grid/keyboard_navigation/group_panel_keyboard_navigation.ts |
Handles partial positions in group-cell lookup. |
packages/devextreme/js/__internal/grids/data_grid/keyboard_navigation/column_keyboard_navigation_mixin.ts |
Reuses shared cell-position type. |
packages/devextreme/js/__internal/grids/data_grid/keyboard_navigation/__tests__/headers_keyboard_navigation.integration.test.ts |
Covers grouping shortcuts and focus restoration. |
packages/devextreme/js/__internal/grids/data_grid/grouping/grouping_module.ts |
Registers the keyboard-navigation extender. |
packages/devextreme/js/__internal/grids/data_grid/grouping/extenders/grouping_keyboard_navigation_controller.ts |
Handles Enter-key group and master-detail expansion. |
packages/devextreme/js/__internal/grids/data_grid/grouping/extenders/__tests__/grouping_keyboard_navigation_controller.enter_key.integration.test.ts |
Covers expansion, continuation rows, and editing delegation. |
Review effort: Balanced
By commits:
_getColumnByCellElementreadsgetVisibleColumns()[cellIndex + offset]without a range check, so it returns undefined for an out-of-range index. Type it Column | undefined and handle it in the four callers that dereferenced the result