I have a contact whose email address local part (i.e, the part before the @) and first name are the same. In TB 24.5.0, if I type the first name all in lower case and then wait for it to auto-complete, it correctly completes the address AND properly upper-cases both its first and last name. On the trunk, if I type the first name all in lower case and then wait for it to auto-complete, it completes correctly but leaves the first name of the contact in lower case. Ugh. I don't know if this new behavior is intended; I certainly find it inferior, and therefore consider this to be a regression.

Oh, man, it's worse than I thought; I'm filing this update here rather than a separate bug because I suspect it has the same underlying cause as the problem I already reported. See the attached screenshot. I have an entry in my address book for "Jonathan Kamens <jik@kamens.us>". When I typed "jik" and let it auto-complete, it completed as shown in the screenshot, i.e., "jik >> Jonathan Kamens <jik@kamens.us>". That's just wrong.

Yes it's symptoms of the same thing. Tabbing doesn't really "confirm" the autocompleted value.

We get this problem because tb has completedefaultindex="true" + no tabscrolling="true" (which we should, i'll file another bug for that) + minresultsforpopup="2" With tabscrolling="true" and more than a single hit we don't get the problem. So this patch makes tab confirm the autcomplete in this case - as it is now we don't really use the suggestion, just the typeahead value which is not what we want. Also fixes bug 1009469 which is another symptom.

Comment on attachment 8429478 [details] [diff] [review] proposed fix > if (this.tabScrolling && this.popup.popupOpen) > cancel = this.mController.handleKeyNavigation(aEvent.shiftKey ? > KeyEvent.DOM_VK_UP : > KeyEvent.DOM_VK_DOWN); >+ else >+ this.mController.handleTab(); > break; When the popup is closed, this will prevent the tab key in the browser address field from tabbing. I think you actually want to enclose the entire block inside the this.popup.popupOpen check.

I do need it to work even when closed since we have minresultsforpopup="2" and otherwise it wouldn't work for the single hit case. I don't see the problem with the address field. It does have tabscrolling and opens for even one hit. And tabbing from empty works find. Can you elaborate on what's not working?

1. Start entering some text in the address field. 2. Press Tab The result autocompletes and starts loading and focus shifts to the page.

Ah, tested the wrong build earlier :/ So how about this? Only handleTab() when we have at least one result

Ping?

Any update on the review?

This has been broken for more than a month, and some of the effects of this issue mean it's very easy to send email to the wrong person, which can be highly embarrassing. Please could I encourage swift resolution of this bug? :-) Let me know if I can help. Gerv

Comment on attachment 8430248 [details] [diff] [review] proposed fix, v2 I meant that we don't want handleTab to be called when tabScrolling is set. The following issue occurs: 1. Press a letter in the browser url field and wait for popup to appear. 2. Press cursor right. The popup closes. 3. Press tab. Actual results: The browser starts loading something and the tab is focused. Expected results: Tab navigation occurs such that the search field is focused and the page doesn't change. Sorry for the delay here. It took me a while to figure out exactly what it is that is desired here. Is there some specific flag that Thunderbird uses that implies that tab should automatically select when tab is pressed? forcecomplete?

Yes, we do have forcecomplete. At the moment we don't have tabScrolling set, but I'd like to add that later. So, I guess I should add && this.forceComplete http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#931 http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.xul#684

In 3+ years of using Thunderbird at work, I've NEVER sent a message to the wrong person. In the past 2-3 weeks, it's happened 3-4 times (I know, not a lot) and I've caught wrong addresses many, many more times. I know it's my responsibility to know what address is in the field, but I'm using the same behavior/steps that I've used for years. Glad to see it confirmed here at least !

Comment on attachment 8445896 [details] [diff] [review] proposed fix, v3 I think this likely gives us the best we can do here.

Backed out for mochitest-other failures. Please make sure this is green on Try before pushing again. https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c363f922fb https://tbpl.mozilla.org/php/getParsedLog.php?id=42651141&tree=Mozilla-Inbound

OSX always wants to be different doesn't it :( I've done some testing and the problem is persistent on mac, it's because of the forcecomplete="true" addition. Even though the VK_DELETE is synthesized here: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/tests/chrome/test_autocomplete4.xul#187 for some reason handleDelete() doesn't end up getting called here: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/autocomplete.xml#497 Since the delete doesn't get called, mResults.Length() is 1 instead of 0 like it is on other platforms - here http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1219 - this problem didn't matter earlier since forcecomplete was false. I don't have a mac so I don't think I can debug this further. Could someone with on osx check it out?

(In reply to Magnus Melin from comment #18) > I don't have a mac so I don't think I can debug this further. Could someone > with on osx check it out? I have a Mac, but I'm not good in debugging. Can I anyway help here?

You might, in particular, see this try run https://tbpl.mozilla.org/?tree=Try&rev=32a0e8fff3f9 For a run of mach mochitest-chrome toolkit/content/tests/chrome/test_autocomplete4.xul At the "xxxmagnus - will synth VK_DELETE", it will look like this on linux: 0:19.81 xxxmagnus - will synth VK_DELETE 0:19.82 xxxmagnus - onKeyPress 0:19.82 xxxmagnus - onKeyPress2 0:19.82 xxxmagnus - onKeyPress3 aEvent.keyCode=46 0:19.82 nsAutoCompleteController::HandleDelete 0:19.82 isOpen=1, mRowCount=1 0:19.82 index=-1 0:19.82 no row is selected in the list 0:19.82 xxxmagnus - at end, cancel=false on mac: 05:33:32 INFO - xxxmagnus - will synth VK_DELETE 05:33:32 INFO - xxxmagnus - onKeyPress 05:33:32 INFO - xxxmagnus - onKeyPress2 05:33:32 INFO - xxxmagnus - onKeyPress3 aEvent.keyCode=46 05:33:32 INFO - xxxmagnus - at end, cancel=false ... so there's a quite limited range of code that needs checking The code gets here: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/autocomplete.xml#477 but not http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/autocomplete.xml#497 I wonder if the info here gives some clue - http://mxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/text/tests/test_texteditor_keyevent_handling.html?force=1#178

I've made a build with your patch and did "mach mochitest-chrome toolkit/content/tests/chrome/test_autocomplete4.xul", to see if I can reproduce this locally on my Mac. But I only get three "TEST-UNEXPECTED-FAIL". And not this detailed log.

You can grab the additional debugging patch from https://hg.mozilla.org/try/rev/4140fd5531cc (sorry, don't have it easily attachable elsewhere)

Ok, so lets just go with not having forcecomplete for the other tests (like they used to). There's something fishy with mac + delete + forcecomplete, but that's not related to this patch directly. Try looks good - https://tbpl.mozilla.org/?tree=Try&rev=ee6f8fedafa3

Will you be backporting this to aurora/beta so its included in Gecko 31 ? I'd love to see this fixed for Thunderbird 31.

Comment on attachment 8455048 [details] [diff] [review] proposed fix, v4 Approval Request Comment [Feature/regressing bug #]: bug 959209 (tb swithed to toolkit autocomplete) [User impact if declined]: autocomplete behaves badly when confirming suggestion with tab [Describe test coverage new/current, TBPL]: automated test in place, tested on trunk [Risks and why]: very limited risk to firefox, as it only changes anything when the forcecomplete attribute is set, and firefox never sets that. thunderbird definitely needs this for tb31 (the next ESR) [String/UUID change made/needed]: none

Comment on attachment 8455048 [details] [diff] [review] proposed fix, v4 We already built the RC of Firefox 31...

While Firefox RC might already be built, Thunderbird betas and finals are not through yet. Would it be possible to push this to mozilla-beta nevertheless? It shouldn't do any harm as the Firefox RC's are tagged anyway.

Comment on attachment 8455048 [details] [diff] [review] proposed fix, v4 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is causing user facing issues in Thunderbird, we narrowly missed the slot for ESR, so we'd like to get it fixed there as well. We will be landing on relbranches for the first 31.0 release but they are a pain to manage, so we'd like to land this for the first point release. User impact if declined: autocomplete behaves badly when confirming suggestion with tab Fix Landed on Version: 32 (landed on central & aurora before merges) Risk to taking this patch (and alternatives if risky): very limited risk to firefox, as it only changes anything when the forcecomplete attribute is set, and firefox never sets that. thunderbird definitely needs this for tb31 (the next ESR) String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Is this in something we can download (a compiled version) and test - I'm on 31.0 beta-update-channel and am still seeing it as of now, and had the huge embarrassment of sending material on several occasions to completely the wrong person as a result of this bug (even though I knew about it). I'm not sure where to get other versions.

This patch got approved for aurora; someone needs to check it in there ASAP before aurora becomes beta in 4 days... In the mean time, if you want to test it, it seems you will need a nightly build. Gerv

It landed on aurora with comment 30. It should also be in tb31 (due to tb-relbranch landing)

This is sadly not fixed in released Thunderbird 31 ESR. It is fixed in a case when user uses TAB to confirm autocomplete suggestion. But it is still broken when user just clicks "Subject", "To" or compose area after it sees autocompleted "To" or "CC" field. This would cause a lot of mails sent to non-capitalized names (which is rude) like this: To: john Doe <jdoe@example.com> (when autocompleted from "john") or, when with ">>" like this: To: jdoe >> John Doe <jdoe@example.com> (when autocompleted from "jdoe"). It needs bugfix release as soon as possible, as it makes TB unusable now. Please reopen.

Comment 35 is right; this is not fully fixed. Gerv

Using the example of Johnathan Nightingale <johnath@mozilla.com> Working: Type "night", press Tab Type "night", press Shift-Tab Type "night", press Enter Type "night", click correct entry in dropdown Type "night", press Down Arrow, press Right Arrow Fails: Type "night", press Right Arrow Type "night", click outside dropdown somewhere Type "night", press Down Arrow, press Up Arrow, press Right Arrow (all lead to: "night >> Johnathan Nightingale <johnath@mozilla.com>") Gerv

True, but let's take it to bug 1043310.

Does the fix for this bug also fix the problem where, after typing two or more characters in the To: field, the selection list contains entries that do not have the typed character string?

No, and I'm not aware of such a bug. Note that we search more fields than we used to, so the string can be located in one of those and not necessarily in the mailbox string.

See bug #1034976, which was closed as a duplicate of this bug. See especially the comment at <https://bugzilla.mozilla.org/show_bug.cgi?id=1034976#c4>.