Closed Bug 823325 Opened 7 years ago Closed 7 years ago

Text cursor handle doesn't disappear when switching tabs

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox18 + affected
firefox19 --- verified
firefox20 --- verified
firefox21 --- verified

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file, 3 obsolete files)

STR:
1) Go to http://google.com
2) Click the search box to make the handle appear
3) Switch tabs
Attached patch Call blur() on onfocused browser (obsolete) — Splinter Review
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
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #694639 - Flags: review?(mark.finkle)
Comment on attachment 694639 [details] [diff] [review]
Call blur() on onfocused browser

Yay! (for focus hacks)

I've seen much worse though :)
Attachment #694639 - Flags: review?(mark.finkle) → review+
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
Attachment #694639 - Attachment is obsolete: true
Attachment #697196 - Flags: review?(mark.finkle)
Comment on attachment 697196 [details] [diff] [review]
Use timeout before calling browser.focus()

I can live with this... for now
Attachment #697196 - Flags: review?(mark.finkle) → review+
Just listens for the Tab:Selected event instead of trying to fix our focus problem.
Attachment #697196 - Attachment is obsolete: true
Attachment #698953 - Flags: review?(mark.finkle)
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
Attachment #698953 - Flags: review?(mark.finkle) → review+
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).
Attachment #698953 - Attachment is obsolete: true
Attachment #699363 - Flags: review?(mark.finkle)
Attachment #699363 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/b47c2682ca07
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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
Attachment #699363 - Flags: approval-mozilla-beta?
Attachment #699363 - Flags: approval-mozilla-aurora?
Comment on attachment 699363 [details] [diff] [review]
Hide thumb when selected tab changes, v2

Blcoks bug 795045, recently approved on aurora.
Attachment #699363 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on trunk (21) (mozilla-central) via Samsung Galaxy Nexus (Android 4.2), and Asus Transformer Prime TF201 (Android 4.0)
Status: RESOLVED → VERIFIED
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.
Attachment #699363 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified fixed on Firefox Mobile 19 and Firefox Mobile 20 beta 5 on the Samsung Galaxy Tab 2 (Android 4.1.1)
You need to log in before you can comment on or make changes to this bug.