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

RESOLVED FIXED in Firefox OS v2.1

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: pbylenga, Assigned: kats)

Tracking

unspecified
mozilla34
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.3 affected, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.1 verified)

Details

(Whiteboard: [apz_testrun])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 8376560 [details]
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
Weird glitch, but I wouldn't block on it.
Blocks: 949585
Component: Gaia::SMS → Panning and Zooming
Product: Firefox OS → Core
z-index issue?
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
I can definitely wait for bug 969483 to land ;)
Nope, the patch in bug 969483 does not fix this.
Ok, thanks for checking. I'll look into it eventually when I go through all the other bugs blocking gaia-apzc-2.
Ah, this is because of improper APZ hit testing in the face of transparent layers. This is tracked by bug 918288.
Depends on: 918288
Duplicate of this bug: 973092
Without this we don't hit APZ correctness target for 1.4.
blocking-b2g: --- → 1.4+
Assign to me as a placeholder until the plates of other engineers clear up.
Assignee: nobody → milan

Comment 11

5 years ago
PM Triage: 1.4+
Uplift if we have a fix, but unless we have a stronger example, go with comment 1 and not block.
Assignee: milan → nobody
blocking-b2g: 1.4+ → ---
Created attachment 8458904 [details] [diff] [review]
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.
Assignee: nobody → bugmail.mozilla
Carrying in flags from dupe.
Blocks: 930939, 1017954
status-b2g-v1.3: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
Duplicate of this bug: 1021413
No longer blocks: 1007735
Duplicate of this bug: 1007735
No longer depends on: 918288
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.
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.
Created attachment 8460359 [details] [diff] [review]
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?
Attachment #8458904 - Attachment is obsolete: true
Flags: needinfo?(mchang)
Hi Kats, just tried with bug 930939. I still have the same issue. I'm also able to still scroll the background.
Flags: needinfo?(mchang)
Created attachment 8461124 [details] [diff] [review]
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.
Attachment #8460359 - Attachment is obsolete: true
Attachment #8461124 - Flags: review?(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.
Attachment #8461124 - Flags: review?(botond) → review+
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
We need to include nsRegion.h from APZCTreeManager.h. Sorry for not spotting that during review.
(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.
https://hg.mozilla.org/mozilla-central/rev/02c5fd1fe559
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
status-b2g-v2.1: affected → fixed
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
status-b2g-v2.1: fixed → verified
You need to log in before you can comment on or make changes to this bug.