Closed
Bug 601183
Opened 14 years ago
Closed 14 years ago
Investigate Bespin/Safari-style completion styling for the Web Console
Categories
(DevTools :: General, defect, P1)
DevTools
General
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)
(deleted),
patch
|
Gavin
:
approval2.0+
|
Details | Diff | Splinter Review |
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.
Updated•14 years ago
|
Blocks: devtools4b8
Updated•14 years ago
|
Assignee: nobody → jwalker
Updated•14 years ago
|
Updated•14 years ago
|
Assignee | ||
Comment 1•14 years ago
|
||
In short:
.jsterm-input-node textarea::-moz-selection { color: #AAA; }
Attachment #480880 -
Flags: feedback?(ddahl)
Assignee | ||
Comment 2•14 years ago
|
||
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 3•14 years ago
|
||
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-
Assignee | ||
Comment 4•14 years ago
|
||
Only tested on Mac.
Just beginning the long task of getting a working Ubuntu.
Comment 5•14 years ago
|
||
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+
Assignee | ||
Comment 6•14 years ago
|
||
(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.
Assignee | ||
Comment 7•14 years ago
|
||
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 8•14 years ago
|
||
Comment on attachment 481805 [details] [diff] [review]
Upload 3
This is major polish work. I like it.
Attachment #481805 -
Flags: feedback?(ddahl) → feedback+
Assignee | ||
Updated•14 years ago
|
Attachment #481805 -
Flags: review?(gavin.sharp)
Updated•14 years ago
|
Blocks: devtools4b8
Assignee | ||
Comment 9•14 years ago
|
||
rebase
Attachment #481805 -
Attachment is obsolete: true
Attachment #487564 -
Flags: review?(gavin.sharp)
Attachment #481805 -
Flags: review?(gavin.sharp)
Comment 10•14 years ago
|
||
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: --- → ?
Assignee | ||
Comment 11•14 years ago
|
||
Rebase, fix a test failure
Attachment #487564 -
Attachment is obsolete: true
Attachment #489132 -
Flags: review?(gavin.sharp)
Attachment #487564 -
Flags: review?(gavin.sharp)
Comment 12•14 years ago
|
||
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)
Assignee | ||
Comment 13•14 years ago
|
||
(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.
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #489132 -
Attachment is obsolete: true
Attachment #491198 -
Flags: superreview?(gavin.sharp)
Assignee | ||
Comment 17•14 years ago
|
||
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 18•14 years ago
|
||
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+
Assignee | ||
Comment 19•14 years ago
|
||
You're right, the !important is not needed. I've removed it, and rebased.
Attachment #492958 -
Attachment is obsolete: true
Comment 20•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #494693 -
Flags: approval2.0? → approval2.0+
Comment 21•14 years ago
|
||
Nice to have, wouldn't hold the release back if this gets landed and re-opened, blocking-.
blocking2.0: ? → -
Comment 22•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Target Milestone: --- → Firefox 4.0b8
Comment 23•14 years ago
|
||
(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...
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•