Skip to content

PivotGrid: open header filter popup by hotkey #33994

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.

Sign up for GitHub

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

Open
sjbur wants to merge 1 commit into
base: 26_1
Choose a base branch
from
+65 −20
Open

PivotGrid: open header filter popup by hotkey #33994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 36 additions & 19 deletions e2e/testcafe-devextreme/tests/common/pivotGrid/kbn/fields.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ const createConfig = () => ({
.ok('first field is focused after click');

await t
.pressKey(area === 'data' ? 'tab' : 'tab tab')
.pressKey('tab')
.expect(secondField.focused)
.ok('second field is focused after Tab Tab');
.ok('second field is focused after Tab');

await t
.pressKey(area === 'data' ? 'shift+tab' : 'shift+tab shift+tab')
.pressKey('shift+tab')
.expect(firstField.focused)
.ok('first field is focused after Shift+Tab Shift+Tab');
.ok('first field is focused after Shift+Tab');
}).before(async () => createWidget('dxPivotGrid', createConfig()));
});

Expand All @@ -148,9 +148,9 @@ const createConfig = () => ({

await t
.click(firstField)
.pressKey('tab tab')
.pressKey('tab')
.expect(secondField.focused)
.ok('second field is focused after Tab Tab')
.ok('second field is focused after Tab')
.expect(secondField.find('.dx-sort-up').exists)
.ok('second field has asc sort indicator initially');

Expand Down Expand Up @@ -181,9 +181,9 @@ const createConfig = () => ({

await t
.click(firstField)
.pressKey('tab tab')
.pressKey('tab')
.expect(secondField.focused)
.ok('second field is focused after Tab Tab')
.ok('second field is focused after Tab')
.expect(secondField.find('.dx-sort-up').exists)
.ok('second field has asc sort indicator initially');

Expand All @@ -202,6 +202,25 @@ const createConfig = () => ({
.ok('second field has asc sort indicator after second Space');
}).before(async () => createWidget('dxPivotGrid', createConfig()));

test(`${testTitlePrefix}: Field in ${area} should open header filter by alt+ArrowDown`, async (t) => {
const pivotGrid = new PivotGrid(PIVOT_GRID_SELECTOR);
const headerFilter = new HeaderFilter();

if (isFieldChooser) {
await t.click(pivotGrid.getFieldChooserButton());
}

const firstField = getField(pivotGrid, area, 0);

await t
.click(firstField)
.expect(firstField.focused)
.ok('field is focused after click')
.pressKey('alt+down')
.expect(headerFilter.element.exists)
.ok('header filter popup is shown after Alt+ArrowDown');
}).before(async () => createWidget('dxPivotGrid', createConfig()));

test(`${testTitlePrefix}: Field in ${area} should have focus after header filter is closed`, async (t) => {
const pivotGrid = new PivotGrid(PIVOT_GRID_SELECTOR);
const headerFilter = new HeaderFilter();
Expand All @@ -214,10 +233,9 @@ const createConfig = () => ({

await t
.click(firstField)
.pressKey('tab')
.pressKey('enter')
.pressKey('alt+down')
.expect(headerFilter.element.exists)
.ok('header filter popup is shown after Enter on icon');
.ok('header filter popup is shown after Alt+ArrowDown');

await t
.pressKey('esc')
Expand All @@ -237,10 +255,9 @@ const createConfig = () => ({

await t
.click(firstField)
.pressKey('tab')
.pressKey('enter')
.pressKey('alt+down')
.expect(headerFilter.element.exists)
.ok('header filter popup is shown after Enter on icon');
.ok('header filter popup is shown after Alt+ArrowDown');

const list = headerFilter.getList();
const okButton = headerFilter.getButtons().nth(0);
Expand Down Expand Up @@ -268,7 +285,7 @@ test('PivotGrid: Should traverse fields in all areas by tab', async (t) => {
.ok('first field in filter area is focused after click');

await t
.pressKey('tab tab tab tab')
.pressKey('tab tab')
.expect(dataFirstField.focused)
.ok('first field in data area is focused');

Expand All @@ -278,7 +295,7 @@ test('PivotGrid: Should traverse fields in all areas by tab', async (t) => {
.ok('first field in column area is focused');

await t
.pressKey('tab tab tab tab tab tab')
.pressKey('tab tab tab tab')
.expect(rowFirstField.focused)
.ok('first field in row area is focused');
}).before(async () => createWidget('dxPivotGrid', createConfig()));
Expand All @@ -300,17 +317,17 @@ test('FieldChooser: Should traverse fields in all areas by tab', async (t) => {
.ok('first field in row area is focused after click');

await t
.pressKey('tab tab tab tab')
.pressKey('tab tab')
.expect(columnFirstField.focused)
.ok('first field in column area is focused');

await t
.pressKey('tab tab tab tab')
.pressKey('tab tab')
.expect(filterFirstField.focused)
.ok('first field in filter area is focused');

await t
.pressKey('tab tab tab tab')
.pressKey('tab tab')
.expect(dataFirstField.focused)
.ok('first field in data area is focused');
}).before(async () => createWidget('dxPivotGrid', createConfig()));
30 changes: 29 additions & 1 deletion packages/devextreme/js/__internal/grids/pivot_grid/field_chooser/m_field_chooser_base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@ export class FieldChooserBase extends mixinWidget {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected _setAriaSortAttribute(_column, _ariaSortState, _$rootElement) { }

protected _applyColumnState(options) {
const $element = super._applyColumnState(options);

if (options.name === 'headerFilter' && $element) {
$element.removeAttr('tabindex');
}

return $element;
}
Comment on lines +130 to +138

@sjbur sjbur Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether we should do this. Suggestions?

@Tucchhaa Tucchhaa Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot has indicated a valid problem, but I don't think that we need to fix it as it suggested.

The problem is that headerFilterMixin._applyColumnState adds several aria-attrs: label, haspopup, role. However we don't need them and we have to remove all of them from header filter icon.

Using headerFilterMixin._applyColumnState to add these attr, so that later they would be removed in the overriden _applyColumnState seems like bad design for me.

I suggest to remove usage of headerFilterMixin, and replace this _applyColumnState with a new function that would render headerFilter icon.


_init() {
super._init();
this._headerFilterView = new HeaderFilterView(this);
Expand Down Expand Up @@ -358,14 +368,32 @@ export class FieldChooserBase extends mixinWidget {
const targetElement = element ?? this.$element();

const handler = (e) => {
const field: any = $(e.currentTarget).data('field');

if (!field) {
return;
}

const isAltArrowDown = e.type === 'keydown' && e.altKey && e.key === 'ArrowDown';

if (isAltArrowDown) {

@Tucchhaa Tucchhaa Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now we have 2 places for opening header filter:

  1. here
  2. the old place: https://github.com/DevExpress/DevExtreme/pull/33994/changes#diff-12a89dcfa5ebbc8ea7357eaf5c97ea7d1eea821081fd156023cd736aa14d4a72R401

I suggest to unify them, remove the old logic and add to this condition a check for header filter icon click

const mainGroupField = getMainGroupField(this._dataSource, field);

@Tucchhaa Tucchhaa Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why we need mainGroupField? Why not just use field?

@Tucchhaa Tucchhaa Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I see, because there can grouped fields, in which only the first group item has header filter.

I see that the condition is similar to the condition for rendering header filter icon:
https://github.com/DevExpress/DevExtreme/blob/26_1/packages/devextreme/js/__internal/grids/pivot_grid/field_chooser/m_field_chooser_base.ts#L221.

Maybe create a helper function, which can be reused?

isFieldHasHeaderFilter = (field) => {
   const mainGroupField = // ...
   const isFirstInGroup = (field.groupIndex === 0 || !isDefined(field.groupIndex);
   return mainGroupField .allowFiltering && isFirstInGroup && field.area !== 'data';
}


if (mainGroupField.allowFiltering && field.area !== 'data' && !field.groupIndex) {
e.preventDefault();
this.handleHeaderFilterIconClick(e, field);
}

return;
}

const shouldHandle = e.type === clickEventName
|| (e.type === 'keydown' && (e.key === 'Enter' || e.key === ' '));

if (!shouldHandle) {
return;
}

const field: any = $(e.currentTarget).data('field');
const isHeaderFilter = $(e.target).hasClass(CLASSES.headerFilter);

if (isHeaderFilter) {
Expand Down
Loading