Bug 177690 – Web Inspector: Layers tab sidebar's DOM highlight should be by row hover, not row selection

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Ross Kirsling]

(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*.

[Ross Kirsling]

Created attachment 322252 [details]
Patch

[Devin Rousso]

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).

[Ross Kirsling]

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.

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 322263 [details]
Patch

[Devin Rousso]

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;
    }

[Ross Kirsling]

Created attachment 322470 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322470 [details]
Patch

Clearing flags on attachment: 322470

Committed r222801: <http://trac.webkit.org/changeset/222801>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34796203>