601183 - Investigate Bespin/Safari-style completion styling for the Web Console

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

Dave points out in bug 599498 that the styling of the completion, with the selection highlight, is a bit distracting. This bug is for investigating the possibility of using a more muted color for the completion, and avoiding highlighting.

cc-ing Joe because he dealt with similar issues with Bespin's autocomplete.

Comment 1

[Joe Walker [:jwalker]]

Attachment: Add ::-moz-selection to change font color

In short:
.jsterm-input-node textarea::-moz-selection { color: #AAA; }

Comment 2

[Joe Walker [:jwalker]]

Attachment: The double input field version of the fix

New patch that implements the alternate text area fix

Comment 3

[David Dahl :ddahl]

Comment on attachment 481212 [details] [diff] [review]
The double input field version of the fix

Did you test on linux? This is a picture of the console after typing "window.docu": http://yfrog.com/ccselection001ap

I saw some nits like elses not after a newline. Perhaps when I get into the office I can test this patch on windows or mac.

Comment 4

[Joe Walker [:jwalker]]

Only tested on Mac.
Just beginning the long task of getting a working Ubuntu.

Comment 5

[David Dahl :ddahl]

Comment on attachment 481212 [details] [diff] [review]
The double input field version of the fix


>+          case 39:
>+            // right arrow: accept proposed completion
>+            self.acceptProposedCompletion();
>+            break;

so this bug is mainly about adding right-arrow support as well as the UI for suggesting completion strings? Cool.

>+
>+    // Only complete if the selection is empty and at the end of the input.
>+    if (inputNode.selectionStart == inputNode.selectionEnd &&
>+        inputNode.selectionEnd != inputValue.length) {
>+      // TODO: shouldnt we do this in the other 'bail' cases?
perhaps. Do we end up in a weird state otherwise? can a test illustrate this problem? (is that overkill?:))

>-      }
>+      this.updateCompleteNode(matches[matchIndexToUse].substring(matchOffset));
>+    } else {
nit: newline after else

>+    let inputStack = this.xulElementFactory("stack");
>+    inputStack.setAttribute("class", "jsterm-stack-node");
>+    inputStack.setAttribute("flex", "1");
>+    inputContainer.appendChild(inputStack);
>+
Is the stack used to cache possible completion strings for the UI? I ask as I am seeing overlapping completion suggestions on top of the initially typed input

>     let inputNode = this.xulElementFactory("textbox");
>     inputNode.setAttribute("class", "jsterm-input-node");
>-    inputNode.setAttribute("flex", "1");
>     inputNode.setAttribute("multiline", "true");
>     inputNode.setAttribute("rows", "1");
>-    inputContainer.appendChild(inputNode);
>+    inputStack.appendChild(inputNode);
> 
Right now the textbox has 2 lines, we had a similar issue styling it initially.

>       // construction
>       term.appendChild(outputNode);
>       term.appendChild(inputNode);
>+      term.appendChild(completeNode);
>+      // TODO: Why do we appendChild these to both term and inputContainter?
the input node append seems wrong. i have no idea. I think julian did that.

> 
>       this.outputNode = outputNode;
>       this.inputNode = inputNode;
>+      this.completeNode = completeNode;
>       this.term = term;
>+      // TODO: refactor these to outside the if?
why refactor outside the if?  


The css does seem a bit jacked, but that was the case when we started this as well. I think you are moving in a good direction. I initially thought you had more working. Sorry about that.

Comment 6

[Joe Walker [:jwalker]]

(In reply to comment #5)
> Comment on attachment 481212 [details] [diff] [review]
> The double input field version of the fix
> 
> 
> >+          case 39:
> >+            // right arrow: accept proposed completion
> >+            self.acceptProposedCompletion();
> >+            break;
> 
> so this bug is mainly about adding right-arrow support as well as the UI for
> suggesting completion strings? Cool.

I don't know about 'mainly'!
Previously right arrow worked because it moved the cursor to the end of the line. Now we don't have any selected text to 'unselect', I needed to add support for that behavior.

> >+    // Only complete if the selection is empty and at the end of the input.
> >+    if (inputNode.selectionStart == inputNode.selectionEnd &&
> >+        inputNode.selectionEnd != inputValue.length) {
> >+      // TODO: shouldnt we do this in the other 'bail' cases?
> perhaps. Do we end up in a weird state otherwise? can a test illustrate this
> problem? (is that overkill?:))

I haven't noticed a bug, although the inconsistency did send alarm bells off. I'm thinking that in late betas, it's probably not the best time to be hacking about with things that I don't understand too well though.

> >-      }
> >+      this.updateCompleteNode(matches[matchIndexToUse].substring(matchOffset));
> >+    } else {
> nit: newline after else

OK.

> >+    let inputStack = this.xulElementFactory("stack");
> >+    inputStack.setAttribute("class", "jsterm-stack-node");
> >+    inputStack.setAttribute("flex", "1");
> >+    inputContainer.appendChild(inputStack);
> >+
> Is the stack used to cache possible completion strings for the UI? I ask as I
> am seeing overlapping completion suggestions on top of the initially typed
> input

The stack is used to group the 2 input boxes on top of each other. That's how this fix works.

> >     let inputNode = this.xulElementFactory("textbox");
> >     inputNode.setAttribute("class", "jsterm-input-node");
> >-    inputNode.setAttribute("flex", "1");
> >     inputNode.setAttribute("multiline", "true");
> >     inputNode.setAttribute("rows", "1");
> >-    inputContainer.appendChild(inputNode);
> >+    inputStack.appendChild(inputNode);
> > 
> Right now the textbox has 2 lines, we had a similar issue styling it initially.

I'm installing ubuntu as I type, so I can check this out.

> >       // construction
> >       term.appendChild(outputNode);
> >       term.appendChild(inputNode);
> >+      term.appendChild(completeNode);
> >+      // TODO: Why do we appendChild these to both term and inputContainter?
> the input node append seems wrong. i have no idea. I think julian did that.
> 
> > 
> >       this.outputNode = outputNode;
> >       this.inputNode = inputNode;
> >+      this.completeNode = completeNode;
> >       this.term = term;
> >+      // TODO: refactor these to outside the if?
> why refactor outside the if?  

The last few lines of both the then and the else are the same.

> The css does seem a bit jacked, but that was the case when we started this as
> well. I think you are moving in a good direction. I initially thought you had
> more working. Sorry about that.

On the Mac, it is ...

Joe.

Comment 7

[Joe Walker [:jwalker]]

Attachment: Upload 3

I started with the intention of simply removing the TODOs and fixing the else nit, however I discovered a minor bug with the whole 'term' thing, and decided that we really needed to simplify the naming, which had obscured the bug in the first place.

This minor update fixes the naming, and in the process makes the creation function much smaller and clearer.

If we need minimum lines of churn, then we should go back and tweak the original patch, but I think this is much better.

The code has been checked on Mac and Linux, and I'll be doing the same on Windows soon.

ddahl - can we go with this version, which I think is much cleaner?

Comment 8

[David Dahl :ddahl]

Comment on attachment 481805 [details] [diff] [review]
Upload 3

This is major polish work. I like it.

Comment 9

[Joe Walker [:jwalker]]

Attachment: Upload 4

rebase

Comment 10

[Kevin Dangoor]

requesting blocking on this: this patch changes the completion to provide proper visual styling in the way that pretty much every other browser-based completion has ended up going. this makes console completion a lot easier on the eyes.

Comment 11

[Joe Walker [:jwalker]]

Attachment: Upload 5

Rebase, fix a test failure

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 489132 [details] [diff] [review]
Upload 5

I'm not going to have the time to read/learn enough of this code to actually provide a full review any time soon, but I shouldn't be blocking it because of that, given that you've already gotten feedback+ from ddahl, so I'll just treat this as a SR instead.

From a quick skim:
- we should avoid making bug 582340 worse (I'm not sure whether you actually need to keep a reference to the nodes you're adding)
- I imagine this conflicts with bug 601667 quite a bit
- we really should have a bug on file on changing those numbers to be Ci.nsIDOMKeyEvent.DOM_VK_* constants (e.g. Ci.nsIDOMKeyEvent.DOM_VK_RIGHT for the case you're adding) if we don't already have one.

The patch is currently out of date given other console changes that have since landed; If you attach an updated patch I promise to be more responsive than I have been!

Comment 13

[Joe Walker [:jwalker]]

(In reply to comment #12)
...
> From a quick skim:
> - we should avoid making bug 582340 worse (I'm not sure whether you actually
> need to keep a reference to the nodes you're adding)

There is some clarification of the nodes we create, which perhaps makes it look worse, but I'm fairly sure I've only added one (completeNode), and that's needed

> - I imagine this conflicts with bug 601667 quite a bit

I guess so, but I don't think it will be hard to unravel.

> - we really should have a bug on file on changing those numbers to be
> Ci.nsIDOMKeyEvent.DOM_VK_* constants (e.g. Ci.nsIDOMKeyEvent.DOM_VK_RIGHT for
> the case you're adding) if we don't already have one.

Fixed in the new patch.

> The patch is currently out of date given other console changes that have since
> landed; If you attach an updated patch I promise to be more responsive than I
> have been!

Huh. HG pull did the merge for me - I hope there isn't something else causing the problem (like bug 601667?)

Anyway the new patch is rebased.

Comment 14

[Joe Walker [:jwalker]]

Attachment: Upload 6

Comment 15

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 16

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 17

[Joe Walker [:jwalker]]

Attachment: Upload 7

Just re-based and re-tested.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 492958 [details] [diff] [review]
Upload 7

Are all of those !importants in the styling actually needed? IIRC that styling was revised recently to remove much of the need for !important.

Comment 19

[Joe Walker [:jwalker]]

Attachment: Upload 8

You're right, the !important is not needed. I've removed it, and rebased.

Comment 20

[Kevin Dangoor]

Comment on attachment 494693 [details] [diff] [review]
Upload 8

requesting approval for this patch: it improves the appearance of the Web Console's command line to match what pretty much everyone else is doing now (WebKit and Firebug)

Comment 21

[Dietrich Ayala (:dietrich)]

Nice to have, wouldn't hold the release back if this gets landed and re-opened, blocking-.

Comment 22

[David Dahl :ddahl]

pushed: http://hg.mozilla.org/mozilla-central/rev/fc18b7041778

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Nice to have, wouldn't hold the release back if this gets landed and re-opened,
> blocking-.

Apropos "re-opened", I actually just considered backing this out because of bug 617877 (also bug 617876), as I think we're slowly approaching a phase in the development cycle where approved patches aren't allowed to cause regressions if the regressions are worse than the fixed bug...