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

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 on attachment 694639 [details] [diff] [review] Call blur() on onfocused browser Yay! (for focus hacks) I've seen much worse though :)

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

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 on attachment 697196 [details] [diff] [review] Use timeout before calling browser.focus() I can live with this... for now

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

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

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?

(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

(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 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

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.

Or more precisely: } else if (this._activeType == this.TYPE_CURSOR) { this.hideThumb(); } right?

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).

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 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 on attachment 699363 [details] [diff] [review] Hide thumb when selected tab changes, v2 Blcoks bug 795045, recently approved on aurora.

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

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.

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