803078 - Text selection "cursor" handle persists across tab switches

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

STR:
1. Open a tab with some page (e.g. mozilla.org)
2. In a new tab, go to a page with an input field (e.g. bugzilla.mozilla.org)
3. Click on the input field (and maybe scroll the page a bit) so that the orange cursor image is visible
4. Switch tabs

Expected results:
The orange cursor image disappears

Actual results:
The orange cursor image is overlaid on the other tab (mozilla.org)

Comment 1

[:Margaret Leibovic]

Looks like we only get rid of selection handles on a "Tab:Selected" event, not get rid of the cursor as well:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1652

We should add a this.hideThumb() call if _activeType is TYPE_CURSOR in this case.

Comment 2

[Ravisankar Sivasubramaniam]

Hi Margaret,

Would you please assign this bug to me and send me instruction to fix it?

Thanks,
Ravi

Comment 3

[:Margaret Leibovic]

(In reply to Ravisankar Sivasubramanaim from comment #2)

> Would you please assign this bug to me and send me instruction to fix it?

Sure! Do you have a build environment set up? If not, you should follow the instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

After that, you should see if you can reproduce the issue on your own build, then you'll just need to add a call to hide the thumb if it's active when we observe a "Tab:Selected" event in SelectionHanlder's observe method.

Find me on IRC if you have issues! Sorry I was a bit slow to respond last time :(

Comment 4

[Ravisankar Sivasubramaniam]

Attachment: Added code to hide thumb for cursor.

Comment 5

[Ravisankar Sivasubramaniam]

Attachment: Added code to hide thumb for cursor.

Comment 6

[Ravisankar Sivasubramaniam]

Hi Margaret,

Would you please review this patch?

Thanks,
Ravi

Comment 7

[:Margaret Leibovic]

Comment on attachment 682872 [details] [diff] [review]
Added code to hide thumb for cursor.

This looks like an accidental attachment. Marking as obsolete. I'll review the patch that was attached :)

Comment 8

[:Margaret Leibovic]

Comment on attachment 682886 [details] [diff] [review]
Added code to hide thumb for cursor.

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

Awesome! Thanks for the patch! I just have one small comment. If you upload a new patch with that change I can land it for you. Also, please read this page about how to properly format your patch so that it's ready to be checked in:
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

::: mobile/android/chrome/content/browser.js
@@ +1660,5 @@
>            // Knowing when the page is done drawing is hard, so let's just cancel
>            // the selection when the window changes. We should fix this later.
>            this.endSelection();
> +        } else if (this._activeType == this.TYPE_CURSOR) {
> +          //  Hide the cursor if a new window is selected

Nit: Update this comment to "Hide the cursor if the window changes." since this code also runs for the "Window:Resize" event.

Comment 9

[Ravisankar Sivasubramaniam]

Attachment: Added code to hide the cursor if the windows change

Comment 10

[:Margaret Leibovic]

Thanks for the patch! I landed it on mozilla-inbound, so it will be merged into mozilla-central within a day, and it will be part of Firefox 20!

https://hg.mozilla.org/integration/mozilla-inbound/rev/45c61a3e9678

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45c61a3e9678

Comment 12

[Cristian Nicolae (:xti)]

The cursor handle doesn't persist any more on the latest Nightly build. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-11-28)
Device: Galaxy Nexus
OS: Android 4.1.1