Closed Bug 601183 Opened 11 years ago Closed 11 years ago

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

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(blocking2.0 -)

RESOLVED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- -

People

(Reporter: pcwalton, Assigned: jwalker)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Blocks: devtools4b8
Assignee: nobody → jwalker
Blocks: 598498
No longer blocks: devtools4b8
Blocks: 599498
No longer blocks: 598498
No longer blocks: 599498
In short:
.jsterm-input-node textarea::-moz-selection { color: #AAA; }
Attachment #480880 - Flags: feedback?(ddahl)
New patch that implements the alternate text area fix
Attachment #480880 - Attachment is obsolete: true
Attachment #481212 - Flags: feedback?(ddahl)
Attachment #480880 - Flags: feedback?(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.
Attachment #481212 - Flags: feedback?(ddahl) → feedback-
Only tested on Mac.
Just beginning the long task of getting a working Ubuntu.
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.
Attachment #481212 - Flags: feedback- → feedback+
(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.
Attached patch Upload 3 (obsolete) — Splinter Review
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?
Attachment #481212 - Attachment is obsolete: true
Attachment #481805 - Flags: feedback?(ddahl)
Comment on attachment 481805 [details] [diff] [review]
Upload 3

This is major polish work. I like it.
Attachment #481805 - Flags: feedback?(ddahl) → feedback+
Attachment #481805 - Flags: review?(gavin.sharp)
Blocks: devtools4b8
Attached patch Upload 4 (obsolete) — Splinter Review
rebase
Attachment #481805 - Attachment is obsolete: true
Attachment #487564 - Flags: review?(gavin.sharp)
Attachment #481805 - Flags: review?(gavin.sharp)
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.
blocking2.0: --- → ?
Attached patch Upload 5 (obsolete) — Splinter Review
Rebase, fix a test failure
Attachment #487564 - Attachment is obsolete: true
Attachment #489132 - Flags: review?(gavin.sharp)
Attachment #487564 - Flags: review?(gavin.sharp)
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!
Attachment #489132 - Flags: review?(gavin.sharp)
(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.
Attached patch Upload 6 (obsolete) — Splinter Review
Attachment #489132 - Attachment is obsolete: true
Attachment #491198 - Flags: superreview?(gavin.sharp)
mass change: filter on PRIORITYSETTING
Priority: -- → P1
mass change: filter on PRIORITYSETTING
Blocks: devtools4
Attached patch Upload 7 (obsolete) — Splinter Review
Just re-based and re-tested.
Attachment #491198 - Attachment is obsolete: true
Attachment #492958 - Flags: superreview?(gavin.sharp)
Attachment #491198 - Flags: superreview?(gavin.sharp)
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.
Attachment #492958 - Flags: superreview?(gavin.sharp) → review+
Attached patch Upload 8Splinter Review
You're right, the !important is not needed. I've removed it, and rebased.
Attachment #492958 - Attachment is obsolete: true
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)
Attachment #494693 - Flags: approval2.0?
Attachment #494693 - Flags: approval2.0? → approval2.0+
Nice to have, wouldn't hold the release back if this gets landed and re-opened, blocking-.
blocking2.0: ? → -
pushed: http://hg.mozilla.org/mozilla-central/rev/fc18b7041778
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Depends on: 617876
Depends on: 617877
(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...
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.