Closed Bug 1338445 Opened 7 years ago Closed 7 years ago

Long tapping in block element which has only a short word selects odd range on Android

Categories

(Core :: DOM: Selection, defect)

All
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: masayuki, Assigned: TYLin)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

STR1:
1. load https://jsfiddle.net/d_toybox/d5y5sa4x/8/ on Android
2. long tap at right blank space of "p", "p2", "c" or "c2" in the table.

Expected result:
Only "p", "p2", "c" or "c2" is selected (selection range is in a text node).

Actual result:
"c" and "c2" cases are fine but "p" and "p2" cases odd. Selection is started from the last of the left cell and ended at the start of the right cell.

STR2:
1. Load https://jsfiddle.net/d_toybox/d5y5sa4x/8/
2. Long tap at right space of "p" below the table.

Expected Result:
Only "p" is selected in the <p> element.

Actual Result:
Selected from the start of the previous <p> element of <div> to the start of the next text node under <body>.


If the text is long word, I cannot reproduce this bug. I don't know how to reproduce this on desktop...
I reproduced this bug both with release (51.0.2) and Nightly.
To reproduce this on desktop, one can flip the following prefs to mimic Fennec.
1) layout.accessiblecaret.enabled -> true
2) layout.accessiblecaret.extend_selection_for_phone_number -> true
3) layout.accessiblecaret.hide_carets_for_mouse_input -> false
4) layout.accessiblecaret.use_long_tap_injector -> true

This must be the phone number selection extension enabled only on Fennec that causes the odd range. AccessibleCaretManager::ExtendPhoneNumberSelection() will extend the selected word both forward and backward until it cannot match the regex [1]. The test cases "p" and "p2" happen to match the regex.

BTW, the feature was ported from js to C++ in bug 1235508, and the original phone number regex is here [2].

[1] http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/layout/base/AccessibleCaretManager.cpp#973
[2] http://searchfox.org/mozilla-central/rev/4428147ca5f614cf3a33ffce1f8b60acc4e6a458/mobile/android/chrome/content/SelectionHandler.js#1152
I feel "\\s" (matching a white space char) in the regex "(^\\+)?[0-9\\s,\\-.()*#pw]{1,30}$" might match too much. It'll match something like "\n" or "\t", which should not appear in a valid phone number string. A plain space char ' ' should be sufficient.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Comment on attachment 8838820 [details]
Bug 1338445 - Restrict whitespace match in phone number regex for AccessibleCaret.

https://reviewboard.mozilla.org/r/113630/#review115318
Attachment #8838820 - Flags: review?(mtseng) → review+
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da077f0a94ce
Restrict whitespace match in phone number regex for AccessibleCaret. r=mtseng
https://hg.mozilla.org/mozilla-central/rev/da077f0a94ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Thank you for your work. I confirmed that this is fixed on the Nightly.

However, I still see similar problem on a website which I found the original problem. I'll file another bug after I create a testcase for that. Then, I'll cc you to new bug.
Status: RESOLVED → VERIFIED
Too late for 52.  Would it make sense to uplift this to beta53?
Flags: needinfo?(tlin)
This is a long-standing bug, and the fix is risky. I'll just let it ride the train.
Flags: needinfo?(tlin)
You need to log in before you can comment on or make changes to this bug.