Bug 101150 – Web Inspector: Console: hovering node wrappers in object tree should highlight them on the page

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Nikita Vasilyev]

Screencast: http://www.screenr.com/sYs7

[Pavel Feldman]

I wanted to do this for quite some time now. Do you have a patch for it?

[Nikita Vasilyev]

No. I can start working on it.

[Pavel Feldman]

Feel free to, I have no code for it yet. You should look at the code for hovering over node, then you should repeat it for ObjectPropertySection elements. The only difference is that you would need to resolve RemoteObjects with type "node" into DOMNodes (use RemoteObject.prototype.pushNodeToFrontend for that). Then existing node highlight routines will work.

[Nikita Vasilyev]

Created attachment 173862 [details]
WIP

Demo: http://www.screenr.com/TJH7

This is what I want except it shouldn’t have ► and shouldn’t be expandable.

It terms of code, it doesn’t look right. ConsoleMessageImpl contains all formatters, but they should also be used in Sources (Watch Expressions and Scope Variables). Thus, formatters doesn’t belong to ConsoleMessageImpl.

[Pavel Feldman]

I don't think we want it this way in the UI. There is a "Reveil in Elements panel" action available on it, so one should use it. Otherwise we end up with a mixture of representations in one tree. I think the title of the bug states it the right way: we only want highlight upon hovering the object.

[Nikita Vasilyev]

(In reply to comment #5)
> I don't think we want it this way in the UI. There is a "Reveil in Elements panel" action available on it, so one should use it. Otherwise we end up with a mixture of representations in one tree.

I think the same. I just don’t yet know how to code it properly. The code responsible for hovering elements is in WebInspector.ElementsTreeOutline. I need only one element instead of a tree.

[Pavel Feldman]

> I think the same. I just don’t yet know how to code it properly. The code responsible for hovering elements is in WebInspector.ElementsTreeOutline. I need only one element instead of a tree.

Comment #3 should give you a clue. Or I can steal it from you.

[Nikita Vasilyev]

(In reply to comment #7)
> > I think the same. I just don’t yet know how to code it properly. The code responsible for hovering elements is in WebInspector.ElementsTreeOutline. I need only one element instead of a tree.
> 
> Comment #3 should give you a clue. Or I can steal it from you.

Are you suggesting not using WebInspector.ElementsTreeOutline and just duplicate hover logic?

[Nikita Vasilyev]

(In reply to comment #7)
> Or I can steal it from you.

No, wait, I almost got it :)

[Pavel Feldman]

> Are you suggesting not using WebInspector.ElementsTreeOutline and just duplicate hover logic?

I don't think ElementsTreeOutline is involved. You want to highlight elements in the screen, not on the elements panel, right? If there is some code to be extracted, it can go into DOMAgent, but I doubt there is any.

[Nikita Vasilyev]

(In reply to comment #10)
> > Are you suggesting not using WebInspector.ElementsTreeOutline and just duplicate hover logic?
> 
> I don't think ElementsTreeOutline is involved. You want to highlight elements in the screen, not on the elements panel, right?

Right. Although, I do want to jump to elements panel by either left-clicking the element or from the context menu (as it currently work for ElementsTreeOutline).

If you know how to implement it really easy, then do steal it from me.

[Pavel Feldman]

(In reply to comment #11)
> (In reply to comment #10)
> > > Are you suggesting not using WebInspector.ElementsTreeOutline and just duplicate hover logic?
> > 
> > I don't think ElementsTreeOutline is involved. You want to highlight elements in the screen, not on the elements panel, right?
> 
> Right. Although, I do want to jump to elements panel by either left-clicking the element or from the context menu (as it currently work for ElementsTreeOutline).
> 

This one is already there. Context menu -> "Reveal in Elements Panel".

> If you know how to implement it really easy, then do steal it from me.

Rest could be done as in Comment #3.

[Nikita Vasilyev]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > I don't think ElementsTreeOutline is involved. You want to highlight elements in the screen, not on the elements panel, right?
> > 
> > Right. Although, I do want to jump to elements panel by either left-clicking the element or from the context menu (as it currently work for ElementsTreeOutline).
> > 
> 
> This one is already there. Context menu -> "Reveal in Elements Panel".

Nope. http://www.screenr.com/RiH7

"Reveal in Elements Panel" implemented in WebInspector.ElementsTreeOutline.prototype._contextMenuEventFired. I’m pretty sure ElementsTreeOutline should be involved.

[Nikita Vasilyev]

Created attachment 174020 [details]
WIP 2

http://www.screenr.com/vIH7

I ended following your advice and not using WebInspector.ElementsTreeOutline directly. However, I abstracted _buildTagDOM and _buildAttributeDOM from WebInspector.ElementsTreeElement.prototype to WebInspector.DOMPresentationUtils so I can use them in WebInspector.ObjectPropertyTreeElement.prototype.update.

[Pavel Feldman]

> Nope. http://www.screenr.com/RiH7
> 

It is a regression. It works from the Elements panel's properties sidebar, but not from the console. Console's context menu has {b:document.body} object as a target when you invoke it. I'll fix it.

[Pavel Feldman]

> It is a regression. It works from the Elements panel's properties sidebar, but not from the console. Console's context menu has {b:document.body} object as a target when you invoke it. I'll fix it.

Fix landed as http://trac.webkit.org/changeset/134595. Take a look at it :)

[Pavel Feldman]

Comment on attachment 174020 [details]
WIP 2

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

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:251
> +                var fragment = WebInspector.DOMPresentationUtils.buildTagDOM(node, false, true);

Please do not render object as DOM. As I pointed out earlier, Object view only shows Object representation.

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:274
> +            this.property.value.pushNodeToFrontend(printNode);

You should only push node upon hover. Otherwise you are at risk of pushing all DOM at once with no good reason.

[Nikita Vasilyev]

(In reply to comment #17)
> > Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:274
> > +            this.property.value.pushNodeToFrontend(printNode);
> 
> You should only push node upon hover. Otherwise you are at risk of pushing all DOM at once with no good reason.

I’m showing all an element with all its attributes. How do I get this information without using pushNodeToFrontend?

[Nikita Vasilyev]

(In reply to comment #17)
> (From update of attachment 174020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174020&action=review
> 
> > Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:251
> > +                var fragment = WebInspector.DOMPresentationUtils.buildTagDOM(node, false, true);
> 
> Please do not render object as DOM. As I pointed out earlier, Object view only shows Object representation.

I’m not really following. An object may contains elements, that’s what the bug all about. 

(In reply to comment #18)
> (In reply to comment #17)
> > > Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:274
> > > +            this.property.value.pushNodeToFrontend(printNode);
> > 
> > You should only push node upon hover. Otherwise you are at risk of pushing all DOM at once with no good reason.
> 
> I’m showing all an element with all its attributes. How do I get this information without using pushNodeToFrontend?

WebInspector.RemoteObject.prototype.description returns no attributes, e.g. <body> when it actually <body class="logged_in page-dashboard macintosh  env-production ">

[Pavel Feldman]

> I’m not really following. An object may contains elements, that’s what the bug all about. 

Oh, I think the bug title is misleading. Updated.

[Pavel Feldman]

Created attachment 176012 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 176012 [details]
Patch

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

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:249
> +            WebInspector.DOMPresentationUtils.createSpansForNodeTitle(this.valueElement, this.property.value.description);

s/this.property.value.description/description/

> Source/WebCore/inspector/front-end/externs.js:68
> +window.isUnderTest = false;

is that related?

[WebKit Review Bot]

Comment on attachment 176012 [details]
Patch

Clearing flags on attachment: 176012

Committed r136144: <http://trac.webkit.org/changeset/136144>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.