[DataGrid] Fix multiple focus behaviors #1421

dtassone · 2021-04-15T17:12:41Z

fix [DataGrid] Broken cell focus on edit with external button #1416
Help with [DataGrid] The checkbox shouldn't be tabbable #1430
fix [DataGrid] Filter UI loses focus while searching / typing #1493
fix wrong initial tab index. it should be on the first header cell, not the first body cell.
fix [DataGrid] Focus lags behind on double click #1420

Preview: https://deploy-preview-1421--material-ui-x.netlify.app/components/data-grid/editing/#edit-using-external-button

oliviertassinari

This breaks the roving index, cc @DanailH.

I think that we absolutely need tests for the tab and focus behavior, it's brittle and easy to break.

dtassone · 2021-04-16T09:48:11Z

This breaks the roving index, cc @DanailH.

I think that we absolutely need tests for the tab and focus behavior, it's brittle and easy to break.

I can do tests but why is the roving index broken?

oliviertassinari · 2021-04-16T10:24:06Z

I can do tests but why is the roving index broken?

@dtassone One example of regression:

https://deploy-preview-1421--material-ui-x.netlify.app/components/data-grid/#mit-version

packages/grid/_modules_/grid/components/cell/GridCell.tsx

DanailH · 2021-04-16T12:53:05Z

packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderItem.tsx

@@ -149,6 +153,10 @@ export const GridColumnHeaderItem = ({
    }
  });

+  const isFirstTabbable =


I'm not sure that the first column header should be tabbable. I was left with the impression that the first cell should be tabbable.

That's the thing, IMO, the first tabbable control of a sortable grid should be the first sortable column header
https://www.w3.org/TR/wai-aria-practices/examples/grid/dataGrids.html

So in our case, I put the first column so we could argue that if none of the columns have an action then it should be the first cell of the first row

Actually, if we use that logic then the first tabbable cell should be either the column header that is sortable or the one that has a column menu as it is also a interaction.

dtassone · 2021-04-16T17:03:29Z

split cell focus state and tabindex state
so when tab leave grid we can focus back on the previously focused element

oliviertassinari · 2021-04-19T00:21:18Z

fix #1420 (not sure about 2.)

I meant that the rating is a radio group, that should handle arrow keys.

DanailH · 2021-04-19T09:32:59Z

This breaks the roving index, cc @DanailH.

I think that we absolutely need tests for the tab and focus behavior, it's brittle and easy to break.

It's on my radar. I'll work on it today.

DanailH · 2021-04-20T08:32:27Z

This breaks the roving index, cc @DanailH.
I think that we absolutely need tests for the tab and focus behavior, it's brittle and easy to break.

It's on my radar. I'll work on it today.

Here is the PR with the e2e setup #1443

oliviertassinari

This is not enough to close [DataGrid] Focus lags behind on double click #1420. Only point 3 is fixed. I would recommend we focus on [DataGrid] Broken cell focus on edit with external button #1416 only.
There is still a regression in the tab index behavior. See how there are two tabbable cells on the page
We need a test case for [DataGrid] Broken cell focus on edit with external button #1416

oliviertassinari · 2021-04-20T13:30:50Z

packages/grid/_modules_/grid/hooks/features/core/gridState.ts

+  keyboard: { isMultipleKeyPressed: false },
+  focus: { cell: null, columnHeader: null },
+  tabIndex: { cell: null, columnHeader: null },


This looks better. We should soon be able to kill keyboard with #1437 from @m4theushw.
I would also advocate to kill focus but it will be for another discussion.

…cusIssue

dtassone · 2021-04-21T17:20:14Z

packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx

+    const inputs = document.querySelectorAll('input');
+    expect(inputs.length).to.equal(2);
+    const nikeInput = getCell(0, 0).querySelector('input');
+    fireEvent.click(nikeInput);


@oliviertassinari any idea why this test does not work?

Could we simplify the test case (remove most of the code)? It doesn't seem that we need all this to create a failing test case.

As for why the test case doesn't work. I recall that your first commit was to add a blur listener to fix the issue, so if you don't handle the activeElement, I don't see how the test could work, e.g. calling .focus(). We are inside a browser, fire event click doesn't trigger all the intermediary event (mouse down/mouse up/focus) that a real browse have. You have to simulate them.

DanailH · 2021-04-22T09:13:51Z

Ok I think I found a bug but I'm struggling to make i video of it I'll try and list the steps:

Tab until you focus the first grid cell
Press the right arrow key until you reach the last cell in the row
Press the right arrow key one more time
Tab again to move out of the grid
Shift + Tab to move back into the grid. It doesn't select the grid bug skips it and selects the tabbable element before the grid.

@dtassone if you want to ping me and I will show you the problem if you cant reproduce it.

oliviertassinari

The fix seems to work and I can't spot a regression 👍 . I'm adding test cases for the one we found #1459.

oliviertassinari · 2021-04-22T23:59:20Z

packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx

+    const inputs = document.querySelectorAll('input');
+    expect(inputs.length).to.equal(2);
+    const nikeInput = getCell(0, 0).querySelector('input');
+    fireEvent.click(nikeInput);


Could we simplify the test case (remove most of the code)? It doesn't seem that we need all this to create a failing test case.

oliviertassinari · 2021-04-23T00:03:21Z

packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx

+    const inputs = document.querySelectorAll('input');
+    expect(inputs.length).to.equal(2);
+    const nikeInput = getCell(0, 0).querySelector('input');
+    fireEvent.click(nikeInput);


As for why the test case doesn't work. I recall that your first commit was to add a blur listener to fix the issue, so if you don't handle the activeElement, I don't see how the test could work, e.g. calling .focus(). We are inside a browser, fire event click doesn't trigger all the intermediary event (mouse down/mouse up/focus) that a real browse have. You have to simulate them.

dtassone · 2021-04-23T08:51:51Z

I thought it would fire focus on click with fireevent
https://testing-library.com/docs/guide-events/

If I simulate the event then the test doesn't fail without the fix

oliviertassinari · 2021-04-24T16:07:51Z

@dtassone you need to rebase, as you are changing the behavior of existing tests that are on HEAD but not on your baseline commit of this PR
I have updated the PR's description to be more accurate on the issues that are fixed
Could we add test cases for each individual bug we bundled together in this PR?

…cusIssue

packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

oliviertassinari

I didn't look closely in the logic, @m4theushw interested in having a closer look?

I didn't see a test case for the change of focused element for the checkboxes @dtassone what do you think about adding a regulae jsdom/Karma test to cover the new behavior?

oliviertassinari · 2021-04-28T16:18:45Z

I have updated the title so developers can understand the problem solved by this PR once it's displayed in the changelog. Also notice the capitalization so it behaves like a full sentence.

refacoring on focus cell and cols

e6979df

oliviertassinari requested changes Apr 15, 2021

View reviewed changes

DanailH reviewed Apr 16, 2021

View reviewed changes

packages/grid/_modules_/grid/components/cell/GridCell.tsx Show resolved Hide resolved

DanailH reviewed Apr 16, 2021

View reviewed changes

dtassone added 2 commits April 19, 2021 17:24

refactor focus

62d5db9

cleanup

021080c

dtassone requested review from oliviertassinari and DanailH April 20, 2021 11:17

oliviertassinari requested changes Apr 20, 2021

View reviewed changes

dtassone added 2 commits April 21, 2021 16:34

Merge branch 'master' of github.com:mui-org/material-ui-x into cellFo…

a2a9f75

…cusIssue

add test for focus

9779067

dtassone commented Apr 21, 2021

View reviewed changes

dtassone added 4 commits April 22, 2021 16:07

fix double tabbable and roving

09c285b

fix test, cleanup

fae10b2

refactor input keydown handling

5dbc0ec

split checkbox renderer file

c8aa6e7

dtassone requested a review from oliviertassinari April 22, 2021 16:57

oliviertassinari reviewed Apr 23, 2021

View reviewed changes

oliviertassinari mentioned this pull request Apr 23, 2021

[test] Improve test coverage of roving tabindex #1459

Merged

dtassone added 2 commits April 26, 2021 17:22

Merge branch 'master' of github.com:mui-org/material-ui-x into cellFo…

afeb0bc

…cusIssue

fix header focus selector

dcd52a1

dtassone added 2 commits April 26, 2021 18:11

prettier

9d44f78

fix e2e tests

aad29c6

dtassone mentioned this pull request Apr 26, 2021

[DataGrid] Filter UI loses focus while searching / typing #1493

Closed

2 tasks

oliviertassinari mentioned this pull request Apr 27, 2021

[test] Add test case for .focus stealing #1509

Closed

merge test from 1509

9d58c14

oliviertassinari reviewed Apr 28, 2021

View reviewed changes

packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx Show resolved Hide resolved

dtassone and others added 3 commits April 28, 2021 12:48

Update packages/grid/x-grid/src/tests/editRows.XGrid.test.tsx

43406a8

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Merge branch 'master' into cellFocusIssue

a718d99

lint

678b549

dtassone requested a review from oliviertassinari April 28, 2021 15:14

oliviertassinari approved these changes Apr 28, 2021

View reviewed changes

dtassone merged commit fda80d8 into mui:master Apr 28, 2021

oliviertassinari changed the title ~~[DataGrid] refactoring on focus cell and cols~~ [DataGrid] Fix multiple focus behaviors Apr 28, 2021

oliviertassinari mentioned this pull request May 2, 2021

[DataGrid] core v4 and dialog creates an infinite loop #1464

Closed

2 tasks

m4theushw mentioned this pull request May 11, 2021

[DataGrid] Clicking on the filter icon in header title is not opening the filter panel #1618

Closed

ZeeshanTamboli mentioned this pull request Jun 9, 2021

[DataGrid] Select All feature does not work on column filtering #1141

Closed

2 tasks

m4theushw mentioned this pull request Aug 3, 2021

[DataGrid] Drop autoFocus #2239

Merged

zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Fix multiple focus behaviors #1421

[DataGrid] Fix multiple focus behaviors #1421

dtassone commented Apr 15, 2021 •

edited by oliviertassinari

Loading

oliviertassinari left a comment •

edited

Loading

dtassone commented Apr 16, 2021

oliviertassinari commented Apr 16, 2021 •

edited

Loading

DanailH Apr 16, 2021 •

edited

Loading

dtassone Apr 16, 2021

DanailH Apr 19, 2021

dtassone commented Apr 16, 2021

oliviertassinari commented Apr 19, 2021 •

edited

Loading

DanailH commented Apr 19, 2021

DanailH commented Apr 20, 2021

oliviertassinari left a comment •

edited

Loading

oliviertassinari Apr 20, 2021

dtassone Apr 21, 2021

oliviertassinari Apr 22, 2021

oliviertassinari Apr 23, 2021

DanailH commented Apr 22, 2021

oliviertassinari left a comment •

edited

Loading

oliviertassinari Apr 22, 2021

oliviertassinari Apr 23, 2021

dtassone commented Apr 23, 2021

oliviertassinari commented Apr 24, 2021 •

edited

Loading

oliviertassinari left a comment

oliviertassinari commented Apr 28, 2021

[DataGrid] Fix multiple focus behaviors #1421

[DataGrid] Fix multiple focus behaviors #1421

Conversation

dtassone commented Apr 15, 2021 • edited by oliviertassinari Loading

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

dtassone commented Apr 16, 2021

oliviertassinari commented Apr 16, 2021 • edited Loading

DanailH Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtassone commented Apr 16, 2021

oliviertassinari commented Apr 19, 2021 • edited Loading

DanailH commented Apr 19, 2021

DanailH commented Apr 20, 2021

oliviertassinari left a comment • 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

Choose a reason for hiding this comment

DanailH commented Apr 22, 2021

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtassone commented Apr 23, 2021

oliviertassinari commented Apr 24, 2021 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari commented Apr 28, 2021

dtassone commented Apr 15, 2021 •

edited by oliviertassinari

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Apr 16, 2021 •

edited

Loading

DanailH Apr 16, 2021 •

edited

Loading

oliviertassinari commented Apr 19, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Apr 24, 2021 •

edited

Loading