Closed Bug 757866 Opened 12 years ago Closed 10 years ago

Highlight and jump to nodes from the Console

Categories

(DevTools :: Console, defect, P3)

Product:
DevTools
Component:
Console
Version:
15 Branch
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 30

People

(Reporter: groovecoder, Assigned: pbro)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [highlighter])

Attachments

(2 files, 9 obsolete files)

bug757866-highlight-and-select-domnodes-in-console v7.patch
39.19 KB, patch
pbro
: review+
Details | Diff | Splinter Review
bug980835-2.diff
4.55 KB, patch
Details | Diff | Splinter Review 
document.getElementById('wikiArticle') returns the object in the console. It would be great to also highlight the object in the inspector if the inspector is also open.
(In reply to Luke Crouch [:groovecoder] from comment #0)
> document.getElementById('wikiArticle') returns the object in the console. It
> would be great to also highlight the object in the inspector if the
> inspector is also open.

But what should happen if you are evaluating an expression that has more than one of those?

As an aside, in the experimental Developer Toolbar (flip devtools.toolbar.enabled to get it), there is a command to do that: "inspect #wikiArticle". This opens the page inspector and selects the element. A reference to that element is then present in the web console's $0 variable, so you can use it for any further tests.
(In reply to Panos Astithas [:past] from comment #1)
> (In reply to Luke Crouch [:groovecoder] from comment #0)
> > document.getElementById('wikiArticle') returns the object in the console. It
> > would be great to also highlight the object in the inspector if the
> > inspector is also open.
> 
> But what should happen if you are evaluating an expression that has more
> than one of those?

I'm thinking that what Luke is asking for here is the same behavior you see in other tools. Anywhere a DOM node appears in console output, if you hover over that console output, the node is highlighted. If you evaluate an expression that returns multiple nodes, each one in the list is highlightable. For our tools, it likely makes sense to only do that highlighting when the page inspector is open.

You're right, though, that the command line in the toolbar may be a convenient replacement for certain uses of this feature.
Kevin nailed it - show all the elements, and highlight each in the inspector when the user hovers over it.
Forgot about this bug when I filed bug 865694.

We need to disentangle the highlighter from the inspector so we can use it elsewhere. Calling this a P3 for now.

Filter on GUNGNIR.
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Summary: integrate console with inspector → Highlight nodes on hovering objects in the Console
Whiteboard: [highlighter]
Depends on: 843004
Should not only highlight the element, but be able to jump to the inspector tab.
Summary: Highlight nodes on hovering objects in the Console → Highlight and jump to nodes from the Console
I'm going to make this depend on bug 916443 for the highlighting part, cause that bug is going to make it a lot easier since it brings the inspector front and highlighter front at toolbox level.
As for jumping to the inspector tab on click, that should be quite straightforward with 916443 too: making sure the inspector is loaded first, setting the current selection to the node, then switching panels.
Depends on: 916443
Blocks: 952277
Mihai, do you mind sharing your views on how we could go from the text-based outputs (bug 843004) to a richer type of output, which would support HTML widgets?

In the case of this bug, it would be necessary to output DOMNodes in containers so we can listen to mousemove/out (to highlight in the content page) and perhaps add an icon to jump to the inspector (which is what was done in bug 952277).
Patrick, the work needs to happen in browser/devtools/webconsole/console-output.js. See Messages.Extended.prototype._renderBodyPiece(). |piece| can be an object actor. You will see the Widgets.JSObject widget is used to display the object actor if it's not a primitive. I suggest the addition of a Widgets.DOMElement. Check if piece.preview.kind == "DOMNode" and that piece.preview.nodeType is ELEMENT_NODE.

For the new widget, pretty much copy/paste JSObject. In render() you might want to not use VariablesView.getString(). See how VariablesView.stringifiers.byObjectKind.DOMNode does it for elements - render() should do something similar. Just add some <span> tags around tag name, attribute name, and value. Set some codemirror class names. That should make it syntax highlight the output. (The .cm-* classes are available in console output)

For hover you can add your methods to the DOMElement widget.
Wow! Thanks Mihai for the very detailed explanation.
It sounds pretty easy then.

Once the DOMElement widget is in place, the method to be called on mouseover would be: toolbox.highlighterUtils.highlightDomValueGrip(piece);

In order to jump to the inspector and select the node, we shouldn't override the click event listener since that's supposed to open the variablesview already, instead, on mouseover, we should probably display an icon just like we do in the variablesview, and clicking on that icon would then jump to the inspector.
We could pretty much copy the code found here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm#2723
Assignee: nobody → pbrosset
Mihai, after a quick investigation, it seems that if I do that, then nodes previewed inside array-like objects won't work.
For instance, if you enter document.querySelectorAll("div") in the console, the output will show the NodeList as an array that contains DOMNodes.

So it's a little bit tricky, because we rely on the VariablesView.stringifiers to get the output, and it works great because it stringifies DOMNodes recursively inside arrays or objects.
But it outputs a string only which we can't reuse to highlight nodes in the page anymore.

If I rework the Widgets.JSObject widget so that it recursively checks values and also calls Widgets.DOMElement if it finds DOMNodes in its items, then we're essentially duplicating some of the logic of VariablesView.stringifiers.

The other solution is to do the code in the VariablesView.stringifiers of course, so that it would also work in the variablesview (if the ArrayLike's concise flag isn't set to true, which I don't know when is the case), but it'd be strange considering the name stringifiers implies it generates a string only.

Thing is, I like the fact that _renderBodyPiece delegates almost the whole process to VariablesView.stringifiers, that simplifies maintenance and ensures consistency among our various panels (and the VariablesView appears in more and more places), so I'd really be tempted to do this in the stringifiers part, and perhaps rename it to something else to make it obvious that it not only outputs strings.
Flags: needinfo?(mihai.sucan)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> Mihai, after a quick investigation, it seems that if I do that, then nodes
> previewed inside array-like objects won't work.
> For instance, if you enter document.querySelectorAll("div") in the console,
> the output will show the NodeList as an array that contains DOMNodes.
> 
> So it's a little bit tricky, because we rely on the
> VariablesView.stringifiers to get the output, and it works great because it
> stringifies DOMNodes recursively inside arrays or objects.
> But it outputs a string only which we can't reuse to highlight nodes in the
> page anymore.
> 
> ...

Known issue. In bug 843004 we've added APIs for easy custom messages. Due to time limitations, we did not add the APIs for customizing output for each kind of object needed to be rendered.

This is why I started work for bug 584733 where we are adding a bit of APIs for that purpose. You can base your patch on top of that work. You only need to add a new widget, Widgets.ObjectRenderers.byKind.DOMNode. this.options.concise is set to true when the widget needs to render the DOM node in an array/object preview.

I would keep the stringifiers as they are, because rich output has different needs and use-cases. Also, Message and Widget objects from console-output.js can be reused with minimal work outside of the web console - they can be reused to output console API messages and object actors.
Flags: needinfo?(mihai.sucan)
Ok that makes sense, thanks for the clarifications.
I'll start work on top of bug 584733.
WIP patch.

For now, it only adds a _renderElementNode method to the Widgets.ObjectRenderers.byKind.DOMNode class in case the ObjectActor.preview.nodeType is Ci.nsIDOMNode.ELEMENT_NODE

The next step is to add mouseover/out event listeners to the element generated in the output and use these to highlight/unhighlight the corresponding element in the page.

Mihai: to do this, I need a reference to the Toolbox instance. I can normally do this from the WebConsoleFrame via "let toolbox = gDevTools.getToolbox(this.owner.target);" but I'm wondering how to best do this from the widget. Should I be passing a reference of the target when instantiating a new Messages.something() ?
Flags: needinfo?(mihai.sucan)
Here's something that seems to work: 
From a Widget: this.message.output.owner.owner.target
part 2: highlights nodes on mouseover/out
Screenshot: https://www.dropbox.com/s/3fgs95xhyyjdfka/Screenshot%202014-02-25%2016.28.03.png

2 open questions:
- getting a reference to the target (to get the toolbox) is done with this.message.output.owner.owner.target
- widgets are not destroyed when the output is cleared today. Not a big deal in other cases, but if we're adding event listeners, we'd better start cleaning after ourselves.
Mihai suggested that, since Message has a .widgets property (a Set), it would be easy to iterate over that and call widget.destroy in the Message's detroy function.
Part 1 - Simple syntax highlighting for DOM nodes.
Attachment #8381397 - Attachment is obsolete: true
Attachment #8381426 - Attachment is obsolete: true
Attachment #8381618 - Flags: review?(mihai.sucan)
Hey, thanks for working on this patch!


(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #17)
> Created attachment 8381426 [details] [diff] [review]
> bug757866-part2-highlight_domnodes_in_page_from_console_output WIP.patch
> 
> part 2: highlights nodes on mouseover/out
> Screenshot:
> https://www.dropbox.com/s/3fgs95xhyyjdfka/Screenshot%202014-02-25%2016.28.03.
> png
> 
> 2 open questions:
> - getting a reference to the target (to get the toolbox) is done with
> this.message.output.owner.owner.target

I suggest adding a getter for target to ConsoleOutput, so we don't have to use that long path to the target in Widgets/Messages. Maybe name the getter toolboxTarget.


> - widgets are not destroyed when the output is cleared today. Not a big deal
> in other cases, but if we're adding event listeners, we'd better start
> cleaning after ourselves.

As discussed in the meeting, Message.destroy() can be used to destroy from the this.widgets Set. You will need to add your widget instance to messageInstance.widgets.
Highlight DOM nodes in the page on mouseover/out.
Open inspector on click.

Things to note about this patch:

- this.message.output.owner.owner.target is used to get the target from the widget. I'm not unsure how to go about this.
- since output nodes are already clickable to expand the variablesview, a new inspector icon was added. It's in fact the exact same than in the vview, so it's consistent, but I'll cc Darrin in case he has a better idea.
- I've added a for(widget of this.widgets){widget.destroy} at message level to make sure event listeners are being removed when the message is cleared away.

Tests are missing still. Mihai, do you have suggestions as to which existing tests I should add to? Or should I be creating new test files?
Attachment #8381620 - Flags: review?(mihai.sucan)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #21)
> Created attachment 8381620 [details] [diff] [review]
> bug757866-part2-highlight_and_jump_to_domnodes_in_page_from_console_output
> v2.patch
> 
> Highlight DOM nodes in the page on mouseover/out.
> Open inspector on click.
> 
> Things to note about this patch:
> 
> - this.message.output.owner.owner.target is used to get the target from the
> widget. I'm not unsure how to go about this.

See comment #20.

> - since output nodes are already clickable to expand the variablesview, a
> new inspector icon was added. It's in fact the exact same than in the vview,
> so it's consistent, but I'll cc Darrin in case he has a better idea.
> - I've added a for(widget of this.widgets){widget.destroy} at message level
> to make sure event listeners are being removed when the message is cleared
> away.

Sounds good. Will review the patch tomorrow.


> Tests are missing still. Mihai, do you have suggestions as to which existing
> tests I should add to? Or should I be creating new test files?

Please create a new test. Expanding existing tests might add too much execution time. Please use Task.spawn() - check how new console tests are written. Thanks!
Flags: needinfo?(mihai.sucan)
new part 2: added a getter for the toolbox target.
Still missing tests. Will start on those tomorrow.
Attachment #8381620 - Attachment is obsolete: true
Attachment #8381620 - Flags: review?(mihai.sucan)
Attachment #8381641 - Flags: review?(mihai.sucan)
No longer blocks: console-output
Depends on: 584733
Comment on attachment 8381618 [details] [diff] [review]
bug757866-part1-syntax_highlight_domnodes_in_console v2.patch

Thanks for your quick work on this bug! This looks good. Please fold into the second patch, after I review that one as well.
Attachment #8381618 - Flags: review?(mihai.sucan) → review+
Comment on attachment 8381641 [details] [diff] [review]
bug757866-part2-highlight_and_jump_to_domnodes_in_page_from_console_output v3.patch

Review of attachment 8381641 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking really good. This is close to landable.

General comments:

- now that we added italic for links, maybe DOM elements should be *all* italic (updated patch for bug 584733). Can you change the styling for DOM elements to only have italic for the tag name?

- the inspector icon seems too obvious with the dark theme. It's really nice with the light theme.

- what happens with DOM elements that are not in the DOM markup view? Try this page[1] and execute foobarObject.frag. You will see a DocumentFragment with child elements that are not in the page. Click the inspector icon to jump to markup view. You get an exception. [2]

[1] http://mihaisucan.github.io/mozilla-work/test.html
[2] https://pastebin.mozilla.org/4403471

- when we make previews for array elements or object property values, we do concise renders. Can you do this as well? For the vview stringifiers I did only <tagName#elementIdIfAvailable>. In _renderElementNode() check this.options.concise to see if you need to output less information.

More comments below.

::: browser/devtools/webconsole/console-output.js
@@ +6,5 @@
>  "use strict";
>  
>  const {Cc, Ci, Cu} = require("chrome");
>  
> +Cu.import("resource://gre/modules/Task.jsm");

lazyImport?

@@ +152,5 @@
>      return this.owner.webConsoleClient;
>    },
>  
>    /**
> +   * Getter for the current toolbox debuggee target

nit: period at the end.

@@ +2116,5 @@
>        default:
>          throw new Error("Unsupported nodeType: " + preview.nodeType);
>      }
>  
> +    this._linkToInspector();

Maybe call this in renderElementNode()?

@@ +2249,5 @@
>  
> +    this.element = doc.createElementNS(XHTML_NS, "span");
> +
> +    let nodeEl = this._renderAnchor();
> +    nodeEl.className = "kind-" + this.objectActor.preview.kind + " elementNode";

Please move these class names to this.element, so we can select the entire widget from CSS, when needed.

@@ +2252,5 @@
> +    let nodeEl = this._renderAnchor();
> +    nodeEl.className = "kind-" + this.objectActor.preview.kind + " elementNode";
> +    this.element.appendChild(nodeEl);
> +
> +    nodeEl.appendChild(doc.createTextNode("<"));

CodeMirror puts '<' in the span.cm-tag. For '>' it adds a another span.cm-tag at the end.

@@ +2262,2 @@
>  
>      for (let name in attributes) {

nit: for (let name of Object.keys(attributes))

@@ +2282,5 @@
> +    // The toolbox is required to highlight and select nodes
> +    let target = this.message.output.toolboxTarget;
> +    this.toolbox = gDevTools.getToolbox(target);
> +
> +    if (this.toolbox) {

nit: you can do early return here if (!this.toolbox).

@@ +2369,5 @@
> +    if (this._linkedToInspector) {
> +      this.element.removeEventListener("mouseover", this.highlightDomNode, false);
> +      this.element.removeEventListener("mouseout", this.unhighlightDomNode, false);
> +      this._openInspectorNode.removeEventListener("mousedown", this.openNodeInInspector, true);
> +      this.toolbox = null;

If we clear this.toolbox, we should also clear this._nodeFront.

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +387,5 @@
> +  margin-left: 5px;
> +  cursor: pointer;
> +}
> +
> +.open-inspector:hover {

Can you make this inspector icon to also show the hover effect when you hover the rendered DOM element? To make the icon more obvious. Currently, it becomes better visible only when you hover the icon itself.
Attachment #8381641 - Flags: review?(mihai.sucan) → feedback+
Status: NEW → ASSIGNED
Thanks Mihai for the review.
Looking forward to landing this stuff. It makes it a lot more useful to inspect dom elements from the console.

(In reply to Mihai Sucan [:msucan] from comment #25)
> - now that we added italic for links, maybe DOM elements should be *all*
> italic (updated patch for bug 584733). Can you change the styling for DOM
> elements to only have italic for the tag name?
Done. Looks good.

> - the inspector icon seems too obvious with the dark theme. It's really nice
> with the light theme.
That's true, especially when you have a lot of DOMNodes output in the console, it's a bit distracting. The thing is, I'm using the same icon as the one used in the vview. It was done by Darrin in a way that it worked both in the light and dark themes (semi-transparent grey). I'll attach a screenshot for Darrin to give us his feedback.

> - what happens with DOM elements that are not in the DOM markup view? Try
> this page[1] and execute foobarObject.frag. You will see a DocumentFragment
> with child elements that are not in the page. Click the inspector icon to
> jump to markup view. You get an exception. [2]
> 
> [1] http://mihaisucan.github.io/mozilla-work/test.html
> [2] https://pastebin.mozilla.org/4403471
Good catch. I didn't think of document fragments.
It's actually a more general problem that also occurs in the vview.
I'll need to make some changes to the inspector actor so it handles them correctly (meaning that it shouldn't handle them at all and let the UI know that a node is inside a fragment, since as of today, the markup-view can't represent them).

> - when we make previews for array elements or object property values, we do
> concise renders. Can you do this as well? For the vview stringifiers I did
> only <tagName#elementIdIfAvailable>. In _renderElementNode() check
> this.options.concise to see if you need to output less information.
Done. Now in concise mode, nodes appear like <nodeName#id> (if there is an id) and like <nodeName> (if there's no id).

> ::: browser/devtools/webconsole/console-output.js
> @@ +6,5 @@
> >  "use strict";
> >  
> >  const {Cc, Ci, Cu} = require("chrome");
> >  
> > +Cu.import("resource://gre/modules/Task.jsm");
> 
> lazyImport?
Done

> @@ +152,5 @@
> >      return this.owner.webConsoleClient;
> >    },
> >  
> >    /**
> > +   * Getter for the current toolbox debuggee target
> 
> nit: period at the end.
Done

> @@ +2116,5 @@
> >        default:
> >          throw new Error("Unsupported nodeType: " + preview.nodeType);
> >      }
> >  
> > +    this._linkToInspector();
> 
> Maybe call this in renderElementNode()?
If I do that, I'll also have to do it in _renderDocumentNode. I preferred calling it here, once and for all, and then checking in _linkToInspector the node type once again.

> @@ +2249,5 @@
> >  
> > +    this.element = doc.createElementNS(XHTML_NS, "span");
> > +
> > +    let nodeEl = this._renderAnchor();
> > +    nodeEl.className = "kind-" + this.objectActor.preview.kind + " elementNode";
> 
> Please move these class names to this.element, so we can select the entire
> widget from CSS, when needed.
Done.

> @@ +2252,5 @@
> > +    let nodeEl = this._renderAnchor();
> > +    nodeEl.className = "kind-" + this.objectActor.preview.kind + " elementNode";
> > +    this.element.appendChild(nodeEl);
> > +
> > +    nodeEl.appendChild(doc.createTextNode("<"));
> 
> CodeMirror puts '<' in the span.cm-tag. For '>' it adds a another
> span.cm-tag at the end.
Done.

> @@ +2262,2 @@
> >  
> >      for (let name in attributes) {
> 
> nit: for (let name of Object.keys(attributes))
Done.

> @@ +2282,5 @@
> > +    // The toolbox is required to highlight and select nodes
> > +    let target = this.message.output.toolboxTarget;
> > +    this.toolbox = gDevTools.getToolbox(target);
> > +
> > +    if (this.toolbox) {
> 
> nit: you can do early return here if (!this.toolbox).
Done.

> @@ +2369,5 @@
> > +    if (this._linkedToInspector) {
> > +      this.element.removeEventListener("mouseover", this.highlightDomNode, false);
> > +      this.element.removeEventListener("mouseout", this.unhighlightDomNode, false);
> > +      this._openInspectorNode.removeEventListener("mousedown", this.openNodeInInspector, true);
> > +      this.toolbox = null;
> 
> If we clear this.toolbox, we should also clear this._nodeFront.
Done.

> ::: browser/themes/shared/devtools/webconsole.inc.css
> @@ +387,5 @@
> > +  margin-left: 5px;
> > +  cursor: pointer;
> > +}
> > +
> > +.open-inspector:hover {
> 
> Can you make this inspector icon to also show the hover effect when you
> hover the rendered DOM element? To make the icon more obvious. Currently, it
> becomes better visible only when you hover the icon itself.
Done.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #26)
> > @@ +2116,5 @@
> > >        default:
> > >          throw new Error("Unsupported nodeType: " + preview.nodeType);
> > >      }
> > >  
> > > +    this._linkToInspector();
> > 
> > Maybe call this in renderElementNode()?
> If I do that, I'll also have to do it in _renderDocumentNode. I preferred
> calling it here, once and for all, and then checking in _linkToInspector the
> node type once again.
Actually, scratch that. Let me move this call to _renderDocumentNode and let's not have the output for `document` be linked to the inspector.
Blocks: 682033
This new version of the part 2 patch contains:

- changes described in comment 26
- including changes to the inspector actor to deal with fragments and unattached DOM nodes
- the concise view now shows both IDs and classes (e.g. <div#id.class1.class2>)
- a new webconsole output test that asserts various outputs on DOM nodes
- a new flag in head.js/checkOutputForInputs to test for the presence of the inspector icon. Because DOMNode output widgets are linked to the inspector asynchronously, this made me rework a little bit the DOMNode widget's linkToInspector method so that it returns a promise (which it caches too).
- an other change to the inspector actor which was triggered by a bug found when working on the test: the objectActors were not being converted into nodeFronts correctly when the inspector wasn't initialized. With Mihai's help, I'm now using this.conn.getActor(id) which finds the actor correctly in the pool.
- some changes to the toolbox were made to make the initInspector method more robust. It now uses Task.spawn to be easier to read and caches the returned promise.

All webconsole tests pass locally but I haven't tested on try yet.
I'll rebase this patch on top of the latest changes in bug 584733 once available.
And I'll fold the 2 patches after R+.
Attachment #8381641 - Attachment is obsolete: true
Attachment #8383779 - Flags: review?(mihai.sucan)
Comment on attachment 8383779 [details] [diff] [review]
bug757866-part2-highlight_and_jump_to_domnodes_in_page_from_console_output v4.patch

Review of attachment 8383779 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch. General comments:

- I no longer get the exception I mentioned in comment #25 for DOM elements that are not yet inserted into the document. However, I get:

TypeError: node is null
HighlighterActor<.showBoxModel<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:86:1

... if I eval to a DOM element that can be highlighted in the page, then reload the page / navigate to another page, and hover/click the DOM element inspector icon. The webconsole has the option of persisting the output after page navigation, and that breaks this code. It seems you check if the element can be highlighted only when it's added.

- I get a test failure:

 2:05.48 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_output_04.js | failed to match rule: console.log() output: DocumentFragment [ <div#foo1>, <div#foo3> ]

- I cannot tab to the inspector icon, using the keyboard, in the console output. Currently, users can tab to clickable objects. Can you please fix this?

More comments below.

::: browser/devtools/framework/toolbox.js
@@ +1040,5 @@
>     * Initialize the inspector/walker/selection/highlighter fronts.
>     * Returns a promise that resolves when the fronts are initialized
>     */
>    initInspector: function() {
> +    if (!this._initInspector) {

++ for the changes here!

::: browser/devtools/webconsole/console-output.js
@@ +2262,5 @@
> +      if (attributes.id) {
> +        tagName.textContent += "#" + attributes.id;
> +      }
> +      if (attributes.class) {
> +        tagName.textContent += "." + attributes.class.split(" ").join(".");

What do you think of wrapping #id.and.classNames in .cm-attribute? So we don't just have a single color for the whole div#id.class text (we currently use the color of .cm-tag).

<span class='cm-tag'>&lt;div</span>
<span class='cm-attribute'>#id.class</span>
<span class='cm-tag'>&gt;</span>

@@ +2295,5 @@
> +   * tree, or not of type Ci.nsIDOMNode.ELEMENT_NODE).
> +   */
> +  linkToInspector: function()
> +  {
> +    if (!this._linkedToInspector) {

to avoid too deep nesting in this function can you please do an early return here?

if (this._linkedToInspector) {
  return this._linkedToInspector;
}
this._linkedToInspector = Task.spawn(...

::: browser/devtools/webconsole/test/browser.ini
@@ +68,5 @@
>    test-console.html
>    test-console-output-02.html
>    test-console-output-03.html
>    test-console-output-04.html
> +  test-console-output-05.html

Maybe this should be test-console-output-dom-elements.html - same for the js file.

::: browser/devtools/webconsole/test/browser_webconsole_output_05.js
@@ +90,5 @@
> +  }
> +];
> +
> +function test() {
> +  addTab(TEST_URI);

This is deprecated. Please use loadTab() in the spawned task.

let {tab} = yield loadTab(TEST_URI);
let hud = yield openConsole(tab);

@@ +96,5 @@
> +    browser.removeEventListener("load", onLoad, true);
> +
> +    Task.spawn(function*() {
> +      let hud = yield openConsole();
> +      yield checkOutputForInputs(hud, inputTests);

Do you have a test that checks if for a given DOM element a click on the inspector icon actually selects the expected element in markup view? From the console. Similarly, a test for checking the correct element is highlighted on hover.

::: browser/devtools/webconsole/test/head.js
@@ +1355,5 @@
>          category: CATEGORY_WEBDEV,
>          severity: SEVERITY_LOG,
>        }],
> +    }).then(([result]) => {
> +      if (typeof entry.inspectorIcon === "boolean") {

To keep this code clean can you please move this function into a standalone function like checkObjectClick()? You could name it checkLinkToInspector().

Also, see how checkJSEval() uses checkObjectClick(). Can you do something similar? Approximately:

let [result] = yield waitForMessages();
if (typeof entry.inspectorIcon == "boolean") {
   yield checkLinkToInspector(messageEl, entry.inspectorIcon);
}

Lastly, this is only checking if the icon exists and works with console.log() calls. Can you also do the check in checkJSEval()? For eval output.

::: toolkit/devtools/server/actors/inspector.js
@@ +2054,5 @@
>     * Given an ObjectActor (identified by its ID), commonly used in the debugger,
>     * webconsole and variablesView, return the corresponding inspector's NodeActor
>     */
>    getNodeActorFromObjectActor: method(function(objectActorID) {
> +    // let debuggerObject = this.conn.poolFor(objectActorID).get(objectActorID).obj;

Do we need to keep this comment?

@@ +2061,5 @@
>  
> +    // Filter out object actors that represent unattached DOM elements (nodes
> +    // removed from the tree, or nodes in document fragments).
> +    let walker = documentWalker(rawNode, this.rootWin), current;
> +    while(current = walker.currentNode) {

nit: space after 'while'. Also, assigning expressions in while will cause a js strict warning. To avoid the warning use |while ((current = walker.currentNode))|.
Attachment #8383779 - Flags: review?(mihai.sucan)
Thanks for the review Mihai. I have corrected all points.
See some comments inline below:

(In reply to Mihai Sucan [:msucan] from comment #29)
> - I no longer get the exception I mentioned in comment #25 for DOM elements
> that are not yet inserted into the document. However, I get:
> 
> TypeError: node is null
> HighlighterActor<.showBoxModel<@resource://gre/modules/commonjs/toolkit/
> loader.js ->
> resource://gre/modules/devtools/server/actors/highlighter.js:86:1
> 
> ... if I eval to a DOM element that can be highlighted in the page, then
> reload the page / navigate to another page, and hover/click the DOM element
> inspector icon. The webconsole has the option of persisting the output after
> page navigation, and that breaks this code. It seems you check if the
> element can be highlighted only when it's added.
Right, I didn't think of the persistent log usecase. I've made a fix to the highlighter that should have been there from the beginning. Now it checks if the node being passed exists.
Hovering over nodes in the output doesn't do anything now, clicking on the inspector still switches over to the inspector, which I think is a good thing, it just doesn't select any node.

> - I cannot tab to the inspector icon, using the keyboard, in the console
> output. Currently, users can tab to clickable objects. Can you please fix
> this?
Fixed. The inspector icon is now a focusable anchor. 
Note though that on mac, by default, non of the console output links can be navigated to. You first need to go to pref/keyboard/shortcuts then check the "All controls" radio button at the bottom.
On Linux though, the links can be focused by default.
Filed bug 979275 about this.

> @@ +96,5 @@
> > +    browser.removeEventListener("load", onLoad, true);
> > +
> > +    Task.spawn(function*() {
> > +      let hud = yield openConsole();
> > +      yield checkOutputForInputs(hud, inputTests);
> 
> Do you have a test that checks if for a given DOM element a click on the
> inspector icon actually selects the expected element in markup view? From
> the console. Similarly, a test for checking the correct element is
> highlighted on hover.
I'm creating those tests now.

I will be attaching a new version of the patch soon.
v5 patch:
- should address all the points raised in the previous review
- now the widget verifies if DOM nodes exist before highlighting or selecting (new method in the walker actor)
- thanks to this, the various inspector-related methods in the ElementNode widget return a promise that resolves or rejects depending on if the node is available (used in tests)
- added 3 new tests to test that the right nodes are being selected and highlighted, and that unattached nodes aren't being selected or highlighted
Attachment #8383779 - Attachment is obsolete: true
Attachment #8386081 - Flags: review?(mihai.sucan)
Forgot to mention that I launched a try build with this latest patch: https://tbpl.mozilla.org/?tree=Try&rev=39b74810f258
browser mochitest are green, but for some reason yet unknown to me, all xpcshell test failed!
The errors don't immediately point to something related to my patch, so I'll have to look at this a bit more. Probably later on today.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #32)
> Forgot to mention that I launched a try build with this latest patch:
> https://tbpl.mozilla.org/?tree=Try&rev=39b74810f258
> browser mochitest are green, but for some reason yet unknown to me, all
> xpcshell test failed!
> The errors don't immediately point to something related to my patch, so I'll
> have to look at this a bit more. Probably later on today.

Its because the method

_isInDOMTree

is not always returning things and then there is a call to the method in an if block

    if (!this._isInDOMTree(rawNode)) {
      return null;
    }

You can simply remove the else block in _isInDOMTree  method and have  a return true at the end (outside while loop)
Comment on attachment 8386081 [details] [diff] [review]
bug757866-part2-highlight_and_jump_to_domnodes_in_page_from_console_output v5.patch

Review of attachment 8386081 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/inspector.js
@@ +2070,5 @@
> +          return true;
> +        }
> +      }
> +    }
> +  },

You need to return a value after the loop in order to appease the SpiderMonkey warnings that are unfortunately enabled by default in tests. Our xpcshell tests abort at every console message, so you'll just have to eliminate the warning for tests to pass.

And in this case the warning seems to be actually useful, since if walker.currentNode is null for some reason, the method will return undefined.
Yes, that's what I just noticed.
Will correct the function now.
Corrects the _isInDomTree function.
New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=9996c8afb3b1
Attachment #8386081 - Attachment is obsolete: true
Attachment #8386081 - Flags: review?(mihai.sucan)
Attachment #8386676 - Flags: review?(mihai.sucan)
Comment on attachment 8386676 [details] [diff] [review]
bug757866-part2-highlight_and_jump_to_domnodes_in_page_from_console_output v6.patch

Review of attachment 8386676 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updates. This is solid.

::: browser/devtools/webconsole/test/browser_webconsole_output_dom_elements_04.js
@@ +49,5 @@
> +      widgets.push(widget);
> +    }
> +
> +    info("Reloading the page");
> +    content.location.reload();

This test assumes reload() is sync. Please wait for the page load event, or the target navigate event.
Attachment #8386676 - Flags: review?(mihai.sucan) → review+
- corrected the last minor feedback about the test not waiting for the page to be reloaded
- folded the 2 patches
- rebased against latest fx-team

Just waiting for the try build to turn green and then landing this patch.
Attachment #8381618 - Attachment is obsolete: true
Attachment #8386676 - Attachment is obsolete: true
Attachment #8386742 - Flags: review+
Landed: https://hg.mozilla.org/integration/fx-team/rev/dcec4c42cd18
Whiteboard: [highlighter] → [highlighter][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dcec4c42cd18
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [highlighter][fixed-in-fx-team] → [highlighter]
Target Milestone: --- → Firefox 30
Backed out for too many test failures (bug 980835):
remote:   https://hg.mozilla.org/mozilla-central/rev/36c081b72fd8
Status: RESOLVED → REOPENED
Depends on: 980835
Resolution: FIXED → ---
Potential fix sent to try servers: https://tbpl.mozilla.org/?tree=Try&rev=cb7cc4660102

I cannot repro the test failures in local builds (tried both dbg and opt builds, with heavy cpu usage, hundreds of runs). From readings logs of failures and the test code, it seems it's possible we open the inspector, then wait for the paragraph element to be selected, but we might first get a selection event for the <body> element, which is first selected by the inspector - it's really a timing thing. I made a small change which hopefully avoids the problem, and I added debug logging, so we can do further investigation.
This patch seems to fix the intermittent failures from bug 980835.

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=cb7cc4660102

This patch also includes two new if this.view checks in style-inspector.js, for RuleView and ComputedView. The onSelect event handlers always expect this.view is available, but in their destroy() methods this.view is set to null. Due to asyncness late events can still arrive, eg. during quick disconnects. TBPL logs showed non-fatal errors coming from these event handlers, decided it's safe to fix these.


r=me (trivial changes)


Re-landed Patrick's patch:
https://hg.mozilla.org/integration/fx-team/rev/47bce8cc31b4

And the test fix:
https://hg.mozilla.org/integration/fx-team/rev/3d8d5d2cb384
Whiteboard: [highlighter] → [highlighter][fixed-in-fx-team]
10.6 debug, WinXP debug and Win7 debug (roughly speaking: "the slow ones") agreed about that, https://tbpl.mozilla.org/php/getParsedLog.php?id=35847998&tree=Fx-Team

Backed out both in https://hg.mozilla.org/integration/fx-team/rev/7231e8751ad5
Whiteboard: [highlighter][fixed-in-fx-team] → [highlighter]
(In reply to Phil Ringnalda (:philor) from comment #44)
> 10.6 debug, WinXP debug and Win7 debug (roughly speaking: "the slow ones")
> agreed about that,
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35847998&tree=Fx-Team
> 
> Backed out both in
> https://hg.mozilla.org/integration/fx-team/rev/7231e8751ad5

"Nice", I've had many green tbpl runs. Do you have any ideas why the error happens only in fx-team? Does fx-team use different build configs or system configs?
Attached patch bug980835-2.diff — — Splinter Review 
Updated the fix for bug 980835.

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=75aa5d9a251f

It seems the test also needed to wait for the selection "new-node" event.

r=me
Attachment #8388114 - Attachment is obsolete: true
Re-landed:

https://hg.mozilla.org/integration/fx-team/rev/610ea14ada94
https://hg.mozilla.org/integration/fx-team/rev/09ba2ea97c87

Hoping it sticks this time.
Whiteboard: [highlighter] → [highlighter][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/610ea14ada94
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [highlighter][fixed-in-fx-team] → [highlighter]
Blocks: 983614
Mihai: I've added a section on this: https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Highlighting_and_inspecting_nodes.

I redid all the screenshots in that section as well, to show off the syntax highlighting. Please let me know if it works for you. Thanks!
Flags: needinfo?(mihai.sucan)
(In reply to Will Bamberg [:wbamberg] from comment #49)
> Mihai: I've added a section on this:
> https://developer.mozilla.org/en-US/docs/Tools/
> Web_Console#Highlighting_and_inspecting_nodes.
> 
> I redid all the screenshots in that section as well, to show off the syntax
> highlighting. Please let me know if it works for you. Thanks!

That's really nice, thank you!

You should probably also mention that the tag name can be clicked to open the JavaScript object inspector for that DOM element.
Flags: needinfo?(mihai.sucan)
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: