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

VERIFIED FIXED in Firefox 17

Status

()

Core
DOM: Events
P1
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: John Shih, Assigned: cjones)

Tracking

({dogfood})

unspecified
mozilla19
x86_64
Linux
dogfood
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox17+ fixed, firefox18+ fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

1.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.10 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
## 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

Updated

5 years ago
Duplicate of this bug: 800832
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.
blocking-basecamp: --- → ?
Summary: [Keyboard][Browser][SMS] Keyboard will show off when press any button on it → [Keyboard][Browser][SMS] Keyboard will vanish when press any button on it

Comment 3

5 years ago
Can we try to bisect this?
Keywords: qawanted
blocking-basecamp: ? → +
Priority: -- → P1
blocking-basecamp: + → ---
Priority: P1 → --
blocking-basecamp: --- → ?
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.
Here is a (wide) regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2fae8bd461da&tochange=83e8792a147a
Maybe bug 795657? Testing...
I believe the regression is from bug 799299:
https://hg.mozilla.org/mozilla-central/rev/43218e560c4a
(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
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.
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.
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.
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.
blocking-basecamp: ? → +
(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.
Blocks: 799299
Severity: normal → critical
Keywords: dogfood
Priority: -- → P1
Assignee: nobody → jones.chris.g
Created attachment 670940 [details] [diff] [review]
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.
Attachment #670940 - Flags: review?(masayuki)
Attachment #670940 - Flags: review?(enndeakin)
Attachment #670940 - Flags: review?(bugs)
Note, this patch is also an exit strategy if bug 799299 causes web compat issues.
Removing QA wanted, since there's a regression range found + a patch.
Keywords: qawanted
Created attachment 670972 [details] [diff] [review]
Test for bug 800817
Attachment #670972 - Flags: review?(masayuki)
Attachment #670972 - Flags: review?(enndeakin)
Attachment #670972 - Flags: review?(bugs)
Component: General → DOM: Events
Product: Boot2Gecko → Core
Duplicate of this bug: 800893
Is there anyone around at this time who knows this code well enough to review?
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.
Attachment #670972 - Attachment is obsolete: true
Attachment #670972 - Flags: review?(masayuki)
Attachment #670972 - Flags: review?(enndeakin)
Attachment #670972 - Flags: review?(bugs)
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.
Attachment #670940 - Flags: review?(masayuki)
Attachment #670940 - Flags: review?(enndeakin)
Attachment #670940 - Flags: review?(bugs)
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".
> 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.
> 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.
Created attachment 671057 [details] [diff] [review]
Add a "mozallowfocuscancel" attribute to let <app> embedders enable <app> frames to receive events without taking focus

Tests coming up.
Attachment #670940 - Attachment is obsolete: true
Attachment #671057 - Flags: superreview?(bzbarsky)
Attachment #671057 - Flags: review?(bugs)
Created attachment 671060 [details] [diff] [review]
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.
Attachment #671060 - Flags: review?(timdream+bugs)
Attachment #671060 - Flags: review?(rlu)
Created attachment 671065 [details] [diff] [review]
Test for bug 800817
Attachment #671065 - Flags: review?(bugs)
Created attachment 671068 [details] [diff] [review]
Test for bug 800817

There was a bug in the previous test.
Attachment #671065 - Attachment is obsolete: true
Attachment #671065 - Flags: review?(bugs)
Attachment #671068 - Flags: review?(bugs)
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.
> 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 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.
Attachment #671057 - Flags: superreview?(bzbarsky)
Attachment #671057 - Flags: review?(bugs)
Attachment #671057 - Flags: review-
(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.
(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()).
Created attachment 671146 [details] [diff] [review]
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.
Attachment #671057 - Attachment is obsolete: true
Attachment #671060 - Attachment is obsolete: true
Attachment #671068 - Attachment is obsolete: true
Attachment #671060 - Flags: review?(timdream+bugs)
Attachment #671060 - Flags: review?(rlu)
Attachment #671068 - Flags: review?(bugs)
Attachment #671146 - Flags: review?(bugs)
Created attachment 671147 [details] [diff] [review]
Test for bug 800817
Attachment #671147 - Flags: review?(bugs)

Updated

5 years ago
Attachment #671146 - Flags: review?(bugs) → review+
Comment on attachment 671147 [details] [diff] [review]
Test for bug 800817

Please test mozbrowser
Attachment #671147 - Flags: review?(bugs) → review-
(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.
what existing web content? FF doesn't load stuff to html iframes.
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.
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.
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
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.
Er right, typo.
Created attachment 671152 [details] [diff] [review]
Test for bug 800817, v2
Attachment #671147 - Attachment is obsolete: true
Attachment #671152 - Flags: review?(bugs)
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.
Attachment #671152 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6c79681a21
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea044d85c1e
(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.
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
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?
https://hg.mozilla.org/mozilla-central/rev/ad6c79681a21
https://hg.mozilla.org/mozilla-central/rev/6ea044d85c1e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This is marked blocking-basecamp+ but does not apply cleanly to aurora. Please post a branch-specific patch or unmark the project flag please.
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.
Why was this marked as blocking-basecamp if the regressing patch isn't on aurora? That seems confusing...
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
Whiteboard: [no-ff18][unless bug 799299 lands on Aurora]
(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 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.
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.
(Reporter)

Comment 58

5 years ago
Verified on 2012-10-16
gaia : 6a6c070293c6cd404fc7d3d7afece401dd114419
mozilla-central : add440caf2f26a55f86e456502689709a9fed143
Status: RESOLVED → VERIFIED
We are tracking bug 799299 for both Beta (17) and Aurora (18) so this needs to be nominated for uplift to Aurora prevent breakage.
tracking-firefox18: --- → +
Depends on: 802436
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
https://hg.mozilla.org/releases/mozilla-beta/rev/3234f1d43ed8
https://hg.mozilla.org/releases/mozilla-beta/rev/b1f37231391a
status-firefox17: --- → fixed
tracking-firefox17: --- → +
Whiteboard: [no-ff18][unless bug 799299 lands on Aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8f505668b0c
status-firefox18: --- → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/adc8c88be25a
Can this bug be marked verified for Firefox 17 and 18 based on in-testsuite+?
You need to log in before you can comment on or make changes to this bug.