Bug 192116

<rdar://problem/46344339>

Created attachment 357019 [details] Patch

It works pretty well, even for edge cases like having multiple selected end-tag TreeElements (</li> etc). I am seeing some asserts, and occasionally a new selected element cannot be found. Looking into the above issues now, but this is largely reviewable.

Comment on attachment 357019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357019&action=review r-, as I'd expect slightly different functionality Having a Delete > Nodes is a good idea :) > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:520 > + let selectedTreeElements = this.selectedTreeElements.filter(hasNoSelectedAncestors); This wouldn't have the effect I'd expect. If I've selected a parent and child, and press delete, I'd expect that they are no longer attached to the DOM tree _and_ that their parent-child relationship no longer exists. This would be testable by having both nodes saved as $x values (e.g. `$1.parentNode !== $2 && $1.parentNode === null`). Is the reason you're performing this check to ensure that the `WI.SelectionController` doesn't try to do funky things? You're already `removeSelectedItems()` before removing each `WI.DOMTreeElement` individually, so I don't think that's an issue there.

Comment on attachment 357019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357019&action=review In addition to the issue you brought up, there is another bug I just noticed that should be resolved. Given the following DOM with selected (*) nodes: <div> * <p> * <span> <b></b> </span> <p> <p></p> </div> When SelectionController looks for a new index for select in preparation for the removal of the selected <p> and <span>, it does so unaware of the parent-child relationships involved. It will select the index for <b>, which is going to be removed with its parent <span>. To resolve this, SelectionController.prototype.removeSelectedItems should use delegate methods `selectionControllerNextSelectableIndex` and `selectionControllerNextSelectableIndex` when determining an index to select. The delegate can skip children of items that are flagged for deletion. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:520 >> + let selectedTreeElements = this.selectedTreeElements.filter(hasNoSelectedAncestors); > > This wouldn't have the effect I'd expect. If I've selected a parent and child, and press delete, I'd expect that they are no longer attached to the DOM tree _and_ that their parent-child relationship no longer exists. This would be testable by having both nodes saved as $x values (e.g. `$1.parentNode !== $2 && $1.parentNode === null`). > > Is the reason you're performing this check to ensure that the `WI.SelectionController` doesn't try to do funky things? You're already `removeSelectedItems()` before removing each `WI.DOMTreeElement` individually, so I don't think that's an issue there. I agree with your assessment. The reason for not removing a child if its parent is going to be removed is to avoid having to sort nodes prior to removing. Removing a node from a subtree that's no longer attached to the root is an error. I'll rework this so that nodes are removed bottom-up.

Created attachment 357212 [details] Patch

Comment on attachment 357212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:274 > { > } I'm amazed this made it through review... > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284 > + let levelMap = new Map; Rather than do all of this work, isn't it just as valid to remove the elements in reverse order? The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue.

Comment on attachment 357212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review >> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284 >> + let levelMap = new Map; > > Rather than do all of this work, isn't it just as valid to remove the elements in reverse order? The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue. Oh, this might have an issue if the user has selected the closing tag of an expanded element. In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`).

Comment on attachment 357212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review >>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284 >>> + let levelMap = new Map; >> >> Rather than do all of this work, isn't it just as valid to remove the elements in reverse order? The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue. > > Oh, this might have an issue if the user has selected the closing tag of an expanded element. In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`). Great observation about reversing the array of TreeElements. I also really like the idea of passing the start tag DOMTreeElement.

Comment on attachment 357212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review >>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284 >>>> + let levelMap = new Map; >>> >>> Rather than do all of this work, isn't it just as valid to remove the elements in reverse order? The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue. >> >> Oh, this might have an issue if the user has selected the closing tag of an expanded element. In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`). > > Great observation about reversing the array of TreeElements. I also really like the idea of passing the start tag DOMTreeElement. Ah, but I think this simpler approach fails in the case where you have selected an element and the closing tag of its parent. If you have: <p> <span> * <b></b> * </span> </p> and you delete <b> and </span>: ondelete() { if (!this._editable) return false; let selectedTreeElements = this.selectedTreeElements.map((element) => element.isCloseTag ? this.findTreeElememnt(element.node) : element); selectedTreeElements.reverse(); this._selectionController.removeSelectedItems(); for (let treeElement of selectedTreeElements) treeElement.remove(); return true; } you would remove the parent before its child.

Comment on attachment 357212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357212&action=review >>>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:284 >>>>> + let levelMap = new Map; >>>> >>>> Rather than do all of this work, isn't it just as valid to remove the elements in reverse order? The list of selected indexes is already sorted, meaning that any selected parent-child "combo"s would be parent < child, so removing the `WI.TreeElement`s in reverse order should completely avoid the issue. >>> >>> Oh, this might have an issue if the user has selected the closing tag of an expanded element. In that case, we can leverage `WI.DOMTreeElement.prototype.isCloseTag` and search for the opening tag `WI.DOMTreeElement` (which should have the same `representedObject`), or even just pass the opening tag `WI.DOMTreeElement` to the closing tag `WI.DOMTreeElement` on creation (e.g. replace `elementCloseTag` with an actual `WI.DOMTreeElement` instead of a `boolean`). >> >> Great observation about reversing the array of TreeElements. I also really like the idea of passing the start tag DOMTreeElement. > > Ah, but I think this simpler approach fails in the case where you have selected an element and the closing tag of its parent. If you have: > > <p> > <span> > * <b></b> > * </span> > </p> > > and you delete <b> and </span>: > > ondelete() > { > if (!this._editable) > return false; > > let selectedTreeElements = this.selectedTreeElements.map((element) => element.isCloseTag ? this.findTreeElememnt(element.node) : element); > selectedTreeElements.reverse(); > > this._selectionController.removeSelectedItems(); > > for (let treeElement of selectedTreeElements) > treeElement.remove(); > > return true; > } > > you would remove the parent before its child. Disregard this comment.

Created attachment 357248 [details] Patch

Comment on attachment 357248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357248&action=review r=me, nice work :) > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:287 > + if (!levelMap.has(element)) { This performs two lookups, which can be avoided. function getLevel(treeElement) { let level = levelMap.get(treeElement); if (isNaN(level)) { level = 0; let current = treeElement; while (current = current.parent) ++level; levelMap.set(treeElement, level); } return level; } Style: you're reusing `level` as both a function name and variable name. Style: since we're expecting `WI.TreeElement` and not DOM nodes, the parameter should be `treeElement` instead of `element`. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:302 > + // same DOMNode can both be selected. NIT: I'd either include the `WI` or separate it into two names, for clarity as to whether you're referring to our model object or the builtin Node (e.g. `WI.DOMNode` vs DOM node).

I forgot to mention, but this case should _definitely_ have a test, as this is not an edge-case by any means. Either add it to this patch, or to <https://webkit.org/b/192044>.

Created attachment 357249 [details] Patch for landing

Comment on attachment 357249 [details] Patch for landing Clearing flags on attachment: 357249 Committed r239178: <https://trac.webkit.org/changeset/239178>

All reviewed patches have been landed. Closing bug.