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

VERIFIED FIXED in Firefox 54

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: masayuki, Assigned: TYLin)

Tracking

(Blocks 1 bug)

Trunk
mozilla54
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 fixed)

Details

()

Attachments

(1 attachment)

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: 2 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.