Attachment 362538 Details for Bug 192681 – Patch

[patch] Patch

bug-192681-20190220140621.patch (text/plain), 20.31 KB, created by Matt Baker on 2019-02-20 14:06:21 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Matt Baker

Created: 2019-02-20 14:06:21 PST

Size: 20.31 KB

patch

obsolete

>Subversion Revision: 241788
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 276a49be0d138579541bc14fdf2d81804fb5c01b..5fb6f1bdbb7047de03033aa7abedf7e7e513a693 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,38 @@
>+2019-02-20  Matt Baker  <mattbaker@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 NOBODY (OOPS!).
>+
>+        * UserInterface/Base/Utilities.js:
>+        (value.defaultComparator):
>+        (value):
>+        Add a dedicated Set.prototype.maxValue, which can be more efficient
>+        than equivalent code using Array.from and Array.sort.
>+
>+        * UserInterface/Controllers/SelectionController.js:
>+        (WI.SelectionController.prototype.selectItems):
>+        Provide a means to select multiple items in a single operation.
>+        The existing `selectItem` method was not reimplemented in terms of this
>+        new method, in order to retain a dedicated efficient path for the more
>+        common single-selection operation.
>+
>+        (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/Test.html:
>+        New files for testing.
>+
>+        * UserInterface/Views/DOMTreeOutline.js:
>+        (WI.DOMTreeOutline.prototype.update):
>+        Restore selected TreeElements after updating.
>+
>+        * UserInterface/Views/TreeOutline.js:
>+        (WI.TreeOutline.prototype.selectTreeElements):
>+
> 2019-02-19  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Improve ES6 Class instances in Heap Snapshot instances view
>diff --git a/Source/WebInspectorUI/UserInterface/Base/Utilities.js b/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>index 4f28dccdb38d44e8082eaf2aece973c4949f257e..d261a30634f6242ecd153f01d6b4939319445d11 100644
>--- a/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>+++ b/Source/WebInspectorUI/UserInterface/Base/Utilities.js
>@@ -180,6 +180,40 @@ Object.defineProperty(Set.prototype, "isSubsetOf",
>     }
> });
> 
>+Object.defineProperty(Set.prototype, "maxValue",
>+{
>+    value(comparator)
>+    {
>+        function defaultComparator(a, b)
>+        {
>+            return a - b;
>+        }
>+        comparator = comparator || defaultComparator;
>+
>+        let iterator = this.values();
>+        let result = iterator.next().value;
>+        for (let value of iterator)
>+            if (comparator(result, value) < 0)
>+                result = value;
>+
>+        return result;
>+    }
>+});
>+
>+Object.defineProperty(Set.prototype, "union",
>+{
>+    value(other)
>+    {
>+        let result = new Set(other);
>+        if (other !== this) {
>+            for (let item of this)
>+                result.add(item);
>+        }
>+
>+        return result;
>+    }
>+});
>+
> Object.defineProperty(Node.prototype, "enclosingNodeOrSelfWithClass",
> {
>     value(className)
>diff --git a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>index 8b3219d8f3a989010ab6b3ad8cefd6a606343e2d..44222e70a5ca32cfbdf1f83953fc16be294e1af7 100644
>--- a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>+++ b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>@@ -103,6 +103,26 @@ WI.SelectionController = class SelectionController extends WI.Object
>         this._updateSelectedItems(newItems);
>     }
> 
>+    selectItems(items, extendSelection = false)
>+    {
>+        if (items.size === 1) {
>+            this.selectItem(items.firstValue, extendSelection);
>+            return;
>+        }
>+
>+        console.assert(this._allowsMultipleSelection, "Cannot extend selection with multiple selection disabled.");
>+        if (!this._allowsMultipleSelection)
>+            return;
>+
>+        let newItems = extendSelection ? this._selectedItems.union(items) : items;
>+        if (!this._lastSelectedItem || !newItems.has(this._lastSelectedItem))
>+            this._lastSelectedItem = newItems.maxValue(this._comparator);
>+
>+        this._shiftAnchorItem = null;
>+
>+        this._updateSelectedItems(newItems);
>+    }
>+
>     deselectItem(item)
>     {
>         console.assert(item, "Invalid item for selection.");
>@@ -297,7 +317,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))
>diff --git a/Source/WebInspectorUI/UserInterface/Test.html b/Source/WebInspectorUI/UserInterface/Test.html
>index 3c474c54aed0d188ece5654ed661f51dec2b2e18..b1cb1129fbe02398675238e54eca965d1b3d5251 100644
>--- a/Source/WebInspectorUI/UserInterface/Test.html
>+++ b/Source/WebInspectorUI/UserInterface/Test.html
>@@ -255,6 +255,8 @@
>     <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;
>diff --git a/Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js b/Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js
>index f544eb72e9a9ce5c667d2c74ad8b5c2b8c376402..5a2a2b7cc8b92d6c21282d3af26bbd311050b697 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js
>@@ -158,6 +158,7 @@ WI.DOMTreeOutline = class DOMTreeOutline extends WI.TreeOutline
>             return;
> 
>         let selectedNode = this.selectedTreeElement ? this.selectedTreeElement.representedObject : null;
>+        let selectedTreeElements = this.selectedTreeElements;
> 
>         this.removeChildren();
> 
>@@ -182,6 +183,21 @@ WI.DOMTreeOutline = class DOMTreeOutline extends WI.TreeOutline
> 
>         if (selectedNode)
>             this._revealAndSelectNode(selectedNode, true);
>+
>+        if (selectedTreeElements) {
>+            // 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 domNode = oldTreeElement.representedObject;
>+                let treeElement = this.findTreeElement(domNode);
>+                if (treeElement && oldTreeElement.isCloseTag())
>+                    treeElement = treeElement.children.lastValue;
>+                return treeElement;
>+            });
>+
>+            this.selectTreeElements(selectedTreeElements);
>+        }
>     }
> 
>     updateSelectionArea()
>diff --git a/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js b/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>index 98159e79cc9772e222b0f5d88e12bff165ebac30..aa7c300b7fae8b27bd0c7a16c80eb9ec81a4800a 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>@@ -703,6 +703,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((x) => this.objectForSelection(x));
>+        this._selectionController.selectItems(new Set(selectableObjects));
>+    }
>+
>     get selectedTreeElementIndex()
>     {
>         if (!this.hasChildren || !this.selectedTreeElement)
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index f1100e82f49ec8970a4f3fc6df47fc67cfe94c1c..9c0173b06507fafc57cec90e402f407ef346d496 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-02-20  Matt Baker  <mattbaker@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 NOBODY (OOPS!).
>+
>+        * inspector/tree-outline/tree-outline-selection-expected.txt: Added.
>+        * inspector/tree-outline/tree-outline-selection.html: Added.
>+        Add TreeOutline tests for single and multiple selection.
>+
>+        * inspector/unit-tests/set-utilities-expected.txt:
>+        * inspector/unit-tests/set-utilities.html:
>+        Update for new Set method.
>+
> 2019-02-19  Simon Fraser  <simon.fraser@apple.com>
> 
>         REGRESSION (r238090): Toggling visibility on the <html> element can result in a blank web view
>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
>index 0000000000000000000000000000000000000000..2b2bb1ce29aa59c7c4428cc9a0e4b29e64e93484
>--- /dev/null
>+++ b/LayoutTests/inspector/tree-outline/tree-outline-selection-expected.txt
>@@ -0,0 +1,43 @@
>+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.
>+
>+-- Running test case: TreeOutline.SelectedTreeElement
>+Selecting TreeElement "Item 1".
>+PASS: TreeOutline should have selected TreeElement "Item 1".
>+PASS: TreeElement "Item 1" should have selected = true.
>+PASS: Other TreeElements should have selected = false.
>+Selecting TreeElement "Item 2".
>+PASS: TreeOutline should have selected TreeElement "Item 2".
>+PASS: TreeElement "Item 2" should have selected = true.
>+PASS: Other TreeElements should have selected = false.
>+
>+-- Running test case: TreeOutline.AllowsMultipleSelection
>+PASS: allowsMultipleSelection enabled.
>+PASS: allowsMultipleSelection disabled.
>+
>+-- Running test case: TreeOutline.SelectTreeElements
>+Selecting TreeElements ["Item 1","Item 2"].
>+PASS: TreeOutline should have correct selected TreeElements.
>+PASS: TreeElements ["Item 1","Item 2"] should have selected = true.
>+PASS: All other TreeElements should have selected = false.
>+PASS: TreeOutline should have selected TreeElement "Item 2".
>+
>+Selecting TreeElements ["Item 3","Item 4"].
>+PASS: TreeOutline should have correct selected TreeElements.
>+PASS: TreeElements ["Item 3","Item 4"] should have selected = true.
>+PASS: All other TreeElements should have selected = false.
>+PASS: TreeOutline should have selected TreeElement "Item 4".
>+
>+-- 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
>index 0000000000000000000000000000000000000000..8b9f0ab74b7ca5a231a9796558538514d2446cf1
>--- /dev/null
>+++ b/LayoutTests/inspector/tree-outline/tree-outline-selection.html
>@@ -0,0 +1,184 @@
>+<!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)
>+                addObject(treeElement, child);
>+        }
>+
>+        let treeOutline = new WI.TreeOutline;
>+        for (let object of objects)
>+            appendObject(treeOutline, object);
>+
>+        return treeOutline;
>+    }
>+
>+    function expectSelectedTreeElement(treeOutline, treeElement) {
>+        InspectorTest.expectEqual(treeOutline.selectedTreeElement, treeElement, `TreeOutline should have selected TreeElement "${treeElement.title}".`);
>+    }
>+
>+    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.");
>+
>+            return true;
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "TreeOutline.FindTreeElement",
>+        description: "Find TreeElement for given represented object.",
>+        test() {
>+            const representedObjects = [
>+                {title: "Item 1"},
>+                {title: "Item 2"},
>+                {title: "Item 3"},
>+            ];
>+
>+            let treeOutline = createTreeOutline(representedObjects);
>+            let treeElement = treeOutline.findTreeElement(representedObjects[0]);
>+
>+            InspectorTest.expectThat(treeElement, "Should find TreeElement for represented object.")
>+            InspectorTest.expectEqual(treeElement.representedObject, representedObjects[0], "TreeElement should have correct represented object.")
>+
>+            return true;
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "TreeOutline.SelectedTreeElement",
>+        description: "Select a tree element, then select a different tree element.",
>+        test() {
>+            function triggerSelectTreeElement(treeElement) {
>+                InspectorTest.log(`Selecting TreeElement "${treeElement.title}".`);
>+                treeOutline.selectedTreeElement = treeElement;
>+
>+                expectSelectedTreeElement(treeOutline, treeElement);
>+                InspectorTest.expectThat(treeElement.selected, `TreeElement "${treeElement.title}" should have selected = true.`);
>+
>+                let otherTreeElementsAreDeselected = treeOutline.children.every((x) => x === treeElement || !x.selected);
>+                InspectorTest.expectThat(otherTreeElementsAreDeselected, "Other TreeElements should have selected = false.");
>+            }
>+
>+            const representedObjects = [
>+                {title: "Item 1"},
>+                {title: "Item 2"},
>+            ];
>+
>+            let treeOutline = createTreeOutline(representedObjects);
>+
>+            triggerSelectTreeElement(treeOutline.children[0]);
>+            triggerSelectTreeElement(treeOutline.children[1]);
>+
>+            return true;
>+        }
>+    });
>+
>+    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.");
>+
>+            return true;
>+        }
>+    });
>+
>+    function triggerSelectTreeElements(treeOutline, treeElements) {
>+        let displayText = `${treeElements.map((x) => `"${x.title}"`)}`;
>+
>+        InspectorTest.log(`Selecting TreeElements [${displayText}].`);
>+        treeOutline.selectTreeElements(treeElements);
>+
>+        InspectorTest.expectShallowEqual(treeOutline.selectedTreeElements, treeElements, "TreeOutline should have correct selected TreeElements.");
>+
>+        let treeElementsSelected = treeElements.every((x) => x.selected);
>+        InspectorTest.expectThat(treeElementsSelected, `TreeElements [${displayText}] should have selected = true.`);
>+
>+        let otherTreeElementsNotSelected = treeOutline.children.every((x) => !x.selected || treeElements.includes(x));
>+        InspectorTest.expectThat(otherTreeElementsNotSelected, "All other TreeElements should have selected = false.");
>+    }
>+
>+    suite.addTestCase({
>+        name: "TreeOutline.SelectTreeElements",
>+        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]]);
>+            expectSelectedTreeElement(treeOutline, children[1]);
>+
>+            InspectorTest.log("");
>+
>+            triggerSelectTreeElements(treeOutline, [children[2], children[3]]);
>+            expectSelectedTreeElement(treeOutline, children[3]);
>+
>+            return true;
>+        }
>+    });
>+
>+    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.")
>+
>+            return true;
>+        }
>+    });
>+
>+    suite.runTestCasesAndFinish();
>+}
>+</script>
>+</head>
>+<body onLoad="runTest()">
>+    <p>Tests for WI.TreeOutline selection.</p>
>+</body>
>+</html>
>diff --git a/LayoutTests/inspector/unit-tests/set-utilities-expected.txt b/LayoutTests/inspector/unit-tests/set-utilities-expected.txt
>index b8da910bbeab3daa45c578bc7feeb03b5b77e9db..b2b3502bb94f00251cf465284fefe71af2b3f7f8 100644
>--- a/LayoutTests/inspector/unit-tests/set-utilities-expected.txt
>+++ b/LayoutTests/inspector/unit-tests/set-utilities-expected.txt
>@@ -41,3 +41,9 @@ 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.maxValue
>+PASS: Set with values [] should have maximum value undefined.
>+PASS: Set with values [1] should have maximum value 1.
>+PASS: Set with values [1,2,0] should have maximum value 2.
>+PASS: Set with values [b,c,a] should have maximum value c.
>+
>diff --git a/LayoutTests/inspector/unit-tests/set-utilities.html b/LayoutTests/inspector/unit-tests/set-utilities.html
>index db7140c12a423b6663e673e5a37ac4ff9acccd6f..d61de196caa768e435ebfdd1b5c7db0a90b1ed7a 100644
>--- a/LayoutTests/inspector/unit-tests/set-utilities.html
>+++ b/LayoutTests/inspector/unit-tests/set-utilities.html
>@@ -138,6 +138,22 @@ function test()
>         }
>     });
> 
>+    suite.addTestCase({
>+        name: "Set.prototype.maxValue",
>+        test() {
>+            function testMaxValue(values, comparator) {
>+                let set = new Set(values);
>+                const expectedMax = values.sort(comparator).lastValue;
>+                InspectorTest.expectEqual(set.maxValue(comparator), expectedMax, `Set with values [${Array.from(set)}] should have maximum value ${expectedMax}.`);
>+            }
>+
>+            testMaxValue([]);
>+            testMaxValue([1]);
>+            testMaxValue([1, 2, 0]);
>+            testMaxValue(["b", "c", "a"], (a, b) => a.localeCompare(b));
>+        }
>+    });
>+
>     suite.runTestCasesAndFinish();
> }
> </script>

Actions: View | Formatted Diff | Diff

Attachments on bug 192681: 362538 | 362563 | 363813 | 365941 | 365945 | 366063 | 366864 | 367164