Text cursor handle doesn't disappear when switching tabs

VERIFIED FIXED in Firefox 19

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

unspecified
Firefox 21
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox18+ affected, firefox19 verified, firefox20 verified, firefox21 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 694639 [details] [diff] [review]
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
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
(Assignee)

Comment 5

5 years ago
Created attachment 697196 [details] [diff] [review]
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
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+
(Assignee)

Comment 8

5 years ago
Created attachment 698953 [details] [diff] [review]
Hide thumb when selected tab changes

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 9

5 years ago
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+

Comment 13

5 years ago
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?
(Assignee)

Comment 15

5 years ago
Created attachment 699363 [details] [diff] [review]
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).
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
(Assignee)

Comment 18

5 years ago
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.
tracking-firefox18: --- → ?
(Assignee)

Comment 19

5 years ago
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
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → 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+

Updated

5 years ago
status-firefox18: --- → affected
tracking-firefox18: ? → +
https://hg.mozilla.org/releases/mozilla-beta/rev/301d54b3c444
status-firefox19: affected → fixed
Verified fixed on Firefox Mobile 19 and Firefox Mobile 20 beta 5 on the Samsung Galaxy Tab 2 (Android 4.1.1)
status-firefox19: fixed → verified
status-firefox20: fixed → verified
You need to log in before you can comment on or make changes to this bug.