Web Inspector: Elements tab: multiple selection lost after navigating to another tab
authordrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 22:44:11 +0000 (22:44 +0000)
committerdrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 22:44:11 +0000 (22:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192681
<rdar://problem/46709392>

Reviewed by Timothy Hatcher.

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

Source/WebInspectorUI:

* UserInterface/Controllers/SelectionController.js:
(WI.SelectionController.prototype.selectItem):
Simplify internal logic by removing an early return.

(WI.SelectionController.prototype.selectItems): Added.
(WI.SelectionController.prototype.selectAll):
Provide a means to select multiple items in a single operation.
If `_lastSelectedItem` is not in the items to select, the last
item being selected will become the last selected item.

(WI.SelectionController.prototype._deselectAllAndSelect):
Drive-by fix: correct a logic error. If no items are selected, the item
passed as an argument should still become selected.

* UserInterface/Views/TreeOutline.js:
(WI.TreeOutline.prototype.selectTreeElements): Added.

* UserInterface/Views/DOMTreeElement.js:
(WI.DOMTreeElement):
(WI.DOMTreeElement.prototype.get closeTagTreeElement): Added.
(WI.DOMTreeElement.prototype._updateChildren):
Make the close tag `TreeElement` available from the open tag `TreeElement`.

* UserInterface/Views/DOMTreeOutline.js:
(WI.DOMTreeOutline.prototype.update):
Restore selected `TreeElement`s after updating.

* UserInterface/Base/Utilities.js:
* UserInterface/Test.html:

LayoutTests:

* inspector/tree-outline/tree-outline-selection.html: Added.
* inspector/tree-outline/tree-outline-selection-expected.txt: Added.
Add `TreeOutline` tests for single and multiple selection.

* inspector/unit-tests/set-utilities.html:
* inspector/unit-tests/set-utilities-expected.txt:
Add tests for `Set.prototype.lastValue`.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/inspector/tree-outline/tree-outline-selection-expected.txt [new file with mode: 0644]
LayoutTests/inspector/tree-outline/tree-outline-selection.html [new file with mode: 0644]
LayoutTests/inspector/unit-tests/set-utilities-expected.txt
LayoutTests/inspector/unit-tests/set-utilities.html
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Base/Utilities.js
Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
Source/WebInspectorUI/UserInterface/Test.html
Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js
Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js
Source/WebInspectorUI/UserInterface/Views/TreeOutline.js

index 7308298..9d600fd 100644 (file)
@@ -1,3 +1,21 @@
+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>
+
+        Reviewed by Timothy Hatcher.
+
+        Orginal patch by Matt Baker <mattbaker@apple.com>.
+
+        * inspector/tree-outline/tree-outline-selection.html: Added.
+        * inspector/tree-outline/tree-outline-selection-expected.txt: Added.
+        Add `TreeOutline` tests for single and multiple selection.
+
+        * inspector/unit-tests/set-utilities.html:
+        * inspector/unit-tests/set-utilities-expected.txt:
+        Add tests for `Set.prototype.lastValue`.
+
 2019-04-10  Youenn Fablet  <youenn@apple.com>
 
         Layout Test http/wpt/fetch/response-opaque-clone.html is sometimes timing out on iOS simulator
diff --git a/LayoutTests/inspector/tree-outline/tree-outline-selection-expected.txt b/LayoutTests/inspector/tree-outline/tree-outline-selection-expected.txt
new file mode 100644 (file)
index 0000000..80057df
--- /dev/null
@@ -0,0 +1,53 @@
+Tests for WI.TreeOutline selection.
+
+
+== Running test suite: TreeOutline.Selection
+-- Running test case: TreeOutline.constructor
+PASS: selectedTreeElement should be null.
+PASS: selectedTreeElements should be empty.
+PASS: allowsMultipleSelection should be false.
+
+-- Running test case: TreeOutline.FindTreeElement
+PASS: Should find TreeElement for represented object.
+PASS: TreeElement should have correct represented object.
+
+PASS: Should not find TreeElement for represented object.
+
+-- Running test case: TreeOutline.SelectedTreeElement
+Selecting TreeElement "Item 1"...
+PASS: TreeOutline should have the correct selection.
+PASS: TreeOutline should have last selected TreeElement "Item 1".
+PASS: TreeElement "Item 1" should be selected.
+PASS: Other TreeElements should not be selected.
+
+Selecting TreeElement "Item 2"...
+PASS: TreeOutline should have the correct selection.
+PASS: TreeOutline should have last selected TreeElement "Item 2".
+PASS: TreeElement "Item 2" should be selected.
+PASS: Other TreeElements should not be selected.
+
+-- Running test case: TreeOutline.AllowsMultipleSelection
+PASS: allowsMultipleSelection enabled.
+PASS: allowsMultipleSelection disabled.
+
+-- Running test case: TreeOutline.MultipleSelectionToggled
+PASS: All TreeElements should be selected.
+PASS: Should have one selected TreeElement.
+PASS: Selected TreeElement should be the last child.
+
+-- Running test case: TreeOutline.SelectTreeElements.MultipleSelectionEnabled
+Selecting TreeElements ["Item 1","Item 2"]...
+PASS: TreeOutline should have the correct selection.
+PASS: TreeOutline should have last selected TreeElement "Item 2".
+PASS: TreeElements ["Item 1","Item 2"] should be selected.
+PASS: All other TreeElements should not be selected.
+
+Selecting TreeElements ["Item 3","Item 4"]...
+PASS: TreeOutline should have the correct selection.
+PASS: TreeOutline should have last selected TreeElement "Item 4".
+PASS: TreeElements ["Item 3","Item 4"] should be selected.
+PASS: All other TreeElements should not be selected.
+
+-- Running test case: TreeOutline.SelectTreeElements.MultipleSelectionDisabled
+PASS: Should have no selected TreeElements.
+
diff --git a/LayoutTests/inspector/tree-outline/tree-outline-selection.html b/LayoutTests/inspector/tree-outline/tree-outline-selection.html
new file mode 100644 (file)
index 0000000..8336c77
--- /dev/null
@@ -0,0 +1,197 @@
+<!doctype html>
+<html>
+<head>
+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
+<script>
+function test()
+{
+    InspectorTest.redirectRequestAnimationFrame();
+
+    let suite = InspectorTest.createSyncSuite("TreeOutline.Selection");
+
+    function createTreeOutline(objects = []) {
+        function appendObject(parent, object) {
+            let options = {};
+            if (object.children)
+                options.hasChildren = true;
+            let treeElement = new WI.TreeElement(object.title, object, options);
+            parent.appendChild(treeElement);
+
+            if (!object.children)
+                return;
+
+            for (let child of object.children)
+                appendObject(treeElement, child);
+        }
+
+        let treeOutline = new WI.TreeOutline;
+        for (let object of objects)
+            appendObject(treeOutline, object);
+
+        return treeOutline;
+    }
+
+    function expectSelectedTreeElements(treeOutline, selectedTreeElements) {
+        InspectorTest.expectShallowEqual(treeOutline.selectedTreeElements, selectedTreeElements, `TreeOutline should have the correct selection.`);
+        InspectorTest.expectEqual(treeOutline.selectedTreeElement, selectedTreeElements.lastValue, `TreeOutline should have last selected TreeElement "${selectedTreeElements.lastValue.title}".`);
+    }
+
+    function triggerSelectTreeElement(treeOutline, treeElement) {
+        InspectorTest.log(`Selecting TreeElement "${treeElement.title}"...`);
+        treeOutline.selectedTreeElement = treeElement;
+
+        expectSelectedTreeElements(treeOutline, [treeElement]);
+        InspectorTest.expectThat(treeElement.selected, `TreeElement "${treeElement.title}" should be selected.`);
+        InspectorTest.expectThat(treeOutline.children.every((x) => x === treeElement || !x.selected), "Other TreeElements should not be selected.");
+    }
+
+    function triggerSelectTreeElements(treeOutline, treeElements) {
+        let displayText = JSON.stringify(treeElements.map((x) => x.title));
+
+        InspectorTest.log(`Selecting TreeElements ${displayText}...`);
+        treeOutline.selectTreeElements(treeElements);
+
+        expectSelectedTreeElements(treeOutline, treeElements);
+        InspectorTest.expectThat(treeElements.every((x) => x.selected), `TreeElements ${displayText} should be selected.`);
+        InspectorTest.expectThat(treeOutline.children.every((x) => !x.selected || treeElements.includes(x)), "All other TreeElements should not be selected.");
+    }
+
+    suite.addTestCase({
+        name: "TreeOutline.constructor",
+        test() {
+            let treeOutline = createTreeOutline();
+
+            InspectorTest.expectNull(treeOutline.selectedTreeElement, "selectedTreeElement should be null.");
+            InspectorTest.expectEqual(treeOutline.selectedTreeElements.length, 0, "selectedTreeElements should be empty.");
+            InspectorTest.expectFalse(treeOutline.allowsMultipleSelection, "allowsMultipleSelection should be false.");
+        }
+    });
+
+    suite.addTestCase({
+        name: "TreeOutline.FindTreeElement",
+        description: "Find a TreeElement based on its associated represented object.",
+        test() {
+            const representedObjects = [
+                {title: "Item 1"},
+                {title: "Item 2"},
+                {title: "Item 3"},
+            ];
+
+            let treeOutline = createTreeOutline(representedObjects);
+
+            let existingTreeElement = treeOutline.findTreeElement(representedObjects[0]);
+            InspectorTest.expectNotNull(existingTreeElement, "Should find TreeElement for represented object.");
+            InspectorTest.expectEqual(existingTreeElement.representedObject, representedObjects[0], "TreeElement should have correct represented object.");
+
+            InspectorTest.log("");
+
+            let missingTreeElement = treeOutline.findTreeElement({title: "Brand New Item"});
+            InspectorTest.expectNull(missingTreeElement, "Should not find TreeElement for represented object.");
+        }
+    });
+
+    suite.addTestCase({
+        name: "TreeOutline.SelectedTreeElement",
+        description: "Select a tree element, then select a different tree element.",
+        test() {
+            const representedObjects = [
+                {title: "Item 1"},
+                {title: "Item 2"},
+            ];
+
+            let treeOutline = createTreeOutline(representedObjects);
+
+            triggerSelectTreeElement(treeOutline, treeOutline.children[0]);
+
+            InspectorTest.log("");
+
+            triggerSelectTreeElement(treeOutline, treeOutline.children[1]);
+        }
+    });
+
+    suite.addTestCase({
+        name: "TreeOutline.AllowsMultipleSelection",
+        description: "Should be able to enable multiple selection.",
+        test() {
+            let treeOutline = createTreeOutline();
+
+            treeOutline.allowsMultipleSelection = true;
+            InspectorTest.expectThat(treeOutline.allowsMultipleSelection, "allowsMultipleSelection enabled.");
+            treeOutline.allowsMultipleSelection = false;
+            InspectorTest.expectFalse(treeOutline.allowsMultipleSelection, "allowsMultipleSelection disabled.");
+        }
+    });
+
+    suite.addTestCase({
+        name: "TreeOutline.MultipleSelectionToggled",
+        description: "Select multiple tree elements and then disable multiple selection.",
+        test() {
+            const representedObjects = [
+                {title: "Item 1"},
+                {title: "Item 2"},
+            ];
+
+            let treeOutline = createTreeOutline(representedObjects);
+            treeOutline.allowsMultipleSelection = true;
+
+            let children = treeOutline.children;
+            treeOutline.selectTreeElements(children);
+
+            InspectorTest.expectShallowEqual(treeOutline.selectedTreeElements, children, "All TreeElements should be selected.");
+
+            treeOutline.allowsMultipleSelection = false;
+
+            InspectorTest.expectEqual(treeOutline.selectedTreeElements.length, 1, "Should have one selected TreeElement.");
+            InspectorTest.expectShallowEqual(treeOutline.selectedTreeElements, [children[1]], "Selected TreeElement should be the last child.");
+        }
+    });
+
+    suite.addTestCase({
+        name: "TreeOutline.SelectTreeElements.MultipleSelectionEnabled",
+        description: "Select two tree elements, then select two different tree elements.",
+        test() {
+            const representedObjects = [
+                {title: "Item 1"},
+                {title: "Item 2"},
+                {title: "Item 3"},
+                {title: "Item 4"},
+            ];
+
+            let treeOutline = createTreeOutline(representedObjects);
+            treeOutline.allowsMultipleSelection = true;
+
+            let children = treeOutline.children;
+
+            triggerSelectTreeElements(treeOutline, [children[0], children[1]]);
+
+            InspectorTest.log("");
+
+            triggerSelectTreeElements(treeOutline, [children[2], children[3]]);
+        }
+    });
+
+    suite.addTestCase({
+        name: "TreeOutline.SelectTreeElements.MultipleSelectionDisabled",
+        description: "Select multiple tree elements with multiple selection disabled.",
+        test() {
+            const representedObjects = [
+                {title: "Item 1"},
+                {title: "Item 2"},
+            ];
+
+            let treeOutline = createTreeOutline(representedObjects);
+            treeOutline.allowsMultipleSelection = false;
+
+            treeOutline.selectTreeElements(treeOutline.children);
+            InspectorTest.expectEqual(treeOutline.selectedTreeElements.length, 0, "Should have no selected TreeElements.");
+        }
+    });
+
+    suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onLoad="runTest()">
+    <p>Tests for WI.TreeOutline selection.</p>
+</body>
+</html>
index f844639..d2e7d22 100644 (file)
@@ -41,3 +41,7 @@ PASS: Set difference should be [1].
 PASS: Set with values [] should have firstValue equal to undefined.
 PASS: Set with values [1,2,3] should have firstValue equal to 1.
 
+-- Running test case: Set.prototype.lastValue
+PASS: Set with values [] should have lastValue equal to undefined.
+PASS: Set with values [1,2,3] should have lastValue equal to 3.
+
index 5033290..5905e2f 100644 (file)
@@ -122,7 +122,8 @@ function test()
         name: "Set.prototype.firstValue",
         test() {
             function testFirstValue(values) {
-                InspectorTest.expectEqual(new Set(values).firstValue, values[0], `Set with values [${values}] should have firstValue equal to ${values[0]}.`);
+                let expected = values[0];
+                InspectorTest.expectEqual(new Set(values).firstValue, expected, `Set with values [${values}] should have firstValue equal to ${expected}.`);
             }
 
             testFirstValue([]);
@@ -130,6 +131,19 @@ function test()
         }
     });
 
+    suite.addTestCase({
+        name: "Set.prototype.lastValue",
+        test() {
+            function testLastValue(values) {
+                let expected = values.lastValue;
+                InspectorTest.expectEqual(new Set(values).lastValue, expected, `Set with values [${values}] should have lastValue equal to ${expected}.`);
+            }
+
+            testLastValue([]);
+            testLastValue([1, 2, 3]);
+        }
+    });
+
     suite.runTestCasesAndFinish();
 }
 </script>
index 5026c6c..d39cad9 100644 (file)
@@ -1,3 +1,43 @@
+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>
+
+        Reviewed by Timothy Hatcher.
+
+        Orginal patch by Matt Baker <mattbaker@apple.com>.
+
+        * UserInterface/Controllers/SelectionController.js:
+        (WI.SelectionController.prototype.selectItem):
+        Simplify internal logic by removing an early return.
+
+        (WI.SelectionController.prototype.selectItems): Added.
+        (WI.SelectionController.prototype.selectAll):
+        Provide a means to select multiple items in a single operation.
+        If `_lastSelectedItem` is not in the items to select, the last
+        item being selected will become the last selected item.
+
+        (WI.SelectionController.prototype._deselectAllAndSelect):
+        Drive-by fix: correct a logic error. If no items are selected, the item
+        passed as an argument should still become selected.
+
+        * UserInterface/Views/TreeOutline.js:
+        (WI.TreeOutline.prototype.selectTreeElements): Added.
+
+        * UserInterface/Views/DOMTreeElement.js:
+        (WI.DOMTreeElement):
+        (WI.DOMTreeElement.prototype.get closeTagTreeElement): Added.
+        (WI.DOMTreeElement.prototype._updateChildren):
+        Make the close tag `TreeElement` available from the open tag `TreeElement`.
+
+        * UserInterface/Views/DOMTreeOutline.js:
+        (WI.DOMTreeOutline.prototype.update):
+        Restore selected `TreeElement`s after updating.
+
+        * UserInterface/Base/Utilities.js:
+        * UserInterface/Test.html:
+
 2019-04-08  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: Elements tab: Classes toggle should use accent color on hover
index 174faab..2bd4fad 100644 (file)
@@ -166,6 +166,14 @@ Object.defineProperty(Set.prototype, "firstValue",
     }
 });
 
+Object.defineProperty(Set.prototype, "lastValue",
+{
+    get()
+    {
+        return Array.from(this.values()).lastValue;
+    }
+});
+
 Object.defineProperty(Set.prototype, "intersects",
 {
     value(other)
index 8b3219d..728b58d 100644 (file)
@@ -88,12 +88,6 @@ WI.SelectionController = class SelectionController extends WI.Object
         if (!this._allowsMultipleSelection)
             extendSelection = false;
 
-        if (this.hasSelectedItem(item)) {
-            if (!extendSelection)
-                this._deselectAllAndSelect(item);
-            return;
-        }
-
         this._lastSelectedItem = item;
         this._shiftAnchorItem = null;
 
@@ -103,6 +97,21 @@ WI.SelectionController = class SelectionController extends WI.Object
         this._updateSelectedItems(newItems);
     }
 
+    selectItems(items)
+    {
+        console.assert(this._allowsMultipleSelection, "Cannot select multiple items with multiple selection disabled.");
+        if (!this._allowsMultipleSelection)
+            return;
+
+        if (!this._lastSelectedItem || !items.has(this._lastSelectedItem))
+            this._lastSelectedItem = items.lastValue;
+
+        if (!this._shiftAnchorItem || !items.has(this._shiftAnchorItem))
+            this._shiftAnchorItem = this._lastSelectedItem;
+
+        this._updateSelectedItems(items);
+    }
+
     deselectItem(item)
     {
         console.assert(item, "Invalid item for selection.");
@@ -150,15 +159,11 @@ WI.SelectionController = class SelectionController extends WI.Object
         if (!this._allowsMultipleSelection)
             return;
 
-        this._lastSelectedItem = this._lastSelectableItem();
+        this.reset();
 
         let newItems = new Set;
-        this._addRange(newItems, this._firstSelectableItem(), this._lastSelectedItem);
-
-        if (!this._shiftAnchorItem)
-            this._shiftAnchorItem = this._lastSelectedItem;
-
-        this._updateSelectedItems(newItems);
+        this._addRange(newItems, this._firstSelectableItem(), this._lastSelectableItem());
+        this.selectItems(newItems);
     }
 
     deselectAll()
@@ -297,7 +302,7 @@ WI.SelectionController = class SelectionController extends WI.Object
 
     _deselectAllAndSelect(item)
     {
-        if (!this._selectedItems.size)
+        if (!this._selectedItems.size && !item)
             return;
 
         if (this._selectedItems.size === 1 && this.hasSelectedItem(item))
index 0c284a6..06f0be2 100644 (file)
     <script src="Views/Resizer.js"></script>
     <script src="Views/Table.js"></script>
     <script src="Views/TableColumn.js"></script>
+    <script src="Views/TreeElement.js"></script>
+    <script src="Views/TreeOutline.js"></script>
 
     <script type="text/javascript">
         WI.sharedApp = new WI.TestAppController;
index 724cc3e..9841431 100644 (file)
@@ -49,6 +49,7 @@ WI.DOMTreeElement = class DOMTreeElement extends WI.TreeElement
 
         this._highlightedAttributes = new Set;
         this._recentlyModifiedAttributes = new Map;
+        this._closeTagTreeElement = null;
 
         node.addEventListener(WI.DOMNode.Event.EnabledPseudoClassesChanged, this._nodePseudoClassesDidChange, this);
 
@@ -106,6 +107,8 @@ WI.DOMTreeElement = class DOMTreeElement extends WI.TreeElement
         }
     }
 
+    get closeTagTreeElement() { return this._closeTagTreeElement; }
+
     revealAndHighlight()
     {
         if (this._animatingHighlight)
@@ -459,6 +462,7 @@ WI.DOMTreeElement = class DOMTreeElement extends WI.TreeElement
         if (this._updateChildrenInProgress || !this.treeOutline._visible)
             return;
 
+        this._closeTagTreeElement = null;
         this._updateChildrenInProgress = true;
 
         var node = this.representedObject;
@@ -534,7 +538,7 @@ WI.DOMTreeElement = class DOMTreeElement extends WI.TreeElement
         // Insert closing tag tree element.
         var lastChild = this.children.lastValue;
         if (node.nodeType() === Node.ELEMENT_NODE && (!lastChild || !lastChild._elementCloseTag))
-            this.insertChildElement(this.representedObject, this.children.length, true);
+            this._closeTagTreeElement = this.insertChildElement(this.representedObject, this.children.length, true);
 
         // We want to restore the original selection and tree scroll position after a full refresh, if possible.
         if (fullRefresh && elementToSelect) {
index c6ed642..07efa41 100644 (file)
@@ -158,7 +158,7 @@ WI.DOMTreeOutline = class DOMTreeOutline extends WI.TreeOutline
         if (!this.rootDOMNode)
             return;
 
-        let selectedNode = this.selectedTreeElement ? this.selectedTreeElement.representedObject : null;
+        let selectedTreeElements = this.selectedTreeElements;
 
         this.removeChildren();
 
@@ -181,8 +181,24 @@ WI.DOMTreeOutline = class DOMTreeOutline extends WI.TreeOutline
             }
         }
 
-        if (selectedNode)
-            this._revealAndSelectNode(selectedNode, true);
+        if (!selectedTreeElements.length)
+            return;
+
+        // The selection cannot be restored from represented objects alone,
+        // since a closing tag DOMTreeElement has the same represented object
+        // as its parent.
+        selectedTreeElements = selectedTreeElements.map((oldTreeElement) => {
+            let treeElement = this.findTreeElement(oldTreeElement.representedObject);
+            if (treeElement && oldTreeElement.isCloseTag()) {
+                console.assert(treeElement.closeTagTreeElement, "Missing close tag TreeElement.", treeElement);
+                if (treeElement.closeTagTreeElement)
+                    treeElement = treeElement.closeTagTreeElement;
+            }
+            return treeElement;
+        });
+
+        this.selectTreeElements(selectedTreeElements);
+        this.selectedTreeElement.reveal();
     }
 
     updateSelectionArea()
index ccc9115..66ecd52 100644 (file)
@@ -711,6 +711,24 @@ WI.TreeOutline = class TreeOutline extends WI.Object
         // this is the root, do nothing
     }
 
+    selectTreeElements(treeElements)
+    {
+        if (!treeElements.length)
+            return;
+
+        if (treeElements.length === 1) {
+            this.selectedTreeElement = treeElements[0];
+            return;
+        }
+
+        console.assert(this.allowsMultipleSelection, "Cannot select TreeElements with multiple selection disabled.");
+        if (!this.allowsMultipleSelection)
+            return;
+
+        let selectableObjects = treeElements.map((treeElement) => this.objectForSelection(treeElement));
+        this._selectionController.selectItems(new Set(selectableObjects));
+    }
+
     get selectedTreeElementIndex()
     {
         if (!this.hasChildren || !this.selectedTreeElement)