980197 - on long press of a phone number, select the whole phone number

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: select_whole_number.patch

With bug 979470, users can call phone numbers by selecting them. Given that, we should select the whole number initially, not just a part of it.

Comment 1

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

Comment on attachment 8386572 [details] [diff] [review]
select_whole_number.patch

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

moving this review too since Wes doesn't seem interested in doing it.

Comment 2

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

Note that Ian signed off on the UX of the combination of this and bug 979470 in bug 979470

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8386572 [details] [diff] [review]
select_whole_number.patch

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +785,4 @@
>      this._closeSelection();
>    },
>  
> +  _phoneRegex: /^[0-9\w,-.\(\)*#pw]{1,17}$/,

You must also support a leading + (don't forget the other 6bn people), and I'd like to see some tests for common international formats:

  France: 04 12 34 56 78
  UK: 022 1234 5678
      0800 1111
      01234 123456
      [phone]
  Germany (old): 01234/123456-12       << note "/"
  China: [phone]

These numbers themselves can end up over 17 characters (E.164 allows up to 15 digits in a number excluding spacing and decoration), so I would bump that limit, especially if you're allowing pauses.

Additionally, you should probably not include trailing "-" or "." in the number, and you should consider whether you want the final anchor -- it'll trip you up if the text selection grows to include extra crap.

Comment 4

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

Attachment: select_whole_number.patch

updated to take leading +, trim trailing/leading whitespace and take up to 30 chars.

I think this regexp would be better, but it is noticeably slower:
 /^\+?[0-9]{1,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9]{0,2}[\w,-.\(\)*]{0,3}[0-9\w,-.*#pw]{0,7}$/,

Comment 5

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

Attachment: select_whole_number.patch

my regex was broken

Comment 6

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

Comment on attachment 8392479 [details] [diff] [review]
select_whole_number.patch

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

>+      while (this._isPhoneNumber(selection.toString().trim())) {
>+	focusNode = selection.focusNode;
>+	focusOffset = selection.focusOffset;
>+	selection.modify("extend", "forward", "word");
>+	// if we hit the end of the text on the page, we can't advance the selection
>+	if (focusNode == selection.focusNode && focusOffset == selection.focusOffset)
>+	  break;
>+      }

1. You have TABs for indents in the while loop
2. {} around the if

>+      while (this._isPhoneNumber(selection.toString().trim())) {
>+	focusNode = selection.focusNode;
>+	focusOffset = selection.focusOffset;
>+	selection.modify("extend", "backward", "word");
>+	// if we hit the end of the text on the page, we can't advance the selection
>+	if (focusNode == selection.focusNode && focusOffset == selection.focusOffset)
>+	  break;
>+      }

1. You have TABs for indents in the while loop
2. {} around the if

Otherwise seems OK. I'd like to get some simple tests for isPhoneNumber in a followup bug

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a0e6d0b027dc