1290839 - Address Autocomplete: Red addresses still occur after bug 1042561 if addresses are added from the Contacts Sidebar following unknown address

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

Address Autocomplete:
Red addresses still occur after bug 1042561 if addresses are added from the Contacts Sidebar following unknown address:

STR:
- Create a new message
- Open contacts sidebar
- add an unknown recipient by hand, this will show in red.
- hit enter or click to position to the next row. Note the red caret.
- now add addresses using "Add to To:" or drag addresses from the contacts sidebar.
Result: The additional addresses are shown in red.

(I found this while reviewing bug 1290729.)

Comment 2

[:aceman]

Yes, I've also seen this :)

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #2)
> Yes, I've also seen this :)
And you didn't report it and didn't provide a patch to fix it :-(

For the record from bug 1042561.
The colour is driven by the "nomatch" attribute:
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/autocomplete.xml#220
here:
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/windows/global/autocomplete.css#20

I have the impression that adding addresses from the sidebar doesn't run autocomplete, since there is no user text input, and onSearchComplete is not run, so the attribute maintains its previous value.

Maybe we need to clear it somewhere in addressingWidgetOverlay.js where the new rows are created. Just a wild guess.

Aceman, do you have any suggestion?

Comment 4

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #3)
> (In reply to :aceman from comment #2)
> > Yes, I've also seen this :)
> And you didn't report it and didn't provide a patch to fix it :-(
Only noticed while playing with that bug :) I have 5 other patches in progress to fix 1075398 so I do not want to get distracted by the autocomplete.

> I have the impression that adding addresses from the sidebar doesn't run
> autocomplete, since there is no user text input, and onSearchComplete is not
> run, so the attribute maintains its previous value.
> 
> Maybe we need to clear it somewhere in addressingWidgetOverlay.js where the
> new rows are created. Just a wild guess.

Yes, in awAppendNewRow we clone the first row to create the new rows at the end of the list (bottom). We already remove focused attribute from the clone. Maybe we also need to remove these autocomplete attributes.

Strangely, if the first recipient is fine (black), then I can't get a manually typed recipient in second row to become red. Even if it is the same junk that would be red if typed into first row.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #4)
> Strangely, if the first recipient is fine (black), then I can't get a
> manually typed recipient in second row to become red. Even if it is the same
> junk that would be red if typed into first row.
Damn. I think I saw this too and when I wanted to reproduce it, I couldn't and got sidetracked to bug 1151520.
But yes, that's another problem. I find this really frustrating. Autocomplete was switched to toolkit with TB 31 and here we are at TB 51 and it still doesn't work.

Let's just clearly state the second case:

STR 2:
- Create a new message
- Open contacts sidebar
- add a known address from the sidebar using "Add to To:"
- add an unknown recipient by hand into the second line that opened automatically.
Result: The unknown recipient will show in *black*.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a) for both problems.

As Aceman observed, new rows are a clone of the first now. If they are red, that is the nomatch attribute is set, the cloned row also starts in red. If the first row it black, then in has no highlightnonmatches = true; and the clone won't have either. So even if autocomplete sets "nomatch", the row doesn't go red due to
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/windows/global/autocomplete.css#20
textbox[nomatch="true"][highlightnonmatches="true"] {
  color: red;
}

This patch fixes both issues ;-)

Let's land this quickly since I want it for TB 49 beta. Wrong colours are an absolute pet hate of mine.
(I hate them with the seething wrath of 1001 exploding suns).

Comment 7

[:aceman]

Comment on attachment 8776921 [details] [diff] [review]
Proposed solution (v1a) for both problems.

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +596,5 @@
>        input[0].setAttribute("value", "");
> +      // Reset autocomplete attributes.
> +      input[0].removeAttribute("nomatch");
> +      if (getPref("mail.autoComplete.highlightNonMatches"))
> +        input[0].setAttribute("highlightnonmatches", true);

Why would there ever be a row without "highlightnonmatches=true" ? We set it on the first row (based on the pref) and then it is always cloned. What does remove it? In my tests I have seen the attribute still there even on black rows.

Comment 8

[Jorg K (CEST = GMT+2)]

You need to experiment in the DOM Inspector. This is the observed behaviour:

1) Open new composition and inspect first row. No "highlightnonmatches".
2) Type a letter. Autocomplete starts, "highlightnonmatches" is now set.
3) Keep typing and finally accept autocomplete. "highlightnonmatches" stays set.
4) Inspect next field: Since it is a clone of the first, it has "highlightnonmatches".

Now use the side panel:
1) Open new composition and inspect first row. No "highlightnonmatches".
2) Add an address via the side panel. Still no "highlightnonmatches".
3) Inspect next field: Since it is a clone of the first, it has no "highlightnonmatches".
   That's the bug.

I admit that my patch is a little crude, however, it fixes the problem.

Repeating: Only manual typing sets "highlightnonmatches", adding an address does not.
So the question is: Where does it get set, not: What does remove it? ;-)

Looking at DXR, I can only see it being set here:
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/autocomplete.xml#334

Looks like there is a little confusion with property (of what?) "highlightNonMatches" and attribute "highlightnonmatches". The latter which is all lowercase drives the colour.

I have the impression that we set "highlightNonMatches" (on what?) and consequently autocomplete sets "highlightnonmatches" on the element with id "addressCol2#nn".

If autocomplete doesn't run since we use the side panel, then there is no "highlightnonmatches". If that row is cloned, it's not set, but my patch sets it.

Do you have a better suggestion?

Comment 9

[Jorg K (CEST = GMT+2)]

Hmm, I think "highlightNonMatches" is a XUL/RDF property. But after the clone we're dealing with DOM attributes, so I think it's correct to clear "nomatch" and set "highlightnonmatches".

I'm open to a different *working* and more elegant solution ;-)

Comment 10

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #8)
> Repeating: Only manual typing sets "highlightnonmatches", adding an address
> does not.
> So the question is: Where does it get set, not: What does remove it? ;-)

It gets set in setupAutocomplete() that is run on each keypress. See bug 1107209.
I'd like to change that if possible.

> Looks like there is a little confusion with property (of what?)
> "highlightNonMatches" and attribute "highlightnonmatches". The latter which
> is all lowercase drives the colour.

No confusion. There is an attribute and you can easily set it via the property.

> I have the impression that we set "highlightNonMatches" (on what?) and
> consequently autocomplete sets "highlightnonmatches" on the element with id
> "addressCol2#nn".

Actually highlightnonmatches is only ever set on the first row (via setupAutocomplete) and then relies on cloning to copy it to next rows.

That would explain the observation that if the first row is copied from AB, it does not get the attribute set. Then if I type in row 2, it is never red as it does not have the attribute (as the first row didn't have it due to no typing). But if you type into first row, and then into second row, both have the attribute and are red.
 
> If autocomplete doesn't run since we use the side panel, then there is no
> "highlightnonmatches". If that row is cloned, it's not set, but my patch
> sets it.

Yes, but it sets it only for 2nd and later rows, never for 1st.

> Do you have a better suggestion?

I suggest we play with highlightnonmatches in bug 1107209. I have some ideas for that.

In this bug please only remove the "nomatch".

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #10)
> No confusion. There is an attribute and you can easily set it via the
> property.
I was confused ;-(

> > ...  but my patch sets it.
> Yes, but it sets it only for 2nd and later rows, never for 1st.
So? That's not necessary. Try this: Add from the sidepanel to the first row. Black. Type junk into the second row. Black=wrong. Delete the stuff from the first row. Type junk into the first row: Red=correct.

> > Do you have a better suggestion?
> I suggest we play with highlightnonmatches in bug 1107209. I have some ideas
> for that.
> In this bug please only remove the "nomatch".
Hmm, maybe I don't agree. There is an ugly bug, we have a fix, maybe not the most elegant one, so why not land that?

If you want to do a more purist thing later, that's good, but bug 1107209 has been sitting there since 2014 with last comment a little short of a year ago.

Let me say it again: I hate the wrong colours and I think it reflects really badly on the product so we should fix it ASAP.

Comment 12

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #11)
> > > Do you have a better suggestion?
> > I suggest we play with highlightnonmatches in bug 1107209. I have some ideas
> > for that.
> > In this bug please only remove the "nomatch".
> Hmm, maybe I don't agree. There is an ugly bug, we have a fix, maybe not the
> most elegant one, so why not land that?

The patch only removing "nomatch" will solve this bug as filed in comment 0.

Setting highlightnonmatches in awAppendNewRow() would go against the idea of that function, that copies all attributes by cloning, so why would one need to be set explicitly?
Also, it would go against the comment in setupAutocomplete() which again claims that highlightnonmatches is propagated via cloning. It would add to the confusion of bug 1107209 on when setupAutocomplete is actually needed.

> If you want to do a more purist thing later, that's good, but bug 1107209
> has been sitting there since 2014 with last comment a little short of a year
> ago.

Yes, it was sitting there because nobody looked at it. Now that we actually did and try to understand what highlightnonmatches does, we can move it forward. The STR2 does semantically belong there, but I only realized it later.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #12)
> The STR2 does semantically belong there, but I only realized it later.
OK, you win ;-)
Let's get it fixed one way or another and I'm happy to do it your way.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a) for first problem.

Comment 15

[:aceman]

Comment on attachment 8777142 [details] [diff] [review]
Proposed solution (v1a) for first problem.

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

Thanks.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +596,5 @@
>        input[0].setAttribute("value", "");
>  
> +      // Reset autocomplete attribute "nomatch" so we don't cause red addresses
> +      // on a cloned row.
> +      input[0].removeAttribute("nomatch");

Ok, just move this below the removing of the "focused" attribute. Because the nomatch only gets copied do to the cloning the comment below describes.

Comment 16

[Jorg K (CEST = GMT+2)]

OK, I'll move it down, but should I change this
  if (input[0].getAttribute('focused') != '')
    input[0].removeAttribute('focused');
to
  input[0].removeAttribute("focused");
to match the
  input[0].removeAttribute("nomatch");
I'm adding?

Why test? Just remove it, won't hurt if it wasn't set to start with.

Comment 17

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #16)
> OK, I'll move it down, but should I change this
>   if (input[0].getAttribute('focused') != '')
>     input[0].removeAttribute('focused');
> to
>   input[0].removeAttribute("focused");
> to match the
>   input[0].removeAttribute("nomatch");
> I'm adding?
> 
> Why test? Just remove it, won't hurt if it wasn't set to start with.

Yes, you can do that, I actually do exactly that in bug 1290755 :)

It could hurt, if removeAttribute throws if the attribute to delete is not found. But according to the docs it does not, so all should be fine.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1b) for first problem.

Sorry to rot bug 1290755 ;-(
(I'll review it soon).

Comment 19

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/3e63d762222f

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777289 [details] [diff] [review]
Proposed solution (v1b) for first problem.

[Approval Request Comment]
Regression caused by (bug #): Looks like this has been wrong for a while, most likely due to switch to toolkit/autocomplete in TB 31.
User impact if declined: irritating red colour for recipients moved via side panel.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Simple change just resetting "nomatch" attribute.

Comment 21

[Jorg K (CEST = GMT+2)]

Aurora (TB 50):
https://hg.mozilla.org/releases/comm-aurora/rev/91b55333fdc1

Comment 22

[Jorg K (CEST = GMT+2)]

Beta (TB 49):
https://hg.mozilla.org/releases/comm-beta/rev/1487c5724454

Comment 23

[Kent James (:rkent)]

Comment on attachment 8777289 [details] [diff] [review]
Proposed solution (v1b) for first problem.

http://hg.mozilla.org/releases/comm-esr45/rev/58013d504268