Closed Bug 1059846 Opened 6 years ago Closed 6 years ago

Location bar loads address including >> instead of just the replaced address

Categories

(Firefox :: Address Bar, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.1
Tracking Status
firefox35 --- verified

People

(Reporter: Gijs, Assigned: mak)

References

Details

Attachments

(1 file)

STR:

0. Clean profile
1. In the first tab that opens (https://www.mozilla.org/en-US/firefox/nightly/firstrun/), stop the load, and clear the location bar
2. Type "www.g"
3. After autocomplete appears, click the "go" arrow with the mouse or hit enter (both seem to work/break similarly)


ER:
Autocomplete to www.google.com

AR:
Loads a search page for "www.g >> google.com"
Flags: qe-verify+
Flags: firefox-backlog+
Points: --- → 5
I may work on that but before taking it I have some questions.

Where does that functionality come from and what's the reason for that? It looks pretty ugly but I guess it was discussed somewhere and there is some reason we do it (personally, I like what we have now much more - just add the rest of the text we suggest without this fancy ">>". It's short and clear)
Flags: needinfo?(gijskruitbosch+bugs)
in urlbar we should =*_NEVER_*= see ">>" completion. unfortunately in some edge-cases autocompleteController still tries to do that, to avoid that the search provider(UnifiedComplete.js) tries to never return an autoFill result that doesn't match the beginning of the current code.
s/code/search string/
I'll be happy to dive into the search/suggestion code. I'm also happy that seeing ">>" at all is a bug as it looks really weird. So I guess this is not a feature but a regression, right?

I did a quick code search. Is the code responsible for that here: http://hg.mozilla.org/mozilla-central/file/e58842c764dd/toolkit/components/autocomplete/nsAutoCompleteController.cpp#l1637?

I'd welcome any more information about this bug. Please keep in mind that I'll see this code for the first time so I don't really know its mechanics.
Flags: needinfo?(mak77)
(I've hidden my earlier comment, which is clearly wrong per comment #3)
(In reply to Tomasz Kołodziejski [:tomasz] from comment #5)
> I'll be happy to dive into the search/suggestion code. I'm also happy that
> seeing ">>" at all is a bug as it looks really weird. So I guess this is not
> a feature but a regression, right?

Let me be clear (or at least I hope so, let me know if you have questions).

" >> " is used to implement what we call in-the-middle completion, that means it matches something in the value, but it's not at the beginning of it.
This is used for example in Thunderbird if you try to complete "Marco Bonardo - mbonardo@..." by typing "mbo", and you will get "mbo >> Marco Bonardo - mbonardo@..."

This is a feature by itself, not a regression and it's useful in Thunderbird.

Though, we did a clear choice in the Firefox urlbar that we NEVER want to complete in-the-middle cause the user is typing and we want to return only results that match at the beginning, for various reasons (we can match tags, title, url... we don't want to distract the user with unrelated results, want provide more value by restricting to specific behavior, and so on...).

Thus, the fact urlbar shows it in some edge-cases IS a regression.

There is no way (currently) to tell controller we don't want in-the-middle completion, so what we did is to only set defaultIndex if the found value and the string match at the beginning. this way we never enter the "else" branch that does in-the-middle completion.
Looks like this is an edge case where some condition causes us to hit that code path.

> I did a quick code search. Is the code responsible for that here:
> http://hg.mozilla.org/mozilla-central/file/e58842c764dd/toolkit/components/
> autocomplete/nsAutoCompleteController.cpp#l1637?

right.

> I'd welcome any more information about this bug. Please keep in mind that
> I'll see this code for the first time so I don't really know its mechanics.

I see 2 possibilities to fix this:
1. find the race condition that causes us to return something that doesn't match at the beginning
2. provide an attribute that allows a consumer to opt-out of in-the-middle completion.

I think even doing both might be a win in the end.
Flags: needinfo?(mak77)
Marco, since my other bug is waiting for review, I'm taking this. please update the backlog accordingly.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Added to IT 35.1

(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #8)
> Marco, since my other bug is waiting for review, I'm taking this. please
> update the backlog accordingly.
Iteration: --- → 35.1
Attached patch patch v1Splinter Review
This fixes the problem.

I had to remove an assertion (that I added in the past!) cause I was wrong.  The final value can also not match the input value, this is something completely up to the search provider to decide, the controller shouldn't have veto power over it.
It's the value that in our case should contain the input, but we can't assert that due to existence of in-the-middle completion.
Attachment #8486651 - Flags: review?(ttaubert)
Comment on attachment 8486651 [details] [diff] [review]
patch v1

Review of attachment 8486651 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: toolkit/components/places/UnifiedComplete.js
@@ +729,5 @@
>  
>      let match = yield PlacesSearchAutocompleteProvider.findMatchByToken(
>                                                             this._searchString);
>      if (match) {
> +      // The match doesn't contains a 'scheme://www.' prefix, but since we have

Nit: doesn't contain
Attachment #8486651 - Flags: review?(ttaubert) → review+
with the nit fixed https://hg.mozilla.org/integration/fx-team/rev/b0e5c955b607
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → Firefox 35
https://hg.mozilla.org/mozilla-central/rev/b0e5c955b607
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida
Reproduced the bug with Nightly 35 (2014-08-28 / 20140828030205).

This is now verified fixed on Nightly 35 (2014-09-14), using Windows 7 64-bit, Ubuntu 14.04 LTS 64-bit and Mac OS X 10.9.4. Testing was based on Comment 0 and Comment 7.

Acceptance criteria for this patch:
- The ">>" symbol is no longer displayed for suggested URLs, as in-the-middle auto-completion is no longer occurring in the awesomebar.
Status: RESOLVED → VERIFIED
Depends on: 1067602
You need to log in before you can comment on or make changes to this bug.