[webkit-reviews] review granted: [Bug 177690] Web Inspector: Layers tab sidebar's DOM highlight should be by row hover, not row selection : [Attachment 322263] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 177690: Web Inspector: Layers tab sidebar's DOM highlight should be by row
hover, not row selection
https://bugs.webkit.org/show_bug.cgi?id=177690

Attachment 322263: Patch

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




--- Comment #6 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 322263
  --> https://bugs.webkit.org/attachment.cgi?id=322263
Patch

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

r=me.  Looks good, with a few minor comments.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:96
> +	   this._dataGrid.element.addEventListener("mouseout",
this._dataGridMouseOut.bind(this), false);

I think you want to use "mouseleave" instead.  "mouseout" will fire every time
the mouse leaves any child element, of which there are a lot in DataGrid.  This
case is already covered by your "mousemove" implementation, so you only need to
add a listener for when the mouse leaves the DataGrid itself, not any of its
children.

<https://developer.mozilla.org/en-US/docs/Web/Events/mouseleave>

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:139
> +	   if (node === this._hoveredDataGridNode)

This check should probably happen before the if on (134), as otherwise we are
firing unnecessary `WI.domTreeManager.hideDOMNodeHighlight` calls.

    let node = this._dataGrid.dataGridNodeFromNode(event.target);
    if (node === this._hoveredDataGridNode)
	return;

    if (!node) {
	this._hideDOMNodeHighlight();
	return;
    }