Summary: | Web Inspector: REGRESSION: deleting an audit puts selection in a selected but invisible state | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, mattbaker, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Devin Rousso
2018-12-19 22:47:13 PST
Created attachment 358026 [details]
Patch
Comment on attachment 358026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358026&action=review r-, as I think there's a simpler (and safer) approach to `removeChildren`, namely just entirely wiping out the `_selectionController`'s list of `_selectedIndexes`. Since we're removing all children from the `WI.TreeOutline`, there shouldn't be anything left to select, meaning that all we really need to do is call `this._selectionController.deselectAll`. > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1103 > + // Include the index of the subtree's root, unless it's the TreeOutline root. > + if (!treeElement.root) > + startIndex--; Also, why is this needed? None of the other index-related functions care about the `root`? This should either be removed or have a better comment. Also, AFACIT, the root is only set on `WI.TreeOutline`, so I don't see how this would ever not get called? (In reply to Devin Rousso from comment #3) > Comment on attachment 358026 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358026&action=review > > r-, as I think there's a simpler (and safer) approach to `removeChildren`, > namely just entirely wiping out the `_selectionController`'s list of > `_selectedIndexes`. Since we're removing all children from the > `WI.TreeOutline`, there shouldn't be anything left to select, meaning that > all we really need to do is call `this._selectionController.deselectAll`. The reason we can't do this is that `this` can be a TreeElement: class TreeElement { removeChildren() { return WI.TreeOutline.prototype.removeChildren.apply(this, arguments); } } (In reply to Matt Baker from comment #4) > The reason we can't do this is that `this` can be a TreeElement: > > class TreeElement > { > removeChildren() { return WI.TreeOutline.prototype.removeChildren.apply(this, arguments); } > } Ah, right. I forgot that we do that. At the very least then, we could create a union of the indexes for all the children and then just call it once. removeChildren(suppressOnDeselect) { let removedIndexes = new IndexSet; while (this.children.length) { let child = this.children[0]; child.deselect(suppressOnDeselect); let treeOutline = child.treeOutline; if (treeOutline) { treeOutline._forgetTreeElement(child); treeOutline._forgetChildrenRecursive(child); } removedIndexes = removedIndexes.union(treeOutline._indexesForSubtree(child)); child._detach(); child.treeOutline = null; child.parent = null; child.nextSibling = null; child.previousSibling = null; this.children.shift(); if (treeOutline) treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementRemoved, {element: child}); } if (this.treeOutline) this.treeOutline._selectionController.didRemoveItems(removedIndexes); } (In reply to Devin Rousso from comment #5) > (In reply to Matt Baker from comment #4) > > The reason we can't do this is that `this` can be a TreeElement: > > > > class TreeElement > > { > > removeChildren() { return WI.TreeOutline.prototype.removeChildren.apply(this, arguments); } > > } > > Ah, right. I forgot that we do that. At the very least then, we could > create a union of the indexes for all the children and then just call it > once. My concern with this approach is that ElementRemoved listeners would then execute before the SelectionController is updated. Comment on attachment 358026 [details]
Patch
r=me, after re-reading the code the `root` check makes more sense
Comment on attachment 358026 [details] Patch Clearing flags on attachment: 358026 Committed r239870: <https://trac.webkit.org/changeset/239870> All reviewed patches have been landed. Closing bug. |