[DataGrid] `Select all` checkbox click should select only filtered rows after filter #1879

ZeeshanTamboli · 2021-06-12T10:08:12Z

Fix point 3. of #1141 (comment)
Closes #1863
Preview: https://deploy-preview-1879--material-ui-x.netlify.app/components/data-grid/

dtassone · 2021-06-14T10:10:00Z

packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts

@@ -239,4 +242,10 @@ export const useGridSelection = (apiRef: GridApiRef): void => {
    });
    forceUpdate();
  }, [apiRef, setGridState, forceUpdate, isRowSelectable]);
+
+  React.useEffect(() => {
+    if (totalSelectedRows > Object.keys(visibleRowIds).length) {


you can use visibleGridRowCountSelector
Also not sure about the logic here. What if we change rows to another set with the same length?

Also not sure about the logic here. What if we change rows to another set with the same length?

So when filtering, is it possible that the rows filtered (visible) are equal to the total rows? Also this logic only runs when the rows are selected and then the filtering is applied, so the visible rows are less after filtering. Would there be a case when the rows are of same length after filtering? I don't think so.

you can use visibleGridRowCountSelector

I need to pass visibleRowIds below to selectRows. Should I still select visibleRowCount for count to replace Object.keys(visibleRowIds).length? I think we should as the selector will be memoized.

So when filtering, is it possible that the rows filtered (visible) are equal to the total rows?

Yes, if all rows satisfy the filter then all rows will be visible.

You can't just check if there're more selected rows than visible rows. The behavior becomes inconsistent if the size of the selection is smaller than the number of visible rows.

I think you need to do the same thing we do when the rows prop is changed, but with visibleRowIds.

https://github.com/mui-org/material-ui-x/blob/a2a1f8eca0f8b188a1ed40abbcc616dc3a837d76/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts#L203-L208

@DanailH was working in #1864 to fix a bug around this part of the code too.

So when filtering, is it possible that the rows filtered (visible) are equal to the total rows?

Yes, if all rows satisfy the filter then all rows will be visible.

Okay, so then the point is that this condition won't satisfy, right? And it would not select rows by deselecting others and selecting visible ones. It would work as it works now in master.

The behavior becomes inconsistent if the size of the selection is smaller than the number of visible rows.

Again, then this effect's condition won't satisfy and selecting rows won't run.

What this effect does?

I will like to give an example:
For grid with 200 rows.
The total selected rows are all 200, after filter the visible rows become 40. The dependencies change, this effect's condition satisfies and now the select rows will deselect others and select only 40. If you add another filter, the rows are now 37, it will select only 37. This is main issue of #1141.

I think you need to do the same thing we do when the rows prop is changed, but with visibleRowIds.

https://github.com/mui-org/material-ui-x/blob/a2a1f8eca0f8b188a1ed40abbcc616dc3a837d76/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts#L203-L208

@DanailH was working in #1864 to fix a bug around this part of the code too.

Does this effect #1141? Is this fix you are suggesting for the related issue of #1141? Are we on the main issue and the related comments issues I linked in description?

Also I would suggest once to look at whats happening currently and after the fix for the issues attached in the description.

you can use visibleGridRowCountSelector

@dtassone I used this selector now.

Again, then this effect's condition won't satisfy and selecting rows won't run.

Yeah, but you might have one selected row, adds another filter where this selected row doesn't satisfy it and the previous row will still be selected. If you select all, that means having a larger selection than visible rows, it unselects those rows that are not more visible. That's the inconsistency I noticed.

Does this effect #1141?

No

Is this fix you are suggesting for the related issue of #1141?

Yes, I would keep it simple. The selection should always contain only visible rows. In the future, we can add an option to allow to accumulate selection even when rows become invisible.

Yeah, but you might have one selected row, adds another filter where this selected row doesn't satisfy it and the previous row will still be selected.

Yeah, even I see this behaviour. It is in intermediate state? Yeah but the row selected is not visible.

The selection should always contain only visible rows

Are you referring with pagination, that it should select only the current page visible rows? #605? Maybe not.

packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts

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

packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts

oliviertassinari

Are we sure about the test coverage? I mean, we add one new test that is passing on HEAD. And a second test is weird. Why does it apply the filter twice? What about the second bug #1141 shouldn't we test it too (checkbox)?

oliviertassinari · 2021-06-17T00:20:30Z

I have opened #1919 to prove my point. The test case is not testing the full scope of the issue, and we can break this down into smaller chunks: #1919 is one of them. Not that #1879 is still relevant. I went on solving different problems.

ZeeshanTamboli · 2021-06-17T13:38:12Z

@oliviertassinari

Are we sure about the test coverage? I mean, we add one new test that is passing on HEAD.

My bad. The test was wrong. The test with the below change if updated will be correct. Pretty much what you did in #1919. This fails in master HEAD. I was setting the filter in initial mounting itself rather than setting it after the render. This leans me to now follow more TDD approach. Thanks.

diff --git a/packages/grid/x-grid/src/tests/selection.XGrid.test.tsx b/packages/grid/x-grid/src/tests/selection.XGrid.test.tsx
index ac18ad6b..3bc07b97 100644
--- a/packages/grid/x-grid/src/tests/selection.XGrid.test.tsx
+++ b/packages/grid/x-grid/src/tests/selection.XGrid.test.tsx
@@ -129,20 +129,16 @@ describe('<XGrid /> - Selection', () => {
   });

   it('should select only filtered rows after filter is applied', () => {
-    render(
-      <Test
-        checkboxSelection
-        filterModel={{
-          items: [
-            {
-              columnField: 'brand',
-              operatorValue: 'contains',
-              value: 'a',
-            },
-          ],
-        }}
-      />,
-    );
+    render(<Test checkboxSelection />);
+    apiRef.current.setFilterModel({
+      items: [
+        {
+          columnField: 'brand',
+          operatorValue: 'contains',
+          value: 'a',
+        },
+      ],
+    });
     const selectAll = screen.getByRole('checkbox', {
       name: /select all rows checkbox/i,
     });

And a second test is weird. Why does it apply the filter twice?

It is testing the main issue of #1141

Current Behavior 😯
Select All feature accurately selects active rows when only 1 filter is active. If a subsequent filter is added, Select All feature breaks.

Expected Behavior 🤔
Select All feature should update total number of active rows when additional filters are added.

If a subsequent filter is added, Select All feature breaks.
This the reason I added the second filter but not same, it is a different filter constraint. But here again I will need to use apiRef.current.setFilterModel.
Does it make sense? 🤔
Actually this statement by user: Select All feature accurately selects active rows when only 1 filter is active is invalid now after April release. Select All breaks even after one filter. The issue was reported in March. So this is DataGrid related as well.

What about the second bug #1141 shouldn't we test it too (checkbox)?

Yes we should which I missed. I I think it is Select All initially after filter (intermediate state) and then again Select All to check that the intermediate state is gone and none of the rows are selected?

The test adequacy is not good. I will rewrite them and add the missed one (checkbox).

we can break this down into smaller chunks: #1919 is one of them.

Yes. All seemed related to me with Select All so I included them in one PR.

oliviertassinari · 2021-06-17T15:05:57Z

@ZeeshanTamboli Ahhhhh. Now I get why you added the second filter in the test! Be very careful with what developers write as expected vs. actually. In 50% of the time, we overrule them. What we care about is hearing the pain point.

So basically, if all the rows are selected without filters, and we add a filter, then, only the new visible rows are selected. Personally, this behavior has never crossed my mind. I didn't cover it in #1141 (comment). I would personally vote against it because it prevents the end-users to fine-tune their selection. But no strong point of view. cc @mui-org/x

It seems that we should break down this PR in smaller chunks. It's overloaded.

checkbox issue, this is a standalone bug. I have it in [DataGrid] Fix double-click issue #1919. I recommend we merge this one first
memo issue, this is a standalone bug. We have a test case for it in [DataGrid] Fix double-click issue #1919 already, and the fix in this PR (likely but I didn't test it).
your new behavior is something isolated. We continue the discussion, see if we want it or not.

dtassone · 2021-06-17T15:23:35Z

if all the rows are selected without filters, and we add a filter, then, only the new visible rows are selected.

I would vote against it in the native behavior, as ppl select a row then apply a filter to quickly find their next pick and select it. So I expect that they would want the selection to be aggregated

ZeeshanTamboli · 2021-06-18T06:02:30Z

@oliviertassinari

It seems that we should break down this PR in smaller chunks. It's overloaded.

checkbox issue, this is a standalone bug. I have it in [DataGrid] Fix double-click issue #1919. I recommend we merge this one first

Sounds good.

memo issue, this is a standalone bug. We have a test case for it in [DataGrid] Fix double-click issue #1919 already, and the fix in this PR (likely but I didn't test it).

Should I rename this PR and have only memo fix in this? Change the description? Or should we close this PR and create a new one for just memo?

your new behavior is something isolated. We continue the discussion, see if we want it or not.

Yes. That's what the useEffect was doing in this PR. We can close this or only do memo fix here after rebase of #1919. We can have discussion in issue #1141.

oliviertassinari · 2021-06-18T22:50:20Z

Sounds good.

@ZeeshanTamboli Done, it's on HEAD #1919

Should I rename this PR and have only memo fix in this? Change the description? Or should we close this PR and create a new one for just memo?

If you could rebase this PR on HEAD, change the title, and focus on this problem:

https://github.com/mui-org/material-ui-x/blob/a75eb4f70a30a998324b03fa9c60c98df7b0c8f3/packages/grid/x-grid/src/tests/selection.XGrid.test.tsx#L157-L158

(and only), it would be 👌. One issue at a time.

Yes. That's what the useEffect was doing in this PR. We can close this or only do memo fix here after rebase of [DataGrid] Fix double-click issue #1919. We can have discussion in issue [DataGrid] Select All feature does not work on column filtering #1141.

We won't move forward with this behavior, please remove it from the PR, see: #1879 (comment).

oliviertassinari · 2021-06-20T13:23:59Z

packages/grid/_modules_/grid/hooks/features/filter/useGridFilter.ts

        return {
          ...state,
          visibleRows: {
+            ...state.visibleRows,
            visibleRowsLookup,
            visibleRows: Object.entries(visibleRowsLookup)
              .filter(([, isVisible]) => isVisible)


The logic of the data grid is not consistent regarding this behavior. Sometimes, we spread extra props even if we override them all, sometimes, we don't. The change seems fair as a systematic approach. It will reduce the cost of future updates. cc @dtassone

ZeeshanTamboli added 4 commits June 9, 2021 21:54

fix select all checkbox not rendering due to React.memo

72f945a

Merge branch 'master' into issue/1141

57f3f19

fix core issue

5193f92

fix indeterminate checkbox state issue

0adfe4b

ZeeshanTamboli added component: data grid This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Jun 12, 2021

ZeeshanTamboli changed the title ~~[DataGrid] Fix select all regression on checkbox selection~~ [DataGrid] Fix select all regression on checkbox selection Jun 12, 2021

add tests for select all after filtering

1f5b93a

ZeeshanTamboli requested review from oliviertassinari, m4theushw, dtassone and DanailH June 12, 2021 11:08

ZeeshanTamboli added 2 commits June 12, 2021 23:49

remove apiRef unnecessary dependency from useEffect

57ff224

move getColumnHeaderElement inside effect

e53e774

dtassone reviewed Jun 14, 2021

View reviewed changes

ZeeshanTamboli added 2 commits June 15, 2021 12:12

use visible row count selector

50a0c2c

resolve conflicts

bc2ae4d

dtassone approved these changes Jun 15, 2021

View reviewed changes

dtassone reviewed Jun 15, 2021

View reviewed changes

packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts Outdated Show resolved Hide resolved

oliviertassinari reviewed Jun 16, 2021

View reviewed changes

packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderItem.tsx Outdated Show resolved Hide resolved

packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts Outdated Show resolved Hide resolved

apply code suggestions

fb69664

oliviertassinari requested changes Jun 16, 2021

View reviewed changes

oliviertassinari mentioned this pull request Jun 17, 2021

[DataGrid] Fix double-click issue #1919

Merged

Merge branch 'master' into issue/1141

f6de641

matandro mentioned this pull request Jun 17, 2021

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

Closed

2 tasks

ZeeshanTamboli added 4 commits June 19, 2021 10:53

remove code

c5c358a

resolve conflicts

cb6914f

make test failing

ff52c74

fix select all select only filtered rows

701fa06

ZeeshanTamboli changed the title ~~[DataGrid] Fix select all regression on checkbox selection~~ [DataGrid] Select all checkbox should select only filtered rows after filter Jun 19, 2021

ZeeshanTamboli changed the title ~~[DataGrid] Select all checkbox should select only filtered rows after filter~~ [DataGrid] Select all checkbox click should select only filtered rows after filter Jun 19, 2021

ZeeshanTamboli requested review from dtassone and oliviertassinari June 19, 2021 06:37

oliviertassinari approved these changes Jun 20, 2021

View reviewed changes

oliviertassinari mentioned this pull request Jun 20, 2021

[DataGrid] Filter and check all Checkbox selects all items #1863

Closed

2 tasks

ZeeshanTamboli merged commit 309c19c into mui:master Jun 21, 2021

ZeeshanTamboli deleted the issue/1141 branch June 21, 2021 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] `Select all` checkbox click should select only filtered rows after filter #1879

[DataGrid] `Select all` checkbox click should select only filtered rows after filter #1879

ZeeshanTamboli commented Jun 12, 2021 •

edited by oliviertassinari

Loading

dtassone Jun 14, 2021

ZeeshanTamboli Jun 14, 2021

ZeeshanTamboli Jun 14, 2021

m4theushw Jun 15, 2021 •

edited

Loading

ZeeshanTamboli Jun 15, 2021 •

edited

Loading

ZeeshanTamboli Jun 15, 2021

m4theushw Jun 15, 2021 •

edited

Loading

ZeeshanTamboli Jun 16, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Jun 17, 2021 •

edited

Loading

ZeeshanTamboli commented Jun 17, 2021 •

edited

Loading

oliviertassinari commented Jun 17, 2021 •

edited

Loading

dtassone commented Jun 17, 2021

ZeeshanTamboli commented Jun 18, 2021 •

edited

Loading

oliviertassinari commented Jun 18, 2021 •

edited

Loading

oliviertassinari Jun 20, 2021 •

edited

Loading

[DataGrid] Select all checkbox click should select only filtered rows after filter #1879

[DataGrid] Select all checkbox click should select only filtered rows after filter #1879

Conversation

ZeeshanTamboli commented Jun 12, 2021 • edited by oliviertassinari Loading

dtassone Jun 14, 2021

Choose a reason for hiding this comment

ZeeshanTamboli Jun 14, 2021

Choose a reason for hiding this comment

ZeeshanTamboli Jun 14, 2021

Choose a reason for hiding this comment

m4theushw Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli Jun 15, 2021

Choose a reason for hiding this comment

m4theushw Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Jun 17, 2021 • edited Loading

ZeeshanTamboli commented Jun 17, 2021 • edited Loading

oliviertassinari commented Jun 17, 2021 • edited Loading

dtassone commented Jun 17, 2021

ZeeshanTamboli commented Jun 18, 2021 • edited Loading

oliviertassinari commented Jun 18, 2021 • edited Loading

oliviertassinari Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

[DataGrid] `Select all` checkbox click should select only filtered rows after filter #1879

[DataGrid] `Select all` checkbox click should select only filtered rows after filter #1879

ZeeshanTamboli commented Jun 12, 2021 •

edited by oliviertassinari

Loading

m4theushw Jun 15, 2021 •

edited

Loading

ZeeshanTamboli Jun 15, 2021 •

edited

Loading

m4theushw Jun 15, 2021 •

edited

Loading

ZeeshanTamboli Jun 16, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Jun 17, 2021 •

edited

Loading

ZeeshanTamboli commented Jun 17, 2021 •

edited

Loading

oliviertassinari commented Jun 17, 2021 •

edited

Loading

ZeeshanTamboli commented Jun 18, 2021 •

edited

Loading

oliviertassinari commented Jun 18, 2021 •

edited

Loading

oliviertassinari Jun 20, 2021 •

edited

Loading