We allow tapping on tel: links. We show them as Opening tel:+12345671234 This will open in another application Cancel OK Safari shows +1 (123) 456-1234 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!
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".
Prompts now show 'Cancel', 'Call' but the phone number is not formatted at all, e.g, (555)-555-5555, shows up as 5555555555
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:+1-234-567-1234 NSString *result2 = [phoneUtil format:number numberFormat:NBEPhoneNumberFormatE164 error:&error]; //+12345671234 NSString *result3 = [phoneUtil format:number numberFormat:NBEPhoneNumberFormatINTERNATIONAL error:&error];//+1 234-567-1234 NSString *result4 = [phoneUtil format:number numberFormat:NBEPhoneNumberFormatNATIONAL error:&error];//(234) 567-1234 For adding parenthesis http://stackoverflow.com/questions/6052966/phone-number-formatting
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?
tracking-fennec: - → ---
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.
Created attachment 8683815 [details] [review] Pull request This PR adds phone number formatting based on libPhoneNumber-iOS. Results are a bit mixed at times (see comments on GitHub).
Attachment #8683815 - Flags: review?(rnewman)
Sorry for the delay getting to this; it's on my list.
Assignee: nobody → johannesmarbach
Status: NEW → ASSIGNED
Attachment #8683815 - Flags: review?(rnewman) → review+
Sorry for the delay. I finally got around to rebase this.
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?
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!
tracking-fxios: --- → 4.0+
Oh, interesting! Thanks for the pointers. I'll give this another look.
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.
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?)
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?
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 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.
Attachment #8683815 - Flags: review?(sarentz) → review?(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.
Attachment #8683815 - Flags: review?(sleroux) → review+
(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.
Sorry that I wasn't fast enough to make this go into 4.0 btw. :(
Comment on attachment 8683815 [details] [review] Pull request Awesome, thank you!
Attachment #8683815 - Flags: review?(bnicholson) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.