fix: update filtering and sorting when grid and columns are attached / detached (#2122)

Fixes: #1969
Warranty: Fixes exception related to sorting when recreating columns 

Co-authored-by: Tulio Garcia <[email protected]>
Co-authored-by: Sascha Ißbrücker <[email protected]>

Author: 3 people

src/vaadin-grid-dynamic-columns-mixin.js

@@ -87,11 +87,14 @@ export const DynamicColumnsMixin = (superClass) =>
         if (rowDetailsTemplate && this._rowDetailsTemplate !== rowDetailsTemplate) {
           this._rowDetailsTemplate = rowDetailsTemplate;
         }
-
-        if (
-          info.addedNodes.filter(this._isColumnElement).length > 0 ||
-          info.removedNodes.filter(this._isColumnElement).length > 0
-        ) {
+        const hasColumnElements = (nodeCollection) => nodeCollection.filter(this._isColumnElement).length > 0;
+        if (hasColumnElements(info.addedNodes) || hasColumnElements(info.removedNodes)) {
+          const allRemovedCells = info.removedNodes.flatMap((c) => c._allCells);
+          const filterNotConnected = (element) =>
+            allRemovedCells.filter((cell) => cell._content.contains(element)).length;
+
+          this.__removeSorters(this._sorters.filter(filterNotConnected));
+          this.__removeFilters(this._filters.filter(filterNotConnected));
           this._updateColumnTree();
         }
 

src/vaadin-grid-filter-mixin.js

@@ -29,13 +29,34 @@ export const FilterMixin = (superClass) =>
 
     /** @private */
     _filterChanged(e) {
-      if (this._filters.indexOf(e.target) === -1) {
-        this._filters.push(e.target);
+      e.stopPropagation();
+
+      this.__addFilter(e.target);
+      this.__applyFilters();
+    }
+
+    /** @private */
+    __removeFilters(filtersToRemove) {
+      if (filtersToRemove.length == 0) {
+        return;
       }
 
-      e.stopPropagation();
+      this._filters = this._filters.filter((filter) => filtersToRemove.indexOf(filter) < 0);
+      this.__applyFilters();
+    }
 
-      if (this.dataProvider) {
+    /** @private */
+    __addFilter(filter) {
+      const filterIndex = this._filters.indexOf(filter);
+
+      if (filterIndex === -1) {
+        this._filters.push(filter);
+      }
+    }
+
+    /** @private */
+    __applyFilters() {
+      if (this.dataProvider && this.isAttached) {
         this.clearCache();
       }
     }

src/vaadin-grid-sort-mixin.js

@@ -51,31 +51,61 @@ export const SortMixin = (superClass) =>
     /** @private */
     _onSorterChanged(e) {
       const sorter = e.target;
+      e.stopPropagation();
+      this.__updateSorter(sorter);
+      this.__applySorters();
+    }
+
+    /** @private */
+    __removeSorters(sortersToRemove) {
+      if (sortersToRemove.length == 0) {
+        return;
+      }
+
+      this._sorters = this._sorters.filter((sorter) => sortersToRemove.indexOf(sorter) < 0);
+      if (this.multiSort) {
+        this.__updateSortOrders();
+      }
+      this.__applySorters();
+    }
+
+    /** @private */
+    __updateSortOrders() {
+      this._sorters.forEach((sorter, index) => (sorter._order = this._sorters.length > 1 ? index : null), this);
+    }
+
+    /** @private */
+    __updateSorter(sorter) {
+      if (!sorter.direction && this._sorters.indexOf(sorter) === -1) {
+        return;
+      }
 
-      this._removeArrayItem(this._sorters, sorter);
       sorter._order = null;
 
       if (this.multiSort) {
+        this._removeArrayItem(this._sorters, sorter);
         if (sorter.direction) {
           this._sorters.unshift(sorter);
         }
-
-        this._sorters.forEach((sorter, index) => (sorter._order = this._sorters.length > 1 ? index : null), this);
+        this.__updateSortOrders();
       } else {
         if (sorter.direction) {
-          this._sorters.forEach((sorter) => {
+          const otherSorters = this._sorters.filter((s) => s != sorter);
+          this._sorters = [sorter];
+          otherSorters.forEach((sorter) => {
             sorter._order = null;
             sorter.direction = null;
           });
-          this._sorters = [sorter];
         }
       }
+    }
 
-      e.stopPropagation();
-
+    /** @private */
+    __applySorters() {
       if (
         this.dataProvider &&
-        // No need to clear cache if sorters didn't change
+        // No need to clear cache if sorters didn't change and grid is attached
+        this.isAttached &&
         JSON.stringify(this._previousSorters) !== JSON.stringify(this._mapSorters())
       ) {
         this.clearCache();

src/vaadin-grid-sorter.js

@@ -145,13 +145,13 @@ class GridSorterElement extends ThemableMixin(DirMixin(PolymerElement)) {
       /** @private */
       _isConnected: {
         type: Boolean,
-        value: false
+        observer: '__isConnectedChanged'
       }
     };
   }
 
   static get observers() {
-    return ['_pathOrDirectionChanged(path, direction, _isConnected)'];
+    return ['_pathOrDirectionChanged(path, direction)'];
   }
 
   /** @protected */
@@ -173,14 +173,26 @@ class GridSorterElement extends ThemableMixin(DirMixin(PolymerElement)) {
   }
 
   /** @private */
-  _pathOrDirectionChanged(path, direction, isConnected) {
-    if (path === undefined || direction === undefined || isConnected === undefined) {
+  _pathOrDirectionChanged() {
+    this.__dispatchSorterChangedEvenIfPossible();
+  }
+
+  /** @private */
+  __isConnectedChanged(newValue, oldValue) {
+    if (oldValue === false) {
       return;
     }
 
-    if (isConnected) {
-      this.dispatchEvent(new CustomEvent('sorter-changed', { bubbles: true, composed: true }));
+    this.__dispatchSorterChangedEvenIfPossible();
+  }
+
+  /** @private */
+  __dispatchSorterChangedEvenIfPossible() {
+    if (this.path === undefined || this.direction === undefined || !this._isConnected) {
+      return;
     }
+
+    this.dispatchEvent(new CustomEvent('sorter-changed', { bubbles: true, composed: true }));
   }
 
   /** @private */

test/filtering.test.js

@@ -3,7 +3,14 @@ import sinon from 'sinon';
 import { fixtureSync } from '@open-wc/testing-helpers';
 import { PolymerElement, html } from '@polymer/polymer/polymer-element.js';
 import { flush } from '@polymer/polymer/lib/utils/flush.js';
-import { flushGrid, getBodyCellContent, getHeaderCellContent, listenOnce, scrollToEnd } from './helpers.js';
+import {
+  flushGrid,
+  getBodyCellContent,
+  getHeaderCellContent,
+  listenOnce,
+  scrollToEnd,
+  getVisibleItems
+} from './helpers.js';
 import '../vaadin-grid.js';
 import '../vaadin-grid-filter.js';
 import '../vaadin-grid-filter-column.js';
@@ -67,7 +74,7 @@ describe('filter', () => {
 });
 
 describe('filtering', () => {
-  let grid;
+  let grid, filter;
 
   beforeEach(() => {
     grid = fixtureSync(`
@@ -95,6 +102,7 @@ describe('filtering', () => {
     if (grid._observer.flush) {
       grid._observer.flush();
     }
+    filter = grid._filters[0];
   });
 
   it('should have filters', () => {
@@ -107,6 +115,21 @@ describe('filtering', () => {
     grid._filters.forEach((filter) => expect(filter.$.filter.clientHeight).to.be.greaterThan(0));
   });
 
+  it('should not keep references to filters when column is removed', () => {
+    grid.removeChild(grid.firstElementChild);
+    flushGrid(grid);
+    expect(grid._filters).to.not.contain(filter);
+  });
+
+  it('should keep references to filters for columns that are not removed', () => {
+    expect(grid._filters.length).to.eql(2);
+    expect(grid._filters[1].path).to.eql('last');
+    grid.removeChild(grid.firstElementChild.nextElementSibling);
+    flushGrid(grid);
+    expect(grid._filters.length).to.eql(1);
+    expect(grid._filters[0].path).to.eql('first');
+  });
+
   it('should pass filters to dataProvider', (done) => {
     grid.size = 10;
 
@@ -190,7 +213,7 @@ describe('filtering', () => {
 });
 
 describe('array data provider', () => {
-  let grid;
+  let grid, filterFirst, filterSecond;
 
   beforeEach(() => {
     grid = fixtureSync(`
@@ -216,8 +239,11 @@ describe('array data provider', () => {
     flushGrid(grid);
 
     flushFilters(grid);
-    grid._filters[0].value = '';
-    grid._filters[1].value = '';
+    filterFirst = grid._filters[0];
+    filterSecond = grid._filters[1];
+
+    filterFirst.value = '';
+    filterSecond.value = '';
     flushFilters(grid);
     grid.items = [
       {
@@ -252,6 +278,32 @@ describe('array data provider', () => {
     expect(getBodyCellContent(grid, 0, 0).innerText).to.equal('bar');
   });
 
+  it('should update filtering when column is removed', () => {
+    filterFirst.value = 'bar';
+    flushFilters(grid);
+
+    grid.removeChild(grid.firstElementChild);
+    flushGrid(grid);
+
+    expect(getVisibleItems(grid).length).to.equal(3);
+  });
+
+  it('should not filter items before grid is re-attached', () => {
+    filterFirst.value = 'bar';
+    flushFilters(grid);
+
+    const parentNode = grid.parentNode;
+    parentNode.removeChild(grid);
+    grid.removeChild(grid.firstElementChild);
+    flushGrid(grid);
+
+    expect(Object.keys(grid._cache.items).length).to.equal(1);
+
+    parentNode.appendChild(grid);
+
+    expect(Object.keys(grid._cache.items).length).to.equal(3);
+  });
+
   it('should sort filtered items', () => {
     grid._filters[1].value = 'r';
     grid.querySelector('vaadin-grid-sorter').direction = 'asc';

test/helpers.js

@@ -113,7 +113,7 @@ export const isVisible = (el) => {
   );
 };
 
-const getVisibleItems = (grid) => {
+export const getVisibleItems = (grid) => {
   flushGrid(grid);
   const rows = grid.$.items.children;
   const visibleRows = [];

test/sorting.test.js

@@ -76,6 +76,89 @@ describe('sorting', () => {
     });
   });
 
+  describe('DOM operations', () => {
+    let grid, sorterFirst, sorterSecond, sorterThird, columnFirst, columnThird;
+
+    beforeEach(async () => {
+      grid = fixtureSync(`
+        <vaadin-grid style="width: 400px; height: 200px;" multi-sort>
+          <vaadin-grid-sort-column path="first" direction="desc"></vaadin-grid-sort-column>
+          <vaadin-grid-sort-column path="second" direction="asc"></vaadin-grid-sort-column>
+          <vaadin-grid-sort-column path="third" direction="desc"></vaadin-grid-sort-column>
+        </vaadin-grid>
+      `);
+      await nextFrame();
+
+      // TODO: find better way to select
+      columnFirst = grid.querySelectorAll('vaadin-grid-sort-column')[0];
+      columnThird = grid.querySelectorAll('vaadin-grid-sort-column')[2];
+
+      sorterFirst = getHeaderCellContent(grid, 0, 0).querySelector('vaadin-grid-sorter');
+      sorterSecond = getHeaderCellContent(grid, 0, 1).querySelector('vaadin-grid-sorter');
+      sorterThird = getHeaderCellContent(grid, 0, 2).querySelector('vaadin-grid-sorter');
+
+      grid.items = [
+        { first: '1', second: '2', third: '3' },
+        { first: '2', second: '3', third: '1' },
+        { first: '3', second: '1', third: '2' }
+      ];
+
+      flushGrid(grid);
+    });
+
+    it('should preserve sort order for sorters when grid is re-attached', () => {
+      click(sorterSecond);
+      const parentNode = grid.parentNode;
+      parentNode.removeChild(grid);
+      parentNode.appendChild(grid);
+
+      expect(sorterFirst._order).to.equal(2);
+      expect(sorterSecond._order).to.equal(0);
+      expect(sorterThird._order).to.equal(1);
+    });
+
+    it('should not keep references to sorters when column is removed', () => {
+      grid.removeChild(columnFirst);
+      flushGrid(grid);
+      expect(grid._sorters).to.not.contain(sorterFirst);
+    });
+
+    it('should update sorting when column is removed', () => {
+      grid.removeChild(columnThird);
+      flushGrid(grid);
+
+      expect(getBodyCellContent(grid, 0, 0).innerText).to.equal('3');
+      expect(getBodyCellContent(grid, 1, 0).innerText).to.equal('1');
+      expect(getBodyCellContent(grid, 2, 0).innerText).to.equal('2');
+
+      expect(getBodyCellContent(grid, 0, 1).innerText).to.equal('1');
+      expect(getBodyCellContent(grid, 1, 1).innerText).to.equal('2');
+      expect(getBodyCellContent(grid, 2, 1).innerText).to.equal('3');
+    });
+
+    it('should update sort order when column removed and grid is not attached', () => {
+      const parentNode = grid.parentNode;
+      parentNode.removeChild(grid);
+
+      grid.removeChild(columnThird);
+      flushGrid(grid);
+      expect(sorterFirst._order).to.equal(1);
+      expect(sorterSecond._order).to.equal(0);
+    });
+
+    it('should not sort items before grid is re-attached', () => {
+      const parentNode = grid.parentNode;
+      parentNode.removeChild(grid);
+
+      grid.removeChild(columnThird);
+      flushGrid(grid);
+      expect(getBodyCellContent(grid, 0, 1).innerText).to.equal('2');
+
+      parentNode.appendChild(grid);
+      expect(getBodyCellContent(grid, 0, 1).innerText).to.equal('1');
+    });
+  });
+
   describe('grid', () => {
     let grid, sorterFirst, sorterLast;
 
@@ -286,7 +369,7 @@ describe('sorting', () => {
         grid.dataProvider.resetHistory();
         sorterFirst.direction = 'desc';
 
-        expect(grid.dataProvider.args[1][0].sortOrders.length).to.eql(1);
+        expect(grid.dataProvider.args[0][0].sortOrders.length).to.eql(1);
       });
 
       it('should remove order from sorters', () => {