Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed...
authordrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 22:44:50 +0000 (22:44 +0000)
committerdrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 22:44:50 +0000 (22:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192711
<rdar://problem/46738990>

Reviewed by Timothy Hatcher.

Original patch by Matt Baker <mattbaker@apple.com>.

Source/WebInspectorUI:

* UserInterface/Controllers/SelectionController.js:
(WI.SelectionController.prototype.removeSelectedItems):
When looking for a new item to select, start with the item preceding the
selection, instead of the item following the selection. This matches
pre-multiple selection behavior, as well as Mail and Xcode.

* UserInterface/Views/DOMTreeElement.js:
(WI.DOMTreeElement.prototype.onexpand):
Drive-by fix: when a hidden node is selected, its selection area is drawn
with a height of 0px. Update the selection area once the hidden node's
parent is expanded. AFAIK, this has always been broken.

* UserInterface/Views/DOMTreeOutline.js:
(WI.DOMTreeOutline.prototype.ondelete):
After a delete the `SelectionController` may have chosen a child of a
collapsed parent as the new selected item. If the item isn't the closing tag (e.g. after
deleting the last child), reveal it.

(WI.DOMTreeOutline.prototype.selectionControllerPreviousSelectableItem):

* UserInterface/Views/TreeElement.js:
(WI.TreeElement.prototype.get previousSelectableSibling): Added.
(WI.TreeElement.prototype.get nextSelectableSibling): Added.

* UserInterface/Views/TreeOutline.js:
(WI.TreeOutline.prototype.selectionControllerPreviousSelectableItem):
(WI.TreeOutline.prototype.selectionControllerNextSelectableItem):
Set `skipUnrevealed` to false, so that children of collapsed parent nodes
are considered when looking for an item to selected after a delete. Hidden `TreeElement`s
are still ignored as they aren't `selectable`.

LayoutTests:

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

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244155 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/DOMTreeElement.js
Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js
Source/WebInspectorUI/UserInterface/Views/TreeElement.js
Source/WebInspectorUI/UserInterface/Views/TreeOutline.js

index 9d600fd..27eeedd 100644 (file)
@@ -1,5 +1,18 @@
 2019-04-10  Devin Rousso  <drousso@apple.com>
 
+        Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed parent selects the parent's next sibling
+        https://bugs.webkit.org/show_bug.cgi?id=192711
+        <rdar://problem/46738990>
+
+        Reviewed by Timothy Hatcher.
+
+        Original patch by Matt Baker <mattbaker@apple.com>.
+
+        * inspector/table/table-remove-rows.html:
+        * inspector/table/table-remove-rows-expected.txt:
+
+2019-04-10  Devin Rousso  <drousso@apple.com>
+
         Web Inspector: Elements tab: multiple selection lost after navigating to another tab
         https://bugs.webkit.org/show_bug.cgi?id=192681
         <rdar://problem/46709392>
index 5fa87ba..558eb04 100644 (file)
@@ -44,31 +44,31 @@ Selection changed before removing rows:
  - Row 3
 PASS: Should remove rows [3].
 
--- Running test case: Table.RemoveSelectedRows.Multiple.SelectFollowing
-Given a Table with selected rows [0,2]:
* Row 0
+-- Running test case: Table.RemoveSelectedRows.Single.SelectPrecedingOverFollowing
+Given a Table with selected rows [2]:
  Row 0
    Row 1
  * Row 2
    Row 3
 Selection changed before removing rows:
- Row 0
  Row 1
  Row 0
* Row 1
  - Row 2
* Row 3
-PASS: Should remove rows [0,2].
  Row 3
+PASS: Should remove rows [2].
 
--- Running test case: Table.RemoveSelectedRows.Multiple.SelectHole
-Given a Table with selected rows [0,3]:
+-- Running test case: Table.RemoveSelectedRows.Multiple.SelectFollowing
+Given a Table with selected rows [0,1]:
  * Row 0
  Row 1
* Row 1
    Row 2
* Row 3
  Row 3
 Selection changed before removing rows:
  - Row 0
* Row 1
  Row 2
- Row 3
-PASS: Should remove rows [0,3].
- Row 1
* Row 2
  Row 3
+PASS: Should remove rows [0,1].
 
 -- Running test case: Table.RemoveSelectedRows.Multiple.SelectPreceding
 Given a Table with selected rows [2,3]:
@@ -83,6 +83,45 @@ Selection changed before removing rows:
  - Row 3
 PASS: Should remove rows [2,3].
 
+-- Running test case: Table.RemoveSelectedRows.Multiple.SelectPrecedingOverFollowing
+Given a Table with selected rows [1,2]:
+   Row 0
+ * Row 1
+ * Row 2
+   Row 3
+Selection changed before removing rows:
+ * Row 0
+ - Row 1
+ - Row 2
+   Row 3
+PASS: Should remove rows [1,2].
+
+-- Running test case: Table.RemoveSelectedRows.Multiple.SelectPrecedingOverGap
+Given a Table with selected rows [1,3]:
+   Row 0
+ * Row 1
+   Row 2
+ * Row 3
+Selection changed before removing rows:
+ * Row 0
+ - Row 1
+   Row 2
+ - Row 3
+PASS: Should remove rows [1,3].
+
+-- Running test case: Table.RemoveSelectedRows.Multiple.SelectGapOverFollowing
+Given a Table with selected rows [0,2]:
+ * Row 0
+   Row 1
+ * Row 2
+   Row 3
+Selection changed before removing rows:
+ - Row 0
+ * Row 1
+ - Row 2
+   Row 3
+PASS: Should remove rows [0,2].
+
 -- Running test case: Table.RemoveRow.NotCached
 Given a Table with selected rows [], remove row 999.
 PASS: Should remove row 999.
index 358e049..2bb6662 100644 (file)
@@ -172,34 +172,52 @@ function test()
 
     addTestCase({
         name: "Table.RemoveSelectedRows.Single.SelectFollowing",
-        description: "Remove the selected row, causing the following row to be selected.",
+        description: "Remove a selected row not preceded by a selectable row.",
         rowIndexes: [0],
     });
 
     addTestCase({
         name: "Table.RemoveSelectedRows.Single.SelectPreceding",
-        description: "Remove the selected row, causing the preceding row to be selected.",
+        description: "Remove a selected row not followed by a selectable row.",
         rowIndexes: [lastRowIndex],
     });
 
     addTestCase({
-        name: "Table.RemoveSelectedRows.Multiple.SelectFollowing",
-        description: "Remove selected rows, causing the row following the selection to be selected.",
-        rowIndexes: [0, lastRowIndex - 1],
+        name: "Table.RemoveSelectedRows.Single.SelectPrecedingOverFollowing",
+        description: "Remove a selected row between two selectable rows.",
+        rowIndexes: [lastRowIndex - 1],
     });
 
     addTestCase({
-        name: "Table.RemoveSelectedRows.Multiple.SelectHole",
-        description: "Remove selected rows, causing the first deselected row inside the selection to be selected.",
-        rowIndexes: [0, lastRowIndex],
+        name: "Table.RemoveSelectedRows.Multiple.SelectFollowing",
+        description: "Remove a contiguous selection not preceded by a selectable row.",
+        rowIndexes: [0, 1],
     });
 
     addTestCase({
         name: "Table.RemoveSelectedRows.Multiple.SelectPreceding",
-        description: "Remove selected rows, causing the row preceding the selection to be selected.",
+        description: "Remove a contiguous selection not followed by a selectable row.",
         rowIndexes: [lastRowIndex - 1, lastRowIndex],
     });
 
+    addTestCase({
+        name: "Table.RemoveSelectedRows.Multiple.SelectPrecedingOverFollowing",
+        description: "Remove a contiguous selection between two selectable rows.",
+        rowIndexes: [1, 2],
+    });
+
+    addTestCase({
+        name: "Table.RemoveSelectedRows.Multiple.SelectPrecedingOverGap",
+        description: "Remove a non-contiguous selection preceded by a selectable row.",
+        rowIndexes: [1, 3],
+    });
+
+    addTestCase({
+        name: "Table.RemoveSelectedRows.Multiple.SelectGapOverFollowing",
+        description: "Remove a non-contiguous selection followed by a selectable row.",
+        rowIndexes: [0, 2],
+    });
+
     suite.addTestCase({
         name: "Table.RemoveRow.NotCached",
         description: "Remove a row that is not in the table cache.",
index d39cad9..245dcd4 100644 (file)
@@ -1,5 +1,46 @@
 2019-04-10  Devin Rousso  <drousso@apple.com>
 
+        Web Inspector: REGRESSION (r238602): Elements: deleting the last child of a collapsed parent selects the parent's next sibling
+        https://bugs.webkit.org/show_bug.cgi?id=192711
+        <rdar://problem/46738990>
+
+        Reviewed by Timothy Hatcher.
+
+        Original patch by Matt Baker <mattbaker@apple.com>.
+
+        * UserInterface/Controllers/SelectionController.js:
+        (WI.SelectionController.prototype.removeSelectedItems):
+        When looking for a new item to select, start with the item preceding the
+        selection, instead of the item following the selection. This matches
+        pre-multiple selection behavior, as well as Mail and Xcode.
+
+        * UserInterface/Views/DOMTreeElement.js:
+        (WI.DOMTreeElement.prototype.onexpand):
+        Drive-by fix: when a hidden node is selected, its selection area is drawn
+        with a height of 0px. Update the selection area once the hidden node's
+        parent is expanded. AFAIK, this has always been broken.
+
+        * UserInterface/Views/DOMTreeOutline.js:
+        (WI.DOMTreeOutline.prototype.ondelete):
+        After a delete the `SelectionController` may have chosen a child of a
+        collapsed parent as the new selected item. If the item isn't the closing tag (e.g. after
+        deleting the last child), reveal it.
+
+        (WI.DOMTreeOutline.prototype.selectionControllerPreviousSelectableItem):
+
+        * UserInterface/Views/TreeElement.js:
+        (WI.TreeElement.prototype.get previousSelectableSibling): Added.
+        (WI.TreeElement.prototype.get nextSelectableSibling): Added.
+
+        * UserInterface/Views/TreeOutline.js:
+        (WI.TreeOutline.prototype.selectionControllerPreviousSelectableItem):
+        (WI.TreeOutline.prototype.selectionControllerNextSelectableItem):
+        Set `skipUnrevealed` to false, so that children of collapsed parent nodes
+        are considered when looking for an item to selected after a delete. Hidden `TreeElement`s
+        are still ignored as they aren't `selectable`.
+
+2019-04-10  Devin Rousso  <drousso@apple.com>
+
         Web Inspector: Elements tab: multiple selection lost after navigating to another tab
         https://bugs.webkit.org/show_bug.cgi?id=192681
         <rdar://problem/46709392>
index 728b58d..dbb0ae7 100644 (file)
@@ -178,20 +178,20 @@ WI.SelectionController = class SelectionController extends WI.Object
 
         let orderedSelection = Array.from(this._selectedItems).sort(this._comparator);
 
-        // Try selecting the item following the selection.
-        let lastSelectedItem = orderedSelection.lastValue;
-        let itemToSelect = this._nextSelectableItem(lastSelectedItem);
+        // Try selecting the item preceding the selection.
+        let firstSelectedItem = orderedSelection[0];
+        let itemToSelect = this._previousSelectableItem(firstSelectedItem);
         if (!itemToSelect) {
-            // If no item exists after the last item in the selection, try selecting
+            // If no item exists before the first item in the selection, try selecting
             // a deselected item (hole) within the selection.
-            itemToSelect = orderedSelection[0];
+            itemToSelect = firstSelectedItem;
             while (itemToSelect && this.hasSelectedItem(itemToSelect))
                 itemToSelect = this._nextSelectableItem(itemToSelect);
 
             if (!itemToSelect || this.hasSelectedItem(itemToSelect)) {
                 // If the selection contains no holes, try selecting the item
-                // preceding the selection.
-                itemToSelect = this._previousSelectableItem(orderedSelection[0]);
+                // following the selection.
+                itemToSelect = this._nextSelectableItem(orderedSelection.lastValue);
             }
         }
 
index 9841431..c87d343 100644 (file)
@@ -606,6 +606,9 @@ WI.DOMTreeElement = class DOMTreeElement extends WI.TreeElement
             return;
 
         this.updateTitle();
+
+        for (let treeElement of this.children)
+            treeElement.updateSelectionArea();
     }
 
     oncollapse()
index 07efa41..430fa76 100644 (file)
@@ -335,9 +335,33 @@ WI.DOMTreeOutline = class DOMTreeOutline extends WI.TreeOutline
 
         this._treeElementsToRemove = null;
 
+        if (this.selectedTreeElement && !this.selectedTreeElement.isCloseTag()) {
+            console.assert(this.selectedTreeElements.length === 1);
+            this.selectedTreeElement.reveal();
+        }
+
         return true;
     }
 
+    // SelectionController delegate overrides
+
+    selectionControllerPreviousSelectableItem(controller, item)
+    {
+        let treeElement = this.getCachedTreeElement(item);
+        console.assert(treeElement, "Missing TreeElement for representedObject.", item);
+        if (!treeElement)
+            return null;
+
+        if (this._treeElementsToRemove) {
+            // When deleting, force the SelectionController to check siblings in
+            // the opposite direction before searching up the parent chain.
+            if (!treeElement.previousSelectableSibling && treeElement.nextSelectableSibling)
+                return null;
+        }
+
+        return super.selectionControllerPreviousSelectableItem(controller, item);
+    }
+
     // Protected
 
     canSelectTreeElement(treeElement)
index 88a6c4d..adf5904 100644 (file)
@@ -184,6 +184,22 @@ WI.TreeElement = class TreeElement extends WI.Object
             this.expand();
     }
 
+    get previousSelectableSibling()
+    {
+        let treeElement = this.previousSibling;
+        while (treeElement && !treeElement.selectable)
+            treeElement = treeElement.previousSibling;
+        return treeElement;
+    }
+
+    get nextSelectableSibling()
+    {
+        let treeElement = this.nextSibling;
+        while (treeElement && !treeElement.selectable)
+            treeElement = treeElement.nextSibling;
+        return treeElement;
+    }
+
     canSelectOnMouseDown(event)
     {
         // Overridden by subclasses if needed.
index 66ecd52..449c66f 100644 (file)
@@ -855,7 +855,7 @@ WI.TreeOutline = class TreeOutline extends WI.Object
         if (!treeElement)
             return null;
 
-        const skipUnrevealed = true;
+        const skipUnrevealed = false;
         const stayWithin = null;
         const dontPopulate = true;
 
@@ -874,7 +874,7 @@ WI.TreeOutline = class TreeOutline extends WI.Object
         if (!treeElement)
             return null;
 
-        const skipUnrevealed = true;
+        const skipUnrevealed = false;
         const stayWithin = null;
         const dontPopulate = true;