[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
bugzilla-daemon at webkit.org
Mon Oct 2 15:59:04 PDT 2017
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;
}
More information about the webkit-reviews
mailing list