The output of console.log(object) isn't expandable/inspectable in the Web Console

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools: Console
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: msucan)

Tracking

Trunk
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
<robcee> "firebug does it"

If I enter "document" in the Web Console, I get back
  [object HTMLDocument]
and it's linkified; if I click on it, I get the Inspector.

If I enter "console.log(document)", I get this:
  [object XrayWrapper [object HTMLDocument]]
and it is not linkified or inspectable.

Comment 1

6 years ago
Created attachment 560757 [details]
testcase (open console to see whats happening)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 685815
In order to make sure it doesn't get overlooked, I'm making an explicit note that bug 685815 requested making object substitution patterns (%o) for console.log and friends expandable, too.

Comment 4

5 years ago
I think this is a pretty important feature. -> P2
Component: Developer Tools → Developer Tools: Console
Priority: -- → P2
QA Contact: developer.tools → developer.tools.console

Updated

5 years ago
Depends on: 660197
This is partly fixed in bug 693269, and partly depends on bug 660197. Either way, it is tracked.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
No longer depends on: 660197
Resolution: --- → DUPLICATE
Duplicate of bug: 693269
(Assignee)

Comment 6

5 years ago
Reopening and taking the bug because GCLI is no longer in the Web Console and we still need to have console.log(object) inspectable in the Web Console. Fixing bug 660197 is a different issue which requires UI work on the JS object inspector, and that will happen at a later point in time.

Will submit a patch soonish.
Assignee: nobody → mihai.sucan
Status: RESOLVED → REOPENED
Depends on: 688981
Resolution: DUPLICATE → ---
Version: unspecified → Trunk
(Assignee)

Updated

5 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 7

5 years ago
Created attachment 642023 [details] [diff] [review]
proposed patch

Proposed patch with included test. Now any console.*() call will underline object toString() representations, allowing a click to open the object inspector.

Please let me know if further changes are needed. Thanks!

As agreed with Rob, this patch does not go into fancy UI/UX. We still use the object inspector. Inline inspection and other UX improvements will come later.
Attachment #642023 - Flags: review?(rcampbell)
(Assignee)

Comment 8

5 years ago
Created attachment 642612 [details] [diff] [review]
updated patch

Updated the patch to include a fix for when there are multiple objects: console.log(object1, object2).
Attachment #642023 - Attachment is obsolete: true
Attachment #642023 - Flags: review?(rcampbell)
Attachment #642612 - Flags: review?(rcampbell)
(Assignee)

Comment 9

5 years ago
Created attachment 647996 [details] [diff] [review]
rebased patch
Attachment #642612 - Attachment is obsolete: true
Attachment #642612 - Flags: review?(rcampbell)
Attachment #647996 - Flags: review?(rcampbell)
Comment on attachment 647996 [details] [diff] [review]
rebased patch

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

This looks pretty good and after playing with it, I really want this functionality. Just a couple of questions in line, but this is great.

::: browser/devtools/webconsole/webconsole.js
@@ +1078,5 @@
> +   * window.console API.
> +   *
> +   * @private
> +   * @param nsIDOMNode aMessage
> +   *        The message element for which we live.

little grammar nit: The message element we... live in?

We don't live in a message element! That's ridiculous! Maybe The message element this handler corresponds to? Or The message element this handler is bound to? Something along those lines might make more sense.

@@ +1080,5 @@
> +   * @private
> +   * @param nsIDOMNode aMessage
> +   *        The message element for which we live.
> +   * @param nsIDOMNode aAnchor
> +   *        The object inspector anchor element. This is the clickbable element

typo: "clickable". teehee.

@@ +1121,5 @@
> +      }
> +    }.bind(this);
> +
> +    propPanel.panel.addEventListener("popuphiding", onPopupHide, false);
> +  },

looks good.

@@ +1897,5 @@
> +        str = aBody.resultString;
> +      }
> +      else if (["log", "info", "warn", "error", "debug"].indexOf(aLevel) > -1 &&
> +               typeof aBody == "object") {
> +        this._makeConsoleLogMessageBody(node, bodyNode, aBody);

you could return early here and do away with the !== undefined check at the bottom.

@@ +2019,5 @@
> +      configurable: false
> +    });
> +
> +    aBody.remoteObjects.forEach(function(aItem, aIndex) {
> +      if (aContainer.lastChild) {

isn't aContainer always going to have a lastChild element? Seems like this check is going to always be true.

@@ +2037,5 @@
> +
> +      this._addMessageLinkCallback(elem,
> +        this._consoleLogClick.bind(this, aMessage, elem, aItem));
> +
> +      aContainer.appendChild(elem);

there's a whole lot of appendChild in this method, which I'm not entirely keen on. Would this make sense to use a documentFragment (or heck, even a string) here to build up your node and then insert the whole thing into aContainer at the end? This feels like it might be a little slow.

::: dom/base/ConsoleAPI.js
@@ +356,5 @@
>      let processed = format.replace(ARGUMENT_PATTERN, function CA_PA_substitute(match, submatch) {
>        switch (submatch) {
>          case "o":
> +          objects.push(args.shift());
> +          return splitter;

cool. :)
(Assignee)

Comment 11

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> Comment on attachment 647996 [details] [diff] [review]
> rebased patch
> 
> Review of attachment 647996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good and after playing with it, I really want this
> functionality. Just a couple of questions in line, but this is great.

Thanks!

> ::: browser/devtools/webconsole/webconsole.js
> @@ +1078,5 @@
> > +   * window.console API.
> > +   *
> > +   * @private
> > +   * @param nsIDOMNode aMessage
> > +   *        The message element for which we live.
> 
> little grammar nit: The message element we... live in?
> 
> We don't live in a message element! That's ridiculous! Maybe The message
> element this handler corresponds to? Or The message element this handler is
> bound to? Something along those lines might make more sense.

Thanks for the grammar fix. Will fix.

> @@ +1080,5 @@
> > +   * @private
> > +   * @param nsIDOMNode aMessage
> > +   *        The message element for which we live.
> > +   * @param nsIDOMNode aAnchor
> > +   *        The object inspector anchor element. This is the clickbable element
> 
> typo: "clickable". teehee.

"click-bable!" ;)


> @@ +1897,5 @@
> > +        str = aBody.resultString;
> > +      }
> > +      else if (["log", "info", "warn", "error", "debug"].indexOf(aLevel) > -1 &&
> > +               typeof aBody == "object") {
> > +        this._makeConsoleLogMessageBody(node, bodyNode, aBody);
> 
> you could return early here and do away with the !== undefined check at the
> bottom.

Not really, we still need the rest of the createMessageNode() code to execute. I know, ugly stuff. Can't return early...

> @@ +2019,5 @@
> > +      configurable: false
> > +    });
> > +
> > +    aBody.remoteObjects.forEach(function(aItem, aIndex) {
> > +      if (aContainer.lastChild) {
> 
> isn't aContainer always going to have a lastChild element? Seems like this
> check is going to always be true.

I meant to only add the space when there's already some content. If the container is empty we don't add the space. Will change to check for "firstChild" - should be less confusing.


> @@ +2037,5 @@
> > +
> > +      this._addMessageLinkCallback(elem,
> > +        this._consoleLogClick.bind(this, aMessage, elem, aItem));
> > +
> > +      aContainer.appendChild(elem);
> 
> there's a whole lot of appendChild in this method, which I'm not entirely
> keen on. Would this make sense to use a documentFragment (or heck, even a
> string) here to build up your node and then insert the whole thing into
> aContainer at the end? This feels like it might be a little slow.

Good point, but the message node is not yet appended into the output. So, for all practical purposes, this should not be slow - once everything is done, it's all appended at once into the output.


Thanks for looking into the patch!
(Assignee)

Comment 12

5 years ago
Created attachment 648427 [details] [diff] [review]
updated patch

Updated to address your comments.
Attachment #647996 - Attachment is obsolete: true
Attachment #647996 - Flags: review?(rcampbell)
Attachment #648427 - Flags: review?(rcampbell)
Comment on attachment 648427 [details] [diff] [review]
updated patch

ah, you make excellent points. Thanks for the update.
Attachment #648427 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/integration/fx-team/rev/7e5f3a8a7ca7
Whiteboard: [fixed-in-fx-team]
Backed out:
https://hg.mozilla.org/integration/fx-team/rev/98cdb3e2d30a

due to test failures in:
https://tbpl.mozilla.org/?tree=Try&rev=b871b8028de9
Whiteboard: [fixed-in-fx-team]

Comment 16

5 years ago
Try run for 8b9ec1e414ee is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8b9ec1e414ee
Results (out of 24 total builds):
    success: 21
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-8b9ec1e414ee

Comment 17

5 years ago
Try run for 8b9ec1e414ee is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8b9ec1e414ee
Results (out of 48 total builds):
    success: 42
    warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-8b9ec1e414ee
(Assignee)

Comment 18

5 years ago
Created attachment 648983 [details] [diff] [review]
test fixed for mac os x

After some oranges on the try server, I got the greens.

Landed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/fe50f2057825
Attachment #648427 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fe50f2057825
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.