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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Can we add tests for this feature too?

Comment 2

[Mark Capella [:capella]]

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

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

I'll get a testcase

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 5

[Mark Capella [:capella]]

Attachment: bug1023418p0.diff

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?

Comment 6

[Mark Capella [:capella]]

Attachment: bug1023418p0.diff

Slight tweak.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 8

[Mark Capella [:capella]]

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.

Comment 9

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/9d56f401fe65

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d56f401fe65