1121033 - [319MB] Having a keyboard active will disable all app interaction if the user has typed anything

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Keyboard Issue Logcat

Description:
When the user types something into an app with the keyboard active, they will no longer be able to interact with any buttons within any app they are in. The Power button, Home button, Keyboard, and Edge Gestures will not be affected by this bug.


Repro Steps:
1) Update a Flame to 20150113010202
2) Open an app with a keyboard, such as Messages
3) Tap on the new message Icon
4) Type a phone number into the "To:" Field
5) Select the Back button, "...", "+", or attach Icon


Actual:
All app interaction will be disabled as long as the keyboard is active


Expected:
User is able to fully interact within any app with the keyboard open

Environmental Variables:
Device: Flame Master (319mb)(Kitkat)(Full Flash)
Build ID: 20150113010202
Gaia: 9946a490a9264b42e65385d703b28fa055ab2d42
Gecko: 3d846527576f
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 38.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Notes: This bug will occur with any active keyboard (QWERTY and number). If the user dismisses the keyboard, they will be able to interact with the app once again.


Repro frequency: 15/15 100%
See attached: Logcat, Video - http://youtu.be/scS0APMasT8

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This issue does NOT reproduce on Flame 2.2

Keyboard does NOT disable user interaction within apps

Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat)(Full Flash)
Build ID: 20150112010228
Gaia: f5e481d4caf9ffa561720a6fc9cf521a28bd8439
Gecko: bb8d6034f5f2
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 
Firmware Version: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[Peter Bylenga [:PBylenga]]

Functional regression that breaks smoke tests.


Requesting a window.

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20150109225731
Gaia: b875c1ddb0a024d1d375d549d0a0268adb27ae80
Gecko: 0d9ceb0ea832
Version: 37.0a1 (2.2 Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20150110075632
Gaia: b875c1ddb0a024d1d375d549d0a0268adb27ae80
Gecko: 27d6d4a76992
Version: 37.0a1 (2.2 Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=0d9ceb0ea832&tochange=27d6d4a76992

Caused by patches to bug 1119497.

Comment 4

[bhavana bajaj [:bajaj]]

:kats, can you please look into this asap and help backout changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1119497 on m-c and b2g_37 branch?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Investigating...

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Why is this 2.2+ if (as per comment 1 and the bug track flags) 2.2 is unaffected?

Also bug 1119497 did land in 2.2 so if 2.2 is unaffected then the regression window can't be correct.

Comment 7

[Peter Bylenga [:PBylenga]]

We don't have a blocking-3.0? option yet, will investigate what we want to do for that.

Adding qawanted to redo the 2.2 check and to verify if the build has the hash for bug 1119497.

Comment 8

[Pi Wei Cheng [:piwei] (inactive)]

Patches for bug 1119497 didn't land in 2.2 master until 6AM of Jan. 12 (See bug 1119497 comment 8). The nightly build that :DerekH tested against was generated on 1AM of Jan. 12, which doesn't include the patches.

Re-tested on latest 2.2 (branched into b2g37) and issue DOES occur.
Device: Flame
BuildID: 20150113083630
Gaia: 7c5b27cad370db377b18a742d3f3fdb0070e899f
Gecko: 5caa5b12dfa6
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a2 (2.2)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I'm also not able to reproduce using a local build from latest master. My base image is v188. I'll try using the latest pvtbuild.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Not able to reproduce using the latest pvtbuild either. Are there any other settings you're changing on the device? Where can I get the v18D-1 base image?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I found the v18D-1 base image and am still not able to reproduce. However if I set the layout.event-regions.enabled pref to false then I *can* reproduce exactly what you're seeing. Do you have that pref set by any chance? It should be set to true by default but in a newsgroup posting I suggested setting it to false in some cases, so it may be that it was left in that state accidentally on your devices. Please ensure that layout.event-regions.enabled is set to true.

That being said it is a bug that this manifests with that pref set to false, and I can look into it, but it shouldn't be a smoketest blocker.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Based on the prefs.js file Peter sent me it doesn't appear to be the pref. So I still can't reproduce this. I'm fine with backing out bug 1119497 until we figure this out but I would like to have STR that I can reproduce locally first.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Ah, PBylenga provided me some info over email, and it seems I have to bump the Flame memory down to 319 MB to make this reproduce, because that forces the keyboard to be in-process. I'll back out 1119497 for now, and will re-land it once I've figured out how to fix this.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Backed out bug 1119497 and part of bug 920036.

https://hg.mozilla.org/integration/b2g-inbound/rev/86bcc35413b4

Comment 15

[Kartikaya Gupta (email:[e-mail])]

The problem here seems to be that the keyboard is in an iframe which is the full screen, and so the hitRegion for that ViewportFrame is the full screen area, and it eats all the input events. The way this works with Gecko hit-testing code is that the mozpasspointerevents flag which is set on the keyboard iframe induces special behaviour. The event regions code however doesn't properly respect that flag.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Respect mozpasspointerevents when building event regions

This seems to fix it for me. I would have used GetType() == nsGkAtoms::viewportFrame but that's virtual so I tried using !GetContent instead. I don't know how correct that is.

Comment 17

[Botond Ballo [:botond]]

I wonder why intersecting the clip rect with the touch-sensitive region didn't handle this: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=27d6d4a76992#257

(Not that we want to keep that "touch-sensitive region" around, just curious.)

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #17)
> I wonder why intersecting the clip rect with the touch-sensitive region
> didn't handle this:

Touch-sensitive regions only exist in child processes but the keyboard iframe lives in the root process and sits on top of everything, so it "intercepts" the input events. I think to use the touch-sensitive region to fix this, we would have to take the inverse of the touch-sensitive region from the child process and apply it on the parent process. But there might be other cases where stuff in the root process is on top of the child process and those might break.

Comment 19

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> (In reply to Botond Ballo [:botond] from comment #17)
> > I wonder why intersecting the clip rect with the touch-sensitive region
> > didn't handle this:
> 
> Touch-sensitive regions only exist in child processes but the keyboard
> iframe lives in the root process and sits on top of everything, so it
> "intercepts" the input events. I think to use the touch-sensitive region to
> fix this, we would have to take the inverse of the touch-sensitive region
> from the child process and apply it on the parent process. But there might
> be other cases where stuff in the root process is on top of the child
> process and those might break.

Ah, I see - the keyboard can be in-process or out-of-process, as described in comment 13, and the touch-sensitive regions only handle the in-process case.

Comment 20

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #19)
> Ah, I see - the keyboard can be in-process or out-of-process, as described
> in comment 13, and the touch-sensitive regions only handle the in-process
> case.

Er, it only hands the out-of-process case, sorry.

Comment 21

[Peter Bylenga [:PBylenga]]

Martijn, do we have automation coverage for this regression?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8548951 [details] [diff] [review]
Respect mozpasspointerevents when building event regions

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

::: layout/base/nsDisplayList.cpp
@@ +3055,5 @@
>                                       nsIFrame* aFrame)
>  {
>    NS_ASSERTION(aBuilder->FindReferenceFrameFor(aFrame) == aBuilder->FindReferenceFrameFor(mFrame),
>                 "Reference frame mismatch");
> +  if (!aFrame->GetContent()) {

Better to test aFrame->GetParent().

That viewport frames have no element is a bit of a wart that would be good to fix one day.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Did a couple of try pushes. First one has the patch attached to this bug, but is opt-only. Second one includes the change roc suggested and gets some debug coverage.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8f44d9ec471
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3ddbc6eeb4f

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Landed on inbound:

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

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Please request b2g37 approval on this when you get a chance :) Or is this something we should consider taking on Aurora/Beta?

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8548951 [details] [diff] [review]
Respect mozpasspointerevents when building event regions

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1119497
User impact if declined: On devices with in-process keyboard (Flame w/ 319MB or other low-mem devices), it is impossible to interact with the content area while the keyboard is up.
Testing completed: locally, on m-c
Risk to taking this patch (and alternatives if risky): I'd say it's fairly low risk. The patch is small and the code is not particularly complicated, it was just never fully implemented. It only affects platforms where layout.event-regions.enabled is true, which currently is only B2G.
String or UUID changes made by this patch: none

Comment 29

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #27)
> Or is this
> something we should consider taking on Aurora/Beta?

Nah, it only affects B2G so need to take it on aurora.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/26b9e00275c7

Comment 31

[Sue]

Attachment: verify_video.MP4

This issue has been verified successfully on Flame v2.2
See attachment:verify_video.MP4

Flame 2.2 build:
Gaia-Rev        e4f9b5da3751798f9cc5d95f302c30722cc11fca
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/75a462a58d7a
Build-ID        20150121002607
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150121.040751
FW-Date         Wed Jan 21 04:08:02 EST 2015
Bootloader      L1TC000118D0

Comment 32

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 3.0 Master and on Flame 2.2.

Observed behavior: Following STR, app can be correctly interacted while the keyboard is invoked.

Device: Flame 3.0 Master (shallow flash, 319MB mem)
BuildID: 20150122143248
Gaia: 966b3a7a13a7f0d5b86cbc9e64cb78d43ec7dba8
Gecko: 568bbf124221
Version: 38.0a1 (3.0 Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Device: Flame 2.2 (shallow flash, 319MB mem)
BuildID: 20150122141946
Gaia: 237008137f6d72b9cad25ff4faff14ff2c40ac63
Gecko: 1090c8c5429f
Version: 37.0a2 (2.2)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 33

[Martijn Wargers (dead)]

(In reply to Peter Bylenga [:PBylenga] from comment #21)
> Martijn, do we have automation coverage for this regression?

Sorry, I haven't seen this before. I don't know if one or more of the automation tests were failing when this bug was occuring. The automation reports don't seem to indicated this was catched in automation.
So I filed bug 1130038 for creating a gaia-ui test for this.

Comment 34

[Martijn Wargers (dead)]

(In reply to Peter Bylenga [:PBylenga] from comment #21)
> Martijn, do we have automation coverage for this regression?

Sorry, I didn't create an automated test for this and at this point, it's probably not that important anymore.