979470 - When a phone number is highlighted, offer to call it from the context menu or action bar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: call_selection.patch

Mostly scratching an itch here, and hopefully preventing people from trying to linkify phone numbers again.

Only asking for feedback right now because the TelURIParser from b2g is too permissive for my tastes. I'll put up another patch that has a narrower view of what a phone number is at some point.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: call_selection.patch

this version steals the regex from the linkify code and adds '.' as a possible separator. It is now a bit more restrictive (kinda US-centric) than I'd like, but good enough to check in and iterate on.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8385528 [details] [diff] [review]
call_selection.patch

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

>+Components.utils.import("resource:///modules/TelURIParser.jsm")

Is this still needed?

>-  },
>+    CALL: {

indent looks off

>+    },
>+
>+ },

please drop the extra blank line while you are here

>+  _getSelectionIfPhoneNumber: function sh_isPhoneNumber() {

naming OCD: since this is a variation of _getSelectedText, why not call it _getSelectedPhoneNumber ? Still returns null

>+      if (selectedText.length && this._phoneRegex.test(selectedText))
>+          return selectedText;
>+      else
>+          return null;

Braces please. Thankfully we have no goto :fail

>+  callSelection: function sh_callSelection() {
>+   let selectedText = this._getSelectionIfPhoneNumber();
>+    if (selectedText) {
>+        BrowserApp.loadURI("tel:" + selectedText);

Would it be possible to call HelperApps directly to open the Android app instead of loading a URL into the browser? Wes can help answer that question.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> 
> >+      if (selectedText.length && this._phoneRegex.test(selectedText))
> >+          return selectedText;
> >+      else
> >+          return null;
> 
> Braces please. Thankfully we have no goto :fail
Hopefully we can compromise on a ternary operator.

> 
> >+  callSelection: function sh_callSelection() {
> >+   let selectedText = this._getSelectionIfPhoneNumber();
> >+    if (selectedText) {
> >+        BrowserApp.loadURI("tel:" + selectedText);
> 
> Would it be possible to call HelperApps directly to open the Android app
> instead of loading a URL into the browser? Wes can help answer that question.
Looks like we can call HelperApps.launchUri(uri) just fine, though I seem to remember adding some code to protect users from accidentally dialing op codes and, while I'm too lazy to check this assertion, I think running it through BrowserApp.loadURI will run through that protected code path.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: call_selection.patch

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Does BrowserApp.loadURI cause any funky changes to session history? I'd rather avoid using that method.

Comment 6

[Wesley Johnston (:wesj)]

Thinking about this today, I still think we need to write a solution that enables clicking on phone numbers to just work. i.e. The flow to me here feels like

1.) User clicks on numbers, nothing happens
2.) Either they switch to a different browser, or they try to select the phone number to copy into their phone app.
3.) Hopefully they notice the phone button in the actionbar and try it.

That seems like a pretty awful flow, and its certainly worse than what our competitors do.

I think when I originally tried something like this, I tried a game where, if the user clicked, but we didn't find anything clickable where they tapped, we'd look and see if there was text where they tapped and if so, check if that text looked like a phone number.

That's why I remember seeing some weird text-selection behavior in my original attempt at this. I was using text selection to see if I could find a phone number. I eventually started making the text selection styling transparent while I was doing this, but was drawn off on other things. I still think, if this is the route we want to go though, we need a solution like that.

Forcing users to long press is going to annoy, and most users aren't even going to find it in the end.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

That's all fine and dandy, but linkifying phone numbers is also not an option. This approach doesn't break anything and gives users a path to call phone numbers from the browser until some one-click UX is devised that satisfies everyone.

Comment 8

[Wesley Johnston (:wesj)]

You should ping UX here then as well and provide screenshots/builds so they can chime in. They're very recently come to me to ask why we don't do the same thing as Chrome.

You'll likely need icons from them anyway, since our actionbar colorscheme isn't the same as Android's.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Here's a try push. Ian, please grab this build and try. Here's a web page to try it on http://romepizza.com/zgrid/proc/site/sitep.jsp

https://tbpl.mozilla.org/?tree=Try&rev=b5496f53656d

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #8)
> You should ping UX here then as well and provide screenshots/builds so they
> can chime in. They're very recently come to me to ask why we don't do the
> same thing as Chrome.
> 
> You'll likely need icons from them anyway, since our actionbar colorscheme
> isn't the same as Android's.

UX sign off doesn't block your review though

Comment 11

[Ian Barlow (:ibarlow)]

Beauty. Just swap in these new icons and i'm good. http://cl.ly/46372S1b472x

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: call_selection.patch

with updated icon

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 8389370 [details] [diff] [review]
call_selection.patch

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> UX sign off doesn't block your review though

Given what I said in comment 6, I would rather get the current code working (or at least tested) instead of checking in more.

i.e. my review would be r- and WONTFX. Given we don't agree, I'd suggest you find someone else to review this, or mark this WONTFX yourself.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8389370 [details] [diff] [review]
call_selection.patch


>diff --git a/mobile/android/base/resources/layout/.#tabs_item_row.xml b/mobile/android/base/resources/layout/.#tabs_item_row.xml
>new file mode 120000
>--- /dev/null
>+++ b/mobile/android/base/resources/layout/.#tabs_item_row.xml
>@@ -0,0 +1,1 @@
>[e-mail].319
>\ No newline at end of file

Cruft file?

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

>+  _phoneRegex: /(?:\s|^)[\+]?(\(?\d{1,8}\)?)?([- \.]+\(?\d{1,8}\)?)+( ?(x|ext) ?\d{1,3})?(?:\s|$)/,

I'm OK with landing this for now, but we should file a followup bug to:
1. Improve the recognition, even if we need to use a few different regexes
2. Move this regex and getSelectedPhoneNumber and callSelection to Linkify.jsm
3. Rename Linkify.jsm to PhoneUtils.jsm or something, pulling all our phone related code together.

>+  _getSelectedPhoneNumber: function sh_isPhoneNumber() {

>+    return (selectedText.length && this._phoneRegex.test(selectedText) ?
>+            selectedText : null);

No wrap please

>+  callSelection: function sh_callSelection() {

>+      BrowserApp.loadURI("tel:" + selectedText);

I still want to change this to something else, maybe nsIExternalProtocolHandler, and not simply trying to load the tel: URI. It just seems like a hack to load the tel: URI

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

I just noticed that the "Phone" icon stays on the actionbar, even if the selected text is not a phone number. I assume the button should not appear if alpha chars are selected.

Did this first:
http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-phone-selection.png

Then did this:
http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-phone-selection-bug.png

Note the "phone" is still in the actionbar.

Comment 16

[Wesley Johnston (:wesj)]

We don't send actionmode updates on every selection change (I didn't want to update while you were dragging). Just when the handles appear/disappear tbh. We may have to change that for this to work, and update when you stop dragging.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #16)
> We don't send actionmode updates on every selection change (I didn't want to
> update while you were dragging). Just when the handles appear/disappear tbh.
> We may have to change that for this to work, and update when you stop
> dragging.

The bug/issue I saw happened from two different selections. I did not just move the selection handles. I made a selection, unselected, and then made a new selection.

Comment 18

[Wesley Johnston (:wesj)]

We also put have a timer to avoid flashing the actionbar if we transition from selecting to selecting. i.e. any time the handles hide, we want a few hundred milliseconds before hiding the bar.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> (In reply to Wesley Johnston (:wesj) from comment #16)
> > We don't send actionmode updates on every selection change (I didn't want to
> > update while you were dragging). Just when the handles appear/disappear tbh.
> > We may have to change that for this to work, and update when you stop
> > dragging.
> 
This patch changes that to change the action bar every time the selection handles get position programmatically (so, the "snap")
> The bug/issue I saw happened from two different selections. I did not just
> move the selection handles. I made a selection, unselected, and then made a
> new selection.
That's really baffling. The test for whether to show the call icon is:
/(?:\s|^)[\+]?(\(?\d{1,8}\)?)?([- \.]+\(?\d{1,8}\)?)+( ?(x|ext) ?\d{1,3})?(?:\s|$)/.test(getSelection().toString()).

Were there any errors in the console?

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Also, based on the icon it looks like you were using the older version of that patch. Not that that should have changed the behavior.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/71734f89f0a1

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46416ef5fc89

Comment 23

[Cristina Madaras, QA [:CristinaM]]

When a number is highlighted, a phone icon is displayed in the action bar. After the phone icon is tapped, a context menu is displayed having two options: Close and Add to contacts.

Verified as fixed in
Build: Firefox for Android 31
Device: Asus Transformer Pad TF300T (Android 4.2.1)