1053774 - The Email button in the Loop UI on desktop opens gmail inside the popup if gmail is registered to handle mailto:

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Paul Silaghi, QA [:pauly]]

Attachment: after sending email with gmail.png

STR:
1. Go to Tools/Options/Applications in Firefox and set 'mailto'='always ask'
2. Click on the Loop icon/Email
3. Choose gmail or yahoo
4. Click again on the Loop icon --> you're asked to login
5. Login with your account --> compose message window appeared having the title and the body auto completed
6. Enter the receiver's address and send the mail with the invitation
7. Click again on the Loop icon

Actual results:
See attached

Comment 1

[Paul Silaghi, QA [:pauly]]

Attachment: after sending email with yahoo.png

Comment 2

[Paul Silaghi, QA [:pauly]]

Tested on 34.0a1 (2014-08-14), Win 7 x64.
Note: everything's fine if using Thunderbird as the mail client

Comment 3

[Paul Silaghi, QA [:pauly]]

The URLs are generated again after restarting the browser.

Comment 4

[Mark Banner (:standard8)]

Updating title to reflect what this is really about.

Comment 6

[Romain Gauthier [:tOkeshu]]

So the code currently use a button and changes window.location by a click handler.
We tried to see if replacing that by an link with target="_blank" would solve the problem but it seems the behavior here is unexpected.

When using a target="_blank" link to open a webmail, the webmail page loads in the current tab.
We probably want to fix that problem first.

Comment 7

[Mark Banner (:standard8)]

I think we either need to get bug 1054290 fixed, or we need to punch through a whole to gecko, and interrogate the external app handlers (?) to see where they are going to open a page or app. Maybe just interrogating them is enough.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Banner (:standard8) from comment #7)
> I think we either need to get bug 1054290 fixed, or we need to punch through
> a whole to gecko, and interrogate the external app handlers (?) to see where
> they are going to open a page or app. Maybe just interrogating them is
> enough.

What about calling window.open("mailto:whatever")?

Comment 9

[Mark Banner (:standard8)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #8)
> (In reply to Mark Banner (:standard8) from comment #7)
> > I think we either need to get bug 1054290 fixed, or we need to punch through
> > a whole to gecko, and interrogate the external app handlers (?) to see where
> > they are going to open a page or app. Maybe just interrogating them is
> > enough.
> 
> What about calling window.open("mailto:whatever")?

That works for the gmail case, but opens a new blank tab when opening the email in Thunderbird.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

I thought that would have been addressed by bug 241972 / bug 343921, but I suppose this path doesn't go through nsExternalAppHandler?

Comment 12

[Mark Banner (:standard8)]

From bug 1065540:

[Tracking Requested - why for this release]:
This appears to be a pretty severe problem for people who have webmail configured as their default mail application. Loop is scheduled to ship with 34, so it is important that we don't have this usability issue.

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: use nsIExternalProtocolService to send emails

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: use nsIExternalProtocolService to send emails

Now without ui-showcase changes

Comment 15

[Mike de Boer [:mikedeboer]]

Marco, can you add this bug to the current iteration?

Comment 16

[Marco Mucci [:MarcoM]]

Added to IT 35.2

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8490823 [details] [diff] [review]
Patch v1: use nsIExternalProtocolService to send emails

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

r+ if you rename sendEmail and switch to loadURI and re-test. An automated test would be good but since I think using mailto is temporary (or at least should be IMO due to lack of use by many webmail services) it's probably okay without it.

Note that I didn't test this so I trust that nsIExternalProtocolService actually uses Firefox's mailto dialog and therefore supports webmail.

::: browser/components/loop/MozLoopAPI.jsm
@@ +465,5 @@
> +     *
> +     * @param {String} subject Subject of the email to send
> +     * @param {String} body    Body message of the email to send
> +     */
> +    sendEmail: {

I think the method name implies actually sending an email (as opposed to mailto's action of composing an email). Perhaps "composeEmail" would be better?

@@ +469,5 @@
> +    sendEmail: {
> +      enumerable: true,
> +      writable: true,
> +      value: function(subject, body) {
> +        let mailtoUrl = "mailto:?subject=" + encodeURIComponent(subject) + "&" +

s/mailtoUrl/mailtoURL/ to match Firefox conventions

@@ +471,5 @@
> +      writable: true,
> +      value: function(subject, body) {
> +        let mailtoUrl = "mailto:?subject=" + encodeURIComponent(subject) + "&" +
> +                        "body=" + encodeURIComponent(body);
> +        extProtocolSvc.loadUrl(CommonUtils.makeURI(mailtoUrl));

loadUrl is deprecated so you should use loadURI but then after reading https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsIExternalProtocolService.idl#100 , I'm not sure about the second argument but if you tested loadUrl then it seems like null would work since that's basically what it passes to loadURI.

::: browser/components/loop/content/js/panel.jsx
@@ +364,5 @@
>      handleEmailButtonClick: function(event) {
>        this.handleLinkExfiltration(event);
> +
> +      navigator.mozLoop.sendEmail(__("share_email_subject3"),
> +        __("share_email_body3", {callUrl: this.state.callUrl}));

add spaces inside curly braces for readability like the example at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

Comment 18

[Mike de Boer [:mikedeboer]]

Pushed to fx-team with comments addressed and properly generated ui-showcase.js: https://hg.mozilla.org/integration/fx-team/rev/40b16e2d0fa7

Comment 19

[Mike de Boer [:mikedeboer]]

Pushed a follow-up bustage fix: https://hg.mozilla.org/integration/fx-team/rev/dae5e0f456d2

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/40b16e2d0fa7
https://hg.mozilla.org/mozilla-central/rev/dae5e0f456d2

Comment 21

[u279076]

Paul, please verify that this is fixed is the latest Nightly.

Comment 22

[Paul Silaghi, QA [:pauly]]

Now the email clients are opened outside of the Loop popup.
Verified fixed 35.0a1 (2014-09-21) Win 7, OS X 10.9.5, Ubuntu 12.10.
Filed bug 1070965 for remaining issues.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Mike - Now that this has been verified on m-c, can you please request approval to uplift to Aurora?

Comment 24

[Paul Silaghi, QA [:pauly]]

(In reply to Lawrence Mandel [:lmandel] from comment #23)
> Mike - Now that this has been verified on m-c, can you please request
> approval to uplift to Aurora?
gmail is still not working properly, see the followup bug

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8490823 [details] [diff] [review]
Patch v1: use nsIExternalProtocolService to send emails

Approval Request Comment
[Feature/regressing bug #]: Loop
[User impact if declined]: When the user chooses to share a Loop URL by clicking the 'Email' button and uses a webmail client to send emails, the webmail mail compose page is loaded inside the panel, which is jarring UX. This patch solves that.
[Describe test coverage new/current, TBPL]: Baked a couple of days on m-c, verified by QA
[Risks and why]: minor
[String/UUID change made/needed]: n/a

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8490823 [details] [diff] [review]
Patch v1: use nsIExternalProtocolService to send emails

Aurora+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 28

[Matthew N. [:MattN]]

This should probably wait to get uplifted with the rest of the [loop-uplift] bugs in batch to avoid rebasing hell if this gets rebased.

Comment 29

[u279076]

Paul can you please test this again across platforms using the following Try-server build?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 30

[Paul Silaghi, QA [:pauly]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #29)
> Paul can you please test this again across platforms using the following
> Try-server build?
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> f9eb2cbac352
Verified fixed Win 7, OS X 10.9.5, Ubuntu 13.04

Comment 31

[Mike de Boer [:mikedeboer]]

We'll uplift all the contacts patches in one go. Thanks for taking a look at this, Ryan!

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76dbf699bd7b
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f1d1816c15b

Comment 33

[Paul Silaghi, QA [:pauly]]

Don't know what's wrong, but the yahoo emails are no longer pre-populated. Everything's fine with gmail, thunderbird. 36.0a1 (2014-10-20)

Comment 34

[Maire Reavy [:mreavy]]

(In reply to Paul Silaghi, QA [:pauly] from comment #33)
> Don't know what's wrong, but the yahoo emails are no longer pre-populated.
> Everything's fine with gmail, thunderbird. 36.0a1 (2014-10-20)

Hi Pauly -- This does not appear to be a recent regression, and there's a good chance it's not due to a change in our code.  Since yahoo is a web app, yahoo likely changed their site.  I tested Nightly from 9/30 (which I believe you used to verify this as fixed), and I see the same problem (yahoo emails aren't pre-populated).   Can you open a new bug to track down what we need to do to get this working again?  Thanks.

Comment 35

[Paul Silaghi, QA [:pauly]]

bug 1086272