181800 – Web Inspector: Elements tab should have "Jump to Layer" functionality

RESOLVED FIXED 181800

Web Inspector: Elements tab should have "Jump to Layer" functionality

https://bugs.webkit.org/show_bug.cgi?id=181800

Summary Web Inspector: Elements tab should have "Jump to Layer" functionality

Ross Kirsling

Reported 2018-01-18 10:01:58 PST

Background ---------- According to JoePeck, one way the apple.com folks would use the legacy Layers sidebar is to quickly check how memory consumption is affected by modifying an element's CSS. Although keeping two versions of the Layers sidebar around is a nonstarter, the fact that we can jump from layer to element but not from element to layer is a definite gap in functionality. Proposal -------- Add "Jump to Layer" functionality to the Elements tab. I figure this could be a nav bar button or a context menu item (or both?). To avoid having to redundantly retrieve all the layer data on the Elements tab, this can be always-enabled and simply jump to the leafmost containing layer.

Attachments
Patch (7.45 KB, patch) 2018-01-18 13:48 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (10.15 KB, patch) 2018-01-19 15:51 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (9.96 KB, patch) 2018-01-22 19:44 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (9.94 KB, patch) 2018-02-01 22:44 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (10.10 KB, patch) 2018-02-06 18:24 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2018-01-18 13:48:00 PST

Created attachment 331657 [details] Patch

Joseph Pecoraro

Comment 2 2018-01-18 15:11:38 PST

Comment on attachment 331657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331657&action=review > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:205 > + if (!options.excludeRevealElement && domNode.ownerDocument) { Should this have a: if (!WI.isShowingElementsTab()) { ... } > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:211 > + if (window.LayerTreeAgent && domNode.parentNode) { This strikes me as unusual since an element may or may not have layers and I don't think a parent is necessary (<html>). What happens if this node doesn't have a layer, it finds the parent that does? > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:212 > + contextMenu.appendItem(WI.UIString("Reveal in Layers Tab"), () => { Should this have a: if (!WI.isShowingLayersTab()) { ... } > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:132 > + let layer = null; > + while (node && !layer) { > + layer = layerForNodeId(node.id); > + if (!layer) > + node = node.parentNode; > + } > + > + this.selectLayerById(layer.layerId); Is it possible that layer is null?

Ross Kirsling

Comment 3 2018-01-18 15:19:57 PST

Note that a page always has a document layer, so there's no danger in finding a containing layer for <html>. (_nodeToSelect appears to be a guard for the case where the layer data hasn't been loaded yet though.)

Devin Rousso

Comment 4 2018-01-19 15:51:16 PST

Created attachment 331802 [details] Patch

Ross Kirsling

Comment 5 2018-01-19 17:37:04 PST

Comment on attachment 331802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331802&action=review Works as advertised -- thanks for taking this on! :D > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:129 > + let layerForNodeId = (nodeId) => { > + for (let layer of this._layers) { > + if (layer.nodeId === nodeId) > + return layer; > + } > + return null; > + }; This is just Array#find, so you can write `this._layers.find(layer => layer.nodeId === node.id);` below. > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:136 > + while (node && !layer) { > + layer = layerForNodeId(node.id); > + if (!layer) > + node = node.parentNode; > + } Even though this is safe in practice, the way it reads does make one wonder about the "!node" case... My Friday evening brain isn't coming up with a beautiful alternative solution though.

Devin Rousso

Comment 6 2018-01-22 19:44:38 PST

Created attachment 331997 [details] Patch

Ross Kirsling

Comment 7 2018-01-31 09:27:19 PST

This is fine from my perspective (unless we want to add a `console.assert(layer);`) -- just need r+ from someone with authority to give it. :)

Joseph Pecoraro

Comment 8 2018-01-31 10:57:16 PST

Comment on attachment 331997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331997&action=review > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:205 > + if (!options.excludeRevealElement && window.DOMAgent && !WI.isShowingElementsTab() && domNode.ownerDocument) { Hmm, I may have mislead on the isShowingElementsTab / WI.isShowingLayersTab here. Because I think you could log an element in the console, and want to show that in either location: js> document.body <body class="make-me-a-layer> And issuing a context menu on this node in the Console should show both "Reveal in DOM Tree" and "Reveal in Layers Tab" no matter what tab is showing, right? Maybe it does? My guess is this was solved with those "exclude" options. And only the DOM Tree includes excludeRevealElement?

Devin Rousso

Comment 9 2018-02-01 22:39:44 PST

Comment on attachment 331997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331997&action=review >> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:205 >> + if (!options.excludeRevealElement && window.DOMAgent && !WI.isShowingElementsTab() && domNode.ownerDocument) { > > Hmm, I may have mislead on the isShowingElementsTab / WI.isShowingLayersTab here. Because I think you could log an element in the console, and want to show that in either location: > > js> document.body > <body class="make-me-a-layer> > > And issuing a context menu on this node in the Console should show both "Reveal in DOM Tree" and "Reveal in Layers Tab" no matter what tab is showing, right? Maybe it does? > > My guess is this was solved with those "exclude" options. And only the DOM Tree includes excludeRevealElement? You are correct that DOMTreeOutline sets that to true, specifically from DOMTreeContentView. I do remember adding `excludeRevealElement`, but I didn't remember why. Now I remember :P

Devin Rousso

Comment 10 2018-02-01 22:44:38 PST

Created attachment 332940 [details] Patch

Joseph Pecoraro

Comment 11 2018-02-01 23:01:06 PST

Comment on attachment 332940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332940&action=review r=me > Source/WebInspectorUI/ChangeLog:11 > + (WI.isShowingElementsTab): Both of these isShowing*Tab are unused by this patch, but fine to keep. > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:130 > + this.selectLayerById(layer.layerId); I still think we should do: console.assert(layer, "Should always be at least some top level layer"); if (!layer) return; Maybe this is possible with an element inside of a Shadow DOM?

Devin Rousso

Comment 12 2018-02-06 18:24:14 PST

Created attachment 333248 [details] Patch

WebKit Commit Bot

Comment 13 2018-02-06 19:20:06 PST

Comment on attachment 333248 [details] Patch Clearing flags on attachment: 333248 Committed r228215: <https://trac.webkit.org/changeset/228215>

WebKit Commit Bot

Comment 14 2018-02-06 19:20:07 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 15 2018-02-06 19:22:06 PST

<rdar://problem/37298738>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Devin Rousso

Reported

2018-01-18 10:01 PST

Modified

2018-02-06 19:22 PST History

CC List

5 users Show

URL

Keywords InRadar

Depends on

Blocks

174176

Dependencies

tree graph