The default bug view has changed. See this FAQ.

AutoCompletion doesn't take capitalization from address book entry, can leave angle brackets characters >> in field, when loosing focus by clicking outside (not enter/tab)

RESOLVED FIXED in Firefox 36

Status

()

Toolkit
Autocomplete
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Magnus Melin, Assigned: Magnus Melin)

Tracking

({regression})

Trunk
mozilla37
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox-esr3136+ fixed)

Details

(Whiteboard: [fixed for TB 31.5 per comment 53])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

3 years ago
+++ This bug was initially created as a clone of Bug #1012398 +++

Autocomplete doesn't properly "confirm" the autocomplete suggestion when the autocomplete field looses focus by clicking outside the field.

STR from bug 1012398 comment 37:
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>")


Bug 632127 would probably also be fixed by this.
(Assignee)

Updated

3 years ago
Assignee: nobody → mkmelin+mozilla
Depends on: 1012398

Comment 1

3 years ago
For anybody which was bitten by this bug:

There's a Thunderbird 24.7.0 version available from http://ftp.mozilla.org/pub/mozilla.org/thunderbird/releases/24.7.0/ which has latest security updates but does not have this bug. There's no link to it from official Thunderbird "all versions" page though (https://www.mozilla.org/en-US/thunderbird/all.html).

Comment 2

3 years ago
Same here on Xubuntu 12.04 Precise

Thanks for the link. Back to 24.

Updated

3 years ago
See Also: → bug 1041462

Updated

3 years ago
See Also: bug 1041462

Comment 3

3 years ago
Windows Vista
TB 31.0
In new compose Write window
in a To field
type 'm' and an option appears in the same field area separated by >> chevons.
If you select the option shown then the result will be an invalid email address  in black font:
    m >> Display Name followed by email address in enclosed chevrons
if you type m followed by a space result will be will be an invalid email address in red font:
    m >> Display Name followed by email address in enclosed chevrons

If you type 'm' and select from the drop down list you get correct email address in black font.
If you type 'ma' and select from the drop down list you get correct email address in red font.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1055149
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1059829
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1061396
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1062499

Comment 8

3 years ago
This isn't fixed in recently released Thunderbird 31.1.

This forces us to use previous ESR: Thunderbird 24.8.0, available from http://ftp.mozilla.org/pub/mozilla.org/thunderbird/releases/24.8.0/

I really hope this will be fixed before version 24 will be unsupported.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1064420
Created attachment 8489095 [details] [diff] [review]
fix, v1

This is the fix.

I also developed two tests to verify that it works, but code hits an underlying bug when trying to receive the recipients of the fake message.

during the development of a MozMill test I hit the following issue:

1. Opened a new compose window.
2. Wrote some letters of a contact I had added to the address book and let it autocomplete.
3. Wrote some letters of the middle of a name of a different contact I had added to the address book and let it autocomplete before saving it to the outbox.
4. Opened the Outbox and selected message.
5. Tried to compare the expected recipients with the message's "recipients" property.

Actual result:
logObject on the message or even message.recipients works and shows the expected values, but using it as parameter for assert_equals lets the test fail with "Cannot modify properties of a WrappedNative". This doesn't seem to happen if the second contact typed isn't in the address book (full address typed and not in the address book). Closing the compose window after the first autocomplete and reopening it (even with recycled windows set to 0) doesn't help.

Any ideas what's wrong?

Source code (appended at the end of test-address-widgets.js):

function test_address_autocomplete() {
  // Open compose window on the existing POP3 account.
  be_in_folder(accountPOP3.incomingServer.rootFolder);
  cwc = open_compose_new_mail();
  cwc.e("msgSubject").value = "dummy subject";

  // Create some contact address book card in the Personal addressbook.
  let defaultAB = MailServices.ab.getDirectory("moz-abmdbdirectory://abook.mab");
  let contact1 = create_contact("test@example.com", "Anthony Jenkis", true);
  let contact2 = create_contact("jaspers@baja.invalid", "Barbara Jaspers", true);
  let contact3 = create_contact("cec@example.com", "Cecilia LaFontaine", true);
  load_contacts_into_address_book(defaultAB, [contact1, contact2, contact3]);

  let addressWidget = cwc.e("addressCol2#1");
  addressWidget.focus();
  let addressWidgetEid = cwc.eid("addressCol2#1");
  cwc.type(addressWidgetEid, "Font");
  cwc.sleep(1000);
  cwc.e("content-frame").focus();
  assert_equals(addressWidget.textValue, "Cecilia LaFontaine <cec@example.com>");

  addressWidget = cwc.e("addressCol2#2");
  addressWidget.focus();
  let addressWidgetEid = cwc.eid("addressCol2#2");
  cwc.type(addressWidgetEid, "aja");
  cwc.sleep(1000);
  Services.io.offline = true;
  cwc.e("button-send").click();
  Services.prefs.setBoolPref("mail.showCondensedAddresses", false);
  outboxFolder = MailServices.accounts
                             .localFoldersServer
                             .rootFolder
                             .getChildNamed("Outbox");
  be_in_folder(outboxFolder);
  let msg = select_click_row(0);
  wait_for_message_display_completion(mc);
  assert_selected_and_displayed(mc, msg);
  logObject(msg);
  //logObject(msg.recipients);
  let curRecipients = msg.recipients;
  assert_equals(curRecipients,
                "Cecilia LaFontaine <cec@example.com>, Barbara Jaspers <jaspers@baja.invalid>");

  press_delete();
  Services.prefs.setBoolPref("mail.showCondensedAddresses", true);
  Services.io.online = true;
}
Assignee: mkmelin+mozilla → archaeopteryx
Status: NEW → ASSIGNED
Attachment #8489095 - Flags: review?(mkmelin+mozilla)

Comment 11

3 years ago
(In reply to Archaeopteryx [:aryx] from comment #10)
> Created attachment 8489095 [details] [diff] [review]
> fix, v1
> 
> This is the fix.
I can confirm, that this fixes the issue (in any way I've tested it). Tested it with TB33.
Note I had some issues with using a blur handler for autocomplete in Lightning's attendee dialog. It doesn't mean its not a valid fix, maybe I have to revisit it, but I thought I'd mention it. See bug 1007040 comment 11.
(Assignee)

Comment 13

3 years ago
Comment on attachment 8489095 [details] [diff] [review]
fix, v1

Unfortuhately, I think this fix is at the wrong level and fixing it only here would still leave the bug in, say lightning. The real bug is in the autocomplete controller.
Attachment #8489095 - Flags: review?(mkmelin+mozilla) → review-
(Assignee)

Comment 14

3 years ago
Created attachment 8489555 [details] [diff] [review]
bug1043310_ac_blur.patch

This is the fix I've been working on, green try here:
https://tbpl.mozilla.org/?tree=Try&rev=d8f8dedfecf8

This fixes these things
 1) click outside makes the autocomplete (for forcecomplete)
 2) for minresultsforpopup > 1: complete entering match didn't work properly
 3) ignoreBlurWhileSearching is not hooked up to anything
 4) KeyEvent.DOM_VK_END was also hooked up the same as left/right/home in xpfe, and it seems logical that it should work here too
Assignee: archaeopteryx → mkmelin+mozilla
Attachment #8489095 - Attachment is obsolete: true
Attachment #8489555 - Flags: review?(enndeakin)
(Assignee)

Comment 15

3 years ago
enndeakin: review ping

Comment 16

3 years ago
Perhaps a silly question:

I started using TB 31 today and had this problem that the capitalisation is not taken from the address book entry.

As an additional problem I observed that some addresses taken from the address book were included in red. Seems to happen at random but often enough to be annoying.

Is this a different problem?

Comment 17

3 years ago
Sorry, it was a silly question: The red has been reported here: bug 1042561.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1077852
(Assignee)

Comment 19

3 years ago
Created attachment 8500605 [details] [diff] [review]
bug1043310_ac_blur.patch

Unbitrot.
Attachment #8489555 - Attachment is obsolete: true
Attachment #8489555 - Flags: review?(enndeakin)
Attachment #8500605 - Flags: review?(enndeakin)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1080546
(Assignee)

Comment 21

3 years ago
pinging for review again. the (original) patch has now been waiting a month
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1069789

Comment 23

2 years ago
Thank you very much for pushing the fix for the spurious extra/blank addressing line issue Magnus!

And an even bigger thank you for working on this one, which has a much greater negative impact for myself and all of our 70+ users here.

I'm hoping that this can get reviewed and the fix included in the next point release for 31 as well.

Thanks again!
(Assignee)

Updated

2 years ago
Attachment #8500605 - Flags: review?(mak77)

Comment 24

2 years ago
I also want to thank you very much in advance as some of our users (30+) do really have problems with this issue.

Comment 25

2 years ago
On TB 34.0b3 and even the 11/18 build, I am still seeing this issue if I begin typing an address and click the mouse  somewhere else to complete the action. Has the patch been merged?

Comment 26

2 years ago
No, it hasn't (as you can see from the daily 18th Nov.)
Comment on attachment 8500605 [details] [diff] [review]
bug1043310_ac_blur.patch

Review of attachment 8500605 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay but there's a giant pile of priorities around...
I think you are trying to do too many changes in a single bug on one of the most race-y parts of the codebase (the autocomplete controller).
As it is, I doubt anybody would take the responsibility to review this patch as a whole.
I think it would be wise to split each fix into its own bug, with a patch and a test (or part of it, dependencies are welcome).

That said, I'll try to give you some feedback here.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +473,5 @@
>               || aKey == nsIDOMKeyEvent::DOM_VK_RIGHT
>  #ifndef XP_MACOSX
>               || aKey == nsIDOMKeyEvent::DOM_VK_HOME
>  #endif
> +             || aKey == nsIDOMKeyEvent::DOM_VK_END

On Mac doesn't looks like it's an expected use case, so it should likely move inside the ifndef (I did some archeology, see bug 255116)

@@ +479,5 @@
>    {
>      // The user hit a text-navigation key.
>      bool isOpen = false;
>      input->GetPopupOpen(&isOpen);
> +    if (isOpen || mRowCount > 0) { // open, open-ish (for minresultsforpopup > 1)

the comment doesn't explain how the popup could be closed and in such a case why we should do anything here (this goes through popup selected index and such, do they make sense if the popup is not even visible?). open-ish doesn't tell me anything, sorry, maybe it's hiding a bug somewhere else. Since there are too many changes here I don't know which change this is related to.

@@ +504,5 @@
>          nsAutoString value;
>          nsAutoString inputValue;
>          input->GetTextValue(inputValue);
>          if (NS_SUCCEEDED(GetDefaultCompleteValue(-1, false, value)) &&
> +            StringEndsWith(inputValue, value, nsCaseInsensitiveStringComparator())) {

I think you should instead look for " >> " and .Equals with the substring at the right of it. That would limit the possibilities of regressions.

@@ +1271,5 @@
>      }
>  
>      if (forceComplete && value.IsEmpty()) {
> +      // See if mSearchString is a complete match. If so use that as value.
> +      // That implies "the match" was already completed by other means.

This comment must be rewritten, it contains too many unknowns:
- what is a "complete match"
- why "use that as value" (use-case)
- what is "the match"
- what are "other means"?

For mind sanity of future devs, the comment needs to explain the situation in which we are, how we intend to proceed to solve it and why we take that path.

@@ +1281,3 @@
>  
> +          for (uint32_t j = 0; j < matchCount; ++j) {
> +            nsAutoString jValue;

matchValue?

@@ +1281,5 @@
>  
> +          for (uint32_t j = 0; j < matchCount; ++j) {
> +            nsAutoString jValue;
> +            result->GetFinalCompleteValueAt(j, jValue);
> +            if (StringEndsWith(mSearchString, jValue, nsCaseInsensitiveStringComparator())) {

should likely do the same suggested above (look for " >> " explicitly)

@@ +1292,5 @@
> +      if (value.IsEmpty()) {
> +        // Since nothing was selected, and forceComplete is specified, that means
> +        // we have to find the first default match and enter it instead
> +        for (uint32_t i = 0; i < mResults.Length(); ++i) {
> +          nsIAutoCompleteResult *result = mResults[i];

I think you could walk the results just once, assign the values to temporaries, and then at the end of the loop check which of the 2 is better (e.g. if you have a best match use that, otherwise use the default value). in the loop you can break out when you get a best match.

Is there some condition on mSearchString that would allow us to know, even before looping, that we can't find a best match? That would greatly help perf of the loop. But we can live with looping regardless (forceComplete has very few consumers, likely none in Firefox)
Attachment #8500605 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1106457

Comment 29

2 years ago
I'd like to review this part of patch:

==================================================================
diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -582,18 +573,23 @@
                action="if (this.mController.input == this) this.mController.handleStartComposition();"/>
 
       <handler event="compositionend" phase="capturing"
                action="if (this.mController.input == this) this.mController.handleEndComposition();"/>
 
       <handler event="focus" phase="capturing"
                action="this.attachController();"/>
 
-      <handler event="blur" phase="capturing"
-               action="if (!this._dontBlur) this.detachController();"/>
+      <handler event="blur" phase="capturing"><![CDATA[
+        if (!this._dontBlur) {
+          if (this.forceComplete && this.mController.matchCount >= 1)
+            this.mController.handleEnter(false);
+          this.detachController();
+        }
+      ]]></handler>
     </handlers>
   </binding>
 
   <binding id="autocomplete-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-base-popup">
     <resources>
       <stylesheet src="chrome://global/content/autocomplete.css"/>
       <stylesheet src="chrome://global/skin/tree.css"/>
       <stylesheet src="chrome://global/skin/autocomplete.css"/>
==================================================================

The problem with this approach is that it autocompletes when it definitely shouldn't. For example:
1. Have "B <address1@example.com>" and "C <address2@example.com>" in addressbook.
2. Write "addr" in input: it would propose to autocomplete "addr >> B <address1@example.com>" and open popup, with everything from space selected.
3. Push DEL or BACKSPACE - it would delete selected text, leaving "addr" and reopen popup.
4. Blur or push ENTER or TAB - it would autocomplete to "B <address1@example.com>", which is bad.

Input, even when it has forcecomplete attribute, should only complete, when a user expects it. It means that value to autocomplete either is visible in input or selected in visible popup.

Maybe this widget needs another field to save what value to autocomplete to when displaying it in "CompleteValue". So it does not need to:
- parse visible value;
- find out what is the default autocomplete value.
This value would be deleted when user closes popup without choosing anything. Or uses any navigation keys.

Updated

2 years ago
Attachment #8500605 - Flags: review?(enndeakin)
(Assignee)

Comment 30

2 years ago
Tomasz: that's a different issue, this patch doesn't change that (except for that it does so also in the blur case in addition to enter and tab)
(Assignee)

Comment 31

2 years ago
Created attachment 8538669 [details] [diff] [review]
bug1043310_ac_blur.patch

So this fixes only the main issue, which has two aspects:
 - for onblur we need to complete to the actual suggestion not to leave addresses uncapitalized or having " >> " as value
 - once we have a valid selection that should not be changed to default because EnterMatch is called again (which it is with blur if the value was confirmed by enter already)

I hope this can be reviewed sometimes soon. The changes should have virtually no impact on firefox since the cases it changes affects options not used in firefox.
Attachment #8500605 - Attachment is obsolete: true
Attachment #8538669 - Flags: review?(mak77)
(Assignee)

Comment 32

2 years ago
Created attachment 8538670 [details] [diff] [review]
bug1043310_ac_blur.patch

Forgot to export after the last qrefresh to fix some comments. 
Anyway, good looking try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=53fe45c6184f
Attachment #8538669 - Attachment is obsolete: true
Attachment #8538669 - Flags: review?(mak77)
Attachment #8538670 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1113180
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1113067
(Assignee)

Comment 35

2 years ago
Split of the other stuff into bug 1114010, bug 1114011 and bug 1114013.
Component: Message Compose Window → Autocomplete
Product: Thunderbird → Toolkit
Blocks: 1089711
(Assignee)

Updated

2 years ago
Depends on: 1107844
Comment on attachment 8538670 [details] [diff] [review]
bug1043310_ac_blur.patch

Review of attachment 8538670 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +505,5 @@
>          nsAutoString inputValue;
>          input->GetTextValue(inputValue);
> +        if (NS_SUCCEEDED(GetDefaultCompleteValue(-1, false, value))) {
> +          nsAutoString match;
> +          int32_t pos = inputValue.RFind(" >> ");

I think that Find would be faster than RFind for our case. Cause by nature the autocompleted value should be wider than the typed value (otherwise the autocomplete algorithm should be fixed to match better)
Yes, that would catch the first occurrence rather than the last one, but having more than 1 match is a very edge case we likely don't care about, while a faster find is a nice to have.
(you'll have to adjust the position if the offset is given on the left side of " >> ")

also s/match/suggestedValue/

@@ +507,5 @@
> +        if (NS_SUCCEEDED(GetDefaultCompleteValue(-1, false, value))) {
> +          nsAutoString match;
> +          int32_t pos = inputValue.RFind(" >> ");
> +          if (pos >= 0)
> +            inputValue.Right(match, inputValue.Length() - pos);

I'm far from being an xpcom strings expert, but sounds like using
const nsDependentSubstring& and Substring() we could save an assignment

@@ +509,5 @@
> +          int32_t pos = inputValue.RFind(" >> ");
> +          if (pos >= 0)
> +            inputValue.Right(match, inputValue.Length() - pos);
> +          else
> +            match = inputValue;

please brace if/else in cpp, per coding style

@@ +510,5 @@
> +          if (pos >= 0)
> +            inputValue.Right(match, inputValue.Length() - pos);
> +          else
> +            match = inputValue;
> +        

trailing spaces

@@ +1280,5 @@
>  
>      if (forceComplete && value.IsEmpty()) {
> +      // See if mStringSearch is one of the autocomplete results. It can be an
> +      // identical value, or if it matched the middle of a result it can be
> +      // something like "bar >> foobar" (user entered bar and foobar is 

trailing space.

@@ +1284,5 @@
> +      // something like "bar >> foobar" (user entered bar and foobar is 
> +      // the result value).
> +      // If it matches one of the autocomplete results we should use that
> +      // result as value and not use a default value: it's possible
> +      // EnterMatch was just called a second time, so the value cannot change!

This could be clarified a little bit:

// If the current search matches one of the autocomplete results, we
// should use that result, and not overwrite it with the default value.
// It's indeed possible EnterMatch gets called a second time (for example
// by the blur handler) and it should not overwrite the current match.

Though, off-hand this handling looks a bit fragile, especially if it exists only to handle the fact onBlur could invoke EnterMatch a second time.
The whole fact blur invokes something that is intended to handle the Enter key sounds error-prone. At this point I'd be prone to add an HandleBlur to nsIAutoCompleteController and do the job there in a more proper way.

::: toolkit/content/tests/chrome/test_autocomplete4.xul
@@ +151,5 @@
>      result: "Result",
>      start: 6, end: 6
>    },
> +
> +  { desc: "RIGHT key compete from end",

typo: compete

@@ +154,5 @@
> +
> +  { desc: "RIGHT key compete from end",
> +    key: "VK_RIGHT",
> +    forceComplete: true,
> +    completeFromMiddle: true, 

trailing space

@@ +202,5 @@
> +  autocomplete.setAttribute("forcecomplete", currentTest.forceComplete ? true : false);
> +
> +  if (currentTest.completeFromMiddle) {
> +    synthesizeKey(currentTest.key, {});
> +    

trailing spaces

@@ +205,5 @@
> +    synthesizeKey(currentTest.key, {});
> +    
> +    // At this point we should have a value like "lt >> Result" showing.
> +    if (!/ >> /.test(autocomplete.value))
> +      throw new Error("Expected an middle-completed value: " + autocomplete.value);

ok(autocomplete.value.contains(" >> "), 
"Expected a middle-completed value: " + autocomplete.value); (should be .includes if this lands after bug 1102219)

@@ +211,5 @@
> +    // For forceComplete a blur should cause a value from the results to get
> +    // completed to. E.g. "lt >> Result" will turn into "Result".
> +    if (currentTest.forceComplete)
> +      autocomplete.blur();
> +    

trailing spaces

::: toolkit/content/widgets/autocomplete.xml
@@ +337,5 @@
>        <method name="detachController">
>          <body><![CDATA[
>            try {
>              if  (this.mController.input == this)
> +              this.mController.input = null;

ugh, while here, could you please also fix the double space between if and (

the indentation bug was caused by adding the try/catch to "avoid noise in the console" (according to blame history), but it doesn't make any sense to me. The blame doesn't even report a bug #.

SetInput cannot throw if it's called with null, mController should always be defined (it's defined in the constructor and we should not hide the error if that fails).
Even if this.mController.input is null, it shouldn't cause any noise just for the comparison.

I'd remove the try/catch, and then, if anyone reports noise, we can properly handle that in the if condition rather than hiding all errors like this.

@@ +598,5 @@
> +        if (!this._dontBlur) {
> +          if (this.forceComplete && this.mController.matchCount >= 1)
> +            this.mController.handleEnter(false);
> +          this.detachController();
> +        }

while it's true that here we are only widening the reach of an existing forcecomplete bug (reported in comment 29), please ensure there's a bug filed about that issue. It should be fixable at the controller level or in the worst case by checking the input value for an autofilled part.
Attachment #8538670 - Flags: review?(mak77)
(Assignee)

Comment 37

2 years ago
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #36)
> // If the current search matches one of the autocomplete results, we
> // should use that result, and not overwrite it with the default value.
> // It's indeed possible EnterMatch gets called a second time (for example
> // by the blur handler) and it should not overwrite the current match.
> 
> Though, off-hand this handling looks a bit fragile, especially if it exists
> only to handle the fact onBlur could invoke EnterMatch a second time.
> The whole fact blur invokes something that is intended to handle the Enter
> key sounds error-prone.

It is also needed for bug 1089711. There the value is already selected (entered) by mouse, but in thunderbird we don't do anything more at that point so if the user then confirms the selection by enter instead of just moving away with the mouse, the selection changes to the default match, which is obviously quite bad.

> while it's true that here we are only widening the reach of an existing
> forcecomplete bug (reported in comment 29), please ensure there's a bug
> filed about that issue. It should be fixable at the controller level or in
> the worst case by checking the input value for an autofilled part.

There's at least bug 533109 and bug 486501, which may be dupes of each other.
(Assignee)

Comment 38

2 years ago
Created attachment 8543146 [details] [diff] [review]
bug1043310_ac_blur.patch

Thx for the review! This should address the review comments. 
I did not see a good way to use nsDependentSubstring, though my cpp string foo is not strong.
Attachment #8538670 - Attachment is obsolete: true
Attachment #8543146 - Flags: review?(mak77)
(Assignee)

Comment 39

2 years ago
Created attachment 8543161 [details] [diff] [review]
bug1043310_ac_blur.patch

Better version.
Attachment #8543146 - Attachment is obsolete: true
Attachment #8543146 - Flags: review?(mak77)
Attachment #8543161 - Flags: review?(mak77)
Comment on attachment 8543161 [details] [diff] [review]
bug1043310_ac_blur.patch

Review of attachment 8543161 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I'm not very happy we will use the Enter handler for blur, but I see your reasons.
I hope you will add a test for bug 1089711, so we don't regress that with future changes, i'm sure soon or later someone will see this changes as exotic and try to clean up things, and regress your fix. A test would ensure safety.
Considered it's limited to forceComplete and that Thunderbird is the major consumer of that attribute, I don't feel like I should block you on feelings.

::: toolkit/components/autocomplete/nsAutoCompleteController.cpp
@@ +513,5 @@
> +          int32_t pos = inputValue.Find(" >> ");
> +          if (pos > 0) {
> +            inputValue.Right(suggestedValue, inputValue.Length() - pos - 4);
> +          }
> +          else {

this code uses cuddled else, like the coding style suggests, so please cuddle this (I know we are not consistent but let's not make it worse)

@@ +1298,5 @@
>  
>      if (forceComplete && value.IsEmpty()) {
> +      // See if inputValue is one of the autocomplete results. It can be an
> +      // identical value, or if it matched the middle of a result it can be
> +      // something like "bar >> foobar" (user entered bar and foobar is 

trailing space

@@ +1311,5 @@
> +      int32_t pos = inputValue.Find(" >> ");
> +      if (pos > 0) {
> +        inputValue.Right(suggestedValue, inputValue.Length() - pos - 4);
> +      }
> +      else {

cuddle else

@@ +1318,1 @@
>        for (uint32_t i = 0; i < mResults.Length(); ++i) {

nit: add new line above the for loop for readability

@@ +1332,5 @@
> +      }
> +      if (value.IsEmpty()) {
> +        // Since nothing was selected, and forceComplete is specified, that means
> +        // we have to find the first default match and enter it instead
> +        for (uint32_t i = 0; i < mResults.Length(); ++i) {

let's avoid the double looping,
create a defaultValue autostring along with suggestedValue

in the first mResults loop you can assign to defaultValue if defaultValue.IsEmpty() (but don't break!)

then here you can just
if (value.IsEmpty()) {
  value = defaultValue;
}
Attachment #8543161 - Flags: review?(mak77) → review+
(Assignee)

Comment 41

2 years ago
Thx for the reviews!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b189101e92c5

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eab42dfb428
https://hg.mozilla.org/mozilla-central/rev/1eab42dfb428
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
What about esr31?

Comment 44

2 years ago
Does this catch Bug 1025684 as well? I think there are a number of other variations of this bug involving data-loss as emails get sent to the wrong person when autocomplete selects the wrong entry and won't let you change it back (not just uses an incorrect variation of the right one).  Personally I've had some embarrassing cases of emails going to the wrong people - which is far worse than having >> appear in their email address.
(Assignee)

Comment 45

2 years ago
Bug 1025684 should no longer happen.

By backing out bug 1043784 for thunderbird 31.4.0  "wrong selection" (bug 1107844) is now a lot less prominent. You don't need the Enter step after selection so it's harder to get into that situation.
(Assignee)

Comment 46

2 years ago
Comment on attachment 8543161 [details] [diff] [review]
bug1043310_ac_blur.patch

Approval Request Comment
[Feature/regressing bug #]: thunderbird switching to toolkit autocomplete - bug 959209
[User impact if declined]: the autocompleted address can be the suggestion text, not the actual address - happens if you move away from the addressing widget without confirming the selection with click or enter. it also ensures the selected address do not change to the default result if we already have something valid in the text box.
[Describe test coverage new/current, TBPL]: landed for 37 with tests
[Risks and why]: 
[String/UUID change made/needed]: no string change. risk it limited for firefox as the changes only come into play when using attributes not really used by firefox core, though possibly by extensions
Attachment #8543161 - Flags: approval-mozilla-esr31?
Attachment #8543161 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Blocks: 1025684
(Assignee)

Updated

2 years ago
Blocks: 486501

Comment 47

2 years ago
Testing with
https://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2015-01-13-03-02-03-comm-central/

The original bug is fixed:
- no more of these >>
- the capitalisation is now taken again from the address book (thank goodness!)

Aspects of other bugs are also fixed:
bug 1107844 - I tabbed or hit enter away from the selected entry and it was NOT changed to some other entry. Great.
Sorry, I don't understand enough about bug 1025684 or bug 486501 to claim that they are fixed or not. However, the original description of bug 486501 states: pick the second choice, edit, click/tab away. This now works, that is, the edits stick as expected.

Two questions:
Will this fix be available in versions other than "Nightly" soon, perhaps the next beta?

And, sorry, it's bad style to refer to other bugs, but ... What about bug 1042561, the misleading red colour?
(Assignee)

Comment 48

2 years ago
Yeah that's unrelated, follow the bug.
It's available for 37. Approval for 36 and 31 ESR has been requested.
status-firefox36: --- → affected
status-firefox37: --- → fixed
status-firefox38: --- → fixed
status-firefox-esr31: --- → affected
Attachment #8543161 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/1d406b3f20db
status-firefox36: affected → fixed
status-firefox38: fixed → ---
tracking-firefox-esr31: --- → 36+
Attachment #8543161 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+

Comment 50

2 years ago
Using Windows Vista and TB 31.4.0
In Write window
Type: tt
the name immediately offered in the autocomplete TO field says: tt >> name@emailaddress.com
this part is in highlighted in blue: >> name@emailaddress.com
name@emailaddress.com is also offered in the drop down selection.

If I click on the drop down name@emailaddress.com OR press TAB or Enter the correct email address is entered in the TO field.

However, if I click on the offered autocomplete: tt >> name@emailaddress.com
and then click in next TO field or Subject or Compose areas ( Not using Tab nor Enter nor drop down selection) then the entry in the TO field remains as the invalid: tt >> name@emailaddress.com and now there is no highlighted section.

So this part of the bug is still fully reproducable in 31.4.0
https://hg.mozilla.org/releases/mozilla-esr31/rev/7788753adc65
status-firefox-esr31: affected → fixed

Comment 52

2 years ago
Please excuse my asking:
How is it possible that the capitalization stuff was fixed in 31.4.0 but not the >> issue.
I was under the impression that the patch (if applied) fixed both issues.
I can see check-ins for "mozilla-central" (12th Jan.), "mozilla-beta" (14th Jan.) and "mozilla-ers31" (16th Jan.), the latter two after 31.4.0 was released (13th Jan.). So what went into 31.4.0?
(Assignee)

Comment 53

2 years ago
Anje: yes, because that is this bug, and it was not landed for 31.4.0. 

Jorg K: this bug did not change anything for 31.4.0, but bug 1043784 was backed out so people wouldn't hit the "wrong selection" so easily. (Which this bug fixes as a side effect.)

THIS bug, is fixed for 31.5.0 when that comes out in ~5-6 weeks - that is the mozilla-ers31 checkin.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1123532
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1127258
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1124925
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1124363

Comment 58

2 years ago
Will this fix the nickname problem at the same time, which have been working well for the past (couple of years-ish) until this release?   (31.4 ignores nicknames completely as far as I can see).  see bug 324548
(Assignee)

Comment 59

2 years ago
This is completely unrelated to that. (They aren't ignored, but anyway, that would be another bug.)

Comment 60

2 years ago
It is another bug, but it occurs during exactly the same activity, ie addressing a message.  I was hoping that reverting to 31.3 code would be a good start.  However I see you've put some life into it in 325458 thanks
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1127911
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1128482
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1128373
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1129902
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1132493
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1135939
See Also: → bug 632127

Comment 67

2 years ago
Feedback:
I would like to thank everyone for fixing this bug.
Using Windows Vista and TB 31.5.0
Regarding the >> issue. It is resolved. 
Whilst it initially returns the >>, upon tab, return or clicking in another TO, Subject or compose area, the error is corrected to show correct email address.
This includes any email address, display name and nickname. 
Many thanks, much appreciated.

Comment 68

2 years ago
Linux, TB 31.0.5.0 (mozilla.org version) works, too. thank you.

Comment 69

2 years ago
Not fixed at all, still experience the same problems at several computers, went back to 24.7
http://download.cdn.mozilla.net/pub/mozilla.org/thunderbird/releases/24.7.0/win32/

Comment 70

2 years ago
This is fixed. The fix will be included in TB38, currently in beta. TB31.x has not received the fix.
(Assignee)

Comment 71

2 years ago
This is fixed for 31.0.5, see comment 53. 

frogsfarms: it's not really useful to spam fixed bugs with "not fixed". Create a new bug and reference this bug if you want, and that report can be investigated there. This bug is fixed, and you'll have to give specific steps to reproduce in case there's some case still not covered.

Comment 72

2 years ago
Oops, so sorry. Yes, this is fixed in 31.5. I was thinking of the red addressed (bug 1042561) which will be fixed in TB38. Once again, my apologies.
Whiteboard: [fixed for TB 38]
Blocks: 1107844
No longer depends on: 1107844
Per comment 53 and comment 71, confirmed by user feedback in comment 67, this is fixed for TB 31.5.
Whiteboard: [fixed for TB 38] → [fixed for TB 31.5 per comment 53]
You need to log in before you can comment on or make changes to this bug.