659332 - trailing // should disappear if input text is something like "filxxxx" in the LocationBar

Status: RESOLVED WORKSFORME

(Firefox :: Address Bar, defect)

Description

[Alice0775 White]

Build Identifier: 
http://hg.mozilla.org/mozilla-central/rev/456c915b3caf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110524 Firefox/6.0a1 ID:20110524030609

Trailing // should disappear if input text is something like "filxxxx" in the LocationBar

Reproducible: Always

Steps to Reproduce:
1. Start Firefox/6.0a1
2. Open Local file (Ex. file:///D:/localfile.html )
3. Key in LocationBar
   fill


Actual Results:  
fill//


Expected Results:  
fill

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, this is totally broken.  Breaks a number of my bookmark keywords, not to mention opening of actual file:// URIs!

Comment 2

[Daniel Holbert [:dholbert]]

(BTW, I don't hit this if I type super-fast. I definitely hit it when typing at the rate of ~2 chars per second, though.)

Comment 3

[Daniel Holbert [:dholbert]]

Taking the issue back one step here -- it looks like with "file:///" completions, the suggested portion isn't fully highlighted.  The highlighting stops before the final "//" of the suggestion.

So it looks like:    file:///  (where 'fi' is what I typed and '^'=highlight)
                       ^^^^

In all other cases I've tested, the *entire* suggested portion is highlighted (and hence deleted when I type another character or hit backspace).

Comment 4

[Michael Ventnor]

Attachment: Patch

This is caused by not having file:// as one of the valid schemes. Removing http:// from the function causes that to show this behaviour too, strangely...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Wait.  Why do we have some sort of string scheme whitelist/blacklist here?  Won't this break for other schemes not in this list too?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Wait.  Why do we have some sort of string scheme whitelist/blacklist here? 
> Won't this break for other schemes not in this list too?
Yes, I didn't realize this would be the interaction with other protocols when I reviewed this initially.  Need to come up with a better solution here.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 535001 [details] [diff] [review]
Patch

Whitelisting is a losing strategy here.  What about other protocols like chrome:?  Can you please explain how we end up adding the // in there in the first place (I clearly don't understand this code as well as I thought I did)?

Comment 8

[Michael Ventnor]

I was doing what Places already does in other parts in JS code. I'm not sure what other strategy we could use.

I'm also not sure why the // is added without the selection, since debugging that code shows CompleteValue getting the expected value and range (up to the first /).

Considering I may need to take a new approach to fix bug 659445, probably backing out the whole thing seems like a good option.
(We can't just turn off the pref as I think I mistakenly removed some important code, see bug 659672)

Comment 10

[Asa Dotzler [:asa]]

don't need to track this for 6 since the feature has been removed for 6.