text typed in awesome bar shows up on previously focused form field

VERIFIED FIXED

Status

defect
--
major
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: aakashd, Assigned: jchen)

Tracking

Trunk
ARM
Android
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b1+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Build Id:
Private build (off of 9/23 nightly) based off of patches made to dependent bug.

Steps to Reproduce:
1. Go to mobile.support.mozilla.com
2. Tap on the search field
3. Tap on the url bar to open the awesome bar
4. Begin typing a set of characters
5. Click on the back button twice to go back to the support page

Actual Results:
The text typed into the url bar is actually on support's search field.

Expected Results:
The text typed into the url bar is visible within the url bar.
tracking-fennec: --- → ?
Status: NEW → ASSIGNED
Comment on attachment 478406 [details] [diff] [review]
text typed in awesome bare shows up on previously focused form field

So I think this is a backend issue, where if any text field is focused on Android the key input always goes to that. I fixed it by blurring any activeElement in child process.

I made the keypress stuff cleaner, even though it's going away soon. Now, keypresses are forwarded only when a particular remote browser is active. If content process doesn't want key, it's dispatched to the parent node of the browser.
Attachment #478406 - Flags: review?(mark.finkle)
The behavior under PuppetWidget is that IME events are redirected to the content process if a text field in content process has focus (this is probably the easiest approach to handle IME events). And this bug, like Ben said, is due to the content text field NOT blurring when a chrome text field focuses (so probably related to Bug 583976).
Comment on attachment 478406 [details] [diff] [review]
text typed in awesome bare shows up on previously focused form field

>+let FocusEvents = {
>+  init: function() {
>+    addMessageListener("Browser:Focus", this);
>+    addMessageListener("Browser:Blur", this);
>+    addMessageListener("Browser:KeyEvent", this);

"Content:Xxx" We are currently using "Content" prefix for <browser> binding-only messages

>+        if (defaultAction && json.type == "keypress") {
>+          const masks = Ci.nsIDOMNSEvent;
>+          sendAsyncMessage("Browser:KeyPress", {

"Content:KeyPress"

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml

>       <constructor>
>         <![CDATA[
>           this.messageManager.addMessageListener("scroll", this._messageListenerRemote);
>+          this.messageManager.addMessageListener("Browser:KeyPress", this._messageListenerRemote);

"Content:KeyPress"

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>     if (browser) {
>       browser.setAttribute("type", "content-primary");
>       browser.style.display = "";
>-      browser.messageManager.sendAsyncMessage("Browser:Focus", {});
>+      getBrowser().focus();

browser.focus(); should be fine

>   tapDown: function tapDown(aX, aY) {

>     this._dispatchMouseEvent("Browser:MouseDown", aX, aY);
>+    // Since overlay lays on top of browser element, we must focus the browser
>+    // when user taps on overlay.
>+    getBrowser().focus();

Add a blank line before the comment

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>-                <html:div id="inputhandler-overlay" style="z-index: 1001" tabindex="-1"/>
>+                <html:div id="inputhandler-overlay" style="z-index: 1001"/>

You need inpputhandler to be focusable? Oh, from browser.xml:

              case "Browser:KeyPress":
                // Content did not want this keypress event, so resend event up the tree.
                let node = this.self.parentNode;

OK

r+ with nits fixed
Attachment #478406 - Flags: review?(mark.finkle) → review+
(In reply to comment #3)
> The behavior under PuppetWidget is that IME events are redirected to the
> content process if a text field in content process has focus (this is probably
> the easiest approach to handle IME events). And this bug, like Ben said, is due
> to the content text field NOT blurring when a chrome text field focuses (so
> probably related to Bug 583976).

I would feel more comfortable if PuppetWidget also checked if browser element is focused before sending it to text box in child process.
tracking-fennec: ? → 2.0b1+
Pushed http://hg.mozilla.org/mobile-browser/rev/60df3aacf8eb
Status: ASSIGNED → RESOLVED
tracking-fennec: 2.0b1+ → ?
Closed: 9 years ago
Resolution: --- → FIXED
Reopened due to Talos regression. Tracking in bug 599576.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The backout has shown that this is the guilty patch for the Ts regression
tracking-fennec: ? → 2.0b1+
We have been unable to find a happy version of this patch, or this overall approach.

Jim Chen is going to work on a patch to workaround the problem. IIRC, he'll check to see if a chrome textbox has focus before sending IME events to the content process.

Bug 583976 might be the "one true fix" for this problem. Bug 586656 may also be related.

Bug 593755 is likely a dupe of this bug. We'll need to test Jim's workaround on Meego too.
Assignee: nobody → jimnchen+bmo
Patch checks for editable content in chrome first before forwarding events to content.
Attachment #478406 - Attachment is obsolete: true
Attachment #478406 - Flags: review+
Attachment #478894 - Flags: review?(Olli.Pettay)
Comment on attachment 478894 [details] [diff] [review]
Patch to check for chrome focus first

I don't think this the right fix.
Chrome should probably focus the <browser remote="true"> when
events should go to content process and use that information in ESM.
Attachment #478894 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 478894 [details] [diff] [review]
Patch to check for chrome focus first

Ok, as a temporary fix this is okish. The code is inside ifdef android, right?
Attachment #478894 - Flags: review- → review+
(In reply to comment #12)

> Ok, as a temporary fix this is okish. The code is inside ifdef android, right?

Actually, it's inside ifdef MOZ_IPC. We might need this for Meego and Android.
m-c http://hg.mozilla.org/mozilla-central/rev/5de7de1ef0d6
relbranch http://hg.mozilla.org/mozilla-central/rev/d7f2e1898c4f
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
verified FIXED on build:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100930 Namoroka/4.0b7pre Fennec/4.0b1pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.