Typed text on url bar is lost with unified complete

VERIFIED FIXED in Firefox 46

Status

()

P3
normal
Rank:
33
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: rodrigozeh, Assigned: adw)

Tracking

(Blocks: 1 bug, {regression})

41 Branch
Firefox 46
regression
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox40 unaffected, firefox41 affected, firefox42 affected, firefox46 verified)

Details

(Whiteboard: [unifiedcomplete][fxsearch][bugday-20151216])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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
status-firefox40: --- → unaffected
status-firefox41: --- → affected
status-firefox42: --- → affected
Component: Untriaged → Location Bar
Ever confirmed: true
Keywords: regression
Version: 42 Branch → 41 Branch

Updated

3 years ago
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
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
tracking-firefox41: ? → ---
tracking-firefox42: ? → ---
(Reporter)

Comment 4

3 years ago
(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]

Updated

3 years ago
Rank: 33
(Assignee)

Comment 6

3 years ago
Created attachment 8664476 [details] [diff] [review]
patch
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)
(Assignee)

Comment 9

3 years ago
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
(Assignee)

Comment 10

3 years ago
Created attachment 8694944 [details] [diff] [review]
patch v2

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)
(Assignee)

Comment 11

3 years ago
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. :-))
(Assignee)

Comment 12

3 years ago
Created attachment 8694962 [details] [diff] [review]
patch v2.1

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)
(Assignee)

Comment 14

3 years ago
Created attachment 8697444 [details] [diff] [review]
patch v3 with test

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)
(Assignee)

Comment 15

3 years ago
(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+
(Assignee)

Comment 17

3 years ago
Try results look OK.  Lots of failures, but they all look known.

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/66129e10f116
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
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
status-firefox46: fixed → verified
Whiteboard: [unifiedcomplete][fxsearch] → [unifiedcomplete][fxsearch][bugday-20151216]
You need to log in before you can comment on or make changes to this bug.