Closed
Bug 1059846
Opened 10 years ago
Closed 10 years ago
Location bar loads address including >> instead of just the replaced address
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
Tracking | Status | |
---|---|---|
firefox35 | --- | verified |
People
(Reporter: Gijs, Assigned: mak)
References
Details
Attachments
(1 file)
6.52 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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"
Updated•10 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
Updated•10 years ago
|
Points: --- → 5
Comment 1•10 years ago
|
||
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)
Comment hidden (obsolete) |
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
s/code/search string/
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 years ago
|
||
(I've hidden my earlier comment, which is clearly wrong per comment #3)
Assignee | ||
Comment 7•10 years ago
|
||
(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)
Assignee | ||
Comment 8•10 years ago
|
||
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
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
with the nit fixed https://hg.mozilla.org/integration/fx-team/rev/b0e5c955b607
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → Firefox 35
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0e5c955b607
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
QA Contact: alexandra.lucinet
Updated•10 years ago
|
QA Contact: alexandra.lucinet → andrei.vaida
Comment 14•10 years ago
|
||
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
status-firefox35:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•