Closed Bug 684380 Opened 8 years ago Closed 6 years ago

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

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: mbrubeck, Assigned: fedepaol)

References

(Blocks 1 open bug)

Details

(Keywords: mobile, Whiteboard: [mentor=mfinkle@mozilla.com][good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

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
Keywords: helpwanted, mobile
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?
This bug would still be useful for the native Android UI.  Moving to the Fennec Native component.
Keywords: helpwanted
Priority: -- → P4
Product: Fennec → Fennec Native
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug] → [mentor=mbrubeck@mozilla.com][good first bug][lang=js]
Assignee: nobody → michaelkohler
Assignee: michaelkohler → nobody
I will start working on it. I will finish it by May 17.
Assignee: nobody → kevin401099
Status: NEW → ASSIGNED
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
Attached patch Patch file (obsolete) — Splinter Review
Sorry for not responding to this sooner.  Is this patch working and ready to be reviewed?
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.)
Attachment #625202 - Flags: feedback+
The patch is a good start and Matt's comments are still relevant. I can take over mentoring.
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=js] → [mentor=mfinkle@mozilla.com][good first bug][lang=js]
I can start taking a look at this.
Can you confirm that you're still working on this bug?
Flags: needinfo?(kevin401099)
Hi there, can I start working on this bug?
We haven't heard back from the current owner in a month; I've reset the Assignee field, feel free to take it.
Assignee: kevin401099 → nobody
Flags: needinfo?(kevin401099)
(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!
Assignee: nobody → fedepaol
(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 :-)
Attached patch First version of the patch (obsolete) — Splinter Review
Attachment #759491 - Flags: feedback?(mark.finkle)
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?
Good to fix this, but if you're interested, bug 862537 is slightly related.
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 |
Attachment #759491 - Flags: feedback?(mark.finkle) → feedback+
> >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.
Attached patch Second versionSplinter Review
Removed email and phone parsing from the context menu callback since they were not needed by the intent.
Attachment #759491 - Attachment is obsolete: true
Attachment #759633 - Flags: review?(mark.finkle)
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.
Attachment #759633 - Flags: review?(mark.finkle)
Attachment #759633 - Flags: review+
Attachment #759633 - Flags: feedback?(ibarlow)
Sorry -- what am I reviewing here?
(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?
Yes. Make it so!
Attachment #625202 - Attachment is obsolete: true
Attachment #759633 - Flags: feedback?(ibarlow) → feedback+
https://hg.mozilla.org/mozilla-central/rev/c96768247e9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.