619910 - Misplaced autocomplete suggestions when typing a command in a new line

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Alex Lakatos[:AlexLakatos]]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

If you write a multi-line command, the autocomplete does not work properly, it is displayed on the first line instead of being displayed after your command. 

Reproducible: Always

Steps to Reproduce:
1.Open Web Console
2.Type a command like console.info("test");
3.Press Shift+Enter from the keyboard to start typing in a new line
4.Write "co" (without quotes) in the console
5.Make sure you get suggestions as you type (autocomplete is working) 
Actual Results:  
The suggestion is shown on the first line. 

Expected Results:  
Suggestion should be placed right after your last typed string.

Comment 1

[Alex Lakatos[:AlexLakatos]]

Attachment: screenshot

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Confirm this on Mozilla/5.0 (Windows NT 6.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

Comment 3

[Alex Lakatos[:AlexLakatos]]

Bug first appeared in Build 20101209030340.

Comment 4

[Joe Walker [:jwalker]]

Attachment: Upload 1

The completion field (which is behind the input field) needs a prefix to push the completion string to the correct location.

Plan A was to replace create a string with the same length. This approach caused this bug because it replaced newlines with spaces.

Plan B is to replace all non control characters with spaces instead. The patch uses the regex /[1-\uF000]/g. I'm open to suggestions of a better regex.
The flaw with plan B is that it assumes mono-spaced fonts work for all characters. Some uncommon characters can not be represented in well in a mono-spaced font.

Plan C uses a span[color=transparent] to separate the padding prefix from the displayed completion value, without doing any character replacement.
Plan C is a significantly more complex patch to address a very rare side case.

This patch implements Plan B.

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 499000 [details] [diff] [review]
Upload 1

The patch looks fine.

Instead of:

+    let prefix = this.inputNode.value.replace(/[!-\uF000]/g, " ");

I'd use:

+    let prefix = this.inputNode.value.replace(/[^\r\n\t ]/g, " ");

But... that's up to the taste of the reviewer as well.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 499000 [details] [diff] [review]
Upload 1

I'm happy to go with either regex. I went for a blanket exclusion of control chars, Mihai suggested something simpler. I suspect that practically there isn't anything to choose between them, so maybe the simpler option is better.

Comment 7

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

yeah, I think I like Mihai's suggestion better. I boggled a bit at the \u at first.

Comment 8

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

Comment on attachment 499000 [details] [diff] [review]
Upload 1

What about just using \S?

It feels kind of wrong to be doing this string munging work just to get alignment right, though... Can we try to come up with a better way to achieve the desired result?

Comment 9

[Joe Walker [:jwalker]]

I'll provide you with a patch for Plan C which is the 'correct way', so we can compare.

Comment 10

[Joe Walker [:jwalker]]

I'll document plan C better, but having thought about it more, I return to my original opinion that while more 'correct' it's too dangerous at this point in the cycle.

Plan C
------
1. Change the type of the completion node from textbox to a div (or similar), so the completion node can contain styled elements.

2. Rather than setting the completion node to be (input text converted to spaces + completion text) use something like:
  <span class=hiddenInput>{inputNode.value}</span><span class=completion>{completion}</span>

3. Use:
    .hiddenInput { font-color:transparent; }
    .completion { font-color:grey; }

4. Take the used styles of the input node (except font-color), and apply them to the completion node (this step was not previously required since they were both textboxes, and had the same default styles)


The problem is that any omissions or errors in copying styles from the inputNode textbox to the completeNode div cause bugs, and sometimes ones that are hard to test for or that only manifest themselves in peculiar circumstances.


Obviously, I'm happy for there to be a plan D, or for plan C2.

Comment 11

[Joe Walker [:jwalker]]

Attachment: [checked-in] Upload 2

I'm not sure we have any better ideas than this:

  let prefix = this.inputNode.value.replace(/[\S]/g, " ");

I've updated the regex to be this and re-tested, and I propose that we commit this in a spare space.

Comment 12

[Kevin Dangoor]

Unless Gavin has any additional comments, I think we should go ahead and ask for approval on the new patch.

Comment 13

[Joe Walker [:jwalker]]

Gavin has given it r+ and suggested the \S. Should I ask someone for super-review?

Comment 14

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

not necessary. I think this is ready to land.

Comment 15

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

got a little ahead of myself. Pending approval this can land.

Comment 16

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

Comment on attachment 500989 [details] [diff] [review]
[checked-in] Upload 2

Sorry - feel free to just ping me directly if I forget to set approval+ when reviewing something.

Comment 17

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

Comment on attachment 500989 [details] [diff] [review]
[checked-in] Upload 2

http://hg.mozilla.org/mozilla-central/rev/90a8518157c3