1303304 - Use autocomplete completeselectedindex="true" in TB

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1302472 +++

To fix bug 1192739 Thunderbird should use completeselectedindex="true", yet it doesn't work.

From bug 1302472 comment #26:

completeselectedindex="true" (no >> seem to occur) (desirable future state):
3a) Cursor-key selected popup value is not confirmed with enter or tab (1st in popup is used)
    (video: attachment 8791806 [details]).
3b) Clicked popup value is not used (1st in popup is used).

From bug 1302472 comment #27:

Q: This is strange, does it happen always, only for autocomplete-in-the-middle? May need a new bug, since it's quite unexpected.
A: As I said above: no >> seem to occur.

===

Let's make completeselectedindex="true" work so TB can start using it in bug 1192739.

Comment 1

[Magnus Melin [:mkmelin]]

completeselectedindex and completedefaultindex give totally different widget behavior, for good or bad...

Comment 2

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #1)
> completeselectedindex and completedefaultindex give totally different widget
> behavior, for good or bad...
Good!!

Yes, it gives the widget behaviour that got lost in TB 31 after switching for XPFE to toolkit/autocomplete and which people have asked to restore ever since. If I had the time, I could find you 101 comments to that effect. BTW, SeaMonkey still has that behaviour and people know it from FF (bug 1192739 comment #0). Now is the time to undo the bad decision taken for TB 31!!

Comment 3

[Jorg K (CEST = GMT+2)]

I've just tried TB 24 and like S/M it copies down the popup value into the "base field" when you use the arrow keys. You've removed that in TB 31 and people, including myself, wanted it back ever since!!

I don't know what attributes were used on the old widget, but it's pretty clear that it doesn't behave like the new one.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: autocomplete.patch

Marco, will you look into this, or do we need to do it?

If so, where is a good starting point? Can that be tested in FF alone, since I know you guys don't want to build Thunderbird.

Another question:
https://dxr.mozilla.org/comm-central/rev/39761f94a020fa794f720670e873bb96c6df6b45/mail/components/compose/content/messengercompose.xul#955

That textbox has an awful lot of attributes:

<textbox id="addressCol2#1" class="plain textbox-addressingWidget uri-element"
  aria-labelledby="addressCol1#1"
  type="autocomplete" flex="1"
  autocompletesearch="mydomain addrbook ldap news"
  autocompletesearchparam="{}"
  timeout="300"
  maxrows="6"
  completedefaultindex="true" forcecomplete="true"
  minresultsforpopup="2" ignoreblurwhilesearching="true"
  ontextentered="awRecipientTextCommand(param, this)"
  onchange="onRecipientsChanged();"
  oninput="onRecipientsChanged();"
  onkeypress="awRecipientKeyPress(event, this)"
  onkeydown="awRecipientKeyDown(event, this)"
  disableonsend="true">
</textbox>

Is it possible that the fault is in the those callbacks awRecipientTextCommand(), onRecipientsChanged(), awRecipientKeyPress() and awRecipientKeyDown()?

Comment 5

[Marco Bonardo [:mak]]

I think you should keep both completedefaultindex and completeselectedindex. they can work together without problems.

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #4)
> Marco, will you look into this, or do we need to do it?
> 
> If so, where is a good starting point? Can that be tested in FF alone, since
> I know you guys don't want to build Thunderbird.

It's not that I don't want, it's that I don't have the time to :(
That said, I can't test this in Firefox, since it's a TB UI change. You should make a decision in the community, maybe even a test build and listen people feedback on it. and it's something that should be handled on the TB UI side, I don't know that specific code or the reasons who brought to the current behavior.

> Is it possible that the fault is in the those callbacks
> awRecipientTextCommand(), onRecipientsChanged(), awRecipientKeyPress() and
> awRecipientKeyDown()?

I don't know that code and unfortunately I don't have resources to learn it, atm.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Marco Bonardo [::mak] from comment #5)
> I think you should keep both completedefaultindex and completeselectedindex.
> they can work together without problems.
Using both seems to works perfectly!!!

I don't see any of the problems:
3a: Cursor-key selected popup value *IS* confirmed with enter or tab. YES!
3b: Clicked popup value *IS* used. YES!
Also problem 2 from bug 1303303:
2) If >> is showing: Hovered popup value is NOT selected with <enter> or <tab>. YES!

FANTASTIC!!

> It's not that I don't want, it's that I don't have the time to :(
> That said, I can't test this in Firefox, since it's a TB UI change. You
> should make a decision in the community, maybe even a test build and listen
> people feedback on it. and it's something that should be handled on the TB
> UI side, I don't know that specific code or the reasons who brought to the
> current behavior.
I appreciate your comment. The history is that for TB 31 the autocomplete was changed from XPFE to toolkit and people absolutely hated it. That's why I'm on the project. I joined because upgrading from TB 24 to TB 31 as an absolute disaster. You know how many bugs were fixed in autocomplete as used by TB. As I said in comment #2, people want the old XPFE behaviour back, which is was Firefox does with completeselectedindex="true" and that's what SeaMonkey still does using XPFE. Repeating: From TB 24 to TB 31 the behaviour was changed without any community consultation (not to mention the many bugs in the new behaviour) and there was an outcry. So why would I now want community involvement to restore the previous accepted state. Had I known that completeselectedindex="true" existed, I would have lobbied it much earlier.

Patch coming. One extra line fixes all problems. Unbelievable.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: autocomplete.patch

You wouldn't believe it, but this one line ends years of agony ;-)
Try it!

And the >> are still there so we only change the cursor key behaviour and stop the selection of the hovered value.

Comment 8

[Jorg K (CEST = GMT+2)]

Magnus, if you agree, I'll fix these as well:

calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
mail/components/addrbook/content/abEditListDialog.xul
mail/components/addrbook/content/abMailListDialog.xul
mail/components/compose/content/messengercompose.xul (covered here).

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: autocomplete.patch

This simple patch makes address entry 500% better and fixes all the problems listed in comment #6.

Comment 10

[[:MakeMyDay]]

I still need  to test this patch, but will that already play together with the upcoming change to the autocomplete popup from bug 1296638 announced here: https://groups.google.com/forum/#!topic/mozilla.dev.platform/iAKIwBZb9rk ?

Comment 11

[Jorg K (CEST = GMT+2)]

I think for the calendar part it's more or less an rs review since the calendar just needs to follow whatever the rest of the system does ;-) But do play around with entering e-mail addresses into messages and invitees by all means.

Look out for:
1) Cursor keys now populate the base field from the popup as we used to up to TB 24.
2) Tab and enter no longer confirm the hovered popup value (what a madness that is).
3) Everything else just works(TM) ;-)

As for your question: We'll be using a valid combination of published parameters on the suggestion of the owner of the module (comment #5). That should continue to work whatever M-C does under the covers.

Comment 12

[[:MakeMyDay]]

Comment on attachment 8791982 [details] [diff] [review]
autocomplete.patch

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

Thanks, r+ for the calendar part.

Regarding bug 1296638, I would assume we need at least to change the type from autocomplete to autocomplete-richlistbox once that gets landed.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to [:MakeMyDay] from comment #12)
> Thanks, r+ for the calendar part.
You like the changed behaviour? Did you try it out extensively?

Comment 14

[[:MakeMyDay]]

Yes, I tried it and I liked it, thanks. Autocomplete has been an endless story since 31...

I did testing and ran into some odd behaviour first with my origanl ldap test setup. After I configured an new service, everything runs as expected. Unfortunately, there doesn't seem to exist a debug setting for ldap, that one can pref on to see in the console what is send to and received from the server. At least I didn't find any.

Comment 15

[:aceman]

Comment on attachment 8791982 [details] [diff] [review]
autocomplete.patch

Nice, selecting with arrow keys work :)
I didn't notice any problems. But I am not a heavy autocomplete user, I didn't notice any problems of the 24->31 upgrade.

Comment 16

[Jorg K (CEST = GMT+2)]

Let me summarise one more time why this patch is *absolutely essential*:

Currently, you can confirm the *hovered* choice with <enter> when >> is shown, leading to all sorts of problems (bug 1192739) - video in attachment 8791678 [details].
The patch fixes that.

Currently you cannot confirm the cursor-key-selected choice any more with enter (bug 1302472).
The patch fixes that, too, before M-C will fix the regression.

Finally with the patch, the field value is populated from the popup when the cursor keys are used, which is FF, SM and TB 24 behaviour that got lost in TB 31 and caused much opposition.

Everything else also works: Click on a popup value works and <enter> or <tab> confirm the field value, not the hovered popup value. And the autocomplete-in-the-middle is also still there.

What more could you possibly ask for?

Comment 17

[:aceman]

Comment on attachment 8791982 [details] [diff] [review]
autocomplete.patch

I think we should get this landed for TB51 to get proper testing/feedback on aurora/beta from the users that complained about this. If we do not land it now, we can get hit by bug 1296638 and have autocomplete broken for weeks when we get none of the testing.

Comment 18

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e610d139e120

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791982 [details] [diff] [review]
autocomplete.patch

[Approval Request Comment]
Regression caused by (bug #): Bug 959209, much hated regression in TB 31 (!!)
User impact if declined: very suboptimal address entry (in an e-mail client!)
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Shouldn't be risky, but we never know what surprises toolkit/autocomplete holds. Let's get it some testing exposure in Daily, Aurora and Beta channels (TB 50 beta coming up after tomorrow's branch date).

Comment 20

[Jorg K (CEST = GMT+2)]

Aurora (TB 50):
https://hg.mozilla.org/releases/comm-aurora/rev/b0b35fc49004
landed with DONTBUILD since tomorrow is branch day and then everything will be built ;-)

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Marco Bonardo [::mak] from bug 1303303 comment #8)
> Bugs should be tracked until the code causing them is fixed or removed.
> We can avoid tracking this with an high priority though.
Marco, this what you said in bug 1303303.

Here we also had a problem. Autocompletion didn't work with only completeselectedindex="true" set. It works fine with both completedefaultindex and completeselectedindex set as you suggested in comment #5. The initial summary was:
*Make* autocomplete completeselectedindex="true" *work* in TB
Well, we made it work, but I have the feeling that we glossed over a problem. I think completeselectedindex="true" should work by itself like in FF's urlbar. Or am I misunderstanding something?

Maybe I am. I'm reading
https://dxr.mozilla.org/comm-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/mozilla/toolkit/components/autocomplete/nsIAutoCompleteInput.idl#34
https://dxr.mozilla.org/comm-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/mozilla/toolkit/components/autocomplete/nsIAutoCompleteResult.idl#38

Since we're setting defaultIndex in our code:
https://dxr.mozilla.org/comm-central/rev/c3daef7d184b8e55f269e168c50ca1cde3035348/mailnews/news/src/nsNewsAutoCompleteSearch.js#127
https://dxr.mozilla.org/comm-central/rev/c3daef7d184b8e55f269e168c50ca1cde3035348/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#448
we most likely also need the completedefaultindex="true". Right? Otherwise unexpected things will happen.

So if you're happy with our solution, we just consider the matter closed.

Comment 22

[Marco Bonardo [:mak]]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #21)
> So if you're happy with our solution, we just consider the matter closed.

toolkit bugs must be reported (and eventually fixed) regardless of consumers, even if TB doesn't use anymore something, we may want to fix bugs for add-ons.
That said, I'm happy if you're satisfied with the current solution.

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #21)
> Well, we made it work, but I have the feeling that we glossed over a
> problem. I think completeselectedindex="true" should work by itself like in
> FF's urlbar. Or am I misunderstanding something?

FF urlbar uses both completeselectedindex and completedefaultindex.

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to Marco Bonardo [::mak] from comment #22)
> toolkit bugs must be reported (and eventually fixed) regardless of
> consumers, even if TB doesn't use anymore something, we may want to fix bugs
> for add-ons.
Indeed, but before I can report a bug, I need to know whether I'm seeing a bug or consequences of a misuse. While trying out options in this bug, I noticed that completeselectedindex="true" alone didn't work in Thunderbird. I asked you whether the callbacks could be causing this (comment #4) but you didn't actually answer the question. The question was not whether the TB callbacks *are* causing the problems, but more general, whether the callbacks *could* cause what I'm seeing. So I still don't know whether to report this further or not.

Repeating what I saw: With completeselectedindex="true" alone, <enter> confirmed the first list entry, not the cursor-key selected one. Mouse-click also confirmed the first list entry.

> That said, I'm happy if you're satisfied with the current solution.
Personally, I'm extremely satisfied. The TB autocomplete behaviour wasn't right for more than two years now since version TB 31.

> FF urlbar uses both completeselectedindex and completedefaultindex.
I must be blind, can you enlighten me, I don't see it:
https://dxr.mozilla.org/comm-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/mozilla/browser/base/content/browser.xul#661

Comment 24

[Marco Bonardo [:mak]]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #23)
> Repeating what I saw: With completeselectedindex="true" alone, <enter>
> confirmed the first list entry, not the cursor-key selected one. Mouse-click
> also confirmed the first list entry.

bug 1302472 will completely revert this as it was before.

> > That said, I'm happy if you're satisfied with the current solution.
> Personally, I'm extremely satisfied. The TB autocomplete behaviour wasn't
> right for more than two years now since version TB 31.
> 
> > FF urlbar uses both completeselectedindex and completedefaultindex.
> I must be blind, can you enlighten me, I don't see it:
> https://dxr.mozilla.org/comm-central/rev/
> f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/mozilla/browser/base/content/
> browser.xul#661

Yeah, it's not done through the attribute, we just set a defaultIndex value on the result directly in the search component, cause sometimes we want to autofill, sometimes not, and we don't want autofill if there is no popup selection. The problem as usual is that the same autocomplete code is used for many different use-cases, each one with its special behavior.

Comment 25

[Kent James (:rkent)]

Per comment 19 and normal practice, this patch will not land in 45.4.0 but earliest 45.5.0

Comment 26

[Kent James (:rkent)]

Comment on attachment 8791982 [details] [diff] [review]
autocomplete.patch

https://hg.mozilla.org/releases/comm-esr45/rev/caa4ed0465b1