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)
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)
8.20 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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"
mozregression gave me this: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bea6758824d9&tochange=6b20dd2e20cc
Comment 2•9 years ago
|
||
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
tracking-firefox41:
--- → ?
tracking-firefox42:
--- → ?
Comment 3•9 years ago
|
||
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.
(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.
Comment 5•9 years ago
|
||
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•9 years ago
|
Rank: 33
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8664476 -
Flags: review?(mak77)
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72bb76128c99
Comment 8•9 years ago
|
||
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•8 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•8 years ago
|
||
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•8 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•8 years ago
|
||
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 13•8 years ago
|
||
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•8 years ago
|
||
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•8 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 16•8 years ago
|
||
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•8 years ago
|
||
Try results look OK. Lots of failures, but they all look known.
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/66129e10f116
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/66129e10f116
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 20•8 years ago
|
||
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]
Comment 21•8 years ago
|
||
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.
Description
•