1147912 - Prompt specially for tel: URIs as phone numbers

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Richard Newman [:rnewman]]

We allow tapping on tel: links. We show them as

  Opening tel:[phone]
  This will open in another
        application
   Cancel              OK
  

Safari shows

  [phone]
  Cancel       Call


We should show "Call", not "OK", and we should show a region-specific phone number format.

I have code kicking around somewhere for validating phone numbers, perhaps even for formatting some, but I'm hoping there's a class somewhere that already does it!

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

If formatting turns out to be difficult then we should at least url-decode the phone number when displaying it. So that "+1%20PIZZAPIZZA" becomes "+1 PIZZAPIZZA".

Comment 2

[Aaron Train [:aaronmt]]

Prompts now show 'Cancel', 'Call' but the phone number is not formatted at all, e.g, (555)-555-5555, shows up as 5555555555

Comment 3

[yinjiaji110]

There are some usefull link, may help.

For Formating
https://github.com/iziz/libPhoneNumber-iOS

 NBPhoneNumberUtil *phoneUtil = [NBPhoneNumberUtil sharedInstance];
    NSError *error = nil;
    NBPhoneNumber *number = [phoneUtil parse:@"12345671234" defaultRegion:@"US" error:&error];
    NSString *result1 = [phoneUtil format:number numberFormat:NBEPhoneNumberFormatRFC3966 error:&error]; //tel:[phone]
    NSString *result2 = [phoneUtil format:number numberFormat:NBEPhoneNumberFormatE164 error:&error]; //[phone]
    NSString *result3 = [phoneUtil format:number numberFormat:NBEPhoneNumberFormatINTERNATIONAL error:&error];//[phone]
    NSString *result4 = [phoneUtil format:number numberFormat:NBEPhoneNumberFormatNATIONAL error:&error];//[phone]


For adding parenthesis
http://stackoverflow.com/questions/6052966/phone-number-formatting

Comment 4

[hennes]

As far as I can tell there is sadly no public interface in Cocoa for formatting phone numbers. Internally, iOS seems to use a database of localized formats stored in plists such as these:

http://whirlpool.net.au/wiki/iPhone_ABPhoneFormats_plist_eg
https://code.google.com/p/iphone-patch/source/browse/trunk/bgfix/UIKit.framework/PhoneFormats/UIPhoneFormats.plist?r=7

We could copy such a plist to the project and utilize it for formatting but I am not sure about potential copyright concerns as these do not seem to be public. Alternatively, we'd have to use a third party library like the one mentioned in the previous comment.

Any opinions on which approach to take?

Comment 5

[Richard Newman [:rnewman]]

libPhoneNumber-iOS is Apache licensed, based on good upstream code, and seems to do the trick.

If that doesn't turn into a giant bloated mess -- it looks pretty minimal, and it supports Carthage -- then I'd take that approach rather than importing a data format file and trying to hand-roll a formatter.

It also gives us stuff like validation for free.

Comment 6

[hennes]

Attachment: Pull request

This PR adds phone number formatting based on libPhoneNumber-iOS. Results are a bit mixed at times (see comments on GitHub).

Comment 7

[Richard Newman [:rnewman]]

Sorry for the delay getting to this; it's on my list.

Comment 8

[hennes]

Sorry for the delay. I finally got around to rebase this.

Comment 9

[hennes]

Hey guys, could you give me a short heads up whenever you plan to review this so that I can give it a final rebase?

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This is very cool. Couple of comments:

A lot of the URL handling code has been simplified recently. Make sure you check that out.

We now use Carthage 'propery' which means we don't want to import source code but instead let Carthage build the libPhonenumber framework.

I have not looked at the code in detail but we show the same dialog for sms, facetime and facetime-audio URLs.  Would be great if this patch also works with those. The last two can take either an email address or a phone number.

Marking this as 4.0, i'd love to see this land. Thank you!

Comment 11

[hennes]

Oh, interesting! Thanks for the pointers. I'll give this another look.

Comment 12

[hennes]

Small update: I rebased the PR on latest master and switched to letting Carthage build the libphonenumber framework. After spending quite some time struggling with Carthage (which refused to checkout nested dependencies), I'm now at a point where the project compiles and runs again. I'll look into extending the formatting to SMS & friends next.

Comment 13

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Good stuff! I'd love to include this for 4.0, but that means it needs to be finished within a week or so. 

Let us know if that is ok for you, otherwise someone else can pick it up to finish the last bit.

(Which I think it just dealing with sms: and facetime:/facetime-audio: links?)

Comment 14

[hennes]

Stefan, sorry again for the delay. :(

I just updated the PR with proper handling for tel:, facetime: and facetime-audio. It does not seem as if there is any existing handling for sms: URLs, however, which means that these go straight into UIApplication.sharedApplication().openURL(...). Should there be a prompt similar to the Call/Cancel prompt for the other schemes?

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I was wrong .. we do pass sms: straight to Messages.app.

https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Browser/BrowserViewController.swift#L1833

The only three that we prompt for in the app are tel, facetime and facetime-audio.

No prompt is needed for sms:

Comment 16

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8683815 [details] [review]
Pull request

Just marking this as needing review again since the last time was 4 months ago and the code changed considerably.

Comment 17

[Stephan Leroux [:sleroux]]

Comment on attachment 8683815 [details] [review]
Pull request

Looks good! Thanks for updating this PR. Only suggestion I would make is to move the unit tests to SharedTests since thats where our test cases for classes that live in Utils are.

Comment 18

[hennes]

(In reply to Stephan Leroux [:sleroux] from comment #17)
> Comment on attachment 8683815 [details] [review]
> Pull request
> 
> Looks good! Thanks for updating this PR. Only suggestion I would make is to
> move the unit tests to SharedTests since thats where our test cases for
> classes that live in Utils are.

Thanks for reviewing, Stephan! I move the utility class to Shared and its tests to SharedTests. PR is rebased on latest master.

Comment 19

[hennes]

Sorry that I wasn't fast enough to make this go into 4.0 btw. :(

Comment 20

[Brian Nicholson (:bnicholson)]

Comment on attachment 8683815 [details] [review]
Pull request

Awesome, thank you!

Comment 21

[Brian Nicholson (:bnicholson)]

https://github.com/mozilla/firefox-ios/commit/01d17f8fa9dc0e0707a22eee6bbea668ed9ead0d