Created attachment 361211 [details] [HTML] Test Page ------- Inspected URL: file:///Users/mattbaker/Desktop/Test%20pages/delete-multiple-dom-nodes.html Loading completed: true Frontend User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_4) AppleWebKit/605.1.15 (KHTML, like Gecko) Uncaught Exceptions: - No node with given id found (at Connection.js:158:29) _dispatchResponseToPromise @ Connection.js:158:29 _dispatchResponse @ Connection.js:120:44 dispatch @ Connection.js:70:35 dispatchMessageFromTarget @ TargetManager.js:101:35 dispatchMessageFromTarget @ TargetObserver.js:42:51 dispatchEvent @ InspectorBackend.js:340:42 _dispatchEvent @ Connection.js:195:32 dispatch @ Connection.js:72:32 dispatch @ InspectorBackend.js:178:52 dispatchNextQueuedMessageFromBackend @ MessageDispatcher.js:42:34 ------- * STEPS TO REPRODUCE 1. Inspect test page 2. Elements tab 3. Select and expand DIV "parent-1" 4. Hit delete => Uncaught exception
<rdar://problem/47828647>
Created attachment 363703 [details] Patch
Comment on attachment 363703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363703&action=review r=me > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:307 > + this._treeElementsToRemove.sort((a, b) => getLevel(b) - getLevel(a)); Should we sort before calling `removeSelectedItems`? It should make performance slightly better since it organizes the array to be bottom-up based on the DOM and we check bottom-up inside `canSelectTreeElement`, meaning that the "lower" nodes are more likely to be earlier on in the array. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:330 > + return super.canSelectTreeElement(treeElement); Rather than only using this path if we aren't removing, I think we should flip this around. ``` if (!super.canSelectTreeElement(treeElement)) return false; let willRemoveAncestorOrSelf = false; if (this._treeElementsToRemove) { while (treeElement && !willRemoveAncestorOrSelf) { willRemoveAncestorOrSelf = this._treeElementsToRemove.includes(treeElement); treeElement = treeElement.parent; } } return !willRemoveAncestorOrSelf; ```
Created attachment 363803 [details] Patch for landing
Comment on attachment 363703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363703&action=review >> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:307 >> + this._treeElementsToRemove.sort((a, b) => getLevel(b) - getLevel(a)); > > Should we sort before calling `removeSelectedItems`? It should make performance slightly better since it organizes the array to be bottom-up based on the DOM and we check bottom-up inside `canSelectTreeElement`, meaning that the "lower" nodes are more likely to be earlier on in the array. The sort would need to go in SelectionController, and this is only needed for the DOMTreeOutline. If this path needs to be optimized in the future I will investigate then.
Comment on attachment 363803 [details] Patch for landing Clearing flags on attachment: 363803 Committed r242577: <https://trac.webkit.org/changeset/242577>
All reviewed patches have been landed. Closing bug.