Web Inspector: Table selection becomes corrupted when deleting selected cookies
authormattbaker@apple.com <mattbaker@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Dec 2018 20:46:59 +0000 (20:46 +0000)
committermattbaker@apple.com <mattbaker@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Dec 2018 20:46:59 +0000 (20:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192388
<rdar://problem/46472364>

Reviewed by Devin Rousso.

Source/WebInspectorUI:

* UserInterface/Controllers/SelectionController.js:
(WI.SelectionController):
(WI.SelectionController.prototype.didRemoveItems):
(WI.SelectionController.prototype._updateSelectedItems):
(WI.SelectionController.prototype.didRemoveItem): Deleted.
Replace `didRemoveItem` with a method taking an IndexSet. Calling the
single-index version while iterating over multiple rows in ascending
order is unsafe, a detail best left to the SelectionController.

* UserInterface/Views/Table.js:
(WI.Table.prototype.removeRow):
(WI.Table.prototype._removeRows):
Notify SelectionController of removed rows.

* UserInterface/Views/TreeOutline.js:
(WI.TreeOutline.prototype.insertChild):
(WI.TreeOutline.prototype.removeChildAtIndex):
Remove the child from the element's `children` after calling `_forgetTreeElement`,
which needs to calculate the child's index to pass to the SelectionController.

(WI.TreeOutline.prototype.removeChildren):
Remove child items during iteration so that `children` doesn't contain
detached TreeElements while calling `_forgetTreeElement`.

(WI.TreeOutline.prototype._rememberTreeElement):
(WI.TreeOutline.prototype._forgetTreeElement):

LayoutTests:

* inspector/table/table-remove-rows-expected.txt:
* inspector/table/table-remove-rows.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239175 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/inspector/table/table-remove-rows-expected.txt
LayoutTests/inspector/table/table-remove-rows.html
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
Source/WebInspectorUI/UserInterface/Views/Table.js
Source/WebInspectorUI/UserInterface/Views/TreeOutline.js

index 83074a5..5650df1 100644 (file)
@@ -1,3 +1,14 @@
+2018-12-13  Matt Baker  <mattbaker@apple.com>
+
+        Web Inspector: Table selection becomes corrupted when deleting selected cookies
+        https://bugs.webkit.org/show_bug.cgi?id=192388
+        <rdar://problem/46472364>
+
+        Reviewed by Devin Rousso.
+
+        * inspector/table/table-remove-rows-expected.txt:
+        * inspector/table/table-remove-rows.html:
+
 2018-12-13  Brent Fulgham  <bfulgham@apple.com>
 
         Don't attempt to animate invalid CSS properties
index 92e34ab..5fa87ba 100644 (file)
@@ -13,6 +13,11 @@ Given a Table with selected rows [0], remove row 0.
 Selection changed to [] before removing row 0.
 PASS: Should remove row 0.
 
+-- Running test case: Table.RemoveRow.PrecedingSelected
+Given a Table with selected rows [1,3], remove row 0.
+PASS: Should remove row 0.
+PASS: Selected row indexes should be adjusted.
+
 -- Running test case: Table.RemoveSelectedRows.Single.SelectFollowing
 Given a Table with selected rows [0]:
  * Row 0
index c461503..358e049 100644 (file)
@@ -134,6 +134,24 @@ function test()
         }
     });
 
+    suite.addTestCase({
+        name: "Table.RemoveRow.PrecedingSelected",
+        description: "Remove a row preceding the selection, causing the selection to shift up.",
+        test() {
+            let testDelegate = new RemoveRowTestDelegate;
+            let table = InspectorTest.createTableWithDelegate(testDelegate, numberOfRows);
+            table.allowsMultipleSelection = true;
+
+            table.selectRow(1);
+            table.selectRow(3, true);
+            testDelegate.triggerRemoveRow(table, 0);
+
+            InspectorTest.expectShallowEqual(table.selectedRows, [0, 2], "Selected row indexes should be adjusted.");
+
+            return true;
+        }
+    });
+
     function addTestCase({name, description, rowIndexes}) {
         suite.addTestCase({
             name, description,
index 183e7ff..dda4c8a 100644 (file)
@@ -1,3 +1,38 @@
+2018-12-13  Matt Baker  <mattbaker@apple.com>
+
+        Web Inspector: Table selection becomes corrupted when deleting selected cookies
+        https://bugs.webkit.org/show_bug.cgi?id=192388
+        <rdar://problem/46472364>
+
+        Reviewed by Devin Rousso.
+
+        * UserInterface/Controllers/SelectionController.js:
+        (WI.SelectionController):
+        (WI.SelectionController.prototype.didRemoveItems):
+        (WI.SelectionController.prototype._updateSelectedItems):
+        (WI.SelectionController.prototype.didRemoveItem): Deleted.
+        Replace `didRemoveItem` with a method taking an IndexSet. Calling the
+        single-index version while iterating over multiple rows in ascending
+        order is unsafe, a detail best left to the SelectionController.
+
+        * UserInterface/Views/Table.js:
+        (WI.Table.prototype.removeRow):
+        (WI.Table.prototype._removeRows):
+        Notify SelectionController of removed rows.
+
+        * UserInterface/Views/TreeOutline.js:
+        (WI.TreeOutline.prototype.insertChild):
+        (WI.TreeOutline.prototype.removeChildAtIndex):
+        Remove the child from the element's `children` after calling `_forgetTreeElement`,
+        which needs to calculate the child's index to pass to the SelectionController.
+
+        (WI.TreeOutline.prototype.removeChildren):
+        Remove child items during iteration so that `children` doesn't contain
+        detached TreeElements while calling `_forgetTreeElement`.
+
+        (WI.TreeOutline.prototype._rememberTreeElement):
+        (WI.TreeOutline.prototype._forgetTreeElement):
+
 2018-12-10  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: REGRESSION (r238599): unable to select specific timeline
index 725ec3e..d1e9f31 100644 (file)
@@ -37,6 +37,7 @@ WI.SelectionController = class SelectionController extends WI.Object
         this._lastSelectedIndex = NaN;
         this._shiftAnchorIndex = NaN;
         this._selectedIndexes = new WI.IndexSet;
+        this._suppressSelectionDidChange = false;
 
         console.assert(this._delegate.selectionControllerNumberOfItems, "SelectionController delegate must implement selectionControllerNumberOfItems.");
         console.assert(this._delegate.selectionControllerNextSelectableIndex, "SelectionController delegate must implement selectionControllerNextSelectableIndex.");
@@ -213,15 +214,63 @@ WI.SelectionController = class SelectionController extends WI.Object
         }
     }
 
-    didRemoveItem(index)
+    didRemoveItems(indexes)
     {
-        if (this.hasSelectedItem(index))
-            this.deselectItem(index);
+        console.assert(indexes instanceof WI.IndexSet);
 
-        while (index = this._selectedIndexes.indexGreaterThan(index)) {
-            this._selectedIndexes.delete(index);
-            this._selectedIndexes.add(index - 1);
+        if (!this._selectedIndexes.size)
+            return;
+
+        let firstRemovedIndex = indexes.firstIndex;
+        if (this._selectedIndexes.lastIndex < firstRemovedIndex)
+            return;
+
+        let newSelectedIndexes = new WI.IndexSet;
+
+        let lastRemovedIndex = indexes.lastIndex;
+        if (this._selectedIndexes.firstIndex < lastRemovedIndex) {
+            let removedCount = 0;
+            let removedIndex = firstRemovedIndex;
+
+            this._suppressSelectionDidChange = true;
+
+            // Adjust the selected indexes that are in the range between the
+            // first and last removed index (inclusive).
+            for (let current = this._selectedIndexes.firstIndex; current < lastRemovedIndex; current = this._selectedIndexes.indexGreaterThan(current)) {
+                if (this.hasSelectedItem(current)) {
+                    this.deselectItem(current);
+                    removedCount++;
+                    continue;
+                }
+
+                while (removedIndex < current) {
+                    removedCount++;
+                    removedIndex = indexes.indexGreaterThan(removedIndex);
+                }
+
+                let newIndex = current - removedCount;
+                newSelectedIndexes.add(newIndex);
+
+                if (this._lastSelectedIndex === current)
+                    this._lastSelectedIndex = newIndex;
+                if (this._shiftAnchorIndex === current)
+                    this._shiftAnchorIndex = newIndex;
+            }
+
+            this._suppressSelectionDidChange = false;
         }
+
+        let removedCount = indexes.size;
+        let current = lastRemovedIndex;
+        while (current = this._selectedIndexes.indexGreaterThan(current))
+            newSelectedIndexes.add(current - removedCount);
+
+        if (this._lastSelectedIndex > lastRemovedIndex)
+            this._lastSelectedIndex -= removedCount;
+        if (this._shiftAnchorIndex > lastRemovedIndex)
+            this._shiftAnchorIndex -= removedCount;
+
+        this._selectedIndexes = newSelectedIndexes;
     }
 
     handleKeyDown(event)
@@ -386,7 +435,7 @@ WI.SelectionController = class SelectionController extends WI.Object
         let oldSelectedIndexes = this._selectedIndexes.copy();
         this._selectedIndexes = indexes;
 
-        if (!this._delegate.selectionControllerSelectionDidChange)
+        if (this._suppressSelectionDidChange || !this._delegate.selectionControllerSelectionDidChange)
             return;
 
         let deselectedItems = oldSelectedIndexes.difference(indexes);
index acd0cf4..2bc6f34 100644 (file)
@@ -338,8 +338,8 @@ WI.Table = class Table extends WI.View
         if (this.isRowSelected(rowIndex))
             this.deselectRow(rowIndex);
 
-        this._removeRows(new WI.IndexSet([rowIndex]));
-        this._selectionController.didRemoveItem(rowIndex);
+        let rowIndexes = new WI.IndexSet([rowIndex]);
+        this._removeRows(rowIndexes);
     }
 
     removeSelectedRows()
@@ -1393,6 +1393,8 @@ WI.Table = class Table extends WI.View
         this._cachedNumberOfRows -= removed;
         console.assert(this._cachedNumberOfRows >= 0);
 
+        this._selectionController.didRemoveItems(rowIndexes);
+
         if (this._delegate.tableDidRemoveRows) {
             this._delegate.tableDidRemoveRows(this, Array.from(rowIndexes));
             console.assert(this._cachedNumberOfRows === this._dataSource.tableNumberOfRows(this), "Table data source should update after removing rows.");
index baea55d..247b657 100644 (file)
@@ -295,12 +295,6 @@ WI.TreeOutline = class TreeOutline extends WI.Object
         if (child.hasChildren && child.treeOutline._treeElementsExpandedState[child.identifier] !== undefined)
             child.expanded = child.treeOutline._treeElementsExpandedState[child.identifier];
 
-        // Update the SelectionController before attaching the TreeElement.
-        // Attaching the TreeElement can cause it to become selected, and
-        // added to the SelectionController.
-        let insertionIndex = this.treeOutline._indexOfTreeElement(child) || 0;
-        this.treeOutline._selectionController.didInsertItem(insertionIndex);
-
         if (this._childrenListNode)
             child._attach();
 
@@ -318,9 +312,8 @@ WI.TreeOutline = class TreeOutline extends WI.Object
             return;
 
         let child = this.children[childIndex];
-        this.children.splice(childIndex, 1);
-
         let parent = child.parent;
+
         if (child.deselect(suppressOnDeselect)) {
             if (child.previousSibling && !suppressSelectSibling)
                 child.previousSibling.select(true, false);
@@ -330,18 +323,19 @@ WI.TreeOutline = class TreeOutline extends WI.Object
                 parent.select(true, false);
         }
 
-        if (child.previousSibling)
-            child.previousSibling.nextSibling = child.nextSibling;
-        if (child.nextSibling)
-            child.nextSibling.previousSibling = child.previousSibling;
-
         let treeOutline = child.treeOutline;
         if (treeOutline) {
             treeOutline._forgetTreeElement(child);
             treeOutline._forgetChildrenRecursive(child);
-            treeOutline._selectionController.didRemoveItem(childIndex);
         }
 
+        if (child.previousSibling)
+            child.previousSibling.nextSibling = child.nextSibling;
+        if (child.nextSibling)
+            child.nextSibling.previousSibling = child.previousSibling;
+
+        this.children.splice(childIndex, 1);
+
         child._detach();
         child.treeOutline = null;
         child.parent = null;
@@ -374,7 +368,8 @@ WI.TreeOutline = class TreeOutline extends WI.Object
 
     removeChildren(suppressOnDeselect)
     {
-        for (let child of this.children) {
+        while (this.children.length) {
+            let child = this.children[0];
             child.deselect(suppressOnDeselect);
 
             let treeOutline = child.treeOutline;
@@ -391,9 +386,9 @@ WI.TreeOutline = class TreeOutline extends WI.Object
 
             if (treeOutline)
                 treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementRemoved, {element: child});
-        }
 
-        this.children = [];
+            this.children.shift();
+        }
     }
 
     _rememberTreeElement(element)
@@ -409,6 +404,10 @@ WI.TreeOutline = class TreeOutline extends WI.Object
         // add the element
         elements.push(element);
         this._cachedNumberOfDescendents++;
+
+        let index = this._indexOfTreeElement(element);
+        if (index >= 0)
+            this._selectionController.didInsertItem(index);
     }
 
     _forgetTreeElement(element)
@@ -418,6 +417,10 @@ WI.TreeOutline = class TreeOutline extends WI.Object
             this.selectedTreeElement = null;
         }
         if (this._knownTreeElements[element.identifier]) {
+            let index = this._indexOfTreeElement(element);
+            if (index >= 0)
+                this._selectionController.didRemoveItems(new WI.IndexSet([index]));
+
             this._knownTreeElements[element.identifier].remove(element, true);
             this._cachedNumberOfDescendents--;
         }
@@ -1030,7 +1033,7 @@ WI.TreeOutline = class TreeOutline extends WI.Object
             ++index;
         }
 
-        console.assert(false, "Unable to get index for tree element.", treeElement, treeOutline);
+        console.assert(false, "Unable to get index for tree element.", treeElement);
         return NaN;
     }