Closed Bug 1182783 Opened 9 years ago Closed 8 years ago

Typed text on url bar is lost with unified complete

Categories

(Firefox :: Address Bar, defect, P3)

41 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox40 --- unaffected
firefox41 --- affected
firefox42 --- affected
firefox46 --- verified

People

(Reporter: rodrigozeh, Assigned: adw)

References

Details

(Keywords: regression, Whiteboard: [unifiedcomplete][fxsearch][bugday-20151216])

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150710030206

Steps to reproduce:

1. Type "www.mozilla.org" in url bar and press enter
2. Type "moz" in url bar
3. Press down and up


Actual results:

url bar is now filled with the entire autocomplete url


Expected results:

url bar should have only the originally typed text "moz"
Probably caused by:
Marco Bonardo — Bug 1168811 - Enable Unified AutoComplete. r=Mossop

Related/Dupe:
Bug #1173202
Bug #1172937
Blocks: 1168811
Status: UNCONFIRMED → NEW
Component: Untriaged → Location Bar
Ever confirmed: true
Keywords: regression
Version: 42 Branch → 41 Branch
it's not exactly a regression, default completion always worked like that (the old autocomplete implementation was cheating), restoring selection would probably be a nice enhancement to implement, but it's not blocking the feature imo, cause the steps involved are not really common or needed to achieve the expected result.
So, while I'd love to have this fixed, I don't think we need to track it.
Blocks: 1071461
(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #3)
> it's not exactly a regression, default completion always worked like that
> (the old autocomplete implementation was cheating), restoring selection
> would probably be a nice enhancement to implement, but it's not blocking the
> feature imo, cause the steps involved are not really common or needed to
> achieve the expected result.
> So, while I'd love to have this fixed, I don't think we need to track it.

Autcomplete has always restored the typed text for 10 years ever since it was introduced, before it was broken (certainly inadvertently) by this update. 

As for being common, I encounter this multiple times everyday. Didn't imagine it was uncommon to type some text and not find the url you wanted then go back to type again.
I'm not saying it's fine, I'm saying it's an old bug we just made visible. the previous autocomplete was sort of an hack and was hiding it.
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [unifiedcomplete][fxsearch]
Rank: 33
Attached patch patch (obsolete) — Splinter Review
Attachment #8664476 - Flags: review?(mak77)
Comment on attachment 8664476 [details] [diff] [review]
patch

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

looks like there are various failures on Try, and off-hand doesn't look like this is restoring "typed[filled]" but just "typed"? i think if we autofilled, we should go back to the autofilled text (as it is now) and restore the selection too. Indeed the user is keying UP to a "visituri" entry, like visit: mozilla.org, we should not fill "moz".  The user will still be able to backspace the autofilled part.

Looks like something is overwriting mSearchString with the autofilled value (could be for valid reasons)? This way it's hard (or impossible) to go back to what the user typed and what we filled, to restore a proper selection.
Attachment #8664476 - Flags: review?(mak77)
Rodze, it doesn't sound like we'll fix this bug quite the way you want, but in the meantime, you can press Esc after keying back up to remove the autofilled part (and also close the popup).
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attached patch patch v2 (obsolete) — Splinter Review
This works, but it seems too simple.  Autofilled results should be unique for any given search string, right?  So there's no danger that value.Equals(mPlaceholderCompletionString) == true even for a result that was not actually autofilled.
Attachment #8664476 - Attachment is obsolete: true
Attachment #8694944 - Flags: feedback?(mak77)
Comment on attachment 8694944 [details] [diff] [review]
patch v2

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

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +450,4 @@
>          if (selectedIndex >= 0) {
>            //  A result is selected, so fill in its value
>            nsAutoString value;
>            if (NS_SUCCEEDED(GetResultValueAt(selectedIndex, false, value))) {

The new chunk should be inside this conditional.  (But I'm only asking for f? not r? right now. :-))
Attached patch patch v2.1 (obsolete) — Splinter Review
Well, here's a slightly better patch with a try run, and might as well ask for r?.  Probably needs a new test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e89811b5d4
Attachment #8694944 - Attachment is obsolete: true
Attachment #8694944 - Flags: feedback?(mak77)
Attachment #8694962 - Flags: review?(mak77)
Comment on attachment 8694962 [details] [diff] [review]
patch v2.1

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

Note, due to the many consumers behaviors, it's very hard to evaluate eventual misbehaviors introduced by these simple changes in autocomplete, so, regardless, I'd suggest to not land it until the 46 cycle begins.

And yes, a test in toolkit/autocomplete would be really nice.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +455,5 @@
> +            // Restore selection in the input if the popup's new selection is
> +            // the previously autofilled string.  Otherwise don't select
> +            // anything and move the caret to the end.
> +            int32_t start = value.Equals(mPlaceholderCompletionString) ?
> +                            mSearchString.Length() : value.Length();

I have one doubt regarding casing.
If the user types "MOZ" we will autofill to "MOZ[illa.com]", I *think* in that case mPlaceholderCompletionString is "MOZilla.com". We can still use it, but we need to do a case insensitive comparison and restore the placeholder instead of the value, to restore the typed text properly.
Attachment #8694962 - Flags: review?(mak77)
This makes the case-related changes and adds a test.  I had to add AutocompletePopupBase to the head.js to handle key presses.  Tests pass locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b7cfa5d4d9b
Attachment #8694962 - Attachment is obsolete: true
Attachment #8697444 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #13)
> If the user types "MOZ" we will autofill to "MOZ[illa.com]", I *think* in
> that case mPlaceholderCompletionString is "MOZilla.com".

That's right BTW. :-)
Comment on attachment 8697444 [details] [diff] [review]
patch v3 with test

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

The merge is in a few days, you can land this as soon as that is done.

Thank you for looking into the issue.
Attachment #8697444 - Flags: review?(mak77) → review+
Try results look OK.  Lots of failures, but they all look known.
https://hg.mozilla.org/mozilla-central/rev/66129e10f116
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
I have reproduced this bug on Nightly 42.0a1 (2015-07-11) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 46.0a1!

Build ID: 20151216030229
User Agent: Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0
QA Whiteboard: [bugday-20151216]
I have reproduced this bug with Firefox Nightly 42.0a1 (Build ID: 20150711030210) on 
windows 8.1 64-bit .

Verified as fixed with Firefox Nightly 46.0a1 (Build ID: 20151221030239)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0

As this bug is also verified on Ubuntu (Comment 20),marking this as verified.
Status: RESOLVED → VERIFIED
Whiteboard: [unifiedcomplete][fxsearch] → [unifiedcomplete][fxsearch][bugday-20151216]
You need to log in before you can comment on or make changes to this bug.