- Revision
- 239175
- Author
- [email protected]
- Date
- 2018-12-13 12:46:59 -0800 (Thu, 13 Dec 2018)
Log Message
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.
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:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (239174 => 239175)
--- trunk/LayoutTests/ChangeLog 2018-12-13 20:30:01 UTC (rev 239174)
+++ trunk/LayoutTests/ChangeLog 2018-12-13 20:46:59 UTC (rev 239175)
@@ -1,3 +1,14 @@
+2018-12-13 Matt Baker <[email protected]>
+
+ 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 <[email protected]>
Don't attempt to animate invalid CSS properties
Modified: trunk/LayoutTests/inspector/table/table-remove-rows-expected.txt (239174 => 239175)
--- trunk/LayoutTests/inspector/table/table-remove-rows-expected.txt 2018-12-13 20:30:01 UTC (rev 239174)
+++ trunk/LayoutTests/inspector/table/table-remove-rows-expected.txt 2018-12-13 20:46:59 UTC (rev 239175)
@@ -13,6 +13,11 @@
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
Modified: trunk/LayoutTests/inspector/table/table-remove-rows.html (239174 => 239175)
--- trunk/LayoutTests/inspector/table/table-remove-rows.html 2018-12-13 20:30:01 UTC (rev 239174)
+++ trunk/LayoutTests/inspector/table/table-remove-rows.html 2018-12-13 20:46:59 UTC (rev 239175)
@@ -134,6 +134,24 @@
}
});
+ 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,
Modified: trunk/Source/WebInspectorUI/ChangeLog (239174 => 239175)
--- trunk/Source/WebInspectorUI/ChangeLog 2018-12-13 20:30:01 UTC (rev 239174)
+++ trunk/Source/WebInspectorUI/ChangeLog 2018-12-13 20:46:59 UTC (rev 239175)
@@ -1,3 +1,38 @@
+2018-12-13 Matt Baker <[email protected]>
+
+ 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 <[email protected]>
Web Inspector: REGRESSION (r238599): unable to select specific timeline
Modified: trunk/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js (239174 => 239175)
--- trunk/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js 2018-12-13 20:30:01 UTC (rev 239174)
+++ trunk/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js 2018-12-13 20:46:59 UTC (rev 239175)
@@ -37,6 +37,7 @@
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 @@
}
}
- 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 @@
let oldSelectedIndexes = this._selectedIndexes.copy();
this._selectedIndexes = indexes;
- if (!this._delegate.selectionControllerSelectionDidChange)
+ if (this._suppressSelectionDidChange || !this._delegate.selectionControllerSelectionDidChange)
return;
let deselectedItems = oldSelectedIndexes.difference(indexes);
Modified: trunk/Source/WebInspectorUI/UserInterface/Views/Table.js (239174 => 239175)
--- trunk/Source/WebInspectorUI/UserInterface/Views/Table.js 2018-12-13 20:30:01 UTC (rev 239174)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/Table.js 2018-12-13 20:46:59 UTC (rev 239175)
@@ -338,8 +338,8 @@
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 @@
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.");
Modified: trunk/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js (239174 => 239175)
--- trunk/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js 2018-12-13 20:30:01 UTC (rev 239174)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js 2018-12-13 20:46:59 UTC (rev 239175)
@@ -295,12 +295,6 @@
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 @@
return;
let child = this.children[childIndex];
- this.children.splice(childIndex, 1);
+ let parent = child.parent;
- let parent = child.parent;
if (child.deselect(suppressOnDeselect)) {
if (child.previousSibling && !suppressSelectSibling)
child.previousSibling.select(true, false);
@@ -330,18 +323,19 @@
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 @@
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 @@
if (treeOutline)
treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementRemoved, {element: child});
+
+ this.children.shift();
}
-
- this.children = [];
}
_rememberTreeElement(element)
@@ -409,6 +404,10 @@
// 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 @@
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 @@
++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;
}
