681640 - When text field is not focused yet, mouse events are consumed, when tapping on it

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

When a textarea isn't focused yet, mouse events seem to be getten 'eaten'. Only when the textarea is focused, the textarea registers mouse events.

See testcase, to reproduce, tap on the textarea.

Expected result:
- The following events should appear in this order: touchstart, touchend, mouseover, mousemove, mousedown, focus, mouseup, click
(Stock Android Browser does this, but fires another focus, mousemove, mousemove event, which seems incorrect to me)

Actual result:
- The following events appear: touchstart, touchend, focus

Comment 1

[Martijn Wargers (dead)]

Attachment: patch?

This fixes this bug and bug 680970.
But I don't know if this is the correct thing to do.

I'm wondering where the code is when the mouseevents are generated when the text control is already focused.

There is the issue that the focus is fired before the mousedown event, the focus of the text control should be a result of the mousedown event.
But that is probably not that bad an issue and could be covered in a follow-up bug.
I think the focus() call is done here:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/input.js#315

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 557351 [details] [diff] [review]
patch?

Looks good to me.

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase2

Btw, I can listen for 'TapSingle','TapDown','TapOver' and 'TapUp' events from inside content.
Stock Android browser doesn't know these event, nor can I find any info about those on the internet.

Why are these events being fired in content? Is there a spec for it somewhere?

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #3)
> Btw, I can listen for 'TapSingle','TapDown','TapOver' and 'TapUp' events
> from inside content.

These events are for Fennec's internal use and should not be exposed to web content.  Please file a new bug if you are able to receive these events from content.

Comment 5

[Martijn Wargers (dead)]

It turns out that I can only listen for these Tap events with my own build, not from regular builds, nor from my debug build, so I'll ignore that for now.

Comment 6

[Ed Morley [:emorley]]

Please may you adjust your hgrc to add author + include a commit message along the lines of:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)

Comment 8

[Martijn Wargers (dead)]

Attachment: browser-chrome test

This is a browser-chrome test, btw, that fails without the patch in this bug and passes with the patch applied.

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2807e45402c

Comment 10

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

This could be a regression from bug 628012. That is when the code this patch tweaks was last changed.

Comment 11

[Matt Brubeck (:mbrubeck)]

Pushed the browser-chrome test to Try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4062815506bc

Comment 12

[Matt Brubeck (:mbrubeck)]

On the Try server, the new test is passing, but it seems to cause a failure in a later test (browser_select.js) for some reason.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

backed out for permaorange on Android b-c

TEST-START | chrome://mochitests/content/browser/mobile/chrome/tests/browser_select.js
TEST-PASS | chrome://mochitests/content/browser/mobile/chrome/tests/browser_select.js | Tab Opened
TEST-PASS | chrome://mochitests/content/browser/mobile/chrome/tests/browser_select.js | Get the select from web content
TEST-INFO | chrome://mochitests/content/browser/mobile/chrome/tests/browser_select.js | Console message: [JavaScript Error: "uncaught exception: waitFor timeout"]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/tests/browser_select.js | Test timed out

Comment 14

[Martijn Wargers (dead)]

Sorry about that. I did check browser-chrome tests with the patch, but I didn't really pay good attention, it turns out (probably because I already was seeing failures without the patch too).

Not really sure what's going on. The form assistent opens the select-popup onfocus. But I'm not sure where the select gets the focus from if it's not receiving click events.

Comment 15

[Matt Brubeck (:mbrubeck)]

I wonder if the problem is this:

>-        } else if (!this._formAssistant.open(element) && this._highlightElement) {
>-          sendAsyncMessage("FindAssist:Hide", { });
>-          this._sendMouseEvent("mousemove", this._highlightElement, x, y);

In the old code, _formAssistant.open is called whether or not _highlightElement is true.

>+        } else if (this._highlightElement) {
>+          if (!this._formAssistant.open(element))
>+            sendAsyncMessage("FindAssist:Hide", { });

...but with this patch, _formAssistant.open is called only if _highlightElement is true.  In this case, the patch from bug 683186 might be a more correct fix.  I'll send that to Try along with the new test.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: alternate patch

Here's my patch, originally from duplicate bug 683186.  I pushed this to Try along with Martijn's browser-chrome test, and it is green:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a26dfdecb489

Comment 17

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e23d05fcd782
http://hg.mozilla.org/integration/mozilla-inbound/rev/44e3478debec

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/44e3478debec
http://hg.mozilla.org/mozilla-central/rev/e23d05fcd782

Comment 19

[Martijn Wargers (dead)]

Ok, nice that this bug is fixed.

But I see a couple of problems with the code (not particularly with this patch) and the browser_select.js test.
- In the browser_select.js test, it is mentioned that ContentTouchHandler.tapSingle is not working properly
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/tests/browser_select.js#28
- The weird this._highlightElement check was added in bug 628012. It is acknowledged that is a hack. Because of this check, ContentTouchHandler.tapSingle is not firing mouse events and was my patch failing on this test.

Another problem I think is that this._formAssistant.open(element) function causes the the form element to get focused if it's not focused yet. I don't think that that function should be doing that, should it?

Changing ContentTouchHandler.tapSingle to:
EventUtils.synthesizeMouse(combo, new_tab.browser.getBoundingClientRect().left + combo.getBoundingClientRect().left + 2, 
                                          new_tab.browser.getBoundingClientRect().top + combo.getBoundingClientRect().top + 2, {});
 works and gives the necessary mouse events on the select.
But after finishing that test, I get "this.browser.fuzzyZoom is not a function", I've filed bug 684706 for that.

I wonder what these ContentTouchHandler.tapSingle and other ContentTouchHandler.tap* should be doing. What events should these be firing?

Comment 20

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #19)
> Another problem I think is that this._formAssistant.open(element) function
> causes the the form element to get focused if it's not focused yet. I don't
> think that that function should be doing that, should it?

More specific, this._formAssistant.open(element) calls currentIndex(aIndex) which has this line in it, which is causing the focus:
gFocusManager.setFocus(element, Ci.nsIFocusManager.FLAG_NOSCROLL | Ci.nsIFocusManager.FLAG_BYMOUSE);

On second thought, I guess this function has to be calling it, it's just the weird side-effect of tapSingle not doing what I would expect it to be doing.