823325 - Text cursor handle doesn't disappear when switching tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Go to http://google.com
2) Click the search box to make the handle appear
3) Switch tabs

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Call blur() on onfocused browser

This is more of a workaround than a fix, because if focus() was working correctly at [1], the previously focused element should automatically be blurred. In nsFocusManager.cpp, checkIfFocusable() returns false at [2], so it appears that our focus() call at [1] isn't actually doing anything. This is also probably the cause of bug 823788.

[1] http://hg.mozilla.org/integration/mozilla-inbound/file/f533eacd8458/mobile/android/chrome/content/browser.js#l2811
[2] http://hg.mozilla.org/integration/mozilla-inbound/file/f533eacd8458/dom/base/nsFocusManager.cpp#l1080

Comment 2

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

Comment on attachment 694639 [details] [diff] [review]
Call blur() on onfocused browser

Yay! (for focus hacks)

I've seen much worse though :)

Comment 3

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/767debcb04d8

Comment 4

[Cameron McCormack (:heycam)]

Backed out for Android test_manyWindows.html failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d294f9a7d19f
e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=18183812&tree=Mozilla-Inbound

3100 INFO TEST-START | /tests/dom/tests/mochitest/geolocation/test_manyWindows.html
3101 INFO TEST-INFO | /tests/dom/tests/mochitest/geolocation/test_manyWindows.html | must wait for focus
3102 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_manyWindows.html | Test timed out.
3103 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_manyWindows.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0
3104 INFO TEST-END | /tests/dom/tests/mochitest/geolocation/test_manyWindows.html | finished in 304763ms
3105 INFO TEST-START | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html
3106 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html | Opened a bunch of windows and didn't crash.
3107 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html | [SimpleTest.finish()] this test already called finish!
3108 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html | /tests/dom/tests/mochitest/geolocation/test_manyWindows.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
3109 INFO TEST-END | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html | finished in 128ms
3110 INFO TEST-START | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html
3111 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | Should have geolocation
3112 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | Should have got an exception
3113 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | null
3114 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | null
3115 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | null
3116 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | Should have got an exception
3117 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | null
3118 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | null
3119 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | null
3120 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | /tests/dom/tests/mochitest/geolocation/test_optional_api_params.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
3121 INFO TEST-END | /tests/dom/tests/mochitest/geolocation/test_timerRestartWatch.html | finished in 1240ms

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Use timeout before calling browser.focus()

Here's another hacky solution. When browser.focus() is called immediately after "this.browser.docShellIsActive = true;", isVisibleConsideringAncestors(), called at [1], returns false. With a timeout, this call returns true. I don't know why a timeout is needed, and I can dig deeper if this is too ugly, but I think we want some fix ASAP so we can uplift bug 795045 (which is tracking 18+).

This one looks like it passes on try: https://tbpl.mozilla.org/?tree=Try&rev=d39b716d634b

[1] http://hg.mozilla.org/integration/mozilla-inbound/file/37f6cc6d5bf3/layout/generic/nsFrame.cpp#l7416

Comment 6

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

Comment on attachment 697196 [details] [diff] [review]
Use timeout before calling browser.focus()

I can live with this... for now

Comment 7

[Ed Morley [:emorley]]

Backed out for turning docshell/test/test_bug590573.html almost permaorange on Android:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=mochitest-3&rev=17b390f86130

https://hg.mozilla.org/integration/mozilla-inbound/rev/34f4d5d47cae

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Hide thumb when selected tab changes

Just listens for the Tab:Selected event instead of trying to fix our focus problem.

Comment 9

[:Margaret Leibovic]

Comment on attachment 698953 [details] [diff] [review]
Hide thumb when selected tab changes

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

::: mobile/android/chrome/content/browser.js
@@ +1932,5 @@
>    },
>  
>    handleEvent: function sh_handleEvent(aEvent) {
>      switch (aEvent.type) {
> +      case "Tab:Selected":

Shouldn't this go in observe, not handleEvent?

Comment 10

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

(In reply to Margaret Leibovic [:margaret] from comment #9)
> Comment on attachment 698953 [details] [diff] [review]
> Hide thumb when selected tab changes
> 
> Review of attachment 698953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +1932,5 @@
> >    },
> >  
> >    handleEvent: function sh_handleEvent(aEvent) {
> >      switch (aEvent.type) {
> > +      case "Tab:Selected":
> 
> Shouldn't this go in observe, not handleEvent?

"Tab:Selected" is an event

Comment 11

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

(In reply to Mark Finkle (:mfinkle) from comment #10)
> (In reply to Margaret Leibovic [:margaret] from comment #9)
> > Comment on attachment 698953 [details] [diff] [review]
> > Hide thumb when selected tab changes
> > 
> > Review of attachment 698953 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/chrome/content/browser.js
> > @@ +1932,5 @@
> > >    },
> > >  
> > >    handleEvent: function sh_handleEvent(aEvent) {
> > >      switch (aEvent.type) {
> > > +      case "Tab:Selected":
> > 
> > Shouldn't this go in observe, not handleEvent?
> 
> "Tab:Selected" is an event

Whoops, I am thinking of "TabSelect"

Comment 12

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

Comment on attachment 698953 [details] [diff] [review]
Hide thumb when selected tab changes

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   handleEvent: function sh_handleEvent(aEvent) {
>     switch (aEvent.type) {
>+      case "Tab:Selected":
>       case "pagehide":

Remove this line. We already have "Tab:Selected" in the sh_observe method, which is why your testing worked:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1876

r+ but remove the case from sh_handleEvent

Comment 13

[:Margaret Leibovic]

But you need to add the |else this.hideThumb()| to the existing observer. That must have (somehow!) failed to make it into the initial mega cursor patch.

Comment 14

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

Or more precisely:

} else if (this._activeType == this.TYPE_CURSOR) {
  this.hideThumb();
}

right?

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Hide thumb when selected tab changes, v2

Oops. This patch doesn't actually work after re-testing it now, so I'm not sure what build I was using yesterday when it worked. We also already have 'Services.obs.addObserver(this, "Tab:Selected", false);' in the init function, so the previous patch was way wrong.

This uses the existing Tab:Selected handler to hide the thumb. We don't want to hide it for Window:Resize, though, since tapping the text area causes the soft keyboard to pop up (which triggers a window resize and would hide the thumb right after it appears).

Comment 16

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b47c2682ca07

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b47c2682ca07

Comment 18

[Brian Nicholson (:bnicholson)]

If we uplift bug 795045 to 18, this should go too. Without it, the cursor handle stays on the screen when tabs are switched. Low risk.

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 699363 [details] [diff] [review]
Hide thumb when selected tab changes, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 795045
User impact if declined: handle stays on screen when switching tabs
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 20

[bhavana bajaj [:bajaj]]

Comment on attachment 699363 [details] [diff] [review]
Hide thumb when selected tab changes, v2

Blcoks bug 795045, recently approved on aurora.

Comment 21

[Aaron Train [:aaronmt]]

Verified fixed on trunk (21) (mozilla-central) via Samsung Galaxy Nexus (Android 4.2), and Asus Transformer Prime TF201 (Android 4.0)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ead7d8638f15

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 699363 [details] [diff] [review]
Hide thumb when selected tab changes, v2

We haven't made a final decision here for FF18, but we can definitely get this fix into FF19.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/301d54b3c444

Comment 25

[Adrian Tamas (:AdrianT)]

Verified fixed on Firefox Mobile 19 and Firefox Mobile 20 beta 5 on the Samsung Galaxy Tab 2 (Android 4.1.1)