Closed Bug 979470 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox31 --- verified
fennec 30+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch call_selection.patch (obsolete) — Splinter Review
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.
Attachment #8385505 - Flags: feedback?(wjohnston)
Attachment #8385505 - Flags: feedback?(mark.finkle)
Attached patch call_selection.patch (obsolete) — Splinter Review
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.
Assignee: nobody → blassey.bugs
Attachment #8385505 - Attachment is obsolete: true
Attachment #8385505 - Flags: feedback?(wjohnston)
Attachment #8385505 - Flags: feedback?(mark.finkle)
Attachment #8385528 - Flags: review?(wjohnston)
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.
Attachment #8385528 - Flags: feedback+
(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.
Attached patch call_selection.patch (obsolete) — Splinter Review
Attachment #8385528 - Attachment is obsolete: true
Attachment #8385528 - Flags: review?(wjohnston)
Attachment #8385874 - Flags: review?(wjohnston)
tracking-fennec: ? → 30+
Does BrowserApp.loadURI cause any funky changes to session history? I'd rather avoid using that method.
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.
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.
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.
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
Flags: needinfo?(ibarlow)
(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
Beauty. Just swap in these new icons and i'm good. http://cl.ly/46372S1b472x
Flags: needinfo?(ibarlow)
with updated icon
Attachment #8385874 - Attachment is obsolete: true
Attachment #8385874 - Flags: review?(wjohnston)
Attachment #8389370 - Flags: review?(wjohnston)
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.
Attachment #8389370 - Flags: review?(wjohnston)
Attachment #8389370 - Flags: review?(mark.finkle)
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 @@
>+blassey@flyingfox.local.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
Attachment #8389370 - Flags: review?(mark.finkle) → review+
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.
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.
(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.
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.
(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?
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.
Blocks: 980197
https://hg.mozilla.org/mozilla-central/rev/71734f89f0a1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Depends on: 988471
Depends on: 1040443
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.