192917 2018-12-19 22:47:13 -0800 Web Inspector: REGRESSION: deleting an audit puts selection in a selected but invisible state 2019-01-11 13:02:36 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 hi mattbaker commit-queue hi inspector-bugzilla-changes mattbaker webkit-bug-importer oldest_to_newest 1489748 0 hi 2018-12-19 22:47:13 -0800 # STEPS TO REPRODUCE: 1. open WebInspector 2. enable the Audits tab 3. add the default audits (assuming they aren't already there) 4. recursively expand all default audits so everything is visible 5. select the first top-level default audit ("Demo Audit") 6. press delete ⌫ => the first top-level default audit ("Demo Audit") will disappear, and the selection styles will apply to the new first top-level default audit ("Accessibility") (which was the second top-level default audit) 7. press delete ⌫ => nothing happens 8. press up ↑ => the "valid-tabindex" test becomes selected, while the first top-level default audit ("Accessibility") is also selected 1489904 1 webkit-bug-importer 2018-12-20 10:13:49 -0800 <rdar://problem/46875285> 1490643 2 358026 mattbaker 2018-12-22 15:57:07 -0800 Created attachment 358026 Patch 1491477 3 358026 hi 2019-01-03 11:34:40 -0800 Comment on attachment 358026 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? 1491505 4 mattbaker 2019-01-03 13:18:04 -0800 (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); } } 1491599 5 hi 2019-01-03 22:24:57 -0800 (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); } 1491883 6 mattbaker 2019-01-04 14:10:41 -0800 (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. 1493607 7 358026 hi 2019-01-10 21:26:00 -0800 Comment on attachment 358026 Patch r=me, after re-reading the code the `root` check makes more sense 1493763 8 358026 commit-queue 2019-01-11 13:02:35 -0800 Comment on attachment 358026 Patch Clearing flags on attachment: 358026 Committed r239870: <https://trac.webkit.org/changeset/239870> 1493764 9 commit-queue 2019-01-11 13:02:36 -0800 All reviewed patches have been landed. Closing bug. 358026 2018-12-22 15:57:07 -0800 2019-01-11 13:02:35 -0800 Patch bug-192917-20181222155706.patch text/plain 5318 mattbaker U3VidmVyc2lvbiBSZXZpc2lvbjogMjM5NTEzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCAw ZTQ2ODQ0NTg3ZDVhMzYzNjM5N2FhZDFjOWQxM2MxZGNjOGM4ZGI3Li5lNmQ1ZDNjYzZjNWFlYmMz MWIwYmNlZDM0NzRmNzFkMTRhNTU4ZGVmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyNiBAQAorMjAxOC0xMi0yMiAgTWF0dCBCYWtlciAgPG1hdHRiYWtlckBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogUkVHUkVTU0lPTjogZGVsZXRpbmcgYW4gYXVkaXQg cHV0cyBzZWxlY3Rpb24gaW4gYSBzZWxlY3RlZCBidXQgaW52aXNpYmxlIHN0YXRlCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTI5MTcKKyAgICAgICAg PHJkYXI6Ly9wcm9ibGVtLzQ2ODc1Mjg1PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFNlbGVjdGlvbkNvbnRyb2xsZXIgc2hvdWxkIG5vdCBiZSBub3Rp ZmllZCBvZiByZW1vdmVkIGNoaWxkcmVuIHVudGlsIHRoZQorICAgICAgICBjaGlsZCBpdGVtcyBo YXZlIGJlZW4gcmVtb3ZlZCBmcm9tIHRoZSBUcmVlT3V0bGluZS4gRG9pbmcgc28gYXQgdGhpcyBz dGFnZQorICAgICAgICBpcyB1bnNhZmUsIHNpbmNlIHRoaXMgbWV0aG9kIGNoZWNrcyBgdGhpcy5z ZWxlY3RlZFRyZWVFbGVtZW50YCwgd2hpY2ggY291bGQKKyAgICAgICAgcmV0dXJuIHRoZSBhZGp1 c3RlZCBpbmRleCBmcm9tIHRoZSBTZWxlY3Rpb25Db250cm9sbGVyIGJlZm9yZSBhbnl0aGluZyBo YXMKKyAgICAgICAgYWN0dWFsbHkgYmVlbiByZW1vdmVkIGZyb20gdGhlIFRyZWVPdXRsaW5lLgor CisgICAgICAgIFRoZSBudW1iZXIgb2YgY2FsbHMgdG8gU2VsZWN0aW9uQ29udHJvbGxlci5wcm90 b3R5cGUuZGlkUmVtb3ZlSXRlbXMgaXMgYWxzbworICAgICAgICByZWR1Y2VkIHNvbWV3aGF0LCBz aW5jZSB3ZSdyZSBubyBsb25nZXIgY2FsbGluZyBpdCBmb3IgZXZlcnkgVHJlZUVsZW1lbnQuCisK KyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1RyZWVPdXRsaW5lLmpzOgorICAgICAgICAo V0kuVHJlZU91dGxpbmUucHJvdG90eXBlLnJlbW92ZUNoaWxkQXRJbmRleCk6CisgICAgICAgIChX SS5UcmVlT3V0bGluZS5wcm90b3R5cGUucmVtb3ZlQ2hpbGRyZW4pOgorICAgICAgICAoV0kuVHJl ZU91dGxpbmUucHJvdG90eXBlLl9mb3JnZXRUcmVlRWxlbWVudCk6CisgICAgICAgIChXSS5UcmVl T3V0bGluZS5wcm90b3R5cGUuX2luZGV4ZXNGb3JTdWJ0cmVlKTogQWRkZWQuICAgICAgICAKKwog MjAxOC0xMi0yMCAgTmlraXRhIFZhc2lseWV2ICA8bnZhc2lseWV2QGFwcGxlLmNvbT4KIAogICAg ICAgICBXZWIgSW5zcGVjdG9yOiBEYXJrIE1vZGU6IFR5cGUgcHJvZmlsZXIgcG9wb3ZlcnMgaGF2 ZSBibGFjayB0ZXh0IG9uIGRhcmsgYmFja2dyb3VuZApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYklu c3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvVHJlZU91dGxpbmUuanMgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9UcmVlT3V0bGluZS5qcwppbmRleCAyMjg0 MGExZTQzNWUzNzViMjEzNWZmMDEzOTVmOWJlNGIyMmVjYmQ4Li45YTAxODNlN2Q2Y2FlMmUwMTcx NzAwMjI3YzI3YmVjNTQ0ZGQ4NjhkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkv VXNlckludGVyZmFjZS9WaWV3cy9UcmVlT3V0bGluZS5qcworKysgYi9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9UcmVlT3V0bGluZS5qcwpAQCAtMzI5LDYgKzMyOSw4 IEBAIFdJLlRyZWVPdXRsaW5lID0gY2xhc3MgVHJlZU91dGxpbmUgZXh0ZW5kcyBXSS5PYmplY3QK ICAgICAgICAgICAgIHRyZWVPdXRsaW5lLl9mb3JnZXRDaGlsZHJlblJlY3Vyc2l2ZShjaGlsZCk7 CiAgICAgICAgIH0KIAorICAgICAgICBsZXQgcmVtb3ZlZEluZGV4ZXMgPSB0aGlzLl9pbmRleGVz Rm9yU3VidHJlZShjaGlsZCk7CisKICAgICAgICAgaWYgKGNoaWxkLnByZXZpb3VzU2libGluZykK ICAgICAgICAgICAgIGNoaWxkLnByZXZpb3VzU2libGluZy5uZXh0U2libGluZyA9IGNoaWxkLm5l eHRTaWJsaW5nOwogICAgICAgICBpZiAoY2hpbGQubmV4dFNpYmxpbmcpCkBAIC0zNDIsOCArMzQ0 LDEwIEBAIFdJLlRyZWVPdXRsaW5lID0gY2xhc3MgVHJlZU91dGxpbmUgZXh0ZW5kcyBXSS5PYmpl Y3QKICAgICAgICAgY2hpbGQubmV4dFNpYmxpbmcgPSBudWxsOwogICAgICAgICBjaGlsZC5wcmV2 aW91c1NpYmxpbmcgPSBudWxsOwogCi0gICAgICAgIGlmICh0cmVlT3V0bGluZSkKKyAgICAgICAg aWYgKHRyZWVPdXRsaW5lKSB7CisgICAgICAgICAgICB0cmVlT3V0bGluZS5fc2VsZWN0aW9uQ29u dHJvbGxlci5kaWRSZW1vdmVJdGVtcyhyZW1vdmVkSW5kZXhlcyk7CiAgICAgICAgICAgICB0cmVl T3V0bGluZS5kaXNwYXRjaEV2ZW50VG9MaXN0ZW5lcnMoV0kuVHJlZU91dGxpbmUuRXZlbnQuRWxl bWVudFJlbW92ZWQsIHtlbGVtZW50OiBjaGlsZH0pOworICAgICAgICB9CiAgICAgfQogCiAgICAg cmVtb3ZlQ2hpbGQoY2hpbGQsIHN1cHByZXNzT25EZXNlbGVjdCwgc3VwcHJlc3NTZWxlY3RTaWJs aW5nKQpAQCAtMzc4LDE2ICszODIsMjAgQEAgV0kuVHJlZU91dGxpbmUgPSBjbGFzcyBUcmVlT3V0 bGluZSBleHRlbmRzIFdJLk9iamVjdAogICAgICAgICAgICAgICAgIHRyZWVPdXRsaW5lLl9mb3Jn ZXRDaGlsZHJlblJlY3Vyc2l2ZShjaGlsZCk7CiAgICAgICAgICAgICB9CiAKKyAgICAgICAgICAg IGxldCByZW1vdmVkSW5kZXhlcyA9IHRyZWVPdXRsaW5lLl9pbmRleGVzRm9yU3VidHJlZShjaGls ZCk7CisKICAgICAgICAgICAgIGNoaWxkLl9kZXRhY2goKTsKICAgICAgICAgICAgIGNoaWxkLnRy ZWVPdXRsaW5lID0gbnVsbDsKICAgICAgICAgICAgIGNoaWxkLnBhcmVudCA9IG51bGw7CiAgICAg ICAgICAgICBjaGlsZC5uZXh0U2libGluZyA9IG51bGw7CiAgICAgICAgICAgICBjaGlsZC5wcmV2 aW91c1NpYmxpbmcgPSBudWxsOwogCi0gICAgICAgICAgICBpZiAodHJlZU91dGxpbmUpCi0gICAg ICAgICAgICAgICAgdHJlZU91dGxpbmUuZGlzcGF0Y2hFdmVudFRvTGlzdGVuZXJzKFdJLlRyZWVP dXRsaW5lLkV2ZW50LkVsZW1lbnRSZW1vdmVkLCB7ZWxlbWVudDogY2hpbGR9KTsKLQogICAgICAg ICAgICAgdGhpcy5jaGlsZHJlbi5zaGlmdCgpOworCisgICAgICAgICAgICBpZiAodHJlZU91dGxp bmUpIHsKKyAgICAgICAgICAgICAgICB0cmVlT3V0bGluZS5fc2VsZWN0aW9uQ29udHJvbGxlci5k aWRSZW1vdmVJdGVtcyhyZW1vdmVkSW5kZXhlcyk7CisgICAgICAgICAgICAgICAgdHJlZU91dGxp bmUuZGlzcGF0Y2hFdmVudFRvTGlzdGVuZXJzKFdJLlRyZWVPdXRsaW5lLkV2ZW50LkVsZW1lbnRS ZW1vdmVkLCB7ZWxlbWVudDogY2hpbGR9KTsKKyAgICAgICAgICAgIH0KICAgICAgICAgfQogICAg IH0KIApAQCAtNDE5LDEwICs0MjcsNiBAQCBXSS5UcmVlT3V0bGluZSA9IGNsYXNzIFRyZWVPdXRs aW5lIGV4dGVuZHMgV0kuT2JqZWN0CiAgICAgICAgICAgICB0aGlzLnNlbGVjdGVkVHJlZUVsZW1l bnQgPSBudWxsOwogICAgICAgICB9CiAgICAgICAgIGlmICh0aGlzLl9rbm93blRyZWVFbGVtZW50 c1tlbGVtZW50LmlkZW50aWZpZXJdKSB7Ci0gICAgICAgICAgICBsZXQgaW5kZXggPSB0aGlzLl9p bmRleE9mVHJlZUVsZW1lbnQoZWxlbWVudCk7Ci0gICAgICAgICAgICBpZiAoaW5kZXggPj0gMCkK LSAgICAgICAgICAgICAgICB0aGlzLl9zZWxlY3Rpb25Db250cm9sbGVyLmRpZFJlbW92ZUl0ZW1z KG5ldyBXSS5JbmRleFNldChbaW5kZXhdKSk7Ci0KICAgICAgICAgICAgIHRoaXMuX2tub3duVHJl ZUVsZW1lbnRzW2VsZW1lbnQuaWRlbnRpZmllcl0ucmVtb3ZlKGVsZW1lbnQsIHRydWUpOwogICAg ICAgICAgICAgdGhpcy5fY2FjaGVkTnVtYmVyT2ZEZXNjZW5kZW50cy0tOwogICAgICAgICB9CkBA IC0xMDcyLDYgKzEwNzYsMzkgQEAgV0kuVHJlZU91dGxpbmUgPSBjbGFzcyBUcmVlT3V0bGluZSBl eHRlbmRzIFdJLk9iamVjdAogCiAgICAgICAgIHRoaXMuZGlzcGF0Y2hFdmVudFRvTGlzdGVuZXJz KFdJLlRyZWVPdXRsaW5lLkV2ZW50LlNlbGVjdGlvbkRpZENoYW5nZSwge3NlbGVjdGVkQnlVc2Vy fSk7CiAgICAgfQorCisgICAgX2luZGV4ZXNGb3JTdWJ0cmVlKHRyZWVFbGVtZW50KQorICAgIHsK KyAgICAgICAgbGV0IHRyZWVPdXRsaW5lID0gdHJlZUVsZW1lbnQudHJlZU91dGxpbmU7CisgICAg ICAgIGlmICghdHJlZU91dGxpbmUpCisgICAgICAgICAgICByZXR1cm4gbmV3IFdJLkluZGV4U2V0 OworCisgICAgICAgIGxldCBmaXJzdENoaWxkID0gdHJlZUVsZW1lbnQuY2hpbGRyZW5bMF07Cisg ICAgICAgIGlmICghZmlyc3RDaGlsZCkKKyAgICAgICAgICAgIHJldHVybiBuZXcgV0kuSW5kZXhT ZXQ7CisKKyAgICAgICAgbGV0IHN0YXJ0SW5kZXggPSB0cmVlT3V0bGluZS5faW5kZXhPZlRyZWVF bGVtZW50KGZpcnN0Q2hpbGQpOworICAgICAgICBsZXQgZW5kSW5kZXggPSBzdGFydEluZGV4Owor CisgICAgICAgIGNvbnN0IHNraXBVbnJldmVhbGVkID0gZmFsc2U7CisgICAgICAgIGNvbnN0IHN0 YXlXaXRoaW4gPSB0cmVlRWxlbWVudDsKKyAgICAgICAgY29uc3QgZG9udFBvcHVsYXRlID0gdHJ1 ZTsKKworICAgICAgICBsZXQgY3VycmVudCA9IGZpcnN0Q2hpbGQ7CisgICAgICAgIHdoaWxlIChj dXJyZW50ID0gY3VycmVudC50cmF2ZXJzZU5leHRUcmVlRWxlbWVudChza2lwVW5yZXZlYWxlZCwg c3RheVdpdGhpbiwgZG9udFBvcHVsYXRlKSkKKyAgICAgICAgICAgIGVuZEluZGV4Kys7CisKKyAg ICAgICAgLy8gSW5jbHVkZSB0aGUgaW5kZXggb2YgdGhlIHN1YnRyZWUncyByb290LCB1bmxlc3Mg aXQncyB0aGUgVHJlZU91dGxpbmUgcm9vdC4KKyAgICAgICAgaWYgKCF0cmVlRWxlbWVudC5yb290 KQorICAgICAgICAgICAgc3RhcnRJbmRleC0tOworCisgICAgICAgIGxldCBjb3VudCA9IGVuZElu ZGV4IC0gc3RhcnRJbmRleCArIDE7CisKKyAgICAgICAgbGV0IGluZGV4ZXMgPSBuZXcgV0kuSW5k ZXhTZXQ7CisgICAgICAgIGluZGV4ZXMuYWRkUmFuZ2Uoc3RhcnRJbmRleCwgY291bnQpOworCisg ICAgICAgIHJldHVybiBpbmRleGVzOworICAgIH0KIH07CiAKIFdJLlRyZWVPdXRsaW5lLl9zdHls ZUVsZW1lbnQgPSBudWxsOwo=