Closed Bug 980197 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

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

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch select_whole_number.patch (obsolete) — Splinter Review
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.
Attachment #8386572 - Flags: review?(wjohnston)
Assignee: nobody → blassey.bugs
tracking-fennec: ? → 30+
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.
Attachment #8386572 - Flags: review?(wjohnston) → review?(mark.finkle)
Note that Ian signed off on the UX of the combination of this and bug 979470 in bug 979470
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
      +44 (22) 1234 5678
  Germany (old): 01234/123456-12       << note "/"
  China: +86 755 1234 1234

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.
Attachment #8386572 - Flags: review-
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
Attached patch select_whole_number.patch (obsolete) — Splinter Review
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}$/,
Attachment #8390216 - Flags: review?(mark.finkle)
Attachment #8386572 - Attachment is obsolete: true
Attachment #8386572 - Flags: review?(mark.finkle)
Depends on: 979470
my regex was broken
Attachment #8390216 - Attachment is obsolete: true
Attachment #8390216 - Flags: review?(mark.finkle)
Attachment #8392479 - Flags: review?(mark.finkle)
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
Attachment #8392479 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/a0e6d0b027dc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Status: RESOLVED → VERIFIED
Depends on: 1035218
You need to log in before you can comment on or make changes to this bug.