800817 - [Keyboard][Browser][SMS] Keyboard will vanish when press any button on it

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P1)

Description

[John Shih]

## Environment :
Otoro phone, build 2012-10-12
Build info: 
* "gaia" revision= 2ab7bd25e21b7931b700731389ad433286a18945
* "gecko" revision= c96b188e4b7d480bdd897eb7b368afc91b8e6e5f

## Repro :
1. Launch the browser or sms
2. Focus on the input field to trigger the keyboard
3. Press arbitrary button on the keyboard

## Expected:
* The button you press will be inserted into the input field

## Actual:
* Keyboard shows off and nothing is inserted into the field

Comment 2

[David Flanagan [:djf]]

I filed this is #800832, but I've closed that as a dup now.  Here's how I described it there: 

This just started for me tonight when building from m-c for otoro.

When I focus an input field the keyboard appears as usual.  But then when I type a key, I see the little feedback popup, then the keyboard vanishes and there is no input in the input field.

It looks to me as if there has been a change in focus management or something. Tapping the keyboard somehow takes focus away from the input field which makes the keyboard disappears.  But I'm just guessing about that... I haven't had time to try any debugging.

I'm cc'ing just about anybody I can think of who has worked on the keyboard, and also cjones because he may know if anything has landed that would have changed focus events.

Also nominating blocking, and clarifying the summary line.

Comment 3

[Andreas Gal :gal]

Can we try to bisect this?

Comment 4

[David Flanagan [:djf]]

Just rebuilt from tip: 110045:cfbf2d4218c8
The bug is still there.

I don't have a last known good revision, but I assume it started sometime yesterday.

Now trying 109977:2efb9e5762ce from yesterday, more or less at random.

Comment 5

[Chris Peterson [:cpeterson]]

Here is a (wide) regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2fae8bd461da&tochange=83e8792a147a

Comment 6

[Gregor Wagner [:gwagner]]

Maybe bug 795657? Testing...

Comment 7

[Chris Peterson [:cpeterson]]

I believe the regression is from bug 799299:
https://hg.mozilla.org/mozilla-central/rev/43218e560c4a

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Chris Peterson (:cpeterson) from comment #7)
> I believe the regression is from bug 799299:
> https://hg.mozilla.org/mozilla-central/rev/43218e560c4a

Right. Rev 43218e560c4a doesn't work, rev e491269db27e works

Comment 9

[David Flanagan [:djf]]

Clutching at straws here, but I just tried setting * {-moz-user-focus: ignore;} in keyboard.css to see if I could prevent the keyboard window from ever taking focus.

It didn't help.

Comment 10

[David Flanagan [:djf]]

So the code that changed in nsEventStateManager.cpp is only triggered on mousedown events. Assuming the gaia keyboard uses touch events, I'll try tweaking it to cancel them, hoping that will prevent the mouse events from being generated in the first place.

Comment 11

[David Flanagan [:djf]]

Nope, I can't fix it in gaia.  I've tried converting the keyboard to use touch events instead of mouse events, and I've tried calling preventDefault() and stopPropagation() on the events the keyboard receives with both mouse events and touch events, but have had no luck.

cc'ing Olli, since #799299 was his. Olli: I don't have any idea what your changes to nsIEventStateManager.cpp do.  Do you see any way that we can alter the Gaia keyboard so that it works correctly with your patch applied?  Is there some way that your patch could be modified to behave differently for touch events and mouse events?

I'm completely over my head here, so I'm going to back out and leave this to the gecko pros.

Comment 12

[Ben Francis [:benfrancis]]

This missed platform triage today due to an error on my part, but fabrice and I discussed this in IRC and I'm setting basecamp-blocking+ so this patch doesn't get delayed as it's blocking a lot of people.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to David Flanagan [:djf] from comment #10)
> So the code that changed in nsEventStateManager.cpp is only triggered on
> mousedown events.

That's a bug in the patch.  We shouldn't rely on that.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Make dispatch of events to one frame while another is focused a pref-able behavior

Working on adding a test for the pref-enabled behavior.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note, this patch is also an exit strategy if bug 799299 causes web compat issues.

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Removing QA wanted, since there's a regression range found + a patch.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test for bug 800817

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Is there anyone around at this time who knows this code well enough to review?

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 670972 [details] [diff] [review]
Test for bug 800817

bz put me on the right path; there's no web compat risk for the original patch because it only applies for different .top's.  So this isn't something we would keep in the long run, and this test is not valuable.

He suggests adding a mozbrowser api to mark a frame as declining to want focus in this case, and then check here.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 670940 [details] [diff] [review]
Make dispatch of events to one frame while another is focused a pref-able behavior

Will try to get a better fix up tonight, but will keep this around as insurance.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

I don't like such hacky way...

First, I don't understand why the VKB isn't in chrome document. It must cause similar regressions continuously. At handling event or focus, whether the contents or documents in chrome or not is very useful information.

Even if it should be in content rather than chrome, I think that the window or chrome should have a special flag such as "like chrome".

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

> Even if it should be in content rather than chrome, I think that the window or
> chrome should have a special flag such as "like chrome".

I meant window and/or document should have a special flag.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> First, I don't understand why the VKB isn't in chrome document.

Because in Gaia _nothing_ is ever in a chrome document, pretty much.  There's only one chrome document, and it's the phone desktop background, basically.

So yes, any time we have an explicit "is chrome" check it's a potential bug for b2g, and if we're doing that in event handling we need to fix those callsites accordingly...

> I think that the window or chrome should have a special flag such as "like chrome".

Yes, that was my proposal and what comment 20 is about.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add a "mozallowfocuscancel" attribute to let <app> embedders enable <app> frames to receive events without taking focus

Tests coming up.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Have the keyboard run as mozapp to unlock more achievements like mozallowfocuscancel

However you guys prefer; here or in https://github.com/mozilla-b2g/gaia/pull/5799.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test for bug 800817

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test for bug 800817

There was a bug in the previous test.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 671057 [details] [diff] [review]
Add a "mozallowfocuscancel" attribute to let <app> embedders enable <app> frames to receive events without taking focus

I feel a little bit odd this patch.

I think that nsEventStateManager::PostHandleEvent() is not the only path to set focus to the window/document. E.g., when the mouse event isn't consumed by preventDefault(), looks like the document takes focus. So, I don't think that it's easy the developers to understand the new attribute.

Isn't it simpler if mouse events never set focus when non-focusable element is clicked on such document? If calling preventDefault() is always necessary for canceling activating the document, it seems that it makes other limitations.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

> I think that nsEventStateManager::PostHandleEvent() is not the only path

I meant that the changed part is not the only path. Not consumed case is above of the change.

Comment 31

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

Comment on attachment 671057 [details] [diff] [review]
Add a "mozallowfocuscancel" attribute to let <app> embedders enable <app> frames to receive events without taking focus

This doesn't handle the case when mDocument is a subdocument of
<iframe mozallowfocuscancel>
Also, couldn't we treat any ReallyIsBrowser iframes the same way as
chrome? It is quite ugly to require one to add 
mozallowfocuscancel attribute.

Would it help to change GetScriptableTop to GetTop?
I wanted to treat things inside mozbrowsers as separate document trees, but
perhaps B2G doesn't want that.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #29)
> Comment on attachment 671057 [details] [diff] [review]
> Add a "mozallowfocuscancel" attribute to let <app> embedders enable <app>
> frames to receive events without taking focus
> 
> Isn't it simpler if mouse events never set focus when non-focusable element
> is clicked on such document? If calling preventDefault() is always necessary
> for canceling activating the document, it seems that it makes other
> limitations.

Ah, the so the goal isn't to create *non*-focusable <iframe mozbrowser>; focus would needed in some cases to e.g. switch languages or keyboard layout.  The goal is to allow the iframe to decline focus when it's just using touch/mouse events to synthesize keys for another focused frame.

The keyboard implementation has been doing this for months, and developers understand it.  I'm open to suggestions, but the current implementation was working fine until bug 799299.

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #31)
> Comment on attachment 671057 [details] [diff] [review]
> Add a "mozallowfocuscancel" attribute to let <app> embedders enable <app>
> frames to receive events without taking focus
> 
> Also, couldn't we treat any ReallyIsBrowser iframes the same way as
> chrome? It is quite ugly to require one to add 
> mozallowfocuscancel attribute.

We could.  The first version of the patch I wrote did that.

> Would it help to change GetScriptableTop to GetTop?

I guess we could do that.  I'll try it.

> I wanted to treat things inside mozbrowsers as separate document trees, but
> perhaps B2G doesn't want that.

I don't understand well what a document tree means specifically, but we do want mozbrowsers to be docshell boundaries (hence GetScriptableTop()).

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Use the "real" top when decide to switch focus on a preventDefault()ed tap

I changed to test to check the existing behavior that was being guarded against with the .top checks.  Coming up in a minute.

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test for bug 800817

Comment 36

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

Comment on attachment 671147 [details] [diff] [review]
Test for bug 800817

Please test mozbrowser

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 671147 [details] [diff] [review]
> Test for bug 800817
> 
> Please test mozbrowser

Since we're deciding to treat mozbrowser no differently than arbitrary iframes in the same .top context, I explicitly removed mozbrowser from this test.  IOW, this is testing that we don't break this behavior in existing web content.

I'd be happy to add an additional test specifically for mozbrowser, if you'd like.

Comment 38

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

what existing web content? FF doesn't load stuff to html iframes.

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If a web page foo.com has two child iframes in the same configuration as this test, then it might be relying on focus staying in one from while mouse events are delivered to and preventDefault()ed in the other frame.

Comment 40

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

We need a test for the case where this bug happened. I believe mozbrowser is involved in that case.
I could be wrong ofc, since I have no idea where gaia stuff even lives.

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Like I said, I'm happy to add an additional test for mozbrowser if you'd like.  The only reason mozbrowser was affected is because GetTop() and GetRealTop() return different things.

https://github.com/mozilla-b2g/gaia

Comment 42

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

GetTop and GetRealTop return the same thing. It is the scriptable thing which returns something else
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindow.idl#122

But please add some tests for mozbrowser.

Comment 43

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Er right, typo.

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test for bug 800817, v2

Comment 45

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

Comment on attachment 671152 [details] [diff] [review]
Test for bug 800817, v2

This is still missing the code path where mozbrowser is
not in chrome. But ok.

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6c79681a21
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea044d85c1e

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #32)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #29)
> > Comment on attachment 671057 [details] [diff] [review]
> > Add a "mozallowfocuscancel" attribute to let <app> embedders enable <app>
> > frames to receive events without taking focus
> > 
> > Isn't it simpler if mouse events never set focus when non-focusable element
> > is clicked on such document? If calling preventDefault() is always necessary
> > for canceling activating the document, it seems that it makes other
> > limitations.
> 
> Ah, the so the goal isn't to create *non*-focusable <iframe mozbrowser>;
> focus would needed in some cases to e.g. switch languages or keyboard
> layout.  The goal is to allow the iframe to decline focus when it's just
> using touch/mouse events to synthesize keys for another focused frame.

I meant that if the clicked element needs to takes focus such as <input type="text">, the window should take focus too. So, I meant that click on non-focusable element shouldn't cause move focus to the window.

My point is, I feel that calling preventDefault() of mousedown events is hacky and tricky...

> The keyboard implementation has been doing this for months, and developers
> understand it.  I'm open to suggestions, but the current implementation was
> working fine until bug 799299.

I worry about the developers who will create apps on b2g. I think that non-focusable document may be required by such developers.

Comment 48

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

I think the behavior the patch provides should be good for b2g. It make b2g apps to behave exactly like
web pages, whether or not there are mozbrowsers

Comment 49

[Justin Lebar (not reading bugmail)]

Without understanding this whole bug: Switching to GetTop instead of GetScriptableTop here means that we'll observe a difference between remote and in-process mozbrowsers, since GetTop for a top-level mozbrowser inside a content process behaves like GetScriptableTop.

How do we ensure we get the right behavior in the OOP case?

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad6c79681a21
https://hg.mozilla.org/mozilla-central/rev/6ea044d85c1e

Comment 51

[Ryan VanderMeulen [:RyanVM]]

This is marked blocking-basecamp+ but does not apply cleanly to aurora. Please post a branch-specific patch or unmark the project flag please.

Comment 52

[:Gavin Sharp [email: [e-mail]]]

That's because bug 799299 isn't on Aurora yet, and that's what caused this bug. It may not be uplifted at this point, but Chris already pointed out that these need to land together in bug 799299 comment 22.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Why was this marked as blocking-basecamp if the regressing patch isn't on aurora? That seems confusing...

Comment 54

[Justin Lebar (not reading bugmail)]

Sounds like we need some whiteboard marker so we can make this not show up in Fabrice's queries [1].  I don't think we want to unmark blocking-basecamp, since we spread that sauce pretty liberally.  Feel free to edit the whiteboard tags I added if you prefer some other wording.

[1] https://people.mozilla.com/~fdesre/aurora.html

Comment 55

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Ryan VanderMeulen from comment #53)
> Why was this marked as blocking-basecamp if the regressing patch isn't on
> aurora? That seems confusing...

This bug severely impacts continued development of b2g on m-c/inbound.

Comment 56

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 671152 [details] [diff] [review]
Test for bug 800817, v2

>diff --git a/dom/tests/mochitest/chrome/file_bug799299.xul b/dom/tests/mochitest/chrome/file_bug800817.xul

>   function runTests() {

>+    var mozbrowserAttr = opener.wrappedJSObject.testMozBrowser ? "true" : "false";
>+    b1.setAttribute("mozbrowser", mozbrowserAttr);
>+    b2.setAttribute("mozbrowser", mozbrowserAttr);

This seems bogus - mozbrowser="true" behaves the same as mozbrowser="false" (this is an HTML boolean attribute, where the test for trueness hasAttribute(), not getAttribute()=="true").

This seems to be intending to test that the behavior is the same in both cases, but it's not, and the only reason this is passing is because of the above error.

Comment 57

[:Gavin Sharp [email: [e-mail]]]

This also seems like a rather hacky solution - from what I can tell b2g wants one behavior, Firefox/Social wants another, and we're making that distinction based on what the relevant frames return for .top, which happens to work in practice because b2g has special .top behavior, but doesn't really make sense conceptually. And neither of these bugs adjusted the comment above this code to explain the reasoning behind these seemingly arbitrary checks.

Comment 58

[John Shih]

Verified on 2012-10-16
gaia : 6a6c070293c6cd404fc7d3d7afece401dd114419
mozilla-central : add440caf2f26a55f86e456502689709a9fed143

Comment 59

[Lukas Blakk [:lsblakk] use ?needinfo]

We are tracking bug 799299 for both Beta (17) and Aurora (18) so this needs to be nominated for uplift to Aurora prevent breakage.

Comment 60

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #57)

Disregard this - I misunderstood the problem (and solution) entirely. The code still could do with a more complete comment though, I think.

Comment 61

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3234f1d43ed8
https://hg.mozilla.org/releases/mozilla-beta/rev/b1f37231391a

Comment 62

[:Gavin Sharp [email: [e-mail]]]

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

Comment 63

[:Gavin Sharp [email: [e-mail]]]

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

Comment 64

[u279076]

Can this bug be marked verified for Firefox 17 and 18 based on in-testsuite+?