[webkit-reviews] review granted: [Bug 192059] Web Inspector: Elements tab should allow selecting/deleting multiple DOM nodes : [Attachment 355839] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 192059: Web Inspector: Elements tab should allow selecting/deleting
multiple DOM nodes
https://bugs.webkit.org/show_bug.cgi?id=192059

Attachment 355839: Patch

https://bugs.webkit.org/attachment.cgi?id=355839&action=review




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 355839
  --> https://bugs.webkit.org/attachment.cgi?id=355839
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355839&action=review

r=me, with a few followups:
 - deleting multiple DOM nodes will cause a significant number of errors to
appear from StyleDetailsPanel.js:77:
 - the styling of the child list bar (the one under the disclosure arrow) needs
to be adjusted to not contrast with multiple selections (e.g. selecting a
parent and it's children)
 - expanding/collapsing with arrow keys only affects the most recently selected
item
 - deleting an element that is hidden will revert the selection to the first
child of the root (e.g. select a child element, collapse the parent, hit
delete)
 - deleting an element will put the previous/next element (whichever is
available) into a permanently selected state until the user manually selects it
again
 - ⌘-A selects the details sidebar instead of the DOM tree
 - ER: I should be able to drag to start a selection after the end (or before
the beginning)

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:-197
> -	   this._adjustIndexesAfter(index - 1, 1);

Oh lol yeah this would've been an infinite loop 😅

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:67
> +	   this._domTreeOutline.allowsEmptySelection = false;

I think this is something we should do for all `WI.TreeOutline`, not just
`WI.DOMTreeOutline`.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:370
> -	   let indicatesTreeOutlineState = this.treeOutline &&
(this.treeOutline.dragOverTreeElement === this ||
this.treeOutline.selectedTreeElement === this || this._animatingHighlight);
> +	   let indicatesTreeOutlineState = this.treeOutline &&
(this.treeOutline.dragOverTreeElement === this || this.selected ||
this._animatingHighlight);

Are you sure that these are always equivalent?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:-613
> -	       if (!handled && this.treeOutline.ondelete)
> -		   handled =
this.treeOutline.ondelete(this.selectedTreeElement);

This is used by the breakpoints list in `WI.DebuggerSidebarPanel`.

    for (let treeElement of this.selectedTreeElements) {
	if (treeElement.ondelete && treeElement.ondelete())
	    handled = true;
    }

    if (!handled && this.treeOutline.ondelete)
	handled = this.treeOutline.ondelete(this.selectedTreeElement);