684380 - "Add to contacts" context menu command for mailto: and tel: links on Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Matt Brubeck (:mbrubeck)]

As suggested by Madhava in bug 664798 comment 6, Fennec could have an "Add to Address Book" context menu item for email addresses and telephone numbers.  On Android this could be implemented with this intent:
http://developer.android.com/reference/android/provider/ContactsContract.Intents.html#SHOW_OR_CREATE_CONTACT

Comment 1

[Josh Matthews [:jdm]]

Matt, is this still a valid bug for a new contributor to be working on? In other words, does this still apply to the Native UI, so that the work done won't be wasted?

Comment 2

[Matt Brubeck (:mbrubeck)]

This bug would still be useful for the native Android UI.  Moving to the Fennec Native component.

Comment 3

[Kevin Suvitcharnpun]

I will start working on it. I will finish it by May 17.

Comment 4

[Matt Brubeck (:mbrubeck)]

You can find the functions for adding context menus here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1140

Your context menu handler should use "sendMessageToJava", and you should add a new message handler to GeckoApp.handleMessage that receives the message and broadcasts the intent mentioned in comment 0:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#818

Comment 5

[kevins1]

Attachment: Patch file

Comment 6

[Matt Brubeck (:mbrubeck)]

Sorry for not responding to this sooner.  Is this patch working and ready to be reviewed?

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 625202 [details] [diff] [review]
Patch file

This is looking good.  Some feedback below:

>+            else if (event.equals("mailto:create")){
>+            else if(event.equals("tel:create")){

I think you will also need to call GeckoAppShell.registerGeckoEventListener in BrowserApp.initialize for this code to work.

Minor style feedback:  I would use a single event type like "Contact:Add" for both types of contacts, but send different data in the message depending on whether it's a phone number or email address.

>+      this.add(Strings.browser.GetStringFromName("contextmenu.mailcontact"),
>+                this.linkOpenableContext,

Instead of using linkOpenableContext (which will cause this to appear for every link), I would create a new "context" function that returns true only for mailto links.

>+      this.add(Strings.browser.GetStringFromName("contextmenu.phonecontact"),
>+                this.textContext,

And this should appear for "tel:" links (and any other phone-related URI protocols like "sip:").  I do like the idea of allowing it for plain text also, but maybe we could use a regular expression to match common phone number formats?  It's hard to get that to work for all locales, but it's okay if we don't catch everything (or if we catch a little too much.)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

The patch is a good start and Matt's comments are still relevant. I can take over mentoring.

Comment 9

[groodt]

I can start taking a look at this.

Comment 10

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 11

[Federico Paolinelli]

Hi there, can I start working on this bug?

Comment 12

[Mike Hoye [:mhoye]]

We haven't heard back from the current owner in a month; I've reset the Assignee field, feel free to take it.

Comment 13

[:Margaret Leibovic]

(In reply to Federico Paolinelli from comment #11)
> Hi there, can I start working on this bug?

I just assigned the bug to you. Let us know if you have any questions!

Comment 14

[Federico Paolinelli]

(In reply to :Margaret Leibovic from comment #13)
> (In reply to Federico Paolinelli from comment #11)
> > Hi there, can I start working on this bug?
> 
> I just assigned the bug to you. Let us know if you have any questions!

I'll certainly will :-)

Comment 15

[Federico Paolinelli]

Attachment: First version of the patch

Comment 16

[Federico Paolinelli]

Some notes:

- emailLinkContext and PhoneLinkContext were already there, I reused them. For thisreason I did not add contexts for plain text through regex's nor :sip links. In case we really want to extend this to email/phone looking plain text I'd guess we should extend those contexts and add some logic after that because the intent needs to be fired with a "mailto: / tel:" uri.

- There is a bit of code sharing between the two callbacks into browser.js, but not less than the other cases. I don't know if it's the case to share a private method between them

- I added one message with two different contents and I check for the content while handling it. Having a sort of flag which describes the content (mail / phone) could be a better option?

Comment 17

[Wesley Johnston (:wesj)]

Good to fix this, but if you're interested, bug 862537 is slightly related.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 759491 [details] [diff] [review]
First version of the patch

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            } else if (event.equals("Contact:Add")) {                
>+                if (!message.isNull("emailAddr")) {

Just use "email"

>+                } else if (!message.isNull("phoneNumber")) {

Just use "phone"

I wish we had a better place for this message receiver. GeckoApp is so "last resort", but I guess it's OK for now. Maybe we'll have somewhere else when the Contacts API lands.

It's too bad we can't just add these from JS. Maybe some day.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+        let emailAddr = NativeWindow.contextmenus._stripScheme(url);

You never use "emailAddr". Should you? I think you do want it.

>+        sendMessageToJava({
>+          type: "Contact:Add",
>+          emailAddr: url

use  | email |  here too

>+        let phoneNumber = NativeWindow.contextmenus._stripScheme(url);

You never use "phoneNumber". Should you? I think you do want it.

>+        sendMessageToJava({
>+          type: "Contact:Add",
>+          phoneNumber: url

Use  | phone | here too

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

>+contextmenu.addMailToContacts=Add Email Address to Contacts
>+contextmenu.addPhoneToContacts=Add Phone Number to Contacts

I think simply using "Add to Contacts" is enough. Use the same string/entity for both menus. No need for two. Maybe just | contextmenu.addToContacts |

Comment 19

[Federico Paolinelli]

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+        let emailAddr = NativeWindow.contextmenus._stripScheme(url);
> 
> You never use "emailAddr". Should you? I think you do want it.
> 

That's a legacy of a first attempt to make it work, before discovering that the intent wants the whole mailto:xxxx uri as argument. I guess I can removed those.

Comment 20

[Federico Paolinelli]

Attachment: Second version

Removed email and phone parsing from the context menu callback since they were not needed by the intent.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 759633 [details] [diff] [review]
Second version

Flagging Ian to get UX approval.

Also, does the APK need any additional permission requests to add items to the Contacts? I assume not.

Comment 22

[Ian Barlow (:ibarlow)]

Sorry -- what am I reviewing here?

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #22)
> Sorry -- what am I reviewing here?

Oops. I should have been more explicit. The main thing to approve is the use of the simplified string "Add to Contacts" for both phone numbers and email links. I felt we did not need to be too explicit (eg "Add Phone Number to Contacts").

Are you OK with the simplified strings?

Comment 24

[Ian Barlow (:ibarlow)]

Yes. Make it so!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c96768247e9b

Thanks for the patch!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c96768247e9b