Closed Bug 1580950 Opened 4 months ago Closed 4 months ago

Address auto-complete click no longer accepts the address and moves to the next line

Categories

(Thunderbird :: Message Compose Window, defect, P2)

x86_64
Linux
defect

Tracking

(thunderbird_esr6869+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 69+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: jorgk-bmo, Assigned: aleca)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

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

I've just landed a snipped of bug 1569492 onto TB 68 here:
https://hg.mozilla.org/releases/comm--esr68/rev/1f74906fee2f0980ec552146596f5bee261fb4d2

In the resulting version, clicking onto an auto-complete result no longer accepts that address.

So that's broken on TB 70, trunk and now TB 68.

Flags: needinfo?(alessandro)

BTW, clicking on a auto-complete suggestion also doesn't work for mailing lists. I have't tried invitees, but it's most likely the same.

Looking at bug 1569492 again, couldn't that have been fixed much simpler using what bug 1569492 comment #4 suggested?

(In reply to Jorg K (GMT+2) from comment #2)

Looking at bug 1569492 again, couldn't that have been fixed much simpler using what bug 1569492 comment #4 suggested?

The notifylegacyevents="true" attribute is the first thing that I tried but never worked. It was completely ignored.
The decision to move to native attributes like oninput and onkeypress was also due to the upcoming conversion of those fields to native html input.

I'm compiling 68 and taking a look at why the snipped patch is not working properly.
How come is broken on trunk and 70?
We tested it and it was working properly when it landed.

Flags: needinfo?(alessandro)
Attached video Screencast from 2019-09-12 19.39.59.webm (obsolete) —

This seems to work fine for me.

This seems to work fine for me.

Well, I believe I see this in the video. You click on an auto-complete suggestion, and that is accepted into the input field. Then you hit return to move to the next field. But that's wrong. Clicking should automatically move to the next field, no hitting return required.

As I said on IRC: "20:08:15 - jorgk: that auto-complete is terribly tricky and sensitive".

What do you guys think about this solution?
Detecting the click event and forcing the return only if the target is not the textbox itself.

It's currently applied only to the messenger compose.
If you think it's a good solution I'll applied it everywhere where needed.

Assignee: nobody → alessandro
Attachment #9092546 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9092719 - Flags: review?(paul)
Attachment #9092719 - Flags: review?(jorgk)
Attachment #9092719 - Attachment description: 1580950-address-autocomplete.patch → 1580950-address-autocomplete-68.patch
Attachment #9092719 - Attachment filename: 1580950-address-autocomplete.patch → 1580950-address-autocomplete-68.patch
Comment on attachment 9092719 [details] [diff] [review]
1580950-address-autocomplete-68.patch

Seems to work. We need if for trunk/beta first, for 68 we can use this or the ontextentered hack.

Maybe this is better. Also, please add it to the two mailing list XUL files (add/edit).
Attachment #9092719 - Attachment description: 1580950-address-autocomplete-68.patch → 1580950-address-autocomplete.patch
Attachment #9092719 - Attachment filename: 1580950-address-autocomplete-68.patch → 1580950-address-autocomplete.patch
Attachment #9092719 - Flags: review?(jorgk) → feedback+
Attachment #9092719 - Attachment description: 1580950-address-autocomplete.patch → 1580950-address-autocomplete-68.patch
Attachment #9092719 - Attachment filename: 1580950-address-autocomplete.patch → 1580950-address-autocomplete-68.patch

Patch for trunk.

I just noticed that this issue doesn't happen for the Attendees invite in the calendar.
That textbox handles this problem by catching the blur event.
A similar solution to mine, but different.

Attachment #9092722 - Flags: review?(jorgk)

Patch updated for 68 with the Mailing List dialogs.

Attachment #9092719 - Attachment is obsolete: true
Attachment #9092719 - Flags: review?(paul)
Attachment #9092723 - Flags: review?(jorgk)
Comment on attachment 9092723 [details] [diff] [review]
1580950-address-autocomplete-68.patch

It's OK to review one patch ;-) - But thanks for the rebased version.
Attachment #9092723 - Flags: review?(jorgk)
Comment on attachment 9092722 [details] [diff] [review]
1580950-address-autocomplete.patch

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

This works. I've added some debug and clicking on auto-complete suggestions I saw span, image, description and richlistitem. If one day that throws up a div, we're hosed. Anyway, traditionally we had `ontextentered="awRecipientTextCommand(param, this); if (param && this.value != '') param.preventDefault();"` which looks much worse, do you agree? Also, I don't understand why clicking onto an auto-complete suggestion triggers this, well, maybe "text entered", but enter, which is what we processed:
https://dxr.mozilla.org/comm-esr60/source/mail/components/compose/content/addressingWidgetOverlay.js#846

Aceman and I have suffered through this quite a bit, for example coming up with the line quoted above, so I'll let him check it, too.

::: mailnews/addrbook/content/abMailListDialog.js
@@ -339,5 @@
>      input[0].setAttribute("id", "addressCol1#" + top.MAX_RECIPIENTS);
>    }
>  }
>  
> -function awNotAnEmptyArea(event) {

What about this stuff, this can just go?
Attachment #9092722 - Flags: review?(jorgk)
Attachment #9092722 - Flags: review?(acelists)
Attachment #9092722 - Flags: review+

I don't understand why clicking onto an auto-complete suggestion triggers this, well, maybe "text entered", but enter, which is what we processed:
https://dxr.mozilla.org/comm-esr60/source/mail/components/compose/content/addressingWidgetOverlay.js#846

The ontextentered method was used to deal with selecting a result from the autocomplete.
From the documentation: "This event handler is called when a result is selected for the textbox."

What about this stuff, this can just go?

That method was used to create an empty row on click in the Mailing List dialog if the last row had any value in it.
I replaced it with my method, which it also creates an extra empty row and also moves the focus on it.

If one day that throws up a div, we're hosed

I'm sure I will have to rework this whole thing once bug 1534455 lands and I'll have to convert those elements to html input.

Comment on attachment 9092722 [details] [diff] [review]
1580950-address-autocomplete.patch

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

This makes sense to me.  Just a few suggestions.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +909,5 @@
>  
> +function onRecipientClicked(event, inputElement) {
> +  if (
> +    event.originalTarget.localName != "div" &&
> +    event.originalTarget.localName != "textbox"

Would it work to check for a class rather than the localName?  If so, that might give us a little more future-proofing.  But maybe we don't have access to the originalTarget elements, and can't add our own class there, because they come from m-c code.  In that case we're relying on them not changing one thing or the other, and it's a coin toss as to which is more likely to change, so whatever works.

In any case, would be good to add a docstring explaining what this is here for.
Attachment #9092722 - Flags: feedback+
Attached patch 1580950.patch v2 (obsolete) — Splinter Review

What about this? Check for a <panel> element to see if a suggestion from the autocomplete list was clicked.

Yes, it will fail again if m-c rewrites autocomplete, but this is probably more indicative. If we see they touched <panel> we grep c-c for occurrences of panel and find this. With just "textbox" or "div" checks, we never find this place.

(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #11)

-function awNotAnEmptyArea(event) {

What about this stuff, this can just go?

In the mailing list editor, if you delete the last empty row of recipients and then click ANY of the existing ones (whether the first or last), a new empty row appears on the END. That looks weird and this patch drops this if we remove awNotAnEmptyArea().

Looking for the panel parent might be a bit more solid than my solution.
I'm good with it.

Do you want me to update the 2 patches with the suggested solution?

Discussed with Aceman over IRC. Let's return early if we're certainly not on the popup.

Attachment #9092722 - Attachment is obsolete: true
Attachment #9092723 - Attachment is obsolete: true
Attachment #9092768 - Attachment is obsolete: true
Attachment #9092722 - Flags: review?(acelists)
Attachment #9092790 - Flags: review+
Comment on attachment 9092790 [details] [diff] [review]
1580950.patch v2b

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

Yes, good to abort if we see we are in the recipient box already to avoid climbing 10 more levels of elements and find no panel.
Attachment #9092790 - Flags: feedback+
Target Milestone: --- → Thunderbird 71.0
Attachment #9092790 - Flags: approval-comm-esr68+
Attachment #9092790 - Flags: approval-comm-beta+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/43f3705dfb3b
Make click onto auto-complete results popup accept suggestion and move to the next line again. r=jorgk DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

Does anyone know why this patch seems to not be present on trunk, but the click works?

You know :-) - The trunk version was ripped out when auto-complete was de-XBL'ed and you actually implemented the new version here: Bug 1565075 comment #57.

Oh man, I completely forgot about it...uff, thanks for the reminder.

You need to log in before you can comment on or make changes to this bug.