1061944 - Clicking link loses text selection on Android before content script click listener runs

Status: VERIFIED FIXED

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

Description

[Adrian Aichner [:anaran]]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20140902030202

Steps to reproduce:

See instructions in
https://github.com/anaran/IssuePigeonFirefox/issues/3

I'm eager to get to the bottom of this.

Comment 1

[Adrian Aichner [:anaran]]

Apparently there's more than 40 text select bugs on Android:

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=product%3Afirefox%20for%20android%20summary%3Atext%20select

Comment 2

[Adrian Aichner [:anaran]]

Here's a sweet little test case:

data:text/html;charset=utf-8,<div>selection lost on android on button click</div><input value="Use Selection" type="button"></input>

Selection remains on Windows (good) but is lost on button click on Android.

Comment 3

[Aaron Train [:aaronmt]]

Mark, is this mentorable?

Comment 4

[Mark Capella [:capella]]

That's an interesting question. The bit of code that's responsible for the issue is:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=070491691b4e&mark=111-122#90

This I believe was in place even before :margaret ported it over from browser, and I subsequently got involved. iirc, it was in place early on when grcko-to-Android SelectionHandler code was rough, and we used it as a catch all for clearing the new left/right handles (Java side) off the screen when the gecko selection was lost, or we became unsure of the actual UI state.

We've made a lot of improvements since then, notably the addition of our SelectionListener logic, and we may not actually need this anymore.

Mentoring someone to remove a block of code is easy of course. We'd need to look at regression bugs in that area flagged by |blame|, bug 970054, bug 906499, and bug 907271 to see if they complain after we remove the code, and we'll most likely need to update our testSelectionHandler robocop test code.

Comment 5

[Manu Jain]

Hi, I'm interested in patching this bug. Can you assign this to me.
I've recently solved "https://bugzilla.mozilla.org/show_bug.cgi?id=1060423" under your guidance.

Comment 6

[Mark Capella [:capella]]

Good show!

This patch will be a little less cut and dried than the last one, where we had a provably wrong issue, with no possible side effects as a result of the change.

To fix this reported problem, we're going to remove old code that I see no current use for. So we need to:
   a) prove that removing the code fixes this specific bug.
   b) prove that removing the code doesn't regress/break anything else.

The last one will be a little harder, and will involve re-creating / re-testing the bugs I mentioned in comment #4 above, and dog-fooding the changes on your (and my) test devices for a while looking for problems.

I don't think there will be any, but proving a negative takes thought.

Finally, when we do push your code change to TRY as we did in the final stages of your first bug, I believe you'll see us fail a test. 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?rev=c474691b9c0f&force=1&mark=180-185#158

Fixing this is easy, we just remove that test, and add it's changes to your patch before final approval.

Basically, this is the problem we're trying to solve as I understand it:
https://www.dropbox.com/s/sh9wvat854osube/test_bug1061944.xhtml?dl=0

If you launch that page on Android and longtap some text to "select" it, then click the button, the text that you selected should appear between the brackets. (This works on Firefox desktop, but *not* here in Android land.)

With me so far ? :-D

Comment 7

[Manu Jain]

Attachment: bug1061944.patch

I think this patch is working fine. I've tested this on my firefox. Thanks!!

Comment 8

[Mark Capella [:capella]]

Comment on attachment 8485896 [details] [diff] [review]
bug1061944.patch

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

This looks right on first glance ... make sure to read and test the situations in comment #4

And the testcase:
https://www.dropbox.com/s/im1q3x0yhj6kd3k/test_bug1061944.html?dl=0

I'll try this out myself and give it a TRY-server push. Take a look here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/testSelectionHandler.html?rev=c474691b9c0f&force=1&mark=180-185#158

I fully expect that test to break, and require you to add a correction to your patch.

btw - you can perform robocop tests on your device to check this ahead of time. See:
https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop

This is a little tricky, but worth learning.

Comment 9

[Mark Capella [:capella]]

push to TRYserver https://tbpl.mozilla.org/?tree=Try&rev=8e35821d65f1

Note the orange (failed) rc5 test for example ... this is the expected failure I mentioned ...
clicking through into there shows the error
https://tbpl.mozilla.org/php/getParsedLog.php?id=47651255&tree=Try&full=1#error0

So now, you'll need to review this, find the appropriate test code and fix it (remove the Gesture:SingleTap specific stuff), and re-post your patch with all the changes.

Comment 10

[Manu Jain]

Attachment: bug_1061944.patch

I have removed the test case.

Comment 11

[Mark Capella [:capella]]

Comment on attachment 8486193 [details] [diff] [review]
bug_1061944.patch

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

That looks pretty close but I'd like to clean it up a little more. See the below comments, and
please briefly answer:

1) Have you managed to run this Robocop test locally to be sure it works?
2) Have you tried any of the tests from the other bugs I mentioned?
3) Have you at least run the testcase I've included in comment #8?

I'm assuming you looked at these things, but you haven't really described your results yet.

::: mobile/android/base/tests/roboextender/testSelectionHandler.html
@@ +170,4 @@
>    // Check the initial state of the selection handler.
>    return Promise.all([
>      ok(!sh.isSelectionActive(), "Selection should not be active at start of testCloseSelection"),
>  

The next four lines don't really seem needed anymore. Let's remove them and have the first test start after
// Various other ways to close an active selection.

@@ +179,1 @@
>    // Various other ways to close an active selection.

And fix-up the comment to reflect how the test works now, maybe change
// Various other ways to close an active selection.
to be
// Various ways to close an active selection.

@@ +179,2 @@
>    // Various other ways to close an active selection.
>    }).then(function() {

Note that after you remove the four lines above that start with
]).then(function() {
then this line right before here will need to be updated ...

Comment 12

[Manu Jain]

Attachment: bug_1061944_.patch

Sorry for the late reply mark. Actually I was trying to install robocop but was not successful. It was saying "unknown mach command: robocop". But I've tried from the other bugs you mentioned in comment #4 and also I've run the testcase you've given in comment #8. They all are working normally i.e there is no failure in those tests. Also I've made the changes as you told in comment #11.
I'll surely try to install robocop though. Please review the new patch. Thanks!!

Comment 13

[Mark Capella [:capella]]

Comment on attachment 8488829 [details] [diff] [review]
bug_1061944_.patch

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

I'd like to see you able to run the robocop tests, It's unfortunate that the |mach| command is changing as we speak, but we're in no hurry with this patch.

Can you take some time to learn this? You can ask in #mobile for help if I'm not around or wait until you can catch me.

fwiw, We're almost done :-) Your patch looks correct.

::: mobile/android/base/tests/roboextender/testSelectionHandler.html
@@ +173,1 @@
>    

nit: The line above here (#173) has a trailing white-space that needs to be removed.

@@ +175,1 @@
>    }).then(function() {

I'd like you to be able to run robocop tests to catch javascript errors locally, without having to push to the TRY-server. In this case, I think you'll find a syntax error on the line above here.

Comment 14

[Manu Jain]

Attachment: bug1061944_v4.patch

Here's the new patch after robocop testing.

Comment 15

[Mark Capella [:capella]]

try-server push
https://tbpl.mozilla.org/?tree=Try&rev=d8cc0bf6cc66

Comment 16

[Manu Jain]

It looks like all the tests passed successfully. :)

Comment 17

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

Comment on attachment 8490540 [details] [diff] [review]
bug1061944_v4.patch

I need to pass this review. I am too out of touch with the selection code to provide good feedback.

Comment 18

[Mark Capella [:capella]]

Comment on attachment 8490540 [details] [diff] [review]
bug1061944_v4.patch

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

This patch brings us in compliance with desktop behaviour, and better conforms to spec http://w3c.github.io/selection-api/#user-interactions 

starting here: "The user agent must not make a selection empty if it was not already empty in response to any user actions (e.g. clicking on a non-editable region)." ...

The Java "Gesture:SingleTap" message does not directly interact with the DOM and modifying selection logic in response I believe is incorrect.

UI interactions that previously depended on this clean-up code have been tested and still work properly. The provided testcase also works properly now.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8490540 [details] [diff] [review]
bug1061944_v4.patch

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

It looks like capella did a good job reviewing this (thanks!). Given the discussion in this bug, this sounds like a reasonable fix to me.

::: mobile/android/chrome/content/SelectionHandler.js
@@ -110,5 @@
>  
>        case "Gesture:SingleTap": {
> -        if (this._activeType == this.TYPE_SELECTION) {
> -          let data = JSON.parse(aData);
> -          if (!this._pointInSelection(data.x, data.y))

You can also remove the _pointInSelection function, since this is the only place it's used.

Comment 20

[Manu Jain]

Attachment: bug_1061944_v5.patch

I've removed the function _pointInSelection.

Comment 21

[Mark Capella [:capella]]

Comment on attachment 8493368 [details] [diff] [review]
bug_1061944_v5.patch

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

nice catch and correction !
manu, can you fix your commit message from |r=markcapella| to |r=margaret| and re-post
this? We'll carry over her approval so you don't need to flag me for r? this last time.

(If you'd prefer, I can fix that bit up and get you pushed)

Comment 22

[Manu Jain]

Comment on attachment 8493368 [details] [diff] [review]
bug_1061944_v5.patch

># HG changeset patch
># Parent f4037194394efcad03d5076860387bcf32ff98a0
># User Manu Jain <[e-mail]>
>Bug 1061944 - Clicking link loses text selection on Android before content script click listener runs; r=margaret
>
>diff --git a/mobile/android/base/tests/roboextender/testSelectionHandler.html b/mobile/android/base/tests/roboextender/testSelectionHandler.html
>--- a/mobile/android/base/tests/roboextender/testSelectionHandler.html
>+++ b/mobile/android/base/tests/roboextender/testSelectionHandler.html
>@@ -189,31 +189,18 @@ function testCloseSelection() {
>   var sh = getSelectionHandler();
>   var inputNode = document.getElementById("inputNode");
>   inputNode.value = INPUT_TEXT;
> 
>   // Check the initial state of the selection handler.
>   return Promise.all([
>     ok(!sh.isSelectionActive(), "Selection should not be active at start of testCloseSelection"),
> 
>+  // Various ways to close an active selection.
>   ]).then(function() {
>-    // Start by selecting all in an <input>.
>-    sh.startSelection(inputNode);
>-    return is(sh._activeType, sh.TYPE_SELECTION, "Selection should be active in <input> before Gesture:SingleTap");
>-
>-  }).then(function() {
>-    // Tap outside <input> to close active selection.
>-    sh.observe(null, "Gesture:SingleTap", JSON.stringify({
>-      x: 1,
>-      y: 1
>-    }));
>-    return ok(!sh.isSelectionActive(), "Gesture:SingleTap outside <input> should close active selection");
>-
>-  // Various other ways to close an active selection.
>-  }).then(function() {
>     sh.startSelection(inputNode);
>     sh.observe(null, "TextSelection:End", {});
>     return ok(!sh.isSelectionActive(), "TextSelection:End should close active selection");
> 
>   }).then(function() {
>     sh.startSelection(inputNode);
>     sh.observe(null, "Tab:Selected", {});
>     return ok(!sh.isSelectionActive(), "Tab:Selected should close active selection");
>diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js
>--- a/mobile/android/chrome/content/SelectionHandler.js
>+++ b/mobile/android/chrome/content/SelectionHandler.js
>@@ -104,21 +104,17 @@ var SelectionHandler = {
> 
>       // Update caret position on keyboard activity
>       case "TextSelection:UpdateCaretPos":
>         // Generated by IME close, autoCorrection / styling
>         this._positionHandles();
>         break;
> 
>       case "Gesture:SingleTap": {
>-        if (this._activeType == this.TYPE_SELECTION) {
>-          let data = JSON.parse(aData);
>-          if (!this._pointInSelection(data.x, data.y))
>-            this._closeSelection();
>-        } else if (this._activeType == this.TYPE_CURSOR) {
>+        if (this._activeType == this.TYPE_CURSOR) {
>           // attachCaret() is called in the "Gesture:SingleTap" handler in BrowserEventHandler
>           // We're guaranteed to call this first, because this observer was added last
>           this._deactivate();
>         }
>         break;
>       }
>       case "Tab:Selected":
>       case "TextSelection:End":
>@@ -1038,26 +1034,16 @@ var SelectionHandler = {
>       offset.y += rect.top;
> 
>       win = win.parent;
>     }
> 
>     return offset;
>   },
> 
>-  _pointInSelection: function sh_pointInSelection(aX, aY) {
>-    let offset = this._getViewOffset();
>-    let rangeRect = this._getSelection().getRangeAt(0).getBoundingClientRect();
>-    let radius = ElementTouchHelper.getTouchRadius();
>-    return (aX - offset.x > rangeRect.left - radius.left &&
>-            aX - offset.x < rangeRect.right + radius.right &&
>-            aY - offset.y > rangeRect.top - radius.top &&
>-            aY - offset.y < rangeRect.bottom + radius.bottom);
>-  },
>-
>   // Returns true if the selection has been reversed. Takes optional aIsStartHandle
>   // param to decide whether the selection has been reversed.
>   _updateCacheForSelection: function sh_updateCacheForSelection(aIsStartHandle) {
>     let rects = this._getSelection().getRangeAt(0).getClientRects();
>     if (!rects[0]) {
>       // nsISelection object exists, but there's nothing actually selected
>       throw "Failed to update cache for invalid selection";
>     }

Comment 23

[Mark Capella [:capella]]

manu, just attach the patch like you did previously.

Comment 24

[Manu Jain]

Attachment: bug_1061944_v5.patch

Review changed to margaret.

Comment 25

[Mark Capella [:capella]]

Comment on attachment 8493376 [details] [diff] [review]
bug_1061944_v5.patch

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

We'll carry over her r+ from before, and I'll get this moved for you.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/654e7e288711

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/654e7e288711

Comment 28

[Adrian Aichner [:anaran]]

(In reply to adrian.aichner from comment #2)
> Here's a sweet little test case:
> 
> data:text/html;charset=utf-8,<div>selection lost on android on button
> click</div><input value="Use Selection" type="button"></input>
> 
> Selection remains on Windows (good) but is lost on button click on Android.

I can confirm this issue fixed in "35.0a1 (2014-09-24)", great!

But there is major new breakage:

Bug 1072276

Comment 29

[Cristina Madaras, QA [:CristinaM]]

Selection remains on button click, verified as fixed on latest builds:
Firefox for Android 37.0a1 (2014-12-09)
Firefox for Android 36.0a2 (2014-12-09)
Firefox for Android 35.b1

Device: Nexus 4 (Android 4.4.4)