Closed Bug 584733 Opened 14 years ago Closed 10 years ago

Code highlight all JS objects and functions in console output

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: ddahl, Assigned: msucan)

References

Details

(Whiteboard: [console-2][console-output])

Attachments

(4 files, 4 obsolete files)

pretty-printing JS should be pretty easy to do as there is a lot of prior work out there to draw ideas from.
Did some work on this already. Basically, it adds >>"<< around strings, prints objects not as [object foo] but as >> foo {...} <<< and arrays as >>> [bar, foo {...}, [...] ] <<<. When you click on foo inside of the array, you get an PropertyPanel to inspect foo for more detail. Same for the array.
(In reply to comment #1)
> Did some work on this already. Basically, it adds >>"<< around strings, prints
> objects not as [object foo] but as >> foo {...} <<< and arrays as >>> [bar, foo
> {...}, [...] ] <<<. When you click on foo inside of the array, you get an
> PropertyPanel to inspect foo for more detail. Same for the array.

Right, also want to see color highlighting like in bespin:)
Summary: Pretty print all JS objects and functions in console output → Code highlight all JS objects and functions in console output
I meant code color highlighting. too early - no coffee.
Whiteboard: [console-2]
Component: Developer Tools → Developer Tools: Console
Whiteboard: [console-2] → [console-2][console-output]
Filter on TARDIS.
Priority: -- → P3
Pretty output for objects will land in bug 843004. Syntax highlighting work should probably happen here.
No longer blocks: console-output
Depends on: 843004, console-output
Hardware: x86 → All
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attached patch wip1 (obsolete) — Splinter Review
Rich output for objects in the console, work-in-progress.

Screenshots:

- light: http://imgur.com/6aMd0GP
- dark: http://imgur.com/CPI7za6

This patch adds rich/colored output for arrays (ArrayLike), map (MapLike), generic JS objects, DOM events, Date, Function, Number, RegExp, strings, long strings and more.

It is missing rich output for DOM elements, which is something Patrick wants to add in bug 757866.

Feedback request: notice that I added "Array" for arrays and "Object" for objects, to provide a click target, so the user can inspect the array/object. Is this "acceptable"? Any better ideas / any preferences? Firebug seems confusing: it allows a click anywhere in the array output to do inline object inspection. For objects it doesnt allow a click inside the preview for quick jump to property values - you can only click on the main object that is the result of your eval. Chrome doesnt allow inspection of arrays beyond showing their elements. For objects it allows inline expansion.
Attachment #8379207 - Flags: feedback?(rcampbell)
Comment on attachment 8379207 [details] [diff] [review]
wip1

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

this looks great! Bonus points for a 4 year old bug.

<3

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +377,5 @@
> +
> +.cm-s-mozilla a[class]:hover,
> +.cm-s-mozilla a[class]:focus {
> +  text-decoration: underline;
> +}

cm is short for "console message"? A bit cryptic and I read through this wondering if you were borrowing styles from our Code Mirror install.

::: toolkit/devtools/server/actors/script.js
@@ +3307,5 @@
>   * Functions must return false if they cannot provide preview
>   * information for the debugger object, or true otherwise.
>   */
>  DebuggerServer.ObjectActorPreviewers = {
> +  String: [function({obj, threadActor}, aGrip) {

why are these array wrapped?
Attachment #8379207 - Flags: feedback?(rcampbell) → feedback+
(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> Comment on attachment 8379207 [details] [diff] [review]
> wip1
> 
> Review of attachment 8379207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks great! Bonus points for a 4 year old bug.

Made possible by the work in bug 843004. :)


> ::: browser/themes/shared/devtools/webconsole.inc.css
> @@ +377,5 @@
> > +
> > +.cm-s-mozilla a[class]:hover,
> > +.cm-s-mozilla a[class]:focus {
> > +  text-decoration: underline;
> > +}
> 
> cm is short for "console message"? A bit cryptic and I read through this
> wondering if you were borrowing styles from our Code Mirror install.

.cm- for codemirror, actually. This is reusing the codemirror class names.


> ::: toolkit/devtools/server/actors/script.js
> @@ +3307,5 @@
> >   * Functions must return false if they cannot provide preview
> >   * information for the debugger object, or true otherwise.
> >   */
> >  DebuggerServer.ObjectActorPreviewers = {
> > +  String: [function({obj, threadActor}, aGrip) {
> 
> why are these array wrapped?

This is allowing addons to prepend their own object previewer candidates... which reminds me I need to write some docs "how to customize console output" (the whole stack, from server to client). Will do once this patch lands.
Attached patch bug584733-2.diff (obsolete) — Splinter Review
Added rich output for ObjectWithURL, ObjectWithText, comment nodes, text nodes, and DOM attribute nodes. Removed underlining of the clickables - it is pretty common that objects and their values are clickable for further inspection.

Fixed the test as well.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=87ec89c4a686
Attachment #8379207 - Attachment is obsolete: true
Attachment #8379858 - Flags: review?(rcampbell)
This is an UX spike based on attachment 8379858 [details] [diff] [review]: bug584733-2.diff. The following changes have been performed:

- the default styling for inspectable objects is `text-decoration: none; font-weight: bold`. This removes the underlining and makes the object be bold.
- to make visualize the user about the ability to inspect the object, I have added a magnifier symbol after the objects. The positioning of the magnifier is not perfect and should be 1-2 px lower, but I couldn't get this working with the hacked up code I used.
Attachment #8380166 - Attachment description: Screen Shot 2014-02-22 at 12.26.18.png → UX spike with changed style for inspectable objects.
Blocks: 757866
Attached image bug584733-dark-2.png
Latest screenshot that shows the attached patch in action. (dark theme)

Darrin, can you please take a look and offer your feedback?

You can play with builds from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-87ec89c4a686/

What I am mainly interested about is links. Notes:

- in the screenshot you can see "Map { ... }", or "Object { ... }". These objects class names, like Map, Object, and others, are clickable - so the user can jump to the object inspector. When you hover the class name, you get an underline.

- arrays behave similarly, "Array [ ... ]" or "Set [ ... ]" have their class names clickable. Hover for underline.

- when these objects are shown in concise mode, as values of other properties, you see something like: Object { fooArr: Array[N], fooMap: Map, }, etc. Again, class names are clickable, so the user can directly inspect the property value. Hover for underline.

- regular expressions are clickable as well, Date, and DOM attribute names. For example, you can click "class" in that DocumentFragment preview to open the object inspector for the DOM attribute node that represents the class attribute. Hover for underline.

- if you eval window or document, you get "Window -> url" / "HTMLDocument -> url". The class name is clickable again - hover for underline. The url is an anchor that opens the url in a new tab, if clicked. This is underlined by default.

- if an object or an array has more elements than shown in the preview, "N more..." is shown, with an underline.

- DOM elements do not have rich output. Patrick is working on that in bug 757866.

I tried showing underlines for all clickable targets - that's way too much underlining. I tried dashed underlines, this is better, but it's still 'weird'. I don't fancy much italics for links, but we could do it, if you think that's better. Currently, you see I don't do any underline, unless hovered.

I am not happy about the inconsistency of having URLs and "N more..." underlined, which might suggest that *these* are clickable, while "Window" and all other cases are not clickable.

Thoughts? Thank you!
Attachment #8382098 - Flags: feedback?(dhenein)
Attached image bug584733-light-2.png
Latest screenshot that shows the attached patch in action. (light theme)
Attachment #8382100 - Flags: feedback?(dhenein)
(In reply to Julian Viereck from comment #10)
> Created attachment 8380166 [details]
> UX spike with changed style for inspectable objects.
> 
> This is an UX spike based on attachment 8379858 [details] [diff] [review]:
> bug584733-2.diff. The following changes have been performed:
> 
> - the default styling for inspectable objects is `text-decoration: none;
> font-weight: bold`. This removes the underlining and makes the object be
> bold.
> - to make visualize the user about the ability to inspect the object, I have
> added a magnifier symbol after the objects. The positioning of the magnifier
> is not perfect and should be 1-2 px lower, but I couldn't get this working
> with the hacked up code I used.

This is an interesting experiment, thanks for posting your result. I am not sure if this would look good with lots of output like in the screenshots I submitted. Take into consideration that inline objects are clickable, array elements, object props, DOM attributes, etc. With this kind of styling we'd have quite many of those icons in the output. Let's see what Darrin suggests we do.
Comment on attachment 8382098 [details]
bug584733-dark-2.png

Thanks Mihai, this looks great! I agree that it's not readily apparent what is clickable vs. not, so I tried a few variations in your try build. I think italic is the best option for clickable elements, it stands out enough and is less distracting than an underline. I agree that we should keep the underline on hover, though. 

I think that should be enough to cue the user that they can click some elements, I don't think we need an icon or any other styling.

Let me know if theres any other questions you had!
Attachment #8382098 - Flags: feedback?(dhenein) → feedback+
Attachment #8382100 - Flags: feedback?(dhenein)
(In reply to Darrin Henein [:darrin] from comment #14)
> Comment on attachment 8382098 [details]
> bug584733-dark-2.png
> 
> Thanks Mihai, this looks great! I agree that it's not readily apparent what
> is clickable vs. not, so I tried a few variations in your try build. I think
> italic is the best option for clickable elements, it stands out enough and
> is less distracting than an underline. I agree that we should keep the
> underline on hover, though. 
> 
> I think that should be enough to cue the user that they can click some
> elements, I don't think we need an icon or any other styling.

Sounds fine. Going to submit an updated patch with italic. Thanks!
Attached patch bug584733-3.diff (obsolete) — Splinter Review
Patch with italic styling for clickable elements. Last try push was green.
Attachment #8379858 - Attachment is obsolete: true
Attachment #8379858 - Flags: review?(rcampbell)
Attachment #8382318 - Flags: review?(rcampbell)
(In reply to Darrin Henein [:darrin] from comment #14)
> Comment on attachment 8382098 [details]
> bug584733-dark-2.png
> 
> Thanks Mihai, this looks great! I agree that it's not readily apparent what
> is clickable vs. not, so I tried a few variations in your try build. I think
> italic is the best option for clickable elements, it stands out enough and
> is less distracting than an underline. I agree that we should keep the
> underline on hover, though. 
> 
> I think that should be enough to cue the user that they can click some
> elements, I don't think we need an icon or any other styling.
> 
> Let me know if theres any other questions you had!
I think italic rather stands for description, secondary text. I think a dotted underline might be a better choice actually.
(In reply to Tim Nguyen [:ntim] from comment #17)
> (In reply to Darrin Henein [:darrin] from comment #14)
> > Comment on attachment 8382098 [details]
> > bug584733-dark-2.png
> > 
> > Thanks Mihai, this looks great! I agree that it's not readily apparent what
> > is clickable vs. not, so I tried a few variations in your try build. I think
> > italic is the best option for clickable elements, it stands out enough and
> > is less distracting than an underline. I agree that we should keep the
> > underline on hover, though. 
> > 
> > I think that should be enough to cue the user that they can click some
> > elements, I don't think we need an icon or any other styling.
> > 
> > Let me know if theres any other questions you had!
> I think italic rather stands for description, secondary text. I think a
> dotted underline might be a better choice actually.

or dashed underline.
Comment on attachment 8382318 [details] [diff] [review]
bug584733-3.diff

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +3424,5 @@
>      return "Date " + new Date(preview.timestamp).toISOString();
>    },
> +
> +  String: function({displayString}) {
> +    if (typeof displayString == "undefined") {

could just be `if (displayString === undefined) {`

::: browser/devtools/webconsole/console-output.js
@@ +1013,5 @@
> +   * @private
> +   * @param object grip
> +   *        The value grip received from the server.
> +   * @param object options
> +   *        Options for displaying the value. Known options:

"Known options" reads kind of strange. Maybe "Available options"?

@@ +1072,5 @@
> +   *         The class name for the grip.
> +   */
> +  getClassNameForValueGrip: function(grip)
> +  {
> +    let map = {

It would be good to centralize these CodeMirror specifics to one localized area of the code, or specifically mark them using some keyword. You have CodeMirror mentioned in the method's comments, but really the CodeMirror specific part is this map.

@@ +1124,5 @@
> +    if (!widget || (widget.canRender && !widget.canRender(objectActor))) {
> +      widget = Widgets.JSObject;
> +    }
> +
> +    let instance = new widget(this, objectActor, options).render();

This implicitly assumes that `widget` is not null by this point, but the code above doesn't obviously guarantee that. Should either add a comment here explicitly stating the assumption, or an assertion to check and throw an error if not.

@@ +1449,2 @@
>   */
> +Widgets.JSObject = function(message, objectActor, options)

`options` could be optional based on how it's used.

@@ +1466,5 @@
>    render: function()
>    {
>      if (this.element) {
>        return this;
>      }

Every render function (I think) starts and ends with the same setup: check if `this.element` exists and if so return this. Do stuff. Then return this. Can we factor this out so that that JSObject.prototype.render is just:

  render: function() {
    if (!this.element) {
      this.element = this.document.createElementNS(XHTML_NS, "span");
      this._render(); // or whatever name
    }
    return this;
  }

Subclasses can still have their own custom render if they need different behavior.

@@ +1491,5 @@
> +   *        Text to show in the anchor.
> +   * @param function|string [handler]
> +   *        Optional click event handler. By default a click on the anchor opens
> +   *        the variables view for the current object actor (this.objectActor).
> +   *        If given a string, the string is used a link and clicks open the

"used [as] as link"

@@ +1496,5 @@
> +   *        link in a new tab.
> +   * @return DOMElement
> +   *         The DOM element of the new anchor.
> +   */
> +  _renderAnchor: function(text, handler = this._onClick)

The way the element return by renderAnchor is used throughout the code here, it would be good if you could pass an option class and also automatically attach it to this.element. The many places renderAnchor are used all (I think) do this.

@@ +1543,5 @@
> +{
> +  Widgets.JSObject.call(this, message, objectActor, options);
> +};
> +
> +Widgets.ObjectRenderers.byClass.Date.prototype = Heritage.extend(Widgets.JSObject.prototype,

Adding a new ObjectRenderer seems to be very boilerplatey. Same constructor, similar prototype setup, etc. We could remove the boilerplate and make it more straightforward to add new renderers. Maybe something like:

Widgets.ObjectRenderers.add("class", "Date", {
  render: ...
});

Or maybe even just:

Widgets.ObjectRenderers.add({
  class: "Date",
  render: ...
});

It could produce the same constructor and prototype setup for you automatically. If something in the future needed a more customized setup it would still be possible to manually hook it all up as is done in this code.

@@ +1558,5 @@
> +    container.className = "class-" + this.objectActor.class;
> +
> +    let addStr = (s) => {
> +      container.appendChild(doc.createTextNode(s));
> +    };

This is used in multiple places and looks like it could live on JSObject.prototype:

  addStr: function(s) {
    this.element.appendChild(this.document.createTextNode(s));
  }

@@ +1579,5 @@
> +
> +    let n = doc.createElementNS(XHTML_NS, "span");
> +    n.className = "cm-string-2";
> +    n.textContent = new Date(preview.timestamp).toISOString();
> +    container.appendChild(n);

The above four lines  (creating a span/element, setting class and text, appending to this.element), are repeated many times and is a good candidate to move to JSObject.prototype.

@@ +1722,5 @@
> +      }
> +
> +      if (item !== null) {
> +        let elem = this.message._renderValueGrip(item, { concise: true });
> +        container.appendChild(elem);

The above two lines are repeated a lot as well and would be good to factor out to a method on JSObject.prototype like `addGrip: function(grip) { /***/ }`.

::: browser/devtools/webconsole/test/browser_bug_865871_variables_view_close_on_esc_key.js
@@ +27,3 @@
>      ok(anchor, "object anchor");
> +    ok(body, "message body");
> +    isnot(body.textContent.indexOf('testProp: "testValue"'), -1,

Prefer using `string.contains("str")` instead of comparing indexOf to -1.

::: toolkit/devtools/server/actors/script.js
@@ +3315,5 @@
> +
> +    let length = DevToolsUtils.getProperty(obj, "length");
> +    if (typeof length != "number") {
> +      return false;
> +    }

Except for the above four lines, the previewers for String, Number, and Boolean are identical. Can we factor out the commonality between these?

@@ +3320,5 @@
> +
> +    let raw = obj.unsafeDereference();
> +    let str = null;
> +    try {
> +      str = String.prototype.toString.call(raw);

String.prototype.valueOf will return the same value as String.prototype.toString, if you're attempting to homogenize these three functions.
Brandon, thanks for your review!


(In reply to Brandon Benvie [:benvie] from comment #19)
> Comment on attachment 8382318 [details] [diff] [review]
> bug584733-3.diff
> 
> Review of attachment 8382318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ...
> 
> @@ +1072,5 @@
> > +   *         The class name for the grip.
> > +   */
> > +  getClassNameForValueGrip: function(grip)
> > +  {
> > +    let map = {
> 
> It would be good to centralize these CodeMirror specifics to one localized
> area of the code, or specifically mark them using some keyword. You have
> CodeMirror mentioned in the method's comments, but really the CodeMirror
> specific part is this map.

I'm not sure what I should change here - as you mentioned the comments mark this as CodeMirror-related. Anything else I should do?


> @@ +1124,5 @@
> > +    if (!widget || (widget.canRender && !widget.canRender(objectActor))) {
> > +      widget = Widgets.JSObject;
> > +    }
> > +
> > +    let instance = new widget(this, objectActor, options).render();
> 
> This implicitly assumes that `widget` is not null by this point, but the
> code above doesn't obviously guarantee that. Should either add a comment
> here explicitly stating the assumption, or an assertion to check and throw
> an error if not.

How is this not guaranteed? `widget` is checked and if it's null, we set it to Widgets.JSObject, which is always defined.


> @@ +1449,2 @@
> >   */
> > +Widgets.JSObject = function(message, objectActor, options)
> 
> `options` could be optional based on how it's used.

Right, I missed this.

Going to update the patch based on your comments. Thanks!
(In reply to Mihai Sucan [:msucan] from comment #20)
> (In reply to Brandon Benvie [:benvie] from comment #19)
> > Comment on attachment 8382318 [details] [diff] [review]
> > bug584733-3.diff
> > 
> > Review of attachment 8382318 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ...
> > 
> > @@ +1072,5 @@
> > > +   *         The class name for the grip.
> > > +   */
> > > +  getClassNameForValueGrip: function(grip)
> > > +  {
> > > +    let map = {
> > 
> > It would be good to centralize these CodeMirror specifics to one localized
> > area of the code, or specifically mark them using some keyword. You have
> > CodeMirror mentioned in the method's comments, but really the CodeMirror
> > specific part is this map.
> 
> I'm not sure what I should change here - as you mentioned the comments mark
> this as CodeMirror-related. Anything else I should do?

You're right, nevermind! It's fine as is.

> > @@ +1124,5 @@
> > > +    if (!widget || (widget.canRender && !widget.canRender(objectActor))) {
> > > +      widget = Widgets.JSObject;
> > > +    }
> > > +
> > > +    let instance = new widget(this, objectActor, options).render();
> > 
> > This implicitly assumes that `widget` is not null by this point, but the
> > code above doesn't obviously guarantee that. Should either add a comment
> > here explicitly stating the assumption, or an assertion to check and throw
> > an error if not.
> 
> How is this not guaranteed? `widget` is checked and if it's null, we set it
> to Widgets.JSObject, which is always defined.

I totally misread the last conditional, you're right! Ignore this.
Attached patch bug584733-4.diff (obsolete) — Splinter Review
WIP patch. Saving current progress. Almost addressed all of benvie's comments. Will ask for review when the patch is ready.

(all tests pass locally)
Attachment #8382318 - Attachment is obsolete: true
Attachment #8382318 - Flags: review?(rcampbell)
Attached patch bug584733-5.diffSplinter Review
This patch should address all of your review comments.

Please let me know if I missed anything. Thanks!

Updated try push: https://tbpl.mozilla.org/?tree=Try&rev=49acf73ae883
Attachment #8384853 - Attachment is obsolete: true
Attachment #8385576 - Flags: review?(bbenvie)
Comment on attachment 8385576 [details] [diff] [review]
bug584733-5.diff

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

LGTM with a couple minor nits.

::: browser/devtools/webconsole/console-output.js
@@ +1006,5 @@
> +    return this._renderValueGrip(piece);
> +  },
> +
> +  /**
> +   * Render a grip that represent a value received from the server. This method

represents

@@ +2032,5 @@
> +      case Ci.nsIDOMNode.ATTRIBUTE_NODE: {
> +        let {preview} = this.objectActor;
> +        this.element = this.el("span.attributeNode.kind-" + preview.kind);
> +        let attr = this._renderAttributeNode(preview.nodeName, preview.value, true);
> +        this.element.appendChild(attr);

Attribute nodes are the only ones that do this work here. This work could be moved to _renderAttributeNode.
Attachment #8385576 - Flags: review?(bbenvie) → review+
(In reply to Brandon Benvie [:benvie] from comment #24)
> Comment on attachment 8385576 [details] [diff] [review]
> bug584733-5.diff
> 
> Review of attachment 8385576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM with a couple minor nits.

Thanks!

> ::: browser/devtools/webconsole/console-output.js
> @@ +1006,5 @@
> > +    return this._renderValueGrip(piece);
> > +  },
> > +
> > +  /**
> > +   * Render a grip that represent a value received from the server. This method
> 
> represents

Fixed.


> @@ +2032,5 @@
> > +      case Ci.nsIDOMNode.ATTRIBUTE_NODE: {
> > +        let {preview} = this.objectActor;
> > +        this.element = this.el("span.attributeNode.kind-" + preview.kind);
> > +        let attr = this._renderAttributeNode(preview.nodeName, preview.value, true);
> > +        this.element.appendChild(attr);
> 
> Attribute nodes are the only ones that do this work here. This work could be
> moved to _renderAttributeNode.

This is left intentionally as-is, for bug 757866 which reuses _renderAttributeNode() in the new _renderElementNode().

Going to land the patch. Had a green try run: https://tbpl.mozilla.org/?tree=Try&rev=49acf73ae883
Landed: https://hg.mozilla.org/integration/fx-team/rev/29b248b0fb47
Whiteboard: [console-2][console-output] → [console-2][console-output][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/29b248b0fb47
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [console-2][console-output][fixed-in-fx-team] → [console-2][console-output]
Target Milestone: --- → Firefox 30
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: