Closed Bug 566225 Opened 10 years ago Closed 7 years ago

Add a context menu item for "Call Phone Number" if the selected text looks like a phone number

Categories

(Firefox for Android :: General, enhancement)

ARM
Android
enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 26

People

(Reporter: d_jan, Assigned: fedepaol)

References

Details

(Whiteboard: [mentor=wesj][lang=js])

Attachments

(3 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.3a5pre) Gecko/20100514 Namoroka/3.7a.5pre

Firefox Mobile could integrate with the phones applications so that one could initiate a phone call from the browser or add an appointment - even if the data is not in a microformat. If the call/adding date would be triggered via the context menu, one could possibly get around the performance problems that occurred in previous attempts to integrate this function.
This function would be especially useful since it eliminates memory load and switching between applications for pretty common tasks.

Reproducible: Always




Mockup:
http://www.flickr.com/photos/31068346@N05/4613291570/
(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3)
> Gecko/20100401 Firefox/3.6.3
> Build Identifier: User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1;
> de; rv:1.9.3a5pre) Gecko/20100514 Namoroka/3.7a.5pre
> 
> Firefox Mobile could integrate with the phones applications so that one could
> initiate a phone call from the browser or add an appointment - even if the data
> is not in a microformat. If the call/adding date would be triggered via the
> context menu, one could possibly get around the performance problems that
> occurred in previous attempts to integrate this function.
> This function would be especially useful since it eliminates memory load and
> switching between applications for pretty common tasks.
> 
> Reproducible: Always
> 
> 
> 
> 
> Mockup:
> http://www.flickr.com/photos/31068346@N05/4613291570/

How could someone actually knows a long tap action is available on the phone number?
we have support for voip and callto links already
Not sure, but maybe he's talking about implicitly giving phone numbers a link using callto which the iphone does?

http://people.mozilla.com/~nhirata/html_tp/iPhone_Tele2.html will show both telephone numbers as links on the iPhone.
We used to do that but found:
* It was too slow. We would do this in the content process now, so it wouldn't affect chrome
* It was too hard to get the patterns for phone numbers right. non-US phone numbers are hard to match.
According to Mark this would be hard to implement. 

At the moment on Nightly/14.0a1 2012-03-29 text can't be selected at all - known issue Bug 695173 - and on Nightly/14.0a1 XUL 2012-03-29 the only option for selected text is to copy it.
Status: UNCONFIRMED → NEW
Depends on: text-selection
Ever confirmed: true
OS: Windows XP → Android
Product: Fennec → Fennec Native
Hardware: x86 → ARM
Duplicate of this bug: 769282
This seems like a fun new contributor bug to me. Lots of ways to try and make it work. I'm happy to mentor.

The approach I'd like to try is, when the user clicks on something, move the cursor and look at the text there. If it looks like a phone number, keep expanding the selection to find the full number, convert it to a tel link, and open it! (i.e. we do the detection on click and not when the document is loaded).
Whiteboard: [mentor=wesj][lang=js]
(In reply to Wesley Johnston (:wesj) from comment #8)
> This seems like a fun new contributor bug to me. Lots of ways to try and
> make it work. I'm happy to mentor.
> 
> The approach I'd like to try is, when the user clicks on something, move the
> cursor and look at the text there. If it looks like a phone number, keep
> expanding the selection to find the full number, convert it to a tel link,
> and open it! (i.e. we do the detection on click and not when the document is
> loaded).

This would be a good way to speed things up, but please note the usability downside of doing the detection on click only: phone numbers will not inherit CSS's :link properties.
CSU student offered to take this. I think this will be a really tricky bug to get right (i.e. not regress page load or anything), but a good base that we can easily pref on/off to get started would be helpful.
Assignee: nobody → lannguyen
Changing description and hijacking a bit for the CS project. I'd still like to have the automatic recognition at some point, but this seems like a good first step. We can open a new bug for the bigger feature.
Summary: integrate with phone-applications: telephone-number and Date recognition → Add a context menu item for "Call Phone Number" if the selected text looks like a phone number
To fix this, we'll want to do something similar to what's been done to add "Copy Phone Number" and "Share Phone Number" support:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#371

except we'll need to modify this selector to
1.) Look if this is a tel link (that's what the current code does), or
2.) Check if the selected text looks like a phone number. We do something kinda like for this for copy:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6088

we'll also need to get the selected text in order to compare it to some phone number regex:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6043

If it does and the user selects this option, we can convert it to a tel url (we may have to clean un-necessary characters out of the string), and open it by calling HelperApps.openUriInApp(uri);
(In reply to Mark Finkle (:mfinkle) from comment #5)
> * It was too hard to get the patterns for phone numbers right. non-US phone
> numbers are hard to match.

android.telephony.PhoneNumberUtils might help with this.
Yeah. Android isn't even very good at this, but since its fairly hidden, I think it might be useful to basically show this for any selection that only contains numbers, or the symbols *()#-.
(In reply to Wesley Johnston (:wesj) from comment #12)
> To fix this, we'll want to do something similar to what's been done to add
> "Copy Phone Number" and "Share Phone Number" support:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#371
> 
> except we'll need to modify this selector to
> 1.) Look if this is a tel link (that's what the current code does), or
> 2.) Check if the selected text looks like a phone number. We do something
> kinda like for this for copy:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6088
> 
> we'll also need to get the selected text in order to compare it to some
> phone number regex:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6043
> 
> If it does and the user selects this option, we can convert it to a tel url
> (we may have to clean un-necessary characters out of the string), and open
> it by calling HelperApps.openUriInApp(uri);

So wesj, all this will be done with js. No java code involve?
Please review syntax and method usage.
Attachment #718010 - Flags: review?(wjohnston)
Comment on attachment 718010 [details] [diff] [review]
First patch attempt

Review of attachment 718010 [details] [diff] [review]:
-----------------------------------------------------------------

Pretty good first run, but we'll need to clean it up a bit more to work right, and I think we need a better phone number regex, either something more lenient (yours doesn't match 555-5555 because it requires an area code), or something more strict like the Android built in one.

::: mobile/android/chrome/content/browser.js
@@ +1506,5 @@
>  
> +    phoneNumberContext: {
> +      matches: function phoneNumberContextMatches(aElement, aX, aY) {
> +        if (NativeWindow.contextmenus.phoneNumberLinkContext.matches(aElement))
> +          return true;

Since clicking on a phone number link will open it, we don't really need this check here (its redundant and our menus are already kinda full, so we try not to clutter them un-necessarily). This should only work on things that aren't already links.

@@ +1510,5 @@
> +          return true;
> +        if (SelectionHandler.shouldShowContextMenu(aX, aY)) {
> +          let selection = SelectionHandler.getSelection();
> +          if (selection) {
> +              selectedText = selection.toString().trim();

Don't indent this further than the other lines inside the if. You also need to declare this with let selectedText =

@@ +1511,5 @@
> +        if (SelectionHandler.shouldShowContextMenu(aX, aY)) {
> +          let selection = SelectionHandler.getSelection();
> +          if (selection) {
> +              selectedText = selection.toString().trim();
> +            var regEx = ^\+?((1-)|(1))?\(?([0-9]{3})\)?[-. ]?([0-9]{3})[-. ]?([0-9]{4})$;

This regex needs to be nested inside some forward slashes (/). We also use let instead of var to avoid scoping problems with var. Also, this regex is basically looking for things like 1 234 567 8901, with some variation in separators. It won't work well for non-US localizations. We can try a few different things to fix that.

1.) Easy fix is just take anything that has [0-9-. ()+] in it? (I think that's all, maybe 'x' for phone numbers with an extension listed)?

2.) Better fix we can do what mfinkle suggested and try to use android.telephony.PhoneNumberUtils.format(String, countryCode) which should return the number if it can parse it, or null if it can't. To do so we can send the number to java through a message with a type like "PhoneNumber:Parse" and get back a response from the built in PhoneNumberUtil. Look at other instances in browser.js where we call sendMessageToJava and other instances in GeckoApp.java where we register listeners for those messages and send back a response ("Webapps:Install" is a good one).

@@ +1834,5 @@
>  
> +    _getPhoneURL: function(aElement) {
> +       // if this matches the phone link context simply return the link
> +       if (NativeWindow.contextmenus.phoneNumberLinkContext.matches(aElement))
> +          return NativeWindow.contextmenus._getLinkURL(aElement);

This will pass the element, not the uri in. But like I said, we don't really need this since we don't want this to show on these tel links.

@@ +1836,5 @@
> +       // if this matches the phone link context simply return the link
> +       if (NativeWindow.contextmenus.phoneNumberLinkContext.matches(aElement))
> +          return NativeWindow.contextmenus._getLinkURL(aElement);
> +       // otherwise, append and return the selection as a "tel: " link:
> +       return ("tel: " + SelectionHandler.endSelection());

HelperApps reallly wants a nsIURI object. You can create it by calling:

return Services.io.newURI("tel: " + SelectionHandler.endSelection(), null, null);
Attachment #718010 - Flags: review?(wjohnston)
Corrected per review, successfully opens context menu and calls phone application.

Currently only supports U.S. phone numbers.
Attachment #721851 - Flags: review?(wjohnston)
Comment on attachment 718010 [details] [diff] [review]
First patch attempt

+    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.callPhoneNumber"),
+      NativeWindow.contextmenus.phoneNumberContext,
+      function(aTarget) {
+        let uri = NativeWindow.contextmenus._getPhoneURL(aTarget);

I wouldn't bother with this helper function. At this point it just returns 

Services.io.newURI("tel: " + SelectionHandler.endSelection(), null, null);

which doesn't really match its description, and isn't likely to be useful to anyone else.

+ var regEx = ^\+?((1-)|(1))?\(?([0-9]{3})\)?[-. ]?([0-9]{3})[-. ]?([0-9]{4})$;

I'm a bit torn on this regex. I still think I'd rather be too lenient than too strict. i.e. if the selection is all numbers, or any of the symbols ().- +x, offer to dial it.

i.e. something like
[+(]*[0-9][0-9().\-+ x]*

(requires at least one number, and can only start with a number, plus, or (. That matches all of these for me:
+1(555)555-5555
555-5555
555-5555x332
(123) 456-7890
123.456.7890
1234567890
(++39) 055 294883
39-055-294-883
011 39 055 294883

and will falsely match something like
2
or 4382
or 34 x 32 + 15

but I'm fine with that. I'd rather be convenient that right all the time. Also, it means we don't have to localize this regex, and the regex will work for people in foreign countries.
Attachment #718010 - Flags: feedback+
(In reply to Wesley Johnston (:wesj) from comment #19)
> Comment on attachment 718010 [details] [diff] [review]
> First patch attempt
> 
> +   
> NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.
> callPhoneNumber"),
> +      NativeWindow.contextmenus.phoneNumberContext,
> +      function(aTarget) {
> +        let uri = NativeWindow.contextmenus._getPhoneURL(aTarget);
> 
> I wouldn't bother with this helper function. At this point it just returns 
> 
> Services.io.newURI("tel: " + SelectionHandler.endSelection(), null, null);
> 
> which doesn't really match its description, and isn't likely to be useful to
> anyone else.
> 
> + var regEx = ^\+?((1-)|(1))?\(?([0-9]{3})\)?[-. ]?([0-9]{3})[-.
> ]?([0-9]{4})$;
> 
> I'm a bit torn on this regex. I still think I'd rather be too lenient than
> too strict. i.e. if the selection is all numbers, or any of the symbols ().-
> +x, offer to dial it.
> 
> i.e. something like
> [+(]*[0-9][0-9().\-+ x]*
> 
> (requires at least one number, and can only start with a number, plus, or (.
> That matches all of these for me:
> +1(555)555-5555
> 555-5555
> 555-5555x332
> (123) 456-7890
> 123.456.7890
> 1234567890
> (++39) 055 294883
> 39-055-294-883
> 011 39 055 294883
> 
> and will falsely match something like
> 2
> or 4382
> or 34 x 32 + 15
> 
> but I'm fine with that. I'd rather be convenient that right all the time.
> Also, it means we don't have to localize this regex, and the regex will work
> for people in foreign countries.

Wesj is this review on first submission? We submit the second patch can you take a look. Thanks
Comment on attachment 721851 [details] [diff] [review]
Second patch attempt (US phone numbers recognized)

Sorry. Yes, I folded the two together (apply the first, then hg qfold secondPatchName) in it, and reviewed them together. If you've got fixes to one patch to submit, its generally better to just submit one entire new patch that way (i.e. each patch should be bug free on its own).
Attachment #721851 - Flags: review?(wjohnston)
Could you confirm that you're still working on this?
Flags: needinfo?(lannguyen)
Assignee: lannguyen → nobody
(In reply to Josh Matthews [:jdm] from comment #22)
> Could you confirm that you're still working on this?

We got the first patch in for US phone number and we are no longer working on this. I have changed the status assign to Nobody so anyone can claim and work on this project. Thanks
Flags: needinfo?(lannguyen)
Hi there. I would like to have a try to work on this as my first bug
I don't really think that someone wants to call to another country's phone number, since international calls usually are quite expensive. 
The most relevant use-case for me is to call to organisation in my city or to my friends. So I propose to consider phone number detection by user's locale.
Let's ping UX and let ian make a final decision here

Option 1:) We show this context menu item any time the selected text contains a number and starts with something phone numberish.

Option 2;) We try to strictly match some subset of phone number types. If we're going to do this, I would rather we try and use Android's telephone number parsing logic, but that will be an async operation and will make this a bit more difficult...
Flags: needinfo?(ibarlow)
Is there an Option 3: Make telephone numbers a clickable link? Having to select text to get a context menu feels like a pretty clumsy interaction.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #27)
> Is there an Option 3: Make telephone numbers a clickable link? Having to
> select text to get a context menu feels like a pretty clumsy interaction.

It's possible, but we learned previously that the "scan and change" code that needs to run right after pageload can affect pageload perforance, as well as responsiveness of panning. If we go this route, we should do so in a new bug and make sure we do not regress any responsiveness.
Makes sense. As for Wes's question in comment 26, I don't have a strong opinion either way for how we decide what a phone number is in a page. 

Maybe start with option 1 which sounds a little simpler and then invest some additional effort into turning it into a link next?
Assignee: nobody → fedepaol
Given that scan and change can be pretty expensive, we can:

- intercept the rendering / the parsing in some way to make the phone numbers look like they are clickable.

or:

- do what Chrome does, which is not altering the look of the phone numbers but opening the dialer whenever the user taps on them.

I think this last solution is a reasonable compromise, it's definitely less intuitive from a user's perspective since the numbers do not look like they are clickable, but the check of whether they are numbers or not can be done at runtime after a tap event. 

I think is less expensive and less complicated than intercepting the rendering in gecko, and it is still better than having to highlight the text and long click it to bring up the context menu.

I would also check if the device is able to make phone calls before opening the dialer.

Ian, do you think this can be a viable solution?
(In reply to Federico Paolinelli from comment #30)

> - do what Chrome does, which is not altering the look of the phone numbers
> but opening the dialer whenever the user taps on them.


It's a start, but it's not at all discoverable so I don't want us to be looking at this as our end solution. 

Let's be sure to follow up in a separate bug as Mark mentioned, and see what kind of regression we are actually looking at when we redraw phone numbers to look like clickable links. 

As an aside, is it worth looking at what Safari does on iOS? If I recall correctly they turn phone numbers into links there.
(In reply to Ian Barlow (:ibarlow) from comment #31)

> 
> As an aside, is it worth looking at what Safari does on iOS? If I recall
> correctly they turn phone numbers into links there.

Safari linkifies the phone numbers. It also looks like that this behaviour can be disabled by the page itself: http://stackoverflow.com/questions/226131/how-to-disable-phone-number-linking-in-mobile-safari
Attached patch First linkification patch (obsolete) — Splinter Review
This is an attempt to provide linkification for "phone number like" text as discussed with wes on irc. 

The idea here is to walk the text nodes, replacing any |text before|phone num|text after| node with separate nodes (or more, in case of multiple phone numbers): 
- text before
- anchor node
-text after

I (hope I) took care of the corner cases like no text after or before a given number, or multiple numbers in the text. 

TreeWalker allows to replace the current visited node, so even if the tree is changed while iterating, all the job can be done in the same iteration.

Given that we still need to measure the impact of this stuff on the performances of the page loading, all the code is under preference and is disabled by default as suggested by wes.

Things I think could be improved:
- The code itself :P

- The regex used to match phone numbers. I used one provided here http://javascript.about.com/library/blre.htm which seemed to be the more comprehensive. Regex depending on the phone's locale could be more accurate.

- Timer tuning. At the moment, a timer processes a node at the time, waiting a different amount of time if the previous iteration had succesfully replaced the node. I am not sure this is the right way to proceeed.

- Filter more tags. At the moment I am filtering the tags that seemed obvious to filter, but the more we filter the less we need to parse.

As a final note, I also checked if it was possible to disable this feature completely in case of devices not able to make phone calls, but after a bit of investigation, TelephonyManager is not available from js, and nsIExternalProtocolService returns availables application even on tablets (that's because the contact application gets opened for tel: links). I would say the best option here is to make no difference between phones and other devices (Skype or apps like that could still be installed).
Attachment #775382 - Flags: feedback?(mark.finkle)
Comment on attachment 775382 [details] [diff] [review]
First linkification patch

Quick feedback:
* I think you should create a separate JS file to hold the code: Linkify.js
* Lazy load the JS file using the lazy script getters in browser.js

More feedback coming
Attached patch Second linkification patch (obsolete) — Splinter Review
Moved all the linkification stuff in a separate file. 
I placed it in a Linkifier object because of the reference to the timer in case it needs to be uninstalled. For this reason I think we need a linkifier per tab.

Not sure if I should check the preference even for the Linkifier creation, but I think I should not. Changing the preference while the tab is open would result in a crash (I think).
Attachment #775382 - Attachment is obsolete: true
Attachment #775382 - Flags: feedback?(mark.finkle)
Attachment #776562 - Flags: feedback?(mark.finkle)
Comment on attachment 776562 [details] [diff] [review]
Second linkification patch

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

I need to walk through this code a bit more

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>+  this._linkifier = null;

Probably not needed. We can just let it be dynamic. See below

>+    this._linkifier = new Linkifier();

Let's delay this more. See below

>         sendMessageToJava({
>           type: "Content:PageShow",
>           tabID: this.id
>         });
> 
>+        if (Services.prefs.getBoolPref("browser.ui.enable_numbers_linkification")) {

Add a linkifier check and create it here

            if (!this._linkifier)
              this._linkifier = new Linkifier();
>+          this._linkifier.linkifyNumbers(this.browser.contentWindow.document);
>+        }

That siad, I'm not sure we want this to happen in "pageload" since that is fired even for pages pulled from the back/forward cache. Those pages are kept in their "living" state, so any DOM chnages made to those docs should still be there. We only want to linkify on fresh page loads. You could use "load" or you could use the event.persisted property. If event.persisted = false, it's a fresh page load:
https://developer.mozilla.org/en-US/docs/Using_Firefox_1.5_caching
Attachment #776562 - Flags: feedback?(mark.finkle) → feedback+
Attached patch Third linkification version (obsolete) — Splinter Review
Added the lazy instantiation of the linkifier and checking if the current page is cached or not using event.persisted flag.
Attachment #776562 - Attachment is obsolete: true
Attachment #776940 - Flags: feedback?(mark.finkle)
Just noticed that I forgot the pref enabled by default in this last patch after doing some tests. I'll revert it to false in the next iteration.
Comment on attachment 776940 [details] [diff] [review]
Third linkification version

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+// When true, phone number linkification is enabled.
>+pref("browser.ui.enable_numbers_linkification", true);

"browser.ui.linkify.phone"

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

Still need to do more looking at this code, but my time ran out today.
Attachment #776940 - Attachment is patch: true
Attachment #776940 - Flags: feedback?(mark.finkle) → feedback+
Comment on attachment 776940 [details] [diff] [review]
Third linkification version

># HG changeset patch
># Parent 04d8c309fe728f61d5a48122bf5706c6f86da209
># User Federico Paolinelli <fedepaol@gmail.com>
>Bug 566225 - Replaced all the text that looks like phone number (that matches given regular expressions) with anchors to call that number; r=mfinkle
>
>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+pref("browser.ui.enable_numbers_linkification", true);

Let's make this | false | by default. We can get some data while it's on Nightly and maybe add a visual Settings for it. Defaulting to | false | makes for the least amount of surprises.

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

General style nit:
In front-end JS we favor using no {} for single line if blocks. If there is a single line else block, it also gets no {}, but if either the if or the else are multi line blocks, both blocks get {}

>+function Linkifier() {
>+	this._linkifyTimer = null;

nit: 2 spaces

>+Linkifier.prototype = {
>+  _buildLinkifiedAnchor : function(doc, numberText) {

General style nit: we use a 'a' prefix for args... mostly. For example | aDoc | and | aNumberText |

>+    anchorNode.setAttribute("href", "tel:"+cleanedText);

nit: spaces around to | + |

>+  _linkifyNodeNumbers : function(nodeToProcess, doc) {
>+    let parent = nodeToProcess.parentNode;
>+    let nodeText = nodeToProcess.nodeValue;
>+
>+    let regex = /((\+\d{1,3}(-| )?\(?\d\)?(-| )?\d{1,5})|(\(?\d{2,6}\)?))(-| )?(\d{3,4})(-| )?(\d{4})(( x| ext)\d{1,5}){0,1}/g;

Can we make this a class member so it's not created every time? _phoneRegex ?

>+  linkifyNumbers: function(doc) {

>+    let filterNode = function (node) {
>+      if(node.parentNode.tagName != 'A' &&

nit: add a space after the "if"

>+    let nodeWalker = doc.createTreeWalker(doc.body,
>+                                          NodeFilter.SHOW_TEXT,
>+                                          filterNode,
>+                                          false);

No need to wrap this line. We are fine with > 80 char lines in the front-end JS files

>+      // we assign a different timeout wether the node was processed or not.

"whether", and why do we use different timeouts?

>+        this._linkifyTimer = setTimeout(parseNode, 100);

Make this a class member. We like named consts like LINKIFY_TIMEOUT

>+        this._linkifyTimer = setTimeout(parseNode, 5);

Why even use 5? Why not just use 0 ?

>+    this._linkifyTimer = setTimeout(parseNode, 100); 

LINKIFY_TIMEOUT

Ready for review. Send the next patch to Wes for review. Thanks Federico.
Attached patch Ready for review(?) (obsolete) — Splinter Review
Corrected all the nits suggested by mfinkle, moved the timer to 0 in case no work was done (still, I wonder if we should tune this values).

Redisabled it by default (as per my previous question, it was already done and waiting for another iteration).
Attachment #776940 - Attachment is obsolete: true
Attachment #783333 - Flags: review?(wjohnston)
Attached patch bugfix-566225 (obsolete) — Splinter Review
Of course it was not ready for review :-) I forgot to change the arguments appending the 'a'
Attachment #783333 - Attachment is obsolete: true
Attachment #783333 - Flags: review?(wjohnston)
Attachment #783346 - Flags: review?(wjohnston)
A few comments since I have, uhm, considered this problem before (thanks Margaret!)...

1) Walking the whole document to linkify it is not going to be good for performance/power. (This less of a concern if, say, you only did it in response to clicking a "Show Phone Numbers On Page" button.) Most of the time you're not going to be calling numbers, so avoiding a every-pageload penalty would be good. Also consider what happens when loading a giant page like http://www.whatwg.org/specs/web-apps/current-work/, which is already slow on _desktop_.

2) A more performant way of doing this is to only do the work in response to a user event, like opening a content menu. You look to see where the user tapped, and if there's a phone number there you add an item for calling it. The event will give you the node it was triggered on. You could also look at something like document.elementFromPoint (or whatever APIs we used a looong time ago in the Nokia browser for finding links "near" where you tapped).

3) Modifying the page can lead to unexpected results (since it's not expecting to be modified, and you may pick up wacky style or break things). This is moot with an approach like #2. Otherwise you might look at how find-in-page styles things (for found items) -- IIRC there was platform work done a while back to make that work without modifying the DOM. Ditto for selecting text. But maybe modifying the DOM isn't too-hacky for mobile.
Comment on attachment 783346 [details] [diff] [review]
bugfix-566225

Review of attachment 783346 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good. I wanted doske/margaret to have a look at this, since the general algorithm was something you and I came up with together, and I wanted a third opinion on it. But I also don't want to block on them either. i.e. this is preffed off for now. We can work on it while its in the tree.

::: mobile/android/chrome/content/Linkify.js
@@ +5,5 @@
> +const LINKIFY_TIMEOUT = 100;
> +
> +function Linkifier() {
> +  this._linkifyTimer = null;
> +  this._phoneRegex = /((\+\d{1,3}(-| )?\(?\d\)?(-| )?\d{1,5})|(\(?\d{2,6}\)?))(-| )?(\d{3,4})(-| )?(\d{4})(( x| ext)\d{1,5}){0,1}/g;

I spent awhile trying to come up with a list of phone numbers from around the world to compare this against. In the end, I just concluded there are too many formats, and think maybe we'd be better to be a bit more generic. Something like:
/(?:\s|^)[\+]?(\(?\d{1,8}\)?)?([- \.]+\(?\d{1,8}\)?)+( ?(x|ext) ?\d{1,3})?(?:\s|$)/g;

this requires
  1.) some whitespace on the start or end of the string. I try not to capture that, but it doesn't seem to work...
  2.) an optional plus sign
  3.) then a number with 1-8 digits, it can be wrapped in parenthesis. It MUST be follow by:
    3a.) a separator from the list -, space, or period (adding backslash to this mess makes it hard to discern dates)
    3b.) another 1-8 digit number
  4.) finally,the number can end with either 'x' or 'ext' and a 1-4 digit number

That matches:
754-3010
(541) 754-3010
541.754.3010
1 555 555 1234
1-555-555-1234 ext 45
+33 1 23 45 67 89
01234 123456
(01234) 123456
001-541-754-3010

It won't match some phone numbers like:
(089) / 636-48018
12345678

But it also won't match things like years or dates, which is nice...

@@ +9,5 @@
> +  this._phoneRegex = /((\+\d{1,3}(-| )?\(?\d\)?(-| )?\d{1,5})|(\(?\d{2,6}\)?))(-| )?(\d{3,4})(-| )?(\d{4})(( x| ext)\d{1,5}){0,1}/g;
> +}
> +
> +Linkifier.prototype = {
> +  _buildLinkifiedAnchor : function(aDoc, aNumberText) {

Naming nit. I'd get rid of the Linkified part here. buildAnchor is probably enough. or buildLink.

@@ +14,5 @@
> +    let anchorNode = aDoc.createElement("a");
> +    let cleanedText = "";
> +    for (let i = 0; i < aNumberText.length; i++) {
> +      let c = aNumberText.charAt(i);
> +      if ((c >= '0' && c <= 9) || c == '+')  //assuming there is only the leading '+'.

Should this 9 be in quotes?

@@ +27,5 @@
> +  _linkifyNodeNumbers : function(aNodeToProcess, aDoc) {
> +    let parent = aNodeToProcess.parentNode;
> +    let nodeText = aNodeToProcess.nodeValue;
> +
> +    // Replacing the original text node with a sequence of 

trailing whitespace

@@ +29,5 @@
> +    let nodeText = aNodeToProcess.nodeValue;
> +
> +    // Replacing the original text node with a sequence of 
> +    // |text before number|anchor with number|text after number nodes.
> +    // Each step a couple of (optional) text node and anchor node are appended. 

I don't think this comment is exactly true. It seems more like we just do an entire text-node each step?

@@ +33,5 @@
> +    // Each step a couple of (optional) text node and anchor node are appended. 
> +    let anchorNode = null;
> +    let m = null;
> +    let startIndex = 0;
> +    let lastProcessedNode = null;

Lets just call this prevNode.

@@ +37,5 @@
> +    let lastProcessedNode = null;
> +    while (m = this._phoneRegex.exec(nodeText)) {
> +      anchorNode = this._buildLinkifiedAnchor(aDoc, nodeText.substr(m.index, m[0].length));
> +
> +      let existsTextBeforeNumber = (m.index > startIndex);

textExistsBeforeNumber

@@ +53,5 @@
> +      if (existsTextBeforeNumber) // if we added the text node before the anchor, we still need to add the anchor node.
> +        parent.insertBefore(anchorNode, nodeToAdd.nextSibling);
> +
> +      // next nodes need to be appended to this node.
> +      lastProcessedNode = anchorNode; 

whitespace here

@@ +57,5 @@
> +      lastProcessedNode = anchorNode; 
> +      startIndex = m.index + m[0].length;
> +    }
> +
> +    // if some text is remaining after the last anchor.

This comment doesn't finish its thought. i.e. what happens if some text is remaining.

@@ +62,5 @@
> +    if (startIndex > 0 && startIndex < nodeText.length) {
> +      let lastNode = aDoc.createTextNode(nodeText.substr(startIndex));
> +      parent.insertBefore(lastNode, lastProcessedNode.nextSibling);
> +      return lastNode;
> +    } else {

I don't think you need this else

@@ +69,5 @@
> +  },
> +
> +  linkifyNumbers: function(aDoc) {
> +    // removing any installed timer in case the page has changed and a previous
> +    // timer is still running.

Don't worry about wrapping lines in our js. Also, we try to captialize the beginnings of sentences in comments.

@@ +86,5 @@
> +        return NodeFilter.FILTER_ACCEPT;
> +      else
> +        return NodeFilter.FILTER_REJECT
> +    }
> +    

whitespace

@@ +97,5 @@
> +      }
> +      let lastAddedNode = this._linkifyNodeNumbers(node, aDoc);
> +      // we assign a different timeout whether the node was processed or not.
> +      if (lastAddedNode) {
> +        nodeWalker.currentNode = lastAddedNode; 

whitespace

@@ +100,5 @@
> +      if (lastAddedNode) {
> +        nodeWalker.currentNode = lastAddedNode; 
> +        this._linkifyTimer = setTimeout(parseNode, LINKIFY_TIMEOUT);
> +      } else {
> +        this._linkifyTimer = setTimeout(parseNode, 0);

I'm not sure why we want different timeouts. I guess we're hoping that last node was non-matching (i.e. really fast) and we should quickly jump to the next one? Lets just use LINKIFY_TIMEOUT here to avoid confusion for now, and set LINKIFY_TIMEOUT = 0;

@@ +104,5 @@
> +        this._linkifyTimer = setTimeout(parseNode, 0);
> +      }
> +    }.bind(this);
> +
> +    this._linkifyTimer = setTimeout(parseNode, 100); 

Use LINKIFY_TIMEOUT here
Attachment #783346 - Flags: review?(wjohnston)
Attachment #783346 - Flags: review+
Attachment #783346 - Flags: feedback?(margaret.leibovic)
Attachment #783346 - Flags: feedback?(dolske)
http://www.wtng.info/ has a host of global phone number formats, sadly I don't think they're exposed in a machine readable format. I found that URL in cldr, which doesn't have any of that data in itself.
I realize my regex will fail on dates like 4.12.13 or floats like 4.13. I wonder if we can run a parseFloat() on them to check for the later, rather than trying to pick them out in the regex... Date.parse() is a bit harder, since our parser doesn't like ".".

Maybe we just don't allow "." as a separator...
Comment on attachment 783346 [details] [diff] [review]
bugfix-566225

See comment 43. No big deal to land pref'd off, though I'd generally discourage the practice unless you've got a clear roadmap to turning it on or know it will get you data in the interm. Your call!
Attachment #783346 - Flags: feedback?(dolske) → feedback-
(In reply to Wesley Johnston (:wesj) from comment #46)
> I realize my regex will fail on dates like 4.12.13 or floats like 4.13. I
> wonder if we can run a parseFloat() on them to check for the later, rather
> than trying to pick them out in the regex... Date.parse() is a bit harder,
> since our parser doesn't like ".".
> 
> Maybe we just don't allow "." as a separator...

Given comment 47, I'd complete the patch removing the . as separator, at least for the sake of having it finished. If we ever decide to land it/ activated or not, we can still refine the regex later.
Attached patch bugfix-566225Splinter Review
Added a patch that removes the "." separator and applies Wes' suggestions. Not asking for review since I did not understand if we want to land it. Anyway I wanted to make it reach a completed stage.
Attachment #783346 - Attachment is obsolete: true
Attachment #783346 - Flags: feedback?(margaret.leibovic)
Great. I think we should land this next week after we've moved to Firefox 26. Then maybe we can pref it on for 26 and get a little bit of data from telemetry/eideticker/etc. on its impact.
https://hg.mozilla.org/mozilla-central/rev/bae16ff99915
https://hg.mozilla.org/mozilla-central/rev/e6505df70193
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Builds: all branches
Device: LG Nexus 4 (Android 4.4.2)
If you select for example a phone number, the action bar is is invoked, an there is a option between "Google search" and "Share", called "Call".
I will mark as verified fixed.
Status: RESOLVED → VERIFIED
Depends on: 957345
Depends on: 1212086
You need to log in before you can comment on or make changes to this bug.