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.

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

[DataGridPro] Fix custom header filter keyboard control when triggered via space key #14067

Closed
wants to merge 6 commits into from
Closed

[DataGridPro] Fix custom header filter keyboard control when triggered via space key #14067

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b3e4ae
[DataGridPro] Fix custom header filter keyboard control when triggere…
KenanYusuf Aug 1, 2024
d791ac9
[DataGridPro] Fix not being able to type in custom header filters
KenanYusuf Aug 1, 2024
843ec91
Merge branch 'mui:master' into fix-custom-header-filter-keyboard-control
KenanYusuf Aug 1, 2024
28a2fe1
run prettier
KenanYusuf Aug 1, 2024
7271531
Merge branch 'mui:master' into fix-custom-header-filter-keyboard-control
KenanYusuf Aug 1, 2024
15dd85e
revert change start edit mode change
KenanYusuf Aug 1, 2024
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
5 changes: 3 additions & 2 deletions packages/x-data-grid-pro/src/components/headerFiltering/GridHeaderFilterCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ const GridHeaderFilterCell = React.forwardRef<HTMLDivElement, GridHeaderFilterCe

const onKeyDown = React.useCallback(
(event: React.KeyboardEvent) => {
if (isMenuOpen || isNavigationKey(event.key) || isFilterReadOnly) {
if (isMenuOpen || (event.key !== ' ' && isNavigationKey(event.key)) || isFilterReadOnly) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Space is technically a navigation key, but this is an exception as it can also be used to trigger inputs

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I chose not to do this for consistency as it's also not with the normal Grid cells. It was the case for the normal Grid cells too, but we removed it while fixing a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok that's interesting. Here's what I am confused about...

In both of the images below, the cells have been navigated to via keyboard, but the controls within the cells look like they are in different states. The text field looks blurred, and the select looks focused.

image

image

Should the select field also be in a blurred state to avoid confusion here?

Copy link
Member

@MBilalShafi MBilalShafi Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that the interaction with the singleSelect column type is not great via keyboard, especially with the customizations. (Select / Autocomplete etc)

We could also rethink how the keyboard navigation works with header filters. We have already discussed this in detail during the feature implementation (point no 5). I have a feeling we have room for improvement here. 🤔
Probably we could focus on the input field by default when the cell is focused, but then how the keyboard navigation will work smoothly is something I'm unsure about.

What do you think?

return;
}
switch (event.key) {
Expand Down Expand Up @@ -259,6 +259,7 @@ const GridHeaderFilterCell = React.forwardRef<HTMLDivElement, GridHeaderFilterCe
if (inputRef.current?.contains?.(event.target as HTMLElement)) {
inputRef.current.focus();
}
apiRef.current.startHeaderFilterEditMode(colDef.field);
Copy link
Member Author

Choose a reason for hiding this comment

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

@MBilalShafi unsure if there will be any side effects to this change, but it made sense to me in theory that when a custom filter is clicked it should be in edit mode

Copy link
Member

@MBilalShafi MBilalShafi Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it will change the current behavior of the header filters.

Each header filter cell has two states:

  1. Focus state: The cell is focused but is not in edit mode. The user can use arrow keys to navigate to other cells.
  2. Edit state: The cell is focused and is in the edit state. The user can use arrow keys to edit/modify the text input.

How to shift from Focus state to Edit state?

  1. Start typing something.
  2. Press enter

How to shift from Edit state to Focus state?
While in Edit state, press enter.

If we make it go to the edit mode on click:

  1. Keyboard navigation will not work as expected (until an Enter is pressed) which might be annoying.
  2. Should it behave the same when navigating using arrow keys? IMO it'll be a bad UX for the users who use keyboard navigation.
  3. Instead of clicking on the cell, the user can directly click on the input to activate the edit mode right away.

Coming to the problem of not activating the Autocomplete field on Start typing, I think that's because the inputRef is not being passed on to the custom component being used. Since the Data Grid internally uses the inputRef to focus the input field when required.
I am not sure if it's the best of the solutions, but it could be partially solved by passing the inputRef to the input component, I tried to do this on top of the original example: https://codesandbox.io/p/sandbox/suspicious-feynman-y3p535

The proper fix would be done with #9243 I guess that would handle selecting multiple values internally.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if it's the best of the solutions, but it could be partially solved by passing the inputRef to the input component, I tried to do this on top of the original example: https://codesandbox.io/p/sandbox/suspicious-feynman-y3p535

This makes sense, and it fixes the user's example. I will go back to them with this fix and revert the change here.

KenanYusuf marked this conversation as resolved.
Show resolved Hide resolved
apiRef.current.setColumnHeaderFilterFocus(colDef.field, event);
}
},
Expand Down Expand Up @@ -319,7 +320,7 @@ const GridHeaderFilterCell = React.forwardRef<HTMLDivElement, GridHeaderFilterCe
}}
role="columnheader"
aria-colindex={colIndex + 1}
aria-label={headerFilterComponent == null ? (colDef.headerName ?? colDef.field) : undefined}
aria-label={headerFilterComponent == null ? colDef.headerName ?? colDef.field : undefined}
{...other}
{...mouseEventsHandlers}
>
Expand Down
Loading