From 7c17b8d7c07c0c05a13e2779735aaad0cfee9bc1 Mon Sep 17 00:00:00 2001 From: Ed Hager Date: Fri, 27 Sep 2013 11:07:10 -0400 Subject: [PATCH] Fix #684: deselect row before removing it on store notify Includes additional unit tests. --- Selection.js | 59 +++++-- test/intern/mixins/Selection.js | 287 ++++++++++++++++++++++++++++++-- 2 files changed, 313 insertions(+), 33 deletions(-) diff --git a/Selection.js b/Selection.js index 765855660..0f993e128 100644 --- a/Selection.js +++ b/Selection.js @@ -157,7 +157,7 @@ return declare(null, { // Remove any extra handles added by Selection. if(this._selectstartHandle){ this._selectstartHandle.remove(); } if(this._unselectableHandle){ this._unselectableHandle.remove(); } - if(this._deselectionHandle){ this._deselectionHandle.remove(); } + if(this._removeDeselectSignals){ this._removeDeselectSignals(); } }, _setSelectionMode: function(mode){ @@ -335,30 +335,50 @@ return declare(null, { // or to the list/grid's removeRow method otherwise. var self = this, - store = this.store; + store = this.store, + beforeSignal, + afterSignal; + + function ifSelected(object, idToUpdate, methodName){ + // Calls a method if the row corresponding to the object is selected. + var id = idToUpdate || (object && object[self.idProperty || "id"]); + if(id != null){ + var row = self.row(id), + selection = row && self.selection[row.id]; + // Is the row currently in the selection list. + if(selection){ + self[methodName](row, null, selection); + } + } + } // Remove anything previously configured - if(this._deselectionSignal){ - this._deselectionSignal.remove(); + if(this._removeDeselectSignals){ + this._removeDeselectSignals(); } - + // Is there currently an observable store? if(store && store.notify){ - this._deselectionSignal = aspect.after(store, "notify", function(object, idToUpdate){ - var id = idToUpdate || (object && object[this.idProperty || "id"]); - if (id != null) { - var row = self.row(id), - selection = row && self.selection[row.id]; - // Is the row currently in the selection list. - if(selection){ - // Ensure consistency between DOM and state on add/put; - // prune from selection on remove - self[(object ? "" : "de") + "select"](row, null, selection); - } + beforeSignal = aspect.before(store, "notify", function(object, idToUpdate){ + if(!object){ + // Call deselect on the row if the object is being removed. This allows the + // deselect event to reference the row element while it still exists in the DOM. + ifSelected(object, idToUpdate, "deselect"); } + }); + afterSignal = aspect.after(store, "notify", function(object, idToUpdate){ + // When List updates an item, the row element is removed and a new one inserted. + // If at this point the object is still in grid.selection, then call select on the row so the + // element's CSS is updated. If the object was removed then the aspect-before has already deselected it. + ifSelected(object, idToUpdate, "select"); }, true); + + this._removeDeselectSignals = function(){ + beforeSignal.remove(); + afterSignal.remove(); + }; }else{ - this._deselectionSignal = aspect.before(this, "removeRow", function(rowElement, justCleanup){ + beforeSignal = aspect.before(this, "removeRow", function(rowElement, justCleanup){ var row; if(!justCleanup){ row = this.row(rowElement); @@ -368,6 +388,9 @@ return declare(null, { } } }); + this._removeDeselectSignals = function(){ + beforeSignal.remove(); + }; } }, @@ -384,7 +407,7 @@ return declare(null, { rows = this[event], // current event queue (actually cells for CellSelection) trigger = this._selectionTriggerEvent; - if (trigger) { + if(trigger) { // If selection was triggered by another event, we want to know its type // to report later. Grab it ahead of the timeout to avoid // "member not found" errors in IE < 9. diff --git a/test/intern/mixins/Selection.js b/test/intern/mixins/Selection.js index de7cfb554..3e9d6c813 100644 --- a/test/intern/mixins/Selection.js +++ b/test/intern/mixins/Selection.js @@ -1,18 +1,20 @@ -require([ +define([ "intern!tdd", "intern/assert", "dojo/_base/declare", "dojo/_base/array", "dojo/_base/lang", + "dojo/json", + "dojo/dom-class", "dojo/store/Memory", "dojo/store/Observable", "dgrid/Grid", "dgrid/OnDemandGrid", "dgrid/Selection", "dgrid/CellSelection", - "dgrid/extensions/Pagination", - "dojo/domReady!" -], function(test, assert, declare, arrayUtil, lang, Memory, Observable, Grid, OnDemandGrid, Selection, CellSelection, Pagination){ + "dgrid/extensions/Pagination" +], function(test, assert, declare, arrayUtil, lang, JSON, domClass, Memory, Observable, + Grid, OnDemandGrid, Selection, CellSelection, Pagination){ var mixins = { Selection: Selection, @@ -174,45 +176,300 @@ require([ document.body.appendChild(grid.domNode); grid.startup(); + function checkStyles(){ + // Checks to see if the rendered rows that are selected have the dgrid-selected style + // and no unselected rows have that style + var selection = grid.selection, + id, + rowElement, + rowObject, + isHighlighted, + shouldBeHighlighted; + + for(id in grid._rowIdToObject){ + rowElement = document.getElementById(id); + rowObject = grid._rowIdToObject[id]; + if(rowElement){ + if(name === "Selection"){ + isHighlighted = domClass.contains(rowElement, "dgrid-selected"); + }else{ + isHighlighted = domClass.contains(grid.cell(rowObject.id, "first").element, "dgrid-selected"); + } + shouldBeHighlighted = !!selection[rowObject.id]; + assert.strictEqual(isHighlighted, shouldBeHighlighted, + "Expected " + JSON.stringify(rowObject) + " to" + (shouldBeHighlighted ? "" : " not") + " be selected."); + } + } + } + function checkSelected(ids){ var selection = grid.selection; - assert.strictEqual(countProperties(selection), ids.length, - "Selection contains the expected number of items"); + var numIds = ids.length; + checkStyles(); + assert.strictEqual(countProperties(selection), numIds, + "Selection contains the expected number of items: " + numIds); assert.ok(arrayUtil.every(ids, function(id){ return id in selection; }), "Selection contains the expected items"); } - + var initSelection = [3, 4, 5, 23, 24, 25]; arrayUtil.forEach(initSelection, function(id){ grid.select(grid.row(id)); }); checkSelected(initSelection); - + grid.gotoPage(4); checkSelected(initSelection); - + grid.gotoPage(1); - store.put({ id: 4, first: "Updated First", last: "Updated Last"}); + store.put({ id: 1, first: "Updated First 1", last: "Updated Last 1"}); checkSelected(initSelection); - + assert.strictEqual(grid.cell(1, "first").element.innerHTML, "Updated First 1"); + store.put({ id: 4, first: "Updated First 4", last: "Updated Last 4"}); + checkSelected(initSelection); + assert.strictEqual(grid.cell(4, "first").element.innerHTML, "Updated First 4"); + store.put({ id: 24, first: "Updated First", last: "Updated Last"}); checkSelected(initSelection); - + store.put({ id: 1999, first: "New First", last: "New Last"}); checkSelected(initSelection); - + store.remove(2); checkSelected(initSelection); - + store.remove(3); checkSelected([4, 5, 23, 24, 25]); - + store.remove(25); checkSelected([4, 5, 23, 24]); grid.destroy(); }; + + notificationTests[name + " events + store"] = function(){ + // Create and remove selections, watch for events + var store = Observable(new Memory({ + data: _createTestData() + })), + grid = new (declare([OnDemandGrid, SelectionMixin]))({ + columns: getColumns(), + store: store, + sort: "id" + }); + + document.body.appendChild(grid.domNode); + grid.startup(); + + var selectEventFired; + var deselectEventFired; + grid.on('dgrid-select', function(){ + selectEventFired++; + }); + grid.on('dgrid-deselect', function(){ + deselectEventFired++; + }); + + function testEvents() { + selectEventFired = 0; + deselectEventFired = 0; + grid.select(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 1, "Select event fired once: " + selectEventFired); + assert.strictEqual(deselectEventFired, 0, "Deselect event not fired: " + deselectEventFired); + + grid.deselect(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 1, "Select event fired once: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + grid.select(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 2, "Select event fired twice: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + grid.select(4); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 3, "Select event fired three times: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + grid.deselect(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 3, "Select event fired three times: " + selectEventFired); + assert.strictEqual(deselectEventFired, 2, "Deselect event fired twice: " + deselectEventFired); + + grid.deselect(4); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 3, "Select event fired three times: " + selectEventFired); + assert.strictEqual(deselectEventFired, 3, "Deselect event fired three times: " + deselectEventFired); + } + + // Run the event tests + testEvents(); + // Change the store + store = Observable(new Memory({ + data: _createTestData() + })); + grid.set("store", store); + // Run the tests again + testEvents(); + + grid.destroy(); + }; + + notificationTests[name + " events + no store"] = function(){ + // Create and remove selections, watch for events + var grid = new (declare([Grid, SelectionMixin]))({ + columns: getColumns() + }), + selectEventFired = 0, + deselectEventFired = 0; + + document.body.appendChild(grid.domNode); + grid.startup(); + grid.renderArray(_createTestData()); + + grid.on('dgrid-select', function(){ + selectEventFired++; + }); + grid.on('dgrid-deselect', function(){ + deselectEventFired++; + }); + + // These tests call _fireSelectionEvent directly to avoid the + // timeout that is usually performed to batch concurrent selections + + grid.select(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 1, "Select event fired once: " + selectEventFired); + assert.strictEqual(deselectEventFired, 0, "Deselect event not fired: " + deselectEventFired); + + grid.deselect(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 1, "Select event fired once: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + grid.select(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 2, "Select event fired twice: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + grid.select(4); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 3, "Select event fired three times: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + grid.deselect(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 3, "Select event fired three times: " + selectEventFired); + assert.strictEqual(deselectEventFired, 2, "Deselect event fired twice: " + deselectEventFired); + + grid.deselect(4); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 3, "Select event fired three times: " + selectEventFired); + assert.strictEqual(deselectEventFired, 3, "Deselect event fired three times: " + deselectEventFired); + + grid.destroy(); + }; + + notificationTests[name + " events + no store + remove"] = function(){ + // Create selections, remove rows, watch for events + var grid = new (declare([Grid, SelectionMixin]))({ + columns: getColumns() + }), + deselectEventFired = 0; + + document.body.appendChild(grid.domNode); + grid.startup(); + grid.renderArray(_createTestData()); + + grid.on('dgrid-deselect', function(){ + deselectEventFired++; + }); + + grid.select(3); + grid.select(4); + grid._fireSelectionEvent(); + assert.strictEqual(deselectEventFired, 0, "Deselect event not fired: " + deselectEventFired); + + grid.row(3).remove(); + grid._fireSelectionEvent(); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + grid.row(5).remove(); + grid._fireSelectionEvent(); + assert.strictEqual(deselectEventFired, 1, "Deselect event not fired again: " + deselectEventFired); + + grid.row(4).remove(); + grid._fireSelectionEvent(); + assert.strictEqual(deselectEventFired, 2, "Deselect event fired a second time: " + deselectEventFired); + + grid.destroy(); + }; + + notificationTests[name + " events + store + remove"] = function(){ + // Create selections, remove data, watch for events + var store = Observable(new Memory({ + data: _createTestData() + })), + grid = new (declare([OnDemandGrid, SelectionMixin]))({ + columns: getColumns(), + store: store, + sort: "id" + }), + selectEventFired, + deselectEventFired; + + document.body.appendChild(grid.domNode); + grid.startup(); + + grid.on('dgrid-select', function(){ + selectEventFired++; + }); + grid.on('dgrid-deselect', function(){ + deselectEventFired++; + }); + + function testEvents(){ + selectEventFired = 0; + deselectEventFired = 0; + grid.select(3); + grid.select(4); + grid._fireSelectionEvent(); + assert.strictEqual(deselectEventFired, 0, "Deselect event not fired: " + deselectEventFired); + + // Reset the select event counter. It should not fire on remove. + selectEventFired = 0; + + store.remove(3); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 0, "Select event not fired: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event fired once: " + deselectEventFired); + + store.remove(5); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 0, "Select event not fired: " + selectEventFired); + assert.strictEqual(deselectEventFired, 1, "Deselect event not fired again: " + deselectEventFired); + + store.remove(4); + grid._fireSelectionEvent(); + assert.strictEqual(selectEventFired, 0, "Select event not fired: " + selectEventFired); + assert.strictEqual(deselectEventFired, 2, "Deselect event fired a second time: " + deselectEventFired); + } + + // Test the events + testEvents(); + // Change the store + store = Observable(new Memory({ + data: _createTestData() + })); + grid.set("store", store); + // Test the events again + testEvents(); + + grid.destroy(); + }; }); test.suite("Selection update handling", function(){