973096 - [B2G][Messaging] Conversation is scrollable after tapping on Header

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Peter Bylenga [:PBylenga]]

Attachment: Messaging_Header_BackgroundScrollable.txt

Description:
In the Messaging app, tapping on the header of a conversation does not disable the ability to scroll the conversation list. This issue does not reproduce with APZ disabled.

Repro Steps:
1) Update a Buri to BuildID: 20140214004001
2) Have a lengthy conversation in the Message app to be scrolled.
3) Launch Message App
4) Tap on lengthy conversation
5) Tap on Header
6) Attempt to scroll Conversation list

Actual:
Conversation list responds to scroll

Expected:
Conversation list does not respond to input when it's in background

v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20140214004001
Gaia: 22e065f75193f569a78a8ec827b08e1ed520e1b2
Gecko: e3766683210c
Version: 28.0
Firmware Version: v1.2-device.cfg


Notes:

Repro frequency: 5/5, 100%
See attached: logcat

Comment 1

[Jason Smith [:jsmith]]

Weird glitch, but I wouldn't block on it.

Comment 2

[Julien Wajsberg [:julienw]]

z-index issue?

Comment 3

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

Not sure; I'd have to investigate. It might be the same underlying cause as bug 969483. If you have some time, could you try that patch and see if it fixes the problem? If not I can check that tomorrow. Either way unless this is flagged as a blocker it's unlikely I'll work on it soon.

Comment 4

[Julien Wajsberg [:julienw]]

I can definitely wait for bug 969483 to land ;)

Comment 5

[Julien Wajsberg [:julienw]]

Nope, the patch in bug 969483 does not fix this.

Comment 6

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

Ok, thanks for checking. I'll look into it eventually when I go through all the other bugs blocking gaia-apzc-2.

Comment 7

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

Ah, this is because of improper APZ hit testing in the face of transparent layers. This is tracked by bug 918288.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Without this we don't hit APZ correctness target for 1.4.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Assign to me as a placeholder until the plates of other engineers clear up.

Comment 11

[Sandip Kamat]

PM Triage: 1.4+

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Uplift if we have a fix, but unless we have a stronger example, go with comment 1 and not block.

Comment 13

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

Attachment: WIP

After a discussion I had with :mchang and :tn the other day I realized that I was wrong about the root cause of this bug. This actually has nothing to do with opacity and can be fixed in the APZC hit-detection code without requiring any layout changes. Here is a patch that accomplishes this, although I haven't tested extensively for unexpected regressions yet. It should also be possible to write gtests for this behavior.

Comment 14

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

Carrying in flags from dupe.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Hey Kats, did some more testing and found something that broke. While trying to manually add a hidden Wifi network, I can't scroll down to enter my password. Likewise, I can't push the "OK" button on the top right. This is just with this patch on master w/o any patches from bug 930939.

Comment 18

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

Good catch, thanks. From the layer tree it looks like when the keyboard comes up it goes onto the same ThebesLayer as the system tray bar at the top, and this ends up being a full-screen layer with an alpha region. My patch doesn't check for alpha and so it thinks the entire screen is covered by this layer. I'll have to think about how to fix this, because the things that jump into mind immediately will almost completely remove the usefulness of this patch to begin with.

Comment 19

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

Attachment: WIP v2

This patch should fix the issue you noticed in the wifi entry screen. I'm not sure if it will un-fix the issue you were seeing with your bug 930939 patches though. Could you check?

Comment 20

[Mason Chang [Inactive] [:mchang]]

Hi Kats, just tried with bug 930939. I still have the same issue. I'm also able to still scroll the background.

Comment 21

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

Attachment: Patch

I spent some more time digging through the code history of the touch-sensitive region and mozpasspointerevents. After discussing with Botond I realized that all of that was just a temporary workaround for the keyboard problem in bug 959198. (Incidentally, removing the GetTouchSensitiveRegion code now no longer shows any problems).

Anyway, all that means is this fix is not fully correct, but I think it's still an improvement over what we have now as it seems to fix a few bugs without regressions. I think it it's probably a good idea to land it and see if anything breaks. If nothing else that will help us find edge cases that will be useful when fixing bug 918288 properly.

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8461124 [details] [diff] [review]
Patch

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

LGTM!

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +250,5 @@
>  
> +        // Not sure what rounding option is the most correct here, but if we ever
> +        // figure it out we can change this. For now I'm rounding in to minimize
> +        // the chances of getting a complex region.
> +        ParentLayerIntRect rounded = RoundedIn(visible);

s/rounded/roundedVisible

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +970,5 @@
>  private:
>    /* This is the visible region of the layer that this APZC corresponds to, in
>     * that layer's screen pixels (the same coordinate system in which this APZC
>     * receives events in ReceiveInputEvent()). */
> +  nsIntRegion mVisibleRegion;

Side note: it's unfortunate that we have to move from typed units to untyped units here. I filed bug 1043013.

Comment 23

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

Made the requested change and landed:

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

Comment 24

[Ed Morley [:emorley]]

Backed out for non-unified build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=44605815&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=44606113&tree=Mozilla-Inbound

In file included from /builds/slave/m-in-osx64-nu-0000000000000000/build/gfx/layers/apz/src/APZCTreeManager.cpp:6:
/builds/slave/m-in-osx64-nu-0000000000000000/build/gfx/layers/apz/src/APZCTreeManager.h:367:61: error: unknown type name 'nsIntRegion'
                                                      const nsIntRegion& aObscured);
                                                            ^
/builds/slave/m-in-osx64-nu-0000000000000000/build/gfx/layers/apz/src/APZCTreeManager.cpp:138:5: error: no matching member function for call to 'UpdatePanZoomControllerTree'
    UpdatePanZoomControllerTree(aCompositor,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/slave/m-in-osx64-nu-0000000000000000/build/gfx/layers/apz/src/APZCTreeManager.h:358:27: note: candidate function not viable: no known conversion from 'nsIntRegion' to 'const int' for 11th argument
  AsyncPanZoomController* UpdatePanZoomControllerTree(CompositorParent* aCompositor,
                          ^
/builds/slave/m-in-osx64-nu-0000000000000000/build/gfx/layers/apz/src/APZCTreeManager.cpp:97:18: note: candidate function not viable: requires 5 arguments, but 11 were provided
APZCTreeManager::UpdatePanZoomControllerTree(CompositorParent* aCompositor,
                 ^
/builds/slave/m-in-osx64-nu-0000000000000000/build/gfx/layers/apz/src/APZCTreeManager.cpp:155:18: error: out-of-line definition of 'UpdatePanZoomControllerTree' does not match any declaration in 'mozilla::layers::APZCTreeManager'
APZCTreeManager::UpdatePanZoomControllerTree(CompositorParent* aCompositor,
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
make[6]: *** [APZCTreeManager.o] Error 1

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/42369dee3289

Comment 25

[Botond Ballo [:botond]]

We need to include nsRegion.h from APZCTreeManager.h. Sorry for not spotting that during review.

Comment 26

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
> We need to include nsRegion.h from APZCTreeManager.h. Sorry for not spotting
> that during review.

Actually, a forward-declaration of nsIntRegion should suffice.

Comment 27

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

Re-landed with the forward-declaration

https://hg.mozilla.org/integration/mozilla-inbound/rev/02c5fd1fe559

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02c5fd1fe559

Comment 29

[Shally Li（leave from Mozilla）]

This issue has been successfully verified on Flame 2.1.
See attachment: verified_v2.1.mp4
Reproducing rate: 0/5

Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880

Comment 30

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1.MP4