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

RESOLVED FIXED

Status

defect
P2
trivial
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: martijn.martijn, Assigned: mbrubeck)

Tracking

({testcase})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(4 attachments, 1 obsolete attachment)

Posted file 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
Severity: normal → trivial
OS: Windows 7 → Android
Priority: -- → P2
Hardware: x86 → ARM
Blocks: 680970
Posted patch patch? (obsolete) — Splinter Review
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
Attachment #557351 - Flags: review?(mbrubeck)
Comment on attachment 557351 [details] [diff] [review]
patch?

Looks good to me.
Attachment #557351 - Flags: review?(mbrubeck) → review+
Posted file 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?
(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.
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.
Keywords: checkin-needed
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 :-)
Assignee: nobody → martijn.martijn
Status: NEW → ASSIGNED
Keywords: checkin-needed
Duplicate of this bug: 683186
This is a browser-chrome test, btw, that fails without the patch in this bug and passes with the patch applied.
Attachment #557995 - Flags: review?(mbrubeck)
This could be a regression from bug 628012. That is when the code this patch tweaks was last changed.
Pushed the browser-chrome test to Try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4062815506bc
Whiteboard: [inbound]
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.
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
Whiteboard: [inbound]
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.
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.
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
Assignee: martijn.martijn → mbrubeck
Attachment #557351 - Attachment is obsolete: true
Attachment #558114 - Flags: review?(mark.finkle)
Attachment #557995 - Flags: review?(mbrubeck) → review+
Attachment #558114 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mozilla-central/rev/44e3478debec
http://hg.mozilla.org/mozilla-central/rev/e23d05fcd782
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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?
Target Milestone: Firefox 9 → ---
Depends on: 684723
(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.
Depends on: 685197
You need to log in before you can comment on or make changes to this bug.