Closed Bug 1012398 Opened 6 years ago Closed 6 years ago

AutoCompletion doesn't take capitalization from address book entry when tabbing to complete

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
firefox-esr31 --- fixed
thunderbird31 --- wontfix
thunderbird32 --- fixed
thunderbird33 --- fixed

People

(Reporter: jik, Assigned: mkmelin)

References

Details

(Keywords: regression, Whiteboard: [followup bug 1043310])

Attachments

(2 files, 3 obsolete files)

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.
Assignee: nobody → mkmelin+mozilla
Blocks: 959209
OS: Linux → All
Hardware: x86_64 → All
Attached patch proposed fix (obsolete) — Splinter Review
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.
Attachment #8429478 - Flags: review?(enndeakin)
Status: NEW → ASSIGNED
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.
Attachment #8429478 - Flags: review?(enndeakin) → review-
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?
Flags: needinfo?(enndeakin)
1. Start entering some text in the address field.
2. Press Tab

The result autocompletes and starts loading and focus shifts to the page.
Flags: needinfo?(enndeakin)
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Ah, tested the wrong build earlier :/

So how about this? Only handleTab() when we have at least one result
Attachment #8430248 - Flags: review?(enndeakin)
Ping?
Blocks: 1009469
Summary: Completion doesn't take capitalization from address book entry → AutoCompletion doesn't take capitalization from address book entry
Attachment #8429478 - Attachment is obsolete: true
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?
Attachment #8430248 - Flags: review?(enndeakin) → review-
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
Attached patch proposed fix, v3 (obsolete) — Splinter Review
Attachment #8430248 - Attachment is obsolete: true
Attachment #8445896 - Flags: review?(enndeakin)
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.
Attachment #8445896 - Flags: review?(enndeakin) → review+
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)
Attached patch proposed fix, v4Splinter Review
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
Attachment #8445896 - Attachment is obsolete: true
Attachment #8455048 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/064025abab4d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
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
Attachment #8455048 - Flags: approval-mozilla-beta?
Attachment #8455048 - Flags: approval-mozilla-aurora?
Comment on attachment 8455048 [details] [diff] [review]
proposed fix, v4

We already built the RC of Firefox 31...
Attachment #8455048 - Flags: approval-mozilla-beta?
Attachment #8455048 - Flags: approval-mozilla-beta-
Attachment #8455048 - Flags: approval-mozilla-aurora?
Attachment #8455048 - Flags: approval-mozilla-aurora+
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.
Attachment #8455048 - Flags: approval-mozilla-esr31?
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.
Flags: needinfo?(mkmelin+mozilla)
Comment 35 is right; this is not fully fixed.

Gerv
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(mkmelin+mozilla)
Resolution: --- → FIXED
Summary: AutoCompletion doesn't take capitalization from address book entry → AutoCompletion doesn't take capitalization from address book entry when tabbing to complete
Blocks: 1043310
Duplicate of this bug: 1034976
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>.
See Also: → 1041462
See Also: 1041462
Attachment #8455048 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Keywords: checkin-needed
Whiteboard: [checkin-needed: esr31]
Component: Message Compose Window → Autocomplete
Product: Thunderbird → Toolkit
Target Milestone: Thunderbird 33.0 → mozilla33
Whiteboard: [followup bug 1043310]
You need to log in before you can comment on or make changes to this bug.