903316 - Text selection handles are not dismissed after deleting selected text

Status: VERIFIED FIXED

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

Description

[u421692]

Attachment: device-2013-08-09-103428.png

Environment:
Device: LG Nexus 4 (Android 4.2.2)
Build: Nightly 26.0a1(2013-08-08)/Aurora 25.0a2(2013-08-08)

Steps to reproduce:
1. Open google.com
2. Insert some text in the search field
3. Long tap on text and choose "Select All"
4. Delete the text by tapping on "Backspace" from VKB

Expected result:
The text selection handles should be dismissed

Actual result:
The text selection handles are not dismissed after deleting selected text

Comment 1

[Mark Capella [:capella]]

Attachment: bug903316

Well, this started out easily enough ... just use our new notifySelectionChanged() routine to determine when a selection has changed and no longer exists, so we can close it and hide the handles as normal.

However, it turns out that selection.removeAllRanges() will also drop the anchor and focus nodes when the last range is removed, effectively losing focus on an editable field. So when the user highights the input and presses a key (backspace or char a-z or whatnot) he can't keep typing.

https://developer.mozilla.org/en-US/docs/Web/API/Selection/removeAllRanges

I don't see a valid reason to brute-force drop the selection that way. Instead, by calling selection.collapseToStart(), we can close the selection gracefully and leave the nsISelection object in the correct state. (whether or not the selection is an instanceof Ci.nsIDOMNSEditableElement).

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelection.idl#80

Comment 3

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=90b9e3683044

Comment 4

[:Margaret Leibovic]

Comment on attachment 791871 [details] [diff] [review]
bug903316

Review of attachment 791871 [details] [diff] [review]:
-----------------------------------------------------------------

Although you made a compelling argument, I'm still a bit nervous about changing this behavior. What happens when you end a selection on regular static text? If this focus issue only affects editable content, maybe we should only do this for editable targets. I just want to think about this more before giving an r+.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +182,5 @@
> +      return;
> +    }
> +
> +    // If selected text no longer exists, close
> +    if (!aSelection.toString()) {

You could just combine this with the if statement up above, e.g.:

if ((aReason & Ci.nsISelectionListener.COLLAPSETOSTART_REASON) ||
    (aReason & Ci.nsISelectionListener.COLLAPSETOEND_REASON) ||
    !aSelection.toString()) {

And then update the comment above that.

Comment 5

[Mark Capella [:capella]]

collapseToStart() works the same when we end a selection of regular static text as for an editable field. ( See the build I pushed to TRY ... if you can break it I'll be surprised :-D ) The only difference is the desired one, editable fields keep focus.

Given that, I think it's clearer to do the same function for both cases, than to perform two different ones based on field type and have the code seem to indicate different outcomes.

I've been debugging this pretty close since Friday in both those cases and am happy to back it up with more detail, here or in IRC if it's faster.

Also, I avoided combining the two cases in your second example to illustrate that there's different causes going on (programmatic loss of selection vs. user interaction), but I'm okay with putting them together.

Comment 6

[:Margaret Leibovic]

Comment on attachment 791871 [details] [diff] [review]
bug903316

Review of attachment 791871 [details] [diff] [review]:
-----------------------------------------------------------------

All good points. You can just land this as-is and ignore my comment :)

Comment 7

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/00d59f610bc4

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/00d59f610bc4

Comment 9

[Paul Feher]

Verified fixed on:
Beta 26.0b4(2013-11-12)
Lg Optimus 4x (4.1.2)