Closed Bug 1053774 Opened 10 years ago Closed 10 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
1

Tracking

(firefox32 unaffected, firefox33 unaffected, firefox34+ verified, firefox35+ verified)

VERIFIED FIXED
mozilla35
Iteration:
35.2
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + verified
firefox35 + verified

People

(Reporter: pauly, Assigned: mikedeboer)

References

Details

(Whiteboard: [investigation][loop-uplift][fig:verified])

Attachments

(3 files, 1 obsolete file)

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
Tested on 34.0a1 (2014-08-14), Win 7 x64.
Note: everything's fine if using Thunderbird as the mail client
The URLs are generated again after restarting the browser.
Blocks: 1000772
Updating title to reflect what this is really about.
Summary: Unable to generate new urls after sending invitations via email → The Email button in the Loop UI on desktop opens gmail inside the popup if gmail is registered to handle mailto:
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.
Depends on: 1054290
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.
Target Milestone: --- → mozilla34
Whiteboard: [investigation]
(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")?
Flags: needinfo?(standard8)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Flags: needinfo?(standard8)
I thought that would have been addressed by bug 241972 / bug 343921, but I suppose this path doesn't go through nsExternalAppHandler?
Target Milestone: mozilla34 → mozilla35
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.
Whiteboard: [investigation] → [investigation][loop-uplift]
Assignee: nobody → mdeboer
OS: Windows 7 → All
Hardware: x86_64 → All
Status: NEW → ASSIGNED
Attachment #8490817 - Flags: review?(MattN+bmo)
Now without ui-showcase changes
Attachment #8490817 - Attachment is obsolete: true
Attachment #8490817 - Flags: review?(MattN+bmo)
Attachment #8490823 - Flags: review?(MattN+bmo)
Iteration: --- → 35.2
Points: --- → 1
Flags: firefox-backlog+
Whiteboard: [investigation][loop-uplift] → [investigation][loop-uplift] [qa+]
Marco, can you add this bug to the current iteration?
Flags: needinfo?(mmucci)
Added to IT 35.2
Flags: needinfo?(mmucci)
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
Attachment #8490823 - Flags: review?(MattN+bmo) → review+
Flags: qe-verify+
Whiteboard: [investigation][loop-uplift] [qa+] → [investigation][loop-uplift]
Pushed to fx-team with comments addressed and properly generated ui-showcase.js: https://hg.mozilla.org/integration/fx-team/rev/40b16e2d0fa7
QA Contact: anthony.s.hughes
Paul, please verify that this is fixed is the latest Nightly.
QA Contact: anthony.s.hughes → paul.silaghi
Depends on: 1070965
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.
Status: RESOLVED → VERIFIED
Mike - Now that this has been verified on m-c, can you please request approval to uplift to Aurora?
Flags: needinfo?(mdeboer)
(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 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
Attachment #8490823 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mdeboer)
Comment on attachment 8490823 [details] [diff] [review]
Patch v1: use nsIExternalProtocolService to send emails

Aurora+
Attachment #8490823 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(mdeboer)
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.
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/rjesup@wgate.com-f9eb2cbac352
Flags: needinfo?(paul.silaghi)
Whiteboard: [investigation][loop-uplift] → [investigation][loop-uplift][fig:verifyme]
(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/rjesup@wgate.com-
> f9eb2cbac352
Verified fixed Win 7, OS X 10.9.5, Ubuntu 13.04
Flags: needinfo?(paul.silaghi)
Whiteboard: [investigation][loop-uplift][fig:verifyme] → [investigation][loop-uplift][fig:verified]
We'll uplift all the contacts patches in one go. Thanks for taking a look at this, Ryan!
Flags: needinfo?(mdeboer)
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)
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(paul.silaghi)
bug 1086272
Flags: needinfo?(paul.silaghi)
Flags: needinfo?(mdeboer)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: