Attachment 356402 Details for Bug 192091 – Patch for landing

[patch] Patch for landing

bug-192091-20181203135657.patch (text/plain), 7.25 KB, created by Matt Baker on 2018-12-03 13:57:03 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Matt Baker

Created: 2018-12-03 13:57:03 PST

Size: 7.25 KB

patch

obsolete

>Subversion Revision: 238804
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 55e1def12249965bbfc5c8d4f3a414d5643602dd..2e1b078e7d580ee8c231815a513b812e9a80dc25 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-12-03  Matt Baker  <mattbaker@apple.com>
>+
>+        Web Inspector: REGRESSION(r238599): Multiple Selection: restoring selection when opening WebInspector puts the TreeElement into a permanent selected state
>+        https://bugs.webkit.org/show_bug.cgi?id=192091
>+        <rdar://problem/46321795>
>+
>+        Reviewed by Devin Rousso.
>+
>+        * UserInterface/Controllers/SelectionController.js:
>+        (WI.SelectionController.prototype.didInsertItem):
>+        Fix a bug where selected indexes were overwritten by the inserted index.
>+
>+        * UserInterface/Views/TreeOutline.js:
>+        (WI.TreeOutline):
>+        (WI.TreeOutline.prototype.insertChild):
>+        Update the SelectionController with the newly inserted index before
>+        attaching the TreeElement. Attaching the TreeElement can cause it to
>+        become selected, which would add the index to the SelectionController,
>+        only to have it immediately incremented by the call to `didInsertItem`.
>+        Additionally, change `insertionIndex` to be the index of the inserted
>+        item instead of the inserted item's previous sibling.
>+
>+        (WI.TreeOutline.prototype._rememberTreeElement):
>+        (WI.TreeOutline.prototype._forgetTreeElement):
>+        (WI.TreeOutline.prototype._indexOfTreeElement.previousElement): Deleted.
>+        Eliminate TreeElement index caching, which could become stale and cause
>+        the wrong index to be calculated. Additionally, instead of walking up the
>+        parent chain to determine the index, start at the root and use existing
>+        method `traverseNextTreeElement`.
>+
> 2018-11-30  Nikita Vasilyev  <nvasilyev@apple.com>
> 
>         Web Inspector: Jumping from Computed to Styles should select property
>diff --git a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>index 945690581b7f875bf113bdb3754df52fe2aeb70c..ea9e3bff5a4fc2e91c57a735ea858862498ae308 100644
>--- a/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>+++ b/Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js
>@@ -203,8 +203,8 @@ WI.SelectionController = class SelectionController extends WI.Object
>     {
>         let current = this._selectedIndexes.lastIndex;
>         while (current >= index) {
>-            this._selectedIndexes.delete(index);
>-            this._selectedIndexes.add(index + 1);
>+            this._selectedIndexes.delete(current);
>+            this._selectedIndexes.add(current + 1);
> 
>             current = this._selectedIndexes.indexLessThan(current);
>         }
>diff --git a/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js b/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>index 61a3c4187dca322326fad759880354b8ec62a59a..a7511d6d55ea50b127e886b2bfd695b46ae4dfe0 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/TreeOutline.js
>@@ -56,7 +56,6 @@ WI.TreeOutline = class TreeOutline extends WI.Object
> 
>         this._cachedNumberOfDescendents = 0;
>         this._selectionController = new WI.SelectionController(this);
>-        this._treeElementIndexCache = new Map;
> 
>         this._itemWasSelectedByUser = false;
>         this._processingSelectionChange = false;
>@@ -295,6 +294,12 @@ 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();
> 
>@@ -303,9 +308,6 @@ WI.TreeOutline = class TreeOutline extends WI.Object
> 
>         if (isFirstChild && this.expanded)
>             this.expand();
>-
>-        let insertionIndex = this.treeOutline._indexOfTreeElement(child.previousSibling) || 0;
>-        this.treeOutline._selectionController.didInsertItem(insertionIndex);
>     }
> 
>     removeChildAtIndex(childIndex, suppressOnDeselect, suppressSelectSibling)
>@@ -395,9 +397,6 @@ WI.TreeOutline = class TreeOutline extends WI.Object
> 
>     _rememberTreeElement(element)
>     {
>-        this._treeElementIndexCache.clear();
>-        this._cachedNumberOfDescendents++;
>-
>         if (!this._knownTreeElements[element.identifier])
>             this._knownTreeElements[element.identifier] = [];
> 
>@@ -408,19 +407,19 @@ WI.TreeOutline = class TreeOutline extends WI.Object
> 
>         // add the element
>         elements.push(element);
>+        this._cachedNumberOfDescendents++;
>     }
> 
>     _forgetTreeElement(element)
>     {
>-        this._treeElementIndexCache.clear();
>-        this._cachedNumberOfDescendents--;
>-
>         if (this.selectedTreeElement === element) {
>             element.deselect(true);
>             this.selectedTreeElement = null;
>         }
>-        if (this._knownTreeElements[element.identifier])
>+        if (this._knownTreeElements[element.identifier]) {
>             this._knownTreeElements[element.identifier].remove(element, true);
>+            this._cachedNumberOfDescendents--;
>+        }
>     }
> 
>     _forgetChildrenRecursive(parentElement)
>@@ -993,34 +992,22 @@ WI.TreeOutline = class TreeOutline extends WI.Object
> 
>     _indexOfTreeElement(treeElement)
>     {
>-        function previousElement(element) {
>-            if (element.previousSibling) {
>-                element = element.previousSibling;
>-                if (element.children.length)
>-                    element = element.children.lastValue;
>-            } else
>-                element = element.parent && element.parent.root ? null : element.parent;
>-            return element;
>-        }
>+        const skipUnrevealed = false;
>+        const stayWithin = null;
>+        const dontPopulate = true;
> 
>         let index = 0;
>-        let current = treeElement;
>+        let current = this.children[0];
>         while (current) {
>-            let closestIndex = this._treeElementIndexCache.get(current);
>-            if (!isNaN(closestIndex)) {
>-                index += closestIndex;
>-                break;
>-            }
>+            if (treeElement === current)
>+                return index;
> 
>-            current = previousElement(current);
>-            if (current)
>-                index++;
>+            current = current.traverseNextTreeElement(skipUnrevealed, stayWithin, dontPopulate);
>+            ++index;
>         }
> 
>-        if (!this._treeElementIndexCache.has(treeElement))
>-            this._treeElementIndexCache.set(treeElement, index);
>-
>-        return index;
>+        console.assert(false, "Unable to get index for tree element.", treeElement, treeOutline);
>+        return NaN;
>     }
> 
>     _treeElementAtIndex(index)

Actions: View | Formatted Diff | Diff

Attachments on bug 192091: 355917 | 356347 | 356402