Closed
Bug 1235508
Opened 8 years ago
Closed 8 years ago
Re-implement fast Phone number selection on long-press
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Firefox for Android Graveyard
Text Selection
Tracking
(firefox48 fixed)
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
Assignee | ||
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
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
Blocks: AccessibleCaret
Flags: needinfo?(tlin) → needinfo?(markcapella)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
(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/
Assignee | ||
Comment 5•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45455/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45455/
Attachment #8739962 -
Flags: review?(tlin)
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
Thanks for reminding me, I always forget that after a qfold :-) Push to TRY https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e6536979049232c2279ebe89194ba60720582e
Assignee | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
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
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dbb14513cb83
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 16•8 years ago
|
||
Mark, this is yours!
Updated•8 years ago
|
Assignee: nobody → markcapella
Assignee | ||
Comment 17•8 years ago
|
||
Thanks!
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•