Bug 101150 - Web Inspector: Console: hovering node wrappers in object tree should highlight them on the page
Summary: Web Inspector: Console: hovering node wrappers in object tree should highligh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-03 10:05 PDT by Nikita Vasilyev
Modified: 2012-11-29 12:00 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2012-11-03 10:05:43 PDT
Screencast: http://www.screenr.com/sYs7
Comment 1 Pavel Feldman 2012-11-03 10:14:03 PDT
I wanted to do this for quite some time now. Do you have a patch for it?
Comment 2 Nikita Vasilyev 2012-11-03 10:15:53 PDT
No. I can start working on it.
Comment 3 Pavel Feldman 2012-11-03 10:36:19 PDT
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 Nikita Vasilyev 2012-11-13 04:39:51 PST
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.
Comment 5 Pavel Feldman 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 Nikita Vasilyev 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 Pavel Feldman 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 Nikita Vasilyev 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 Nikita Vasilyev 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 Pavel Feldman 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 Nikita Vasilyev 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 Pavel Feldman 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 Nikita Vasilyev 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 Nikita Vasilyev 2012-11-13 16:11:02 PST
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.
Comment 15 Pavel Feldman 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 Pavel Feldman 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 Pavel Feldman 2012-11-14 05:24:21 PST
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.
Comment 18 Nikita Vasilyev 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 Nikita Vasilyev 2012-11-14 05:45:25 PST
(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 ">
Comment 20 Pavel Feldman 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 Pavel Feldman 2012-11-26 09:00:00 PST
Created attachment 176012 [details]
Patch
Comment 22 Vsevolod Vlasov 2012-11-29 10:51:03 PST
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?
Comment 23 WebKit Review Bot 2012-11-29 12:00:03 PST
Comment on attachment 176012 [details]
Patch

Clearing flags on attachment: 176012

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