Last Comment Bug 803078 - Text selection "cursor" handle persists across tab switches
: Text selection "cursor" handle persists across tab switches
Status: VERIFIED FIXED
[mentor=margaret][lang=js]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 20
Assigned To: Ravisankar Sivasubramaniam
: general
Mentors:
Depends on:
Blocks: 652168
  Show dependency treegraph
 
Reported: 2012-10-18 07:00 PDT by (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-11-28 04:20 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Added code to hide thumb for cursor. (265.41 KB, patch)
2012-11-18 04:16 PST, Ravisankar Sivasubramaniam
no flags Details | Diff | Review
Added code to hide thumb for cursor. (1016 bytes, patch)
2012-11-18 07:10 PST, Ravisankar Sivasubramaniam
margaret.leibovic: review+
Details | Diff | Review
Added code to hide the cursor if the windows change (1.20 KB, patch)
2012-11-22 08:26 PST, Ravisankar Sivasubramaniam
margaret.leibovic: review+
Details | Diff | Review

Description (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-10-18 07:00:16 PDT
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 2012-10-18 09:03:36 PDT
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 2012-10-23 05:34:00 PDT
Hi Margaret,

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

Thanks,
Ravi
Comment 3 :Margaret Leibovic 2012-10-23 11:16:44 PDT
(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 2012-11-18 04:16:38 PST
Created attachment 682872 [details] [diff] [review]
Added code to hide thumb for cursor.
Comment 5 Ravisankar Sivasubramaniam 2012-11-18 07:10:14 PST
Created attachment 682886 [details] [diff] [review]
Added code to hide thumb for cursor.
Comment 6 Ravisankar Sivasubramaniam 2012-11-18 20:12:26 PST
Hi Margaret,

Would you please review this patch?

Thanks,
Ravi
Comment 7 :Margaret Leibovic 2012-11-19 08:39:48 PST
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 2012-11-19 08:46:04 PST
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 2012-11-22 08:26:46 PST
Created attachment 684432 [details] [diff] [review]
Added code to hide the cursor if the windows change
Comment 10 :Margaret Leibovic 2012-11-26 12:16:54 PST
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] 2012-11-26 17:55:31 PST
https://hg.mozilla.org/mozilla-central/rev/45c61a3e9678
Comment 12 Cristian Nicolae (:xti) 2012-11-28 04:20:30 PST
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

Note You need to log in before you can comment on or make changes to this bug.