WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
101150
Web Inspector: Console: hovering node wrappers in object tree should highlight them on the page
https://bugs.webkit.org/show_bug.cgi?id=101150
Summary
Web Inspector: Console: hovering node wrappers in object tree should highligh...
Nikita Vasilyev
Reported
2012-11-03 10:05:43 PDT
Screencast:
http://www.screenr.com/sYs7
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Pavel Feldman
Comment 1
2012-11-03 10:14:03 PDT
I wanted to do this for quite some time now. Do you have a patch for it?
Nikita Vasilyev
Comment 2
2012-11-03 10:15:53 PDT
No. I can start working on it.
Pavel Feldman
Comment 3
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.
Nikita Vasilyev
Comment 4
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.
Pavel Feldman
Comment 5
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.
Nikita Vasilyev
Comment 6
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.
Pavel Feldman
Comment 7
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.
Nikita Vasilyev
Comment 8
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?
Nikita Vasilyev
Comment 9
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 :)
Pavel Feldman
Comment 10
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.
Nikita Vasilyev
Comment 11
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.
Pavel Feldman
Comment 12
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
.
Nikita Vasilyev
Comment 13
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.
Nikita Vasilyev
Comment 14
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.
Pavel Feldman
Comment 15
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.
Pavel Feldman
Comment 16
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 :)
Pavel Feldman
Comment 17
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.
Nikita Vasilyev
Comment 18
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?
Nikita Vasilyev
Comment 19
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 ">
Pavel Feldman
Comment 20
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.
Pavel Feldman
Comment 21
2012-11-26 09:00:00 PST
Created
attachment 176012
[details]
Patch
Vsevolod Vlasov
Comment 22
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?
WebKit Review Bot
Comment 23
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
>
WebKit Review Bot
Comment 24
2012-11-29 12:00:08 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug