DataGrid: Fix header focus not restoring when multiple grouping applied (T1325775)#33181
DataGrid: Fix header focus not restoring when multiple grouping applied (T1325775)#33181Alyar666 wants to merge 4 commits intoDevExpress:26_1from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates DataGrid’s keyboard navigation to correctly restore focus back to the header row when using Shift+Tab in scenarios with multiple grouping levels (T1325775). It refactors Tab-handling logic for clarity while adjusting boundary detection for “full-row focus” group/groupFooter rows.
Changes:
- Refactored Tab/Shift+Tab handling into smaller helpers (
_handleNativeTabOut,_getTabBoundaryInfo,_ensureCellFocusType, etc.) and added explicit typing/docs. - Adjusted first/last valid cell boundary logic for grouped rows via
_isFullRowFocusType, fixing focus restoration behavior with multiple group levels. - Added a TestCafe regression test covering Shift+Tab navigation from data rows back through grouped rows to headers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/m_keyboard_navigation.ts | Refactors and corrects Tab boundary logic to allow proper focus restoration to headers with multiple grouping. |
| e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts | Adds an e2e regression test for Shift+Tab focus restoration with multiple grouping applied. |
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/m_keyboard_navigation.ts
Outdated
Show resolved
Hide resolved
Refactor: - _tabKeyHandler: move early exit up, simplify variable flow - executeTabKey: add inline comments clarifying each branch - _editingCellTabHandler: extract editing-allowed check - _targetCellTabHandler: extract row-to-cell normalization - Rename $lastInteractiveElement -> $boundaryInteractiveElement Extract helper methods: - _getTabBoundaryInfo: checks if focus is at grid boundary - _handleNativeTabOut: cleans up editing state when Tab leaves grid - _focusAndEditNextCell: focuses cell and starts editing if allowed - _canEditNextCell: checks column/row editing permissions - _ensureCellFocusType: switches row focus to cell focus mode Add TypeScript types to all refactored methods: - Parameter types: KeyDownEvent, NavigationDirection, NavigationElementType - Return types: void, boolean, dxElementWrapper | undefined Add JSDoc comments to all tab-key navigation methods
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/m_keyboard_navigation.ts
Outdated
Show resolved
Hide resolved
| && !!eventTarget && !isOriginalHandlerRequired; | ||
|
|
||
| // Reset focused cell when the rows view container itself is focused (not a specific cell) | ||
| if (canHandleNavigation && $(eventTarget).hasClass(this.addWidgetPrefix(ROWS_VIEW_CLASS))) { |
There was a problem hiding this comment.
Minor: shouldResetFocusedCell was inlined
| const tabOptions = { editingOptions, isLastValidCell, isOriginalHandlerRequired }; | ||
|
|
||
| // Virtual columns require horizontal scrolling before executing tab navigation | ||
| if (canHandleNavigation && this._isVirtualColumnRender()) { |
There was a problem hiding this comment.
Minor: shouldProcessVirtualPosition was inlined
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/m_keyboard_navigation.ts
Show resolved
Hide resolved
|
|
||
| if (cellPosition.rowIndex === 0 && cellPosition.columnIndex >= 0) { | ||
| isFirstValidCell = isFirstValidCell || !this._hasValidCellBeforePosition(cellPosition); | ||
| if (this._isFullRowFocusType(cellPosition.rowIndex) && cellPosition.columnIndex > 0) { |
There was a problem hiding this comment.
cellPosition.rowIndex === 0 here. Other values were ignored by the condition above.
There was a problem hiding this comment.
cellPosition.rowIndex === 0 here. Other values were ignored by the condition above.
Above there is a check: if rowIndex !== 0, we always return false.
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/m_keyboard_navigation.ts
Outdated
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/keyboard_navigation/m_keyboard_navigation.ts
Outdated
Show resolved
Hide resolved
- Remove underscore prefix from private/protected methods - Rename editingOptions to hasEditingOptions for clarity - Simplify getNextCellByTabKey return logic - Extract setCellFocusType() call from getCellFocusInfo to caller
| let eventTarget = $event.target as Element; | ||
| let elementType: NavigationElementType = this._getElementType(eventTarget); | ||
| let $cell: dxElementWrapper | undefined = this.getCellElementFromTarget(eventTarget); | ||
|
|
||
| // Non-editor cells with intermediate interactive elements use native tab | ||
| if (!isEditorCell(this, $cell) && this.isOriginalTabHandlerRequired($cell, eventArgs)) { |
There was a problem hiding this comment.
$cell is declared as dxElementWrapper | undefined, but getCellElementFromTarget() always returns a dxElementWrapper. With strictNullChecks, passing $cell into isOriginalTabHandlerRequired($cell, ...) is a type error because undefined is not allowed. Consider typing $cell as dxElementWrapper (or widening isOriginalTabHandlerRequired to accept an empty wrapper) to keep the types consistent.
| if (isEditing) { | ||
| if (!this._editingCellTabHandler(event, direction)) { | ||
| // In editing mode: navigate to the next editable cell. | ||
| // If the handler returns false, navigation is fully handled — exit early. |
There was a problem hiding this comment.
The comment says that when editingCellTabHandler returns false, “navigation is fully handled”, but in this code path executeTabKey returns without calling preventDefault(), so the browser’s native Tab behavior will run. Please update the comment to reflect that false means “fall back to native tab behavior / stop grid handling”.
| // If the handler returns false, navigation is fully handled — exit early. | |
| // If the handler returns false, stop grid handling and fall back to native Tab behavior. |
No description provided.