Bug 101150 - Web Inspector: Console: hovering node wrappers in object tree should highlight them on the page
: Web Inspector: Console: hovering node wrappers in object tree should highligh...
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-11-03 10:05 PST by
Modified: 2012-11-29 12:00 PST (History)


Attachments
WIP (1.35 KB, patch)
2012-11-13 04:39 PST, Nikita Vasilyev
no flags Review Patch | Details | Formatted Diff | Diff
WIP 2 (13.36 KB, patch)
2012-11-13 16:11 PST, Nikita Vasilyev
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.79 KB, patch)
2012-11-26 09:00 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-11-03 10:05:43 PST
Screencast: http://www.screenr.com/sYs7
------- Comment #1 From 2012-11-03 10:14:03 PST -------
I wanted to do this for quite some time now. Do you have a patch for it?
------- Comment #2 From 2012-11-03 10:15:53 PST -------
No. I can start working on it.
------- Comment #3 From 2012-11-03 10:36:19 PST -------
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.
------- Comment #4 From 2012-11-13 04:39:51 PST -------
Created an attachment (id=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.
------- Comment #5 From 2012-11-13 05:35:03 PST -------
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.
------- Comment #6 From 2012-11-13 05:42:32 PST -------
(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.
------- Comment #7 From 2012-11-13 06:22:41 PST -------
> 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.
------- Comment #8 From 2012-11-13 06:24:50 PST -------
(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?
------- Comment #9 From 2012-11-13 06:34:09 PST -------
(In reply to comment #7)
> Or I can steal it from you.

No, wait, I almost got it :)
------- Comment #10 From 2012-11-13 06:34:54 PST -------
> 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.
------- Comment #11 From 2012-11-13 09:16:58 PST -------
(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.
------- Comment #12 From 2012-11-13 10:37:54 PST -------
(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.
------- Comment #13 From 2012-11-13 11:21:13 PST -------
(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.
------- Comment #14 From 2012-11-13 16:11:02 PST -------
Created an attachment (id=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.
------- Comment #15 From 2012-11-13 18:17:11 PST -------
> 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.
------- Comment #16 From 2012-11-14 05:22:49 PST -------
> 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 :)
------- Comment #17 From 2012-11-14 05:24:21 PST -------
(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.

> 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.
------- Comment #18 From 2012-11-14 05:37:43 PST -------
(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?
------- Comment #19 From 2012-11-14 05:45:25 PST -------
(In reply to comment #17)
> (From update of attachment 174020 [details] [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 ">
------- Comment #20 From 2012-11-14 05:48:51 PST -------
> 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.
------- Comment #21 From 2012-11-26 09:00:00 PST -------
Created an attachment (id=176012) [details]
Patch
------- Comment #22 From 2012-11-29 10:51:03 PST -------
(From update of attachment 176012 [details])
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?
------- Comment #23 From 2012-11-29 12:00:03 PST -------
(From update of attachment 176012 [details])
Clearing flags on attachment: 176012

Committed r136144: <http://trac.webkit.org/changeset/136144>
------- Comment #24 From 2012-11-29 12:00:08 PST -------
All reviewed patches have been landed.  Closing bug.