494410 - autocomplete fields with completeDefaultIndex can't be reasonably edited

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Simon Bünzli]

Steps to Reproduce:
1. Set browser.urlbar.autoFill to true.
2. Type "moz", wait for a suggestion to show up and then hit the Home key

Actual result:
The typed text is replaced e.g. with "http://www.mozilla.com/en-US/firefox/about/".

Expected result:
The typed text remains: "moz".

Last working nightly:
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1b5pre) Gecko/20090520 Shiretoko/3.5b5pre
Broken in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre

Comment 1

[Shawn Wilsher :sdwilsh]

I don't think this should block since we don't ship with this pref set this way by default.

Comment 2

[Simon Bünzli]

Reasons for blocking: This pretty significantly changes the behavior of the completeDefaultIndex attribute for all autocomplete textboxes.

Comment 3

[Marco Bonardo [:mak]]

could be i missed a condition, need to check. i'm pretty sad sounds like there is no test for this!

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Simon: can you give me some STR that apply to comment 2?

Comment 5

[Simon Bünzli]

Steps to Reproduce:
1. Open the Error Console and evaluate the following two code blocks (turning the evaluation field into a form autocomplete field):

with (top.gTextBoxEval) { setAttribute("type", "autocomplete"); setAttribute("autocompletesearch", "form-history"); setAttribute("autocompletesearchparam", "JSConsoleWindow#TextboxEval"); }

with (top.gTextBoxEval) { completeDefaultIndex = true; Components.classes["@mozilla.org/satchel/form-history;1"].getService(Components.interfaces.nsIFormHistory2).addEntry(getAttribute("autocompletesearchparam"), "test"); }

2. Clear the evaluation field and enter "te" (which should automatically complete to "te[st]").
3. Hit Backspace or Delete (to delete the "[st]") and Home or Left arrow

Actual result:
The field reads once again "test".

Expected result:
Autofill shouldn't have been activated and the field should allow me to edit only what I've actually entered: "te".

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think this blocks, but it's probably really-really wanted. Here's the wide regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-05-20&enddate=2009-05-21

Dolske, Marco: could this be from

Bug 492701 - form history should cap the number of fields saved per form submission. r=dolske, a192=beltzner

Bug 493934 - Autocomplete queries are wrongly cutting away some result, r=sdwilsh a191=beltzner

Comment 7

[Simon Bünzli]

Beltzner: This is due to bug 491520 and will quite obviously affect at least Thunderbird and SeaMonkey (which both use autocomplete fields for email addresses, and SeaMonkey also exposes the autofill pref for the browser part). Should this not make Gecko 1.9.1, will a fix at least be acceptable for a point release?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I think we need to block on this... it's a pretty annoying core functionality regression that you can see it in Firefox in the bookmark tag field, and even more importantly in SeaMonkey/Thunderbird address entry fields.

I think this might actually be worse than bug 491520 (you can always override the autocompleted case if you really want to there, can't you?).

Comment 9

[:Gavin Sharp [email: [e-mail]]]

mcsmurf tells me that SeaMonkey uses it's own autocomplete controller, so it presumably wouldn't be affected by this bug. I'm not sure about Thunderbird, trying to find out more.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Sadly for messaging folks (but happily for 1.9.1!), Thunderbird is still using XPFE autocomplete for Thunderbird 3.

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #8)
> I think this might actually be worse than bug 491520 (you can always override
> the autocompleted case if you really want to there, can't you?).

Till bug 491520 was fixed if i had result CA and i wanted to write cat, there was no way to avoid finishing up with CAt, that was quite bad for user interaction with the field, since the filed was taking decision for the user. The only scope of that bug was exactly that, so if it caused any regression the regression should be fixed, and a test should be added for that specific case (flagging in-testsuite).

Clearly the scope of the above bug was to act only when user wants explicitly to autocomplete, not when he wants to edit. I'll try to look into this.

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #8)
> I think this might actually be worse than bug 491520 (you can always override
> the autocompleted case if you really want to there, can't you?).

How? i don't know what the user typed in since autocomplete code replaces it on the fly with result case, user should be able to autocomplete to a result or write what he wants in the field, should not be forced to write what the field wants.

Comment 13

[Marco Bonardo [:mak]]

So my plan would be to change that so:
- pressing VK_RIGHT when actual search string and defaultIndex vresult value are the same will autocomplete.
- special case this behavior only for VK_RIGHT, so VK_LEFT and VK_HOME won't change their behavior (this was unwanted, unfortunately managed by the same code)
- fix casing in handleEnter in similar way as for VK_RIGHT (so if values are the same will autocomplete to default index value)

Any complain about this plan?

There should be no noticeable differences from now apart the field will respect user casing unless user wants to autocomplete, and user will be able to autocomplete with VK_RIGHT (but only if the current text is the same as the complete default index suggested).

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I'm a bit confused about this bug now that I've looked into it more. I can reproduce the bug using the URL bar, and can confirm that it goes away when I back out the patch for bug 491520. But when I try with a normal autocomplete textbox, the bug doesn't go away when I back out the patch for bug 491520 (home causes the default value to be filled in with or without the patch).

<textbox completeselectedindex="true" type="autocomplete" autocompletesearch="search-autocomplete" autocompletesearchparam="searchbar-history" completedefaultindex="true"/>

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #12)
> (In reply to comment #8)
> > I think this might actually be worse than bug 491520 (you can always override
> > the autocompleted case if you really want to there, can't you?).
> 
> How?

All I'm saying is that bug 491520 has a workaround (edit the value manually after it was autocompleted). This bug doesn't, and I get the feeling that editing already-typed text is more common than entering differently-cased tags.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Gavin, can you comment on comment 13.

Mak: can you take this bug?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> All I'm saying is that bug 491520 has a workaround (edit the value manually
> after it was autocompleted). This bug doesn't

That isn't quite fair, since both bugs are "annoyances" rather than data/functionality loss. I do think the symptoms of this one are more annoying, though.

The approach in comment 13 sounds error prone (this code is hard to prove correct, given the different code paths and cases to consider). I think the only solutions to this bug that I'd be comfortable with at this point are:

1) undoing the HandleKeyNavigation changes from bug 491520
2) back out the patch for 491520
3) ignore this bug on branch, fix this bug otherwise on the trunk

We'd need to understand comment 14 before making a call here, I think. If it really only affects the Firefox URL bar specifically, then perhaps we should just go with 3).

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> The approach in comment 13 sounds error prone (this code is hard to prove
> correct, given the different code paths and cases to consider). 

tests can prove it is correct, let me try a patch, if you don't like it we can backout everything and we'll workaround the autocomplete casing bug another way.

Comment 19

[Simon Bünzli]

(In reply to comment #14)
> <textbox completeselectedindex="true" type="autocomplete"
> autocompletesearch="search-autocomplete"
> autocompletesearchparam="searchbar-history" completedefaultindex="true"/>

For me, this works as expected with the 20090520 nightly and breaks as described in the 20090521 one. So it's not address bar specific.

Comment 20

[Marco Bonardo [:mak]]

ops, you wrongly removed me from assignee field :)

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This is what i have so far, if you want to apply this patch and test you have to rebuild toolkit/components.

Includes a chrome test, not asking final review because i want to read it again and i've run out of time, but if you want to do a first pass review it will be appreciated.

Comment 22

[Marco Bonardo [:mak]]

please ignore the change to test_autocomplete3.xul, was a wrong change and i forgot to revert it.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #18)
> tests can prove it is correct

Assuming you write tests for every existing behavior we care to preserve, sure. That's hard to do - the tests in that patch are a great start, but they're not complete tests of the controller's behavior.

I'm a little bit reluctant to make even more changes to this code at this point, mostly because I don't think bug 491520 is really a blocker (there was some confusion as to whether it had a workaround in that bug, I think). Perhaps I'm being overly cautious, but we're pretty late in the process to be making significant changes to poorly tested (and frankly, poorly written) code.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

I still think it would be useful to determine if someone other than Simon can reproduce this bug using the textbox in comment 14, using a trunk build with 491520 backed out.

To be clear, the STR I was using are:

1) Load http://pastebin.mozilla.org/652930 in a chrome document
2) Type a string in the textbox that causes a match to be filled in
3) Press Home

Regardless of whether I have the patch for bug 491520 backed out, I see the text that was filled in remain after pressing Home.

Comment 25

[Simon Bünzli]

(In reply to comment #24)
> 1) Load http://pastebin.mozilla.org/652930 in a chrome document
> 2) Type a string in the textbox that causes a match to be filled in
> 3) Press Home

A small correction: For the general bug to manifest itself, you have to press Backspace/Delete before pressing Home in step 3 (cf. comment #5).

Comment 26

[Marco Bonardo [:mak]]

I can understand the will to avoid changes so late, but i'd like people to try what i have before saying we should go back.
That change touched a really small portion of code on a single attribute. and this patch limits that code an every smaller set of cases. So with this, for the most part, the behavior is unchanged from what we have.
Please, try what i provided and give me some good comment or suggestion, i think being constructive will be more useful for us and for users. We can think to a backout even later.

If you want i can push to the tryserver, just let me know.

Comment 27

[Mike Shaver (:shaver emeritus)]

Please push to try server!

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Talked it over with Shaver, and we think this does need to block.

Simon: can you try Marco's patch and see if it fixes what you're talking about?

Comment 29

[Simon Bünzli]

(In reply to comment #28)
> Simon: can you try Marco's patch and see if it fixes what you're talking about?

It indeed seems to fix what I'm talking about. Thanks.

Comment 30

[Marco Bonardo [:mak]]

hm, there is still a small issue with autocomplete-in-the-middle.

If we have an available autocomplete option "result" and the user writes "ret" we replace what user has typed so far with "ret[ >> result]" (the [ braces shows the selection). (implemented in bug 325842, from 3.1b1 on).
Now if the user presses right/left/home we simply confirm in the field the value "ret >> result" and then move cursor... this sounds crazy since the user wants to autocomplete or he does not want to.
My patch originally changed that so it would autocomplete, but for VK_LEFT or VK_HOME i think we should restore autocomplete value to what the user has typed before going home or back. But this is bad for RTL. The feature has originally been poorly implemented, is not RTL friendly (we always use >> even if RTL is active), and is adding unwanted content to the field, leaving to the user the task to remove it manually if he does not want it.

Since i don't want to fix that here and i have to limit scope of my patch to avoid regressions, i will revert any autocomplete-in-the-middle behavior to previous one (e.g. i will revert some change to test_autocomplete3.xul i made since i won't anymore complete on VK_RIGHT) and i'll file a bug to fix that feature in a sane way for .next.

Comment 31

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 32

[Marco Bonardo [:mak]]

filed Bug 494809 about the autocomplete-in-the-middle case, that won't probably make 3.5 at this stage.

Comment 33

[Marco Bonardo [:mak]]

Attachment: patch v1.1 (correct one)

ops, i attached a wip patch instead of the final one, this is what i pushed really to the tryserver.

Comment 34

[Marco Bonardo [:mak]]

Attachment: patch v1.2

this version disables HOME key tests on Mac, since it acts differently (code in the controller is ifndef for Mac).

this builds are for 1.1, no behavior difference from 1.2:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-b2d3082cbdc/

gavin, since i talked with you yesterday i'm asking review to you, but if you are too busy please forward review request to Enn or Mano.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 379662 [details] [diff] [review]
patch v1.2

Thanks again for your work on this, Marco.

Comment 36

[Dietrich Ayala (:dietrich)]

trunk: http://hg.mozilla.org/mozilla-central/rev/e31e8be2736a

Comment 37

[Dietrich Ayala (:dietrich)]

branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/06acb0d5a9f8