Closed Bug 959209 Opened 6 years ago Closed 6 years ago

Investigate whether Thunderbird still needs the XPFE autocomplete widget

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: neil, Assigned: mkmelin, NeedInfo)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

Now that LDAP autocomplete uses the toolkit interfaces, Thunderbird might not need to use the XPFE widget any more.

As a basic test, remove the %define AUTOCOMPLETE_OLD_STYLE after the %ifdef MOZ_THUNDERBIRD in toolkit/content/xul.css and rebuild. As a more advanced test, also remove the line referencing xpfe in bridge/bridge.mk and clobber.
More complete list of testing:

- Check that everything builds and basically runs as per comment 0
- Test autocomplete widget behaviors are the same before and after for:
-- local address books
-- ldap address books
-- gloda autocomplete (aka global search)

Note this is basically a dupe of bug 360648, but as that is a meta, lets set this as blocking it as a neat sub-set.
Blocks: 360648
Attached patch mozilla-central patch (obsolete) — Splinter Review
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8376855 - Flags: review?(mbanner)
Attached patch comm-central patch (obsolete) — Splinter Review
After this patch, things seem to work like before.

So what i changed:
 - toolkit uses param, not eventParam
 - ontextentered (calls awRecipientTextCommand) sends in a different type of argument, what we currently check there looks bogus though (but i didn't verify). What we get from toolkit seems to be a KeyboardEvent or null
 - onerrorcommand appears not to exist in toolkit. 
 - in awRecipientKeyPress the arrow key stuff never gets hit - looks like autocomplete consumes them already
 - bogus comment removed in awRecipientKeyDown. If i read it correctly that's the comment says not to do what the next line actually does.
Attachment #8376858 - Flags: review?(mbanner)
Attachment #8376858 - Flags: feedback?(neil)
Keywords: helpwanted
(In reply to Magnus Melin from comment #3)
>  - ontextentered (calls awRecipientTextCommand) sends in a different type of
> argument, what we currently check there looks bogus though (but i didn't
XPFE sends "typing" (if the value was typed), "scrolling" (if the value was selected from autocomplete using the keyboard) or "clicking" (if the value was selected from autocomplete using the mouse). (There is a fourth value of "none" but that doesn't apply here as we ignore blur while searching.)

> verify). What we get from toolkit seems to be a KeyboardEvent or null
As far as I see it, you get the Enter event that triggered the toolkit equivalent of "typing" or "scrolling" and null is the toolkit equivalent of "clicking" or "none".

>  - onerrorcommand appears not to exist in toolkit.
We don't actually generate error items any more, and we definitely don't have any parameters in which we can store an extended description.
Comment on attachment 8376858 [details] [diff] [review]
comm-central patch

>-function awRecipientTextCommand(userAction, element)
>+function awRecipientTextCommand(enterEvent, element)
> {
>-  if (userAction == "typing" || userAction == "scrolling")
So this probably needs to say if (enterEvent) if you want to maintain the previous behaviour of only advancing to a new row when you use the keyboard rather than clicking on an autocomplete suggestion.
Attachment #8376858 - Flags: feedback?(neil)
(In reply to neil@parkwaycc.co.uk from comment #5)
> Comment on attachment 8376858 [details] [diff] [review]
> comm-central patch
> 
> >-function awRecipientTextCommand(userAction, element)
> >+function awRecipientTextCommand(enterEvent, element)
> > {
> >-  if (userAction == "typing" || userAction == "scrolling")
> So this probably needs to say if (enterEvent) if you want to maintain the
> previous behaviour of only advancing to a new row when you use the keyboard
> rather than clicking on an autocomplete suggestion.

True. I just retested that. However, I'd be inclined to change it to add a new row for both cases (like the patch does). Since we can't autocomplete more than one address per line, so otherwise you're kind of stuck there at the end of the line after you select one address in the popup.

Thx for the feedback!
Comment on attachment 8376855 [details] [diff] [review]
mozilla-central patch

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

Officially, I'm not a toolkit reviewer, but I suspect Neil can r+ this.
Attachment #8376855 - Flags: review?(neil)
Attachment #8376855 - Flags: review?(mbanner)
Attachment #8376855 - Flags: feedback+
Comment on attachment 8376858 [details] [diff] [review]
comm-central patch

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

The general idea looks fine, however from the comments, it looks like you want to update the patch based on Neil's comments, so I'd like to see that before reviewing it. If I've miss-understood, feel free to re-request review.
Attachment #8376858 - Flags: review?(standard8) → feedback+
Comment on attachment 8376858 [details] [diff] [review]
comm-central patch

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

Actually, I don't think there were anything to address, except potentially comment 6. I'd prefer what the patch does as is, but i can add the if clause if you want.
Attachment #8376858 - Flags: review?(standard8)
Comment on attachment 8376858 [details] [diff] [review]
comm-central patch

This looks good, giving this a tentative r+. I'm seeing a bit of a flicker on my Mac when new entries are received - It could be because we're removing all the items then adding them back again, but that doesn't quite make sense in my experience of autocomplete.

If there's nothing obvious for fixing, I think we can probably land this and get some feedback on it. We might need to have a bug on it if the flicker is bad on other platforms (especially windows) as well.
Attachment #8376858 - Flags: review?(standard8) → review+
Comment on attachment 8376855 [details] [diff] [review]
mozilla-central patch

Is MOZ_STANDALONE_COMPOSER actually used any more?
Flags: needinfo?(daniel)
(In reply to neil@parkwaycc.co.uk from comment #11)
> Comment on attachment 8376855 [details] [diff] [review]
> mozilla-central patch
> 
> Is MOZ_STANDALONE_COMPOSER actually used any more?

http://mxr.mozilla.org/comm-central/search?string=MOZ_STANDALONE_COMPOSER&filter=^[^\0]*%24 do list very little usage...
The one /editor/ autocomplete usage I'd think likely works with toolkit too - http://mxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdDialogOverlay.xul#55

Either way, would be good to move this bug forward, so we can land in good time for the next tb ESR, which moves to alpha in about two weeks.
Flags: needinfo?(fabien)
Unbitrot cc.
Attachment #8376858 - Attachment is obsolete: true
Attachment #8413171 - Flags: review+
Removing standalone composer too, per comment over irc
Attachment #8376855 - Attachment is obsolete: true
Attachment #8376855 - Flags: review?(neil)
Attachment #8413173 - Flags: review?(neil)
Attachment #8413173 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/eadb8b61b59a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Comment on attachment 8413173 [details] [diff] [review]
mozilla-central patch

>+/* SeaMonkey don't use the new toolkit's autocomplete widget yet.... */
>+%ifdef MOZ_SUITE
> %define AUTOCOMPLETE_OLD_STYLE
> %endif
> 
> %ifdef AUTOCOMPLETE_OLD_STYLE
Bah, I meant to say that you should eliminate the AUTOCOMPLETE_OLD_STYLE variable.
Blocks: 1002278
Flags: needinfo?(fabien)
fabien, see comment 12
Flags: needinfo?(fabien)
Depends on: 1007040
Depends on: 1009469
Depends on: 1014105
Depends on: 1012398
Depends on: 1043310
Depends on: 1043784
Depends on: 1012397
No longer depends on: 1055149
Depends on: 1044336
Depends on: 1068036
Flags: needinfo?(fabien)
Depends on: 1151520
You need to log in before you can comment on or make changes to this bug.