580820 - Visually distinguish JavaScript input from output in the Console

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Patrick Walton (:pcwalton)]

Attachment: Patch.

Currently, the user has no easy way to distinguish input from output when entering expressions into the Console. This patch fixes this in three ways:

(1) Internally, input is distinguished from output in the JavaScript part of the console.
(2) ">" is prepended to the input.
(3) Separate CSS classes for both input and output are used, so that the theme may distinguish them.

This patch is part of the Firefox 4 Console UI. The current UI for the console is simply a placeholder and has not yet been designed from a UX perspective (see bug 559481). Because I am requesting blocking status for the Console UI, and this is a critical part of the Console user experience, I am requesting that this bug block the Firefox 4 release as well.

Comment 1

[David Dahl :ddahl]

Comment on attachment 459199 [details] [diff] [review]
Patch.


>+  writeOutput: function JST_writeOutput(aOutputMessage, aIsInput)
>   {
>     var node = this.elementFactory("div");
>-    if (this.cssClassOverride) {
>-      node.setAttribute("class", this.cssClassOverride);
>+    if (aIsInput) {
>+      node.setAttribute("class", "jsterm-input-line");
>+      aOutputMessage = "> " + aOutputMessage;
>     }
>     else {
>       node.setAttribute("class", "jsterm-output-line");
>     }
nice.

The only thing this is missing is the css class selectors and how they are defined.

Comment 2

[Patrick Walton (:pcwalton)]

> The only thing this is missing is the css class selectors and how they are
> defined.

This was intentional. There are three reasons:

(1) Breaking up functionality and styling makes each patch smaller.
(2) The styling will look out of place until the rest of the functionality dependencies of bug 559481 land.
(3) Putting all output console UI into one bug will make it easier for UX review.

Nevertheless, if you want me to, I'll add some placeholder CSS rules into the themes.

Comment 3

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

New version of the patch adds unit tests.

Comment 4

[Johnathan Nightingale [:johnath]]

Important to the functionality of the js terminal in the console. blocking:betaN+

Comment 5

[David Dahl :ddahl]

Comment on attachment 459973 [details] [diff] [review]
Proposed patch, version 2.

nice patch. just needs a followup css patch, correct?

Comment 6

[Patrick Walton (:pcwalton)]

Yup.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 459973 [details] [diff] [review]
Proposed patch, version 2.

>       if (result || result === false || result === " ") {
>-        this.writeOutput(result);
>+        this.writeOutput(result, false);
>       }
>       else if (result === undefined) {
>-        this.writeOutput("undefined");
>+        this.writeOutput("undefined", false);
>       }
>       else if (result === null) {
>-        this.writeOutput("null");
>+        this.writeOutput("null", false);

I'm guessing the regular case is false. You could just not pass the argument in that case. I'm fine either way really.

>+    if (this.cssClassOverride) {
>+      let klass = node.getAttribute("class") + " " + this.cssClassOverride;
>+      node.setAttribute("class", klass);

Use node.classList.add instead.

Comment 8

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, version 3.

Fixed per Mossop's comments.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 460923 [details] [diff] [review]
[checked-in] Proposed patch, version 3.

http://hg.mozilla.org/mozilla-central/rev/ca2c35a64ad1