(Follow-up to https://webkit.org/b/177477.) The Layers sidebar does DOM highlighting when a DataGridNode is selected (and the DataGrid is focused), but this is like nowhere else in Web Inspector. For consistency, we should highlight on *hover*.
Created attachment 322252 [details] Patch
Comment on attachment 322252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322252&action=review > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:134 > + if (!node || node === this._hoveredDataGridNode) As a precaution against having a highlight that stays visible even after the user has moved their mouse, I would recommend you also have a call to `WI.domTreeManager.hideDOMNodeHighlight();` inside an early-return for `if (!node)`. > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:148 > + if (!this._hoveredDataGridNode) Ditto (134).
I(In reply to Devin Rousso from comment #2) > Comment on attachment 322252 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322252&action=review > > > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:134 > > + if (!node || node === this._hoveredDataGridNode) > > As a precaution against having a highlight that stays visible even after the > user has moved their mouse, I would recommend you also have a call to > `WI.domTreeManager.hideDOMNodeHighlight();` inside an early-return for `if > (!node)`. > > > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:148 > > + if (!this._hoveredDataGridNode) > > Ditto (134). Is this a conceivable scenario? FWIW, the logic here is ripped straight from https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/DOMTreeDataGrid.js.
Hmm, perhaps DOMTreeDataGrid isn't even being used? And admittedly, DOMTreeOutline is almost the same, except does the thing you suggested: https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js#L295
Created attachment 322263 [details] Patch
Comment on attachment 322263 [details] 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; }
Created attachment 322470 [details] Patch
Comment on attachment 322470 [details] Patch Clearing flags on attachment: 322470 Committed r222801: <http://trac.webkit.org/changeset/222801>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34796203>