912983 - Text selection "cursor" persists on blur

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[stefan.stratmeier]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0 (Beta/Release)
Build ID: 20130805152501

Steps to reproduce:

1. go to bugzilla.mozilla.org
2. tap into search input field
3. tap outside search input field


Actual results:

On focus there is an orange pointer targetting the focussed field. When leaving the field the pointer persists.
Calling blur() on the element shows same behavior.


Expected results:

The orange pointer should vanish on leaving the input field.

Comment 1

[Aaron Train [:aaronmt]]

A couple of potentially related fixes have made it to Firefox 26 which may have resolved this last month. Can you try out Nightly for Android (http://nightly.mozilla.org) and report back if that works out for you?

Comment 2

[Mark Capella [:capella]]

Tapping outside the field WFM and (always should have) ...

The "calling blur" part I'm not sure about ... if you've got got some custom code you've written, I don't believe anything about our SelectionHandler recognizes loss of focus to clear a displayed handle / caret. You'd have to perhaps code that part also.

This would be similar to the bug 801421 I'm looking at where addon software programmatically modifes DOM elements then expects our Android / Java base handles to react.

Comment 3

[Mark Capella [:capella]]

Quick test case; http://jsfiddle.net/tEudp/1/

When field is blurred, caret should hide

Comment 4

[Mark Capella [:capella]]

Attachment: bug912983 (v0)

re: |When leaving the field the pointer persists.|
I couldn't see evidence this was ever true.

re: |Calling blur() on the element shows same behavior.|
This was true, and this patch corrects it.

Comment 5

[:Margaret Leibovic]

Comment on attachment 819420 [details] [diff] [review]
bug912983 (v0)

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

Looking good, I just have a few suggestions for improvements.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +304,5 @@
>    _initTargetInfo: function sh_initTargetInfo(aElement) {
>      this._targetElement = aElement;
> +
> +    if (this._targetElement instanceof Ci.nsIDOMNSEditableElement) {
> +      this._targetElement.focus();

Not related to your patch, but why do we need to set focus like this? Shouldn't the element already have focus if we're preparing to show the caret?

@@ +306,5 @@
> +
> +    if (this._targetElement instanceof Ci.nsIDOMNSEditableElement) {
> +      this._targetElement.focus();
> +      // Add an element blur listener to watch or loss of focus
> +      this._targetElement.addEventListener("blur", this._onBlurTargetElement);

For the "blur" listener set on _contentWindow, we set useCapture to true. Should we be doing the same thing here? If not, let's explictly set useCapture to false (same goes for removeEventListener down below), to be consistent with our other event listeners.

@@ +534,5 @@
>      this._contentWindow.removeEventListener("blur", this, true);
>      this._contentWindow = null;
> +
> +    // Remove the blur listener, clear the target element reference
> +    this._targetElement.removeEventListener("blur", this._onBlurTargetElement);

Seeing this in here, it's kinda confusing that we let SelectionHandler directly handle other events, but not this one. Looking at the "blur" handler, though, it actually already does what we want:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#156

I think that instead of making this new _onBlurTargetElement method, we should just add/remove SelectionHandler itself as the event listener.

Comment 6

[Mark Capella [:capella]]

re: |this._targetElement.focus();|
I was surprised by this bit also, and am more than happy to remove it :)

re: |useCapture|
I guess I misread the docs page, I'll review that...

re: the link to the document blur handler
I assumed, since the code in the test link I posted was failing, that the particular "blur" wasn't being received on element level / programmatic blurs and just hacked the solution ... but you got me double thinking and I've now tested for that ... I confirmed I still don't see that an element specific blur is being propogated to the document ... so either the patch s/b right for this code / test situation, ... or I misunderstand DOM events

Perhaps in my case, the element loses focus but not the document, as there is a second input field in the page?

Comment 7

[Mark Capella [:capella]]

So wait now I'm really confused ... 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?mark=289-289#279

Seems to be the real problem, we're adding the event listener currently using useCapture:false (and removing it using true)

If I simply switch it to true and let the document capture all blur events first, then the existing code and my test case works.

Comment 8

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #7)
> So wait now I'm really confused ... 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> SelectionHandler.js?mark=289-289#279
> 
> Seems to be the real problem, we're adding the event listener currently
> using useCapture:false (and removing it using true)
> 
> If I simply switch it to true and let the document capture all blur events
> first, then the existing code and my test case works.

Well that sounds like a good fix! Nice catch.

Comment 9

[Mark Capella [:capella]]

Attachment: bug912983 (v1)

Comment 10

[:Margaret Leibovic]

Comment on attachment 820463 [details] [diff] [review]
bug912983 (v1)

I was wondering about the history of this, and found that I regressed this long ago in this changeset :(
http://hg.mozilla.org/mozilla-central/rev/e2ec9e008ee9

Once again, good catch. We should try to uplift this if we can.

Comment 11

[:Margaret Leibovic]

This could also be a good candidate for a simple robocop test... load a page with an input box, tap on it, check to make sure the handle is visible, tap outside the input box, check to make sure the handle isn't visible.

Comment 12

[Mark Capella [:capella]]

Would you like I should file a followup for re: |this._targetElement.focus();| ?

Comment 13

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #12)
> Would you like I should file a followup for re:
> |this._targetElement.focus();| ?

Sounds good! We should try looking into hg history to see if there's some valid reason for this focus() call, but if not, I think we can get rid of it.

Comment 14

[Mark Capella [:capella]]

I already tried using blame, it seems like it was that way when ported into a lazy handler so ... how would I track it back through code removed from browser.js?

Comment 15

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=d8110b1365f1

Comment 16

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #14)
> I already tried using blame, it seems like it was that way when ported into
> a lazy handler so ... how would I track it back through code removed from
> browser.js?

Good skill to learn! You should look at hg annotate for browser.js for the parent of that lazy handler patch. So, this:
http://hg.mozilla.org/mozilla-central/annotate/45813096b581/mobile/android/chrome/content/browser.js#l1949

Although that doesn't look particularly useful, so you may need to look farther up the parent tree.

Comment 17

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/d25a408a3efb

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Mark Capella [:capella]]

Comment on attachment 820463 [details] [diff] [review]
bug912983 (v1)

[Approval Request Comment] Simple regression in Text Selection code
Bug caused by (feature/regressing bug #): caused back in bug 854605
User impact if declined: Minor, annoying screen artifact observable where none should be in some situations
Testing completed (on m-c, etc.): local builds, not sure Q/A verified yet in m-c
Risk to taking this patch (and alternatives if risky): minor, extremely simple one line fix, well understood, backout quite simple
String or IDL/UUID changes made by this patch: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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