Prompt specially for tel: URIs as phone numbers

RESOLVED FIXED

Status

()

Firefox for iOS
Browser
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: rnewman, Assigned: hennes, Mentored)

Tracking

(Blocks: 1 bug, {polish})

unspecified
All
iOS
polish
Dependency tree / graph

Firefox Tracking Flags

(fxios5.0+)

Details

Attachments

(1 attachment)

48 bytes, text/x-github-pull-request
sleroux
: review+
bnicholson
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
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!

Updated

3 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → -
Keywords: polish
Mentor: sarentz
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

Comment 3

3 years ago
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
(Assignee)

Comment 4

3 years ago
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: - → ---
(Reporter)

Updated

3 years ago
Flags: needinfo?(rnewman)
(Reporter)

Comment 5

3 years ago
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.
Flags: needinfo?(rnewman)
(Assignee)

Comment 6

3 years ago
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)
(Reporter)

Comment 7

3 years ago
Sorry for the delay getting to this; it's on my list.
(Reporter)

Updated

3 years ago
Assignee: nobody → johannesmarbach
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Attachment #8683815 - Flags: review?(rnewman) → review+
(Reporter)

Updated

2 years ago
OS: iOS 8 → iOS
(Assignee)

Updated

2 years ago
Blocks: 1228732
(Reporter)

Updated

2 years ago
Whiteboard: [needs rebase]
(Assignee)

Comment 8

2 years ago
Sorry for the delay. I finally got around to rebase this.
(Assignee)

Updated

2 years ago
Whiteboard: [needs rebase]
(Assignee)

Comment 9

2 years ago
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+
(Assignee)

Comment 11

2 years ago
Oh, interesting! Thanks for the pointers. I'll give this another look.
(Assignee)

Comment 12

2 years ago
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?)
(Assignee)

Comment 14

2 years ago
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(sarentz)
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:
Flags: needinfo?(sarentz)
Attachment #8683815 - Flags: review?(sarentz)
Attachment #8683815 - Flags: review?
Attachment #8683815 - Flags: review+
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?
Attachment #8683815 - Flags: review?(sarentz) → review?(sleroux)
Attachment #8683815 - Flags: review?(bnicholson)
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+
Whiteboard: [5.0]
tracking-fxios: 4.0+ → 5.0+
Whiteboard: [5.0]
(Assignee)

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

2 years ago
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+
https://github.com/mozilla/firefox-ios/commit/01d17f8fa9dc0e0707a22eee6bbea668ed9ead0d
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.