838547 - Appending an iframe makes input field lose focus.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Evelyn Hung]

I'm trying to create an iframe for loading 3rd party keyboard when an input field gets focus. However, the the input field always lose its focus once the iframe is appended to the DOM tree (so I get a 'blur' event immediately).

Comment 1

[Evelyn Hung]

Attachment: a test case for input field focus

Description:
  The onfocus callback of the first input field will append an iframe, while the second input field will append a button. 

Result:
I tested this case on three different environments:
1. Firefox desktop: both input fields won't lose their focus
2. Browser App on Firefox OS: both input fields won't lose their focus
3. an app on Firefox OS: the first input field will lose its focus once the iframe is appended.

So I suspect the problem is related to frame script of Firefox OS.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Only listen to pagehide event of the content window

In bug 820057 we added the pagehide event listener but it receives pagehide events when an iframe is appended. Add the event listener to the content window should let us receive only the event we are interested.

Perhaps someone more familiar with frame scripts could tell why we are receiving extra pagehide event from the global object.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 715085 [details] [diff] [review]
Only listen to pagehide event of the content window

I unfortunately don't have a current build to test and investigate this. I hope Margaret still does? I also think David might be a good candidate as well, he knows about forms.js, too :)

Comment 4

[:Margaret Leibovic]

I also don't have a current build to test, and it's been a while since I've thought about this code.

My first question would be why we'd only change this one event listener to be added to the content window. Couldn't there be similar event bubbling issues that affect the other listeners?

Looking at the XUL fennec implementation, there's a different check before deciding whether or not to hide the keyboard:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/forms.js#316

Maybe we need to do something like that in b2g. I also noticed that the event listener in XUL fennec doesn't initiate capture, but the listener in b2g does. I don't know that this would cause the issue you're seeing here, but it would be nice to know the reasoning behind that difference. Maybe Tim knows.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 715085 [details] [diff] [review]
Only listen to pagehide event of the content window

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

I'm not good with chrome code.... Help me understand this patch so I can review it.

IIRC, forms.js gets injected into every running app, correct?  What is the practical difference in forms.js between addEventListener() and content.addEventListener()?  content is the window object, right?  What is the global object of forms.js? What kind of pagehide event is being fired on that global object rather than bubbling up from content?

Basically, I just can't see why this works.

Instead of registering this one listener on a different object than all the others, could we instead check the target field of the event object when we handle the event?

I'm not giving r+ or r- now, because I don't yet understand the patch.

Comment 6

[David Flanagan [:djf]]

Tim,

It looks like you added the pagehide event handler as part of the fix for bug 820057. Do you recall what it was needed for? What case is there where we'd get a pagehide but not get a blur or submit before the pagehide? I'm guess I'm asking whether it might be possible to get away with not listening for pagehide at all.

Also, does anyone on the bug understand why a pagehide event is being generated when an iframe is added?

Comment 7

[:Margaret Leibovic]

(In reply to David Flanagan [:djf] from comment #6)

> Also, does anyone on the bug understand why a pagehide event is being
> generated when an iframe is added?

I agree we should try to find an answer to this question. Does that happen on desktop Firefox?

Comment 8

[David Flanagan [:djf]]

Comment on attachment 715085 [details] [diff] [review]
Only listen to pagehide event of the content window

Taking myself off the review since as noted above I don't understand that patch.  I'm happy to try again with some explanation.  But I don't want it to look like this is a review I'm still working on.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 715085 [details] [diff] [review]
Only listen to pagehide event of the content window

Remove all reviews until I found the root cause.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: We are only interested to the pagehide event from the root document

The extra pagehide events are chrome events from subframes. This patch checks if event target is the root document.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 721628 [details] [diff] [review]
We are only interested to the pagehide event from the root document

Don't know why you have 'target instanceof HTMLDocument' check.
What if SVGDocument is loaded?
Why you filter out also blur and submit?

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 721628 [details] [diff] [review]
> We are only interested to the pagehide event from the root document
> 
> Don't know why you have 'target instanceof HTMLDocument' check.
> What if SVGDocument is loaded?
> Why you filter out also blur and submit?

The check is there so that we don't filter out blur and submit event. Maybe I should instead move pagehide to its own block.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: We are only interested to the pagehide event from the root document v2

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 721637 [details] [diff] [review]
We are only interested to the pagehide event from the root document v2

Just drop 
+        if (this.focusedElement)
+          this.hideKeyboard();
+        break;
so the execution continues to the blur/submit case.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: We are only interested to the pagehide event from the root document v2.1

Carry over r+.

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

Seems inbound will be closed for a while. Add checkin-needed so I don't forget this.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Going to nominate this as leo+, because it blocks a leo+ issue, Bug 890441.

Comment 20

[Wayne Chang [:wchang]]

Leo+ per comment 19.

Thanks Rudy.

Comment 21

[Borja Salguero [:borjasalguero]]

Cool! Leo+ so we need to uplift this right? Kan-Ru could you help us on this? I will take a look for ensuring that the other bug relate is working as expected once we have this uplifted. Thanks!

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8c0127cc7be3

Comment 23

[Borja Salguero [:borjasalguero]]

Thanks for the quick uplift! Gracias ;)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8c0127cc7be3

Comment 25

[Chris Kreinbring [:CKreinbring]]

Verified fixed on Leo 1.1 mozilla RIL.  The Maps app is no longer present on the homescreen but can be downloaded from the Marketplace.

Build ID: 20130723070209
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/68fb0a2e0114
Gaia: ffe25bfdf0e71c820ca710cb61fb8306564a8f4e
Platform Version: 18.1