Closed Bug 1023418 Opened 6 years ago Closed 5 years ago

Auto-selecting a phone number does not stop at element breaks

Categories

(Firefox for Android :: Text Selection, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34
Tracking Status
fennec + ---

People

(Reporter: mfinkle, Assigned: capella)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 980197 added support for auto-selecting a phone number when long tapping on something like looks like a phone number. The current implementation does not stop selecting when hitting a linebreak, so it can select too much text.
Can we add tests for this feature too?
tracking-fennec: --- → ?
Can you provide a screenshot? Does this happen when selecting (long-press) editable text <input> / <textarea> or just display text?

I can get a (what I'm imagining you refer to) similar effect as here:
   https://www.dropbox.com/s/pft4elao8fj45it/bug1023418n.png

But it's not phone-number logic related ... you can do the same thing by long pressing a blank char in between two words, or simply the final white space on a line as here:
   https://www.dropbox.com/s/xn6mr7l446hpyzl/bug1023418a.png

Based on SELECT_WORDNOSPACE selection logic here:
   http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=cf75dbf2383d&mark=393-393#388
I'll get a testcase
tracking-fennec: ? → +
Flags: needinfo?(mark.finkle)
Testcase: http://people.mozilla.org/~mfinkle/autophone.html

example: long tap on the "456-7890" and we select "123 | 456-7890"

So it's not a linebreak issue. It's an element break issue. We should not go outside the TD in the table. We should not go outside the div in both of the div examples. But I assume we want to select numbers that go across styling. Like the "styling" case.

Maybe we could generalize this to look for "container" type elements, and stop trying to select numbers outside the container where we start.
Flags: needinfo?(mark.finkle)
Summary: Auto-selecting a phone number does not stop at linebreaks → Auto-selecting a phone number does not stop at element breaks
Attached patch bug1023418p0.diff (obsolete) — Splinter Review
This makes things a little better. We'll still have interesting edge-cases based on where the user fine-grain-taps initially, and the subsequent results returned from Ci.nsIDOMWindowUtils.SELECT_WORDNOSPACE.

Consider where <CONTAINER>a has text and <CONTAINER>b has text and the user long-taps (somewhere in and around) <DIV>a ...

Does our initial nsISelection return focusNode = <CONTAINER>b offset.0? Or do we have <CONTAINER>a offset.testLen?
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8456745 - Flags: review?(mark.finkle)
Slight tweak.
Attachment #8456745 - Attachment is obsolete: true
Attachment #8456745 - Flags: review?(mark.finkle)
Attachment #8456818 - Flags: review?(mark.finkle)
Comment on attachment 8456818 [details] [diff] [review]
bug1023418p0.diff

>diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js

>+// Define elements that bound phone number containers.
>+const PHONE_NUMBER_CONTAINERS = "td,div";

I wish we could make the check a different way. Something less "a list of HTML elements"

>+      // Don't extend the selection into a new container.
>+      if (selection.focusNode != focusNode) {
>+        let nextContainer = (selection.focusNode instanceof Text) ?
>+          selection.focusNode.parentNode : selection.focusNode;
>+        if (nextContainer.mozMatchesSelector &&
>+            nextContainer.mozMatchesSelector(PHONE_NUMBER_CONTAINERS)) {

Do we need the | nextContainer.mozMatchesSelector | check? All elements should have mozMatchesSelecor, iirc.
Attachment #8456818 - Flags: review?(mark.finkle) → review+
I had cribbed that from http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=1310981a48bb&mark=5255#5242 , and did try to remove the line to explore why it was there.

Originally in testing I encountered something that barfed into logcat complaining of "not a function" or some-such... though I've removed it and tested again and can't seen to re-create towards identifying the element type.
https://hg.mozilla.org/mozilla-central/rev/9d56f401fe65
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.