765390 - Text selection should be cleared on page navigation

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

The selection won't be visible, but if you single tap in the correct place, endSelection will copy the selected text from the previous page to the clipboard.

Comment 1

[:Margaret Leibovic]

Attachment: (Part 1) Create _active flag

(This is built on top of my patch queue, so it may not apply to m-c)

We've been checking _view to see if there's an active selection or not, and that can be flaky, since the view could be destroyed before we get the chance to officially clean up after ourselves in endSelection. I also feel like this just makes the code less confusing to follow.

Comment 2

[:Margaret Leibovic]

Attachment: (Part 2) End selection on pagehide

I tested this by navigating to a new page in a tab with an active selection, as well as closing a tab with an active selection. I tried to make sure hideHandles is always called, so we're always removing this listener, and it looks like it is.

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 635978 [details] [diff] [review]
(Part 2) End selection on pagehide

>+      case "pagehide":
>+        this.endSelection(0, 0);
>+        break;

I'm slightly concerned that in the rare cases where (0,0) is within the selection, this will still copy the text.  Maybe instead you should omit the arguments or pass null, and check for that in endSelection?  (The same problem already exists in the call from startSelection.)

Comment 4

[:Margaret Leibovic]

Attachment: (Part 3) Don't always pass coordinates to endSelection

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Comment on attachment 635978 [details] [diff] [review]
> (Part 2) End selection on pagehide
> 
> >+      case "pagehide":
> >+        this.endSelection(0, 0);
> >+        break;
> 
> I'm slightly concerned that in the rare cases where (0,0) is within the
> selection, this will still copy the text.  Maybe instead you should omit the
> arguments or pass null, and check for that in endSelection?  (The same
> problem already exists in the call from startSelection.)

Good call.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 635995 [details] [diff] [review]
(Part 3) Don't always pass coordinates to endSelection

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

r=me with one nit fixed.

::: mobile/android/chrome/content/browser.js
@@ +1586,5 @@
>        }
>      }
>  
>      // Only try copying text if there's text to copy!
> +    if (aX && aY && selectedText.length) {

This will fail when aX or aY is legitimately zero.  You should check arguments.length instead, or (aX !== undefined && aY !== undefined), or typeof aX == "number"... choose your poison.  :)

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fba3307010b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/19366546d058
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d05d7a6d8f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fba3307010b0
https://hg.mozilla.org/mozilla-central/rev/19366546d058
https://hg.mozilla.org/mozilla-central/rev/04d05d7a6d8f

Comment 8

[:Margaret Leibovic]

Uplifted to aurora as part of a roll-up patch:
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fb0a358eaf6