Closed Bug 580820 Opened 12 years ago Closed 12 years ago

Visually distinguish JavaScript input from output in the Console


(DevTools :: General, defect, P2)



(blocking2.0 betaN+)

Firefox 4.0b3
Tracking Status
blocking2.0 --- betaN+


(Reporter: pcwalton, Assigned: pcwalton)




(2 files, 1 obsolete file)

Attached patch Patch. (obsolete) — Splinter Review
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.
Attachment #459199 - Flags: review?(ddahl)
Attachment #459199 - Flags: review?(ddahl) → feedback?(ddahl)
Comment on attachment 459199 [details] [diff] [review]

>+  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");
>     }

The only thing this is missing is the css class selectors and how they are defined.
Attachment #459199 - Flags: feedback?(ddahl) → feedback-
> 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.
New version of the patch adds unit tests.
Attachment #459199 - Attachment is obsolete: true
Attachment #459973 - Flags: feedback?(ddahl)
Severity: normal → major
Priority: -- → P2
Target Milestone: --- → Firefox 4.0b3
Version: unspecified → Trunk
Important to the functionality of the js terminal in the console. blocking:betaN+
blocking2.0: ? → betaN+
Comment on attachment 459973 [details] [diff] [review]
Proposed patch, version 2.

nice patch. just needs a followup css patch, correct?
Attachment #459973 - Flags: feedback?(ddahl) → feedback+
Attachment #459973 - Flags: review?(dtownsend)
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.
Attachment #459973 - Flags: review?(dtownsend) → review+
Fixed per Mossop's comments.
Keywords: checkin-needed
Comment on attachment 460923 [details] [diff] [review]
[checked-in] Proposed patch, version 3.
Attachment #460923 - Attachment description: Proposed patch, version 3. → [checked-in] Proposed patch, version 3.
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.