[DataGrid] Fix input element in custom header #3624

alexfauquette · 2022-01-14T14:45:35Z

The previous test was not catching the error, because it was checking that event.defaultPrented=false. But the onKeyDown is first run by the input, and then by the header. Such that the event.preventdefault() is applied after we observe it.

I tried the fireEvent.change but this does not trigger the keyDown event. Then I'm adding the library @testing-library/user-event to use userEvent.type which imitates the input writing.

Now the two tests break when we set a preventDefault at the wrong place

m4theushw

Regarding the solution, it would be simple if we could check if the event is coming from a custom rendered header. If it does, then we don't need to care about the keyboard event handling. One way is to wrap headerComponent below in a div. Then, we know clearly that the event came from the custom component.

https://github.com/mui-org/material-ui-x/blob/7cc6c3c68e139836fec65505254a311534f1e929/packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderItem.tsx#L235

packages/grid/x-data-grid/src/tests/keyboard.DataGrid.test.tsx

m4theushw · 2022-01-15T04:18:35Z

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboardNavigation.ts

-      event.preventDefault();
+      const isNativeFocusOnHeader = event.currentTarget === event.target;
+      if (isNavigationKey(event.key) && isNativeFocusOnHeader) {
+        event.preventDefault();


If the key is not a navigation key shouldn't it do nothing? If the focus is in another element of the column header cell, lets say the sort button, it won't call event.preventDefault() but it still tries to navigate to the cell below.

Click into the sort icon button then press Arrow Down

It tooks me time to understand what append

In the PR #3275 that was only refact for column header key down, I missed the following condition

if (!isGridHeaderCellRoot(event.target as HTMLElement)) { return; }

https://github.com/mui-org/material-ui-x/pull/3275/files#diff-130975bb0d3117df317939124519a996a0f43f29799b345003e063ea6c1a181aL103-L124

Before the introduction of the bug, the rule was the next one: If the focus is not on the root component, the columnHeaderKeyDown event has no effect. One exception is for that checkbox selection which has the focus set in the checkbox input.

I think this PR is a "regression" fix and I will open an issue to discuss how events from custom render should be managed.

Note that isGridHeaderCellRoot only checks if event.target is the column header. If the target is a child of the header (e.g. the sort icon) it evaluates to false. This creates a regression because if I click the sort icon and I press Arrow Down it will scroll the page instead of going to the cell below.

I think this PR is a "regression" fix and I will open an issue to discuss how events from custom render should be managed.

Does this mean that you won't be fixing the column header with a custom component? If yes, could you update the title and description so #3599 doesn't get closed once this one is merged?

Yes, isGridHeaderCellRoot is a hard condition, but that is what we used before the regression occurs.

You can test the codesandbox from the issue with different versions of DataGrid:
https://codesandbox.io/s/renderheadergrid-material-demo-forked-suwsf?file=/index.tsx

v5.2.0:

TextField works perfectly

keyboard navigation is blocked if the focus is on the sort button

v5.2.1:

TextField broken

keyboard navigation is working if the focus is on the sort button

The modification between v5.2.0 and v5.2.1 is not intentional. So I propose to correct the regression I introduced in v5.2.1 to solve the issue.

With the modification added in the commit the Arrow Down keep same behavior as v5.2.0

Why not to also return here? We only handle the navigation keys.

Suggested change

event.preventDefault();

event.preventDefault();

return;

keyboard navigation is working if the focus is on the sort button

I'm not sure about this statement. In https://deploy-preview-3624--material-ui-x.netlify.app/components/data-grid/demo/, if I click the sort icon, then press Page Down, it moves the page down.

I could put an early return as follow

if (isNavigationKey(event.key)) { event.preventDefault(); } else{ return; }

The page moves down, but not the grid. That's the behavior before the regression. Here is a comparison of key board before/after the regression and the preview of this commit.

From left to righ:

before regression: https://deploy-preview-3302--material-ui-x.netlify.app/components/data-grid/demo/

after regresion: https://deploy-preview-3275--material-ui-x.netlify.app/components/data-grid/demo/

this PR: https://deploy-preview-3624--material-ui-x.netlify.app/components/data-grid/demo/

When focus on sort

When focus on column header

I could put an early return as follow

The problem is isGridHeaderCellRoot only checking if the event was fired by the root.

Let's review what happened here:

Before 5.2.1: clicking in any child of the column header and ArrowDown would scroll the page (nobody complained but I think it's not good)

5.2.1: Unintentionally fixed the bug above

This PR reverts the fix

I see this as inserting a regression because 5.2.1 is already released. Could you take a look in #3692? I think it already fixed part of the problem here. The solution was to only call event.preventDefault for navigation keys. #3599 was kinda fixed for free by #3692. However, pressing a navigation key inside the input won't work.

Yes, I had a look at #3692 It is equivalent to add

if(isNavigationKey(event.key)) { event.preventDefault() }

That was my first idea but I stopped doing it because it does not allow you to use keyboard navigation in a custom input, event more problematic, it does not allow you to use space key. The worst I imagine is putting a select input in the header. In such a case, the keyboard navigation will be completely down, because it requires keyboard arrows.

My conclusion is that only the developer knows if the focusable element added requires or not the event.preventDefault to prevent the page to scroll. But how to adapt the interface to let them choose, I don't know

What about then wrapping the custom component in a div so we know from where the event is coming from and ignore it? I proposed this in #3624 (review). I don't see a simpler and future-proof alternative. The other way, I think, would be to add a specific attribute to each of the icons and the title to allows us to detect if the event came from them or from a custom component.

github-actions · 2022-01-17T08:24:17Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

mui-bot · 2022-01-18T14:57:13Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	229	456.9	286.8	306.7	80.357
Sort 100k rows	ms	498.6	815.4	621.6	637.04	101.863
Select 100k rows	ms	150.4	281.5	207.9	218.78	48.847
Deselect 100k rows	ms	106.7	288.9	185.2	198.58	63.325

Generated by 🚫 dangerJS against 4ccbbc7

github-actions · 2022-01-25T01:27:05Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

m4theushw · 2022-01-26T14:07:58Z

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboardNavigation.ts

-      event.preventDefault();
+      const isNativeFocusOnHeader = event.currentTarget === event.target;
+      if (isNavigationKey(event.key) && isNativeFocusOnHeader) {
+        event.preventDefault();


Why not to also return here? We only handle the navigation keys.

Suggested change

event.preventDefault();

event.preventDefault();

return;

keyboard navigation is working if the focus is on the sort button

I'm not sure about this statement. In https://deploy-preview-3624--material-ui-x.netlify.app/components/data-grid/demo/, if I click the sort icon, then press Page Down, it moves the page down.

m4theushw · 2022-01-26T14:08:27Z

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboardNavigation.ts

      if (!params.field) {
        return;
      }


To improve readability.

Suggested change

if (!params.field) {

return;

}

if (!params.field) {

return;

}

This condition is useless I added it to apply this PR comment but I miss understood it 😅 #3275 (comment)

m4theushw · 2022-01-26T14:09:29Z

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboardNavigation.ts

-      event.preventDefault();
+      if (
+        !isGridHeaderCellRoot(event.target as HTMLElement) &&
+        !(params.field === GRID_CHECKBOX_SELECTION_COL_DEF.field && isNavigationKey(event.key))


Why to also check if it's a navigation key? We only event.preventDefault on the navigation keys.

Yes, there is no reason

github-actions · 2022-01-26T20:02:52Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

siriwatknp · 2022-02-08T12:25:12Z

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

pull latest master from upstream to your branch
if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
2.1 revert the change on those markdown files you did
2.2 pull latest master from upstream (you should not see conflict)
2.3 make the changes again at docs/data/data-grid/*
run yarn docs:api
- you might see the changes in docs/pages/x/api/data-grid/* if your PR touches some of the components
- it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

github-actions · 2022-02-17T15:38:56Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

alexfauquette · 2022-02-25T12:50:32Z

To verify if the event comes from the custom title I added a class classes.columnHeaderTitleContentContainer to wrap the title without the buttons.

It's only used to make a query selector, but I let it accessible for developers

packages/grid/x-data-grid/src/internals/hooks/features/keyboard/useGridKeyboardNavigation.ts

packages/grid/x-data-grid/src/internals/components/columnHeaders/GridColumnHeaderItem.tsx

m4theushw

I noticed a bug: after clicking the sort icon, I'm stuck and the arrow keys don't work.

packages/grid/x-data-grid/src/tests/keyboard.DataGrid.test.tsx

packages/grid/x-data-grid/src/internals/hooks/features/keyboard/useGridKeyboardNavigation.ts

alexfauquette · 2022-02-28T17:30:24Z

I'm adding a test to ensure that clicking on the sort button does not lock the focus

github-actions · 2022-03-01T08:05:07Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

m4theushw · 2022-03-02T13:01:18Z

packages/grid/x-data-grid/src/tests/keyboard.DataGrid.test.tsx

+      // This does not work with navigation keys.
+      // For now, the workaround for developers is to stop the propagation
+      // But adding input is discouraged, action column or edit mode are better options
+      expect(fireEvent.keyDown(input, { key: 'a' })).to.equal(true);


I don't know if we should have this test. An input inside a cell in view mode is not encouraged because in this mode cells are supposed to be read-only. If you fire a ArrowDown event the assertion will fail correctly because any keydown event should move the focus to the next cell. However, in edit mode, the assertion (once changed to ArrowDown) is correct. I would move this test to the cell editing suite and render the input inside a cell in edit mode.

mui-x/packages/grid/x-data-grid/src/hooks/features/keyboard/useGridKeyboard.ts

Lines 60 to 63 in 62bb727

const isEditMode = cellParams.cellMode === GridCellModes.Edit;

if (isEditMode) {

return;

}

This can be done in another PR.

OK, I will open another PR for this one 👍

packages/grid/x-data-grid/src/tests/keyboard.DataGrid.test.tsx

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

alexfauquette self-assigned this Jan 14, 2022

alexfauquette requested review from flaviendelangle and m4theushw January 14, 2022 16:10

m4theushw reviewed Jan 15, 2022

View reviewed changes

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 17, 2022

alexfauquette requested a review from m4theushw January 18, 2022 15:28

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2022

alexfauquette added 2 commits January 25, 2022 16:22

fix the regression

4c3f29d

add tests

bdc8bcf

alexfauquette force-pushed the issue-3599 branch from 89139e8 to bdc8bcf Compare January 25, 2022 15:22

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2022

m4theushw reviewed Jan 26, 2022

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 26, 2022

alexfauquette added 2 commits January 27, 2022 12:55

feedback

e3d553d

Merge remote-tracking branch 'upstream/master' into issue-3599

7492d5e

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 27, 2022

oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jan 29, 2022

alexfauquette added 3 commits February 7, 2022 15:21

test to prevent focus

e1355e8

prettier

90be856

Merge remote-tracking branch 'upstream/master' into issue-3599

6fa46ae

Merge remote-tracking branch 'upstream/master' into issue-3599

1cc0e76

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 17, 2022

alexfauquette added 3 commits February 24, 2022 17:57

keep innput test with exmplaination

5bb32e1

prettier

70137b8

remove buttons from the container

a927737

m4theushw reviewed Feb 25, 2022

View reviewed changes

alexfauquette added 5 commits February 28, 2022 09:36

feedbacks

cc67c2d

docs:api

69c41ec

Merge remote-tracking branch 'upstream/master' into issue-3599

e06da3d

add the 'not' operator

dda1b40

fix style

8345824

alexfauquette force-pushed the issue-3599 branch from d6d3564 to 8345824 Compare February 28, 2022 10:51

m4theushw reviewed Feb 28, 2022

View reviewed changes

alexfauquette added 5 commits February 28, 2022 16:35

renaming

492f7ce

fix tests

701d17c

move eslint line

6ba52c1

remove isGridHeaderCellRoot

39094de

add test

95c41b8

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 1, 2022

Merge remote-tracking branch 'upstream/master' into issue-3599

9368b11

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 1, 2022

fix import

e6bde22

alexfauquette requested a review from m4theushw March 2, 2022 09:36

m4theushw approved these changes Mar 2, 2022

View reviewed changes

alexfauquette and others added 2 commits March 2, 2022 14:14

Update packages/grid/x-data-grid/src/tests/keyboard.DataGrid.test.tsx

27c80e5

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

Trigger CI

4ccbbc7

alexfauquette merged commit 91cd50d into mui:master Mar 2, 2022

alexfauquette deleted the issue-3599 branch March 2, 2022 14:14

alexfauquette mentioned this pull request Mar 2, 2022

[test] Only test custom input keyboard event in edit mode #4075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Fix input element in custom header #3624

[DataGrid] Fix input element in custom header #3624

alexfauquette commented Jan 14, 2022 •

edited

Loading

m4theushw left a comment

m4theushw Jan 15, 2022

alexfauquette Jan 18, 2022

m4theushw Jan 19, 2022 •

edited

Loading

alexfauquette Jan 19, 2022

m4theushw Jan 26, 2022

alexfauquette Jan 27, 2022

m4theushw Jan 27, 2022

alexfauquette Jan 27, 2022

m4theushw Jan 27, 2022

github-actions bot commented Jan 17, 2022

mui-bot commented Jan 18, 2022 •

edited

Loading

github-actions bot commented Jan 25, 2022

m4theushw Jan 26, 2022

m4theushw Jan 26, 2022

alexfauquette Jan 27, 2022

m4theushw Jan 26, 2022

alexfauquette Jan 27, 2022

github-actions bot commented Jan 26, 2022

siriwatknp commented Feb 8, 2022 •

edited

Loading

github-actions bot commented Feb 17, 2022

alexfauquette commented Feb 25, 2022

m4theushw left a comment •

edited

Loading

alexfauquette commented Feb 28, 2022 •

edited

Loading

github-actions bot commented Mar 1, 2022

m4theushw Mar 2, 2022 •

edited

Loading

alexfauquette Mar 2, 2022

	const isEditMode = cellParams.cellMode === GridCellModes.Edit;
	if (isEditMode) {
	return;
	}

[DataGrid] Fix input element in custom header #3624

[DataGrid] Fix input element in custom header #3624

Conversation

alexfauquette commented Jan 14, 2022 • edited Loading

m4theushw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

When focus on sort

When focus on column header

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2022

mui-bot commented Jan 18, 2022 • edited Loading

github-actions bot commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 26, 2022

siriwatknp commented Feb 8, 2022 • edited Loading

github-actions bot commented Feb 17, 2022

alexfauquette commented Feb 25, 2022

m4theushw left a comment • edited Loading

Choose a reason for hiding this comment

alexfauquette commented Feb 28, 2022 • edited Loading

github-actions bot commented Mar 1, 2022

m4theushw Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette commented Jan 14, 2022 •

edited

Loading

m4theushw Jan 19, 2022 •

edited

Loading

mui-bot commented Jan 18, 2022 •

edited

Loading

siriwatknp commented Feb 8, 2022 •

edited

Loading

m4theushw left a comment •

edited

Loading

alexfauquette commented Feb 28, 2022 •

edited

Loading

m4theushw Mar 2, 2022 •

edited

Loading