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

DataGrid: Fix focus stuck in grid when first column is fixed and it contains a link (T1216832) #27397

Merged
merged 3 commits into from
May 21, 2024

Conversation

Alyar666
Copy link
Contributor

@Alyar666 Alyar666 added the 24_1 label May 15, 2024
@Alyar666 Alyar666 self-assigned this May 15, 2024
@Alyar666 Alyar666 requested a review from a team as a code owner May 15, 2024 18:34
@@ -1148,6 +1149,18 @@ const resizing = (Base: ModuleType<ResizingController>) => class ResizingColumnF
}
};

const keyboardNavigation = (Base: ModuleType<KeyboardNavigationController>) => class KeyboardNavigationExtender extends Base {
protected _toggleInertAttr(value = false): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the default argument value
Because we don't use this method without passing the value + passing the value is better than the default one defined in the method (more obvious)

Suggested change
protected _toggleInertAttr(value = false): void {
protected _toggleInertAttr(value: boolean): void {

@@ -1148,6 +1149,18 @@ const resizing = (Base: ModuleType<ResizingController>) => class ResizingColumnF
}
};

const keyboardNavigation = (Base: ModuleType<KeyboardNavigationController>) => class KeyboardNavigationExtender extends Base {
protected _toggleInertAttr(value = false): void {
const $fixedContent = this._rowsView?.element()?.children(`.${this.addWidgetPrefix(CONTENT_FIXED_CLASS)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add to the rowsView extender (in them_column_fixing) the method that returns fixed content element.

Something like:

const rowsView = (Base: ModuleType<RowsView>) => class RowsViewFixedColumnsExtender extends baseFixedColumns(Base) {
  // other methods & fields
  public getFixedContentElement(): dxElementWrapper {
    const fixedContentClass = this.addWidgetPrefix(CONTENT_FIXED_CLASS);
    return this.element()?.children(`.${fixedContentClass}`);

And use this method here:

Suggested change
const $fixedContent = this._rowsView?.element()?.children(`.${this.addWidgetPrefix(CONTENT_FIXED_CLASS)}`);
const $fixedContent = this._rowsView?.getFixedContentElement();

Comment on lines 165 to 166
// eslint-disable-next-line max-len
this.rowsViewFocusOutHandlerContext = this.rowsViewFocusOutHandlerContext ?? this.rowsViewFocusOutHandler.bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the eslint-disable-next-line comment and just write the ?? on the next line:

Suggested change
// eslint-disable-next-line max-len
this.rowsViewFocusOutHandlerContext = this.rowsViewFocusOutHandlerContext ?? this.rowsViewFocusOutHandler.bind(this);
this.rowsViewFocusOutHandlerContext = this.rowsViewFocusOutHandlerContext
?? this.rowsViewFocusOutHandler.bind(this);

@@ -688,14 +698,18 @@ export class KeyboardNavigationController extends modules.ViewController {
}
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected _toggleInertAttr(value = false): void {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for the default argument value


let isOriginalHandlerRequired = !isCellPositionDefined
|| (!eventArgs.shift && this._isLastValidCell(this._focusedCellPosition))
|| isLastValidCell
|| (eventArgs.shift && this._isFirstValidCell(this._focusedCellPosition));
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor refactoring [OPTIONAL].

Let's make the code more uniform and add the isFirstValidCell variable.
Something like:

const isFirstValidCell = eventArgs.shift && this._isFirstValidCell(this._focusedCellPosition);
const isLastValidCell = !eventArgs.shift && this._isLastValidCell(this._focusedCellPosition);

let isOriginalHandlerRequired = !isCellPositionDefined
      || isFirstValidCell
      || isLastValidCell;

@@ -906,4 +906,74 @@ QUnit.module('Keyboard navigation accessibility', {
assert.ok($headerItem.is(':focus'), 'Header cell has focus');
assert.notOk($headersElement.hasClass('dx-state-focused'), 'Headers main element has no dx-state-focused class');
});

// T1216832
testInDesktop('Navigation should not get stuck in the grid when the first cell is fixed and a template with links is specified for it', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's split this test case into two:

  • The first one will check the navigationOfAllCells
  • The second one will check the inert attribute

And let's add the third test case like in the original ticket issue:

  1. Focus on the last cell (in the DataGrid with fixed columns)
  2. Press the Tab
  3. Check that DataGrid loses focus or that the element after the DataGrid gains focus

We will check the original issue case with this test case
(In this issue focus goes to the fixed column instead of "move out" from the DataGrid)

@@ -906,4 +906,74 @@ QUnit.module('Keyboard navigation accessibility', {
assert.ok($headerItem.is(':focus'), 'Header cell has focus');
assert.notOk($headersElement.hasClass('dx-state-focused'), 'Headers main element has no dx-state-focused class');
});

// T1216832
testInDesktop('Navigation should not get stuck in the grid when the first cell is fixed and a template with links is specified for it', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question:
Why QUnit and not Testcafe? :)

@Alyar666 Alyar666 requested review from a team as code owners May 21, 2024 09:21
@Alyar666 Alyar666 merged commit 97a8d01 into DevExpress:24_1 May 21, 2024
272 checks passed
Alyar666 added a commit to Alyar666/DevExtreme that referenced this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants