Closed Bug 1235508 Opened 4 years ago Closed 4 years ago

Re-implement fast Phone number selection on long-press

Categories

(Firefox for Android :: Text Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: capella, Assigned: capella)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In transitioning from Java carets -> Touch/Selection carets -> Accessible Caret, we've currently dropped the ability to fast-select a phone number on long press.

This was originally implemented using a combination of regex-tests and rapid Text Selection changes in SelectionHandler.js [0]

In the new scheme, SelectionHandler (which previously was heavily involved with Selection changes and actively controlled / updated the selection) has been replaced by ActionBarHandler.js, a passive link from Core to Java (TextSelection.java).

I left the code involved in auto phone-number selection up in the air, as it seemed the right way to re-implement would be on the Core side, perhaps |AccessibleCaretManager::SelectWordOrShortCut()|. 

In the worst-case that we want to keep that feature, and can't provide it in Core, we should just be able to quickly drop-in the old code to mobile/Gecko.

[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=2d4cfc1c9e97&mark=535-535,546-546,587-587#531
TYLin, with AccessibleCarets for mobile now targeted for v48, I wonder if you have any thoughts regarding the appropriateness of enhancing AccessibleCaretManager::SelectWordOrShortCut() for mobile to include some sort of phone number match.

SelectWordShortCutOrPhoneNumber() ???  :-O

I think we'd rather do it in AccessibleCaretManager than dynamically changing the selection downstream in ActionBarHandler.
Flags: needinfo?(tlin)
I see no issue to port the smart phone number selection into AccessibleCaret. What it did is just extends the current selection range to select a string which matches the regexp of a possible phone number patterns. It should not be too hard since using Selection object in C++ should be as easy as in js except the regexp matching. I don't know whether we have regexp class in C++.

I'd imagine we port something like [1] from SelectionHandler.js and insert the code after SelectWord() in [2]. Since it changes the default selection behavior, we'll need a preference for this feature.

Do you think this will block shipping gecko carets for v48?

[1] https://dxr.mozilla.org/mozilla-central/rev/dd1abe874252e507b825a0a4e1063b0e13578288/mobile/android/chrome/content/SelectionHandler.js#484-486
[2] https://dxr.mozilla.org/mozilla-central/rev/dd1abe874252e507b825a0a4e1063b0e13578288/layout/base/AccessibleCaretManager.cpp#606
Flags: needinfo?(tlin) → needinfo?(markcapella)
I don't know if it would block v48, (I'd let margaret comment on that.) I suppose it would depend partly on current Telemetry for use of the ActionBar "Call" command.

I think we can have this in place in time if it's deemed required.
Flags: needinfo?(markcapella)
(In reply to Mark Capella [:capella] from comment #3)
> I don't know if it would block v48, (I'd let margaret comment on that.) I
> suppose it would depend partly on current Telemetry for use of the ActionBar
> "Call" command.

Very good idea! My initial instinct was going to be that it's fine to regress this, but looking at the UI telemetry dashboard, I see the following totals for actionbar actions:

add_search_engine 32,879
call		  58,462
copy		  1,746,084
cut		  57,336
paste		  1,794,538
search		  252,790
select_all	  230,619
share		  83,624

So while not the most popular item, it's certainly used a decent amount. I don't think we'd accept regressing "cut", so I don't think we should regress "call".


FYI, this dashboard is public, so anyone can do what I just did: https://people.mozilla.org/~mfinkle/uitelemetry/
Nice! Thanks for the link, and fyi, we noted in IRC that it's only the auto-select bit of making calls that doesn't exist in nightly right now. Users can still manually select a number, and get a useable "Call" icon in the ActionBar.
(In reply to Mark Capella [:capella] from comment #5)
> Nice! Thanks for the link, and fyi, we noted in IRC that it's only the
> auto-select bit of making calls that doesn't exist in nightly right now.
> Users can still manually select a number, and get a useable "Call" icon in
> the ActionBar.

In that case I'm a bit less concerned... but it would still be nice to fix this as part of 48 if it's not too difficult.
Comment on attachment 8739962 [details]
MozReview Request: Bug 1235508 - Re-implement fast Phone number selection on long-press, r=TYLin

https://reviewboard.mozilla.org/r/45455/#review42181

This patch is close! And I'm sorry that you'll need to rebase since it might conflict with my bug 1168891

::: layout/base/AccessibleCaretManager.cpp:818
(Diff revision 1)
>  
>    SetSelectionDragState(false);
>    ClearMaintainedSelection();
>  
> +  // Smart-select phone numbers if possible.
> +  SelectMoreIfPhoneNumber();

We'll need a pref like "layout.accessiblecaret.extend_selection_for_phone_number" to guard the extend selection and turn it on for Fennec. Other platform don't have the direcct dial button. Extend the selection might not be useful.

::: layout/base/AccessibleCaretManager.cpp:824
(Diff revision 1)
> +
>    return rs;
>  }
>  
> +/**
> + * Called to expand a selection if possible that it's a phone number.

Please change "expand" to "extend" so that it matches the selection operation, and move this comment and the comments to |ExtendPhoneNumberSelection| and |SwapPhoneNumberSelectionNodes| to AccessibleCaretManager.h

::: layout/base/AccessibleCaretManager.cpp:829
(Diff revision 1)
> + * Called to expand a selection if possible that it's a phone number.
> + */
> +void
> +AccessibleCaretManager::SelectMoreIfPhoneNumber() const
> +{
> +  ExtendPhoneNumberSelection(NS_LITERAL_STRING("forward"));

I think we can set selection direction to influence the selection when calling GetFocusNode(), FocusOffset(), GetAnchorNode(), and GetAnchorOffset() without explicit exchanging the nodes. Thas is: we might use

    SetSelectionDirection(eDirNext);
    ExtendPhoneNumberSelection(NS_LITERAL_STRING("forward"));

    SetSelectionDirection(eDirPrevious);
    ExtendPhoneNumberSelection(NS_LITERAL_STRING("backward"));
  
And SwapPhoneNumberSelectionNodes() is not needed.

::: layout/base/AccessibleCaretManager.cpp:863
(Diff revision 1)
> +    }
> +
> +    // If the changed selection isn't a valid phone number, we're done.
> +    nsAutoString selectedText;
> +    selection->Stringify(selectedText);
> +    nsAutoString phone_regex(NS_LITERAL_STRING("(^\\+)?[0-9\\s,\\-.()*#pw]{1,30}$"));

Please use |phoneRegex| to match the C++ coding style.
Attachment #8739962 - Flags: review?(tlin)
Comment on attachment 8739962 [details]
MozReview Request: Bug 1235508 - Re-implement fast Phone number selection on long-press, r=TYLin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45455/diff/1-2/
Attachment #8739962 - Flags: review?(tlin)
Comment on attachment 8739962 [details]
MozReview Request: Bug 1235508 - Re-implement fast Phone number selection on long-press, r=TYLin

https://reviewboard.mozilla.org/r/45455/#review42523

Looks good to me! BTW, you might want to clean up lines like "[mq]: MRE1262916.diff" in the commit messasge before landing.
Attachment #8739962 - Flags: review?(tlin) → review+
Thanks for reminding me, I always forget that after a qfold :-)

Push to TRY
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e6536979049232c2279ebe89194ba60720582e
Interesting ... I'm seeing a discrepancy between the try results and my local test results selecting the RTL phone number ...

Locally the method produces |+972 3 7347514 |, but on TRY I get |+972 3 7347514| ... without the final "space".

... digging
Well, I ran like 100 tests locally and all were fine, so I pushed that failing robocop test multiple times to TRY with additional logging and all ran fine [0], then I pushed multiple times to TRY without the logging and all went fine again[1].

There were some normal unrelated oranges, but I'm going to assume there was an odd infra issue with the initial TRY push, and as everything else was fine go ahead and push as originally planned.

[0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed1adbe5d4d236d41258f1ed13e2ea9fa202f503
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=86088e4e6706be2823ceee51d72118749e766c97
https://hg.mozilla.org/mozilla-central/rev/dbb14513cb83
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Mark, this is yours!
Assignee: nobody → markcapella
Thanks!
You need to log in before you can comment on or make changes to this bug.