Closed
Bug 973096
Opened 10 years ago
Closed 10 years ago
[B2G][Messaging] Conversation is scrollable after tapping on Header
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: pbylenga, Assigned: kats)
References
Details
(Whiteboard: [apz_testrun])
Attachments
(3 files, 2 obsolete files)
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•10 years ago
|
||
Weird glitch, but I wouldn't block on it.
Assignee | ||
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
I can definitely wait for bug 969483 to land ;)
Comment 5•10 years ago
|
||
Nope, the patch in bug 969483 does not fix this.
Assignee | ||
Comment 6•10 years ago
|
||
Ok, thanks for checking. I'll look into it eventually when I go through all the other bugs blocking gaia-apzc-2.
Assignee | ||
Comment 7•10 years ago
|
||
Ah, this is because of improper APZ hit testing in the face of transparent layers. This is tracked by bug 918288.
Depends on: 918288
Comment 9•10 years ago
|
||
Without this we don't hit APZ correctness target for 1.4.
blocking-b2g: --- → 1.4+
Comment 10•10 years ago
|
||
Assign to me as a placeholder until the plates of other engineers clear up.
Assignee: nobody → milan
Comment 11•10 years ago
|
||
PM Triage: 1.4+
Comment 12•10 years ago
|
||
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+ → ---
Assignee | ||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 14•10 years ago
|
||
Carrying in flags from dupe.
Blocks: input-thread, vertical-home-next
status-b2g-v1.3:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
Comment 17•10 years ago
|
||
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.
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
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)
Comment 20•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
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 22•10 years ago
|
||
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+
Assignee | ||
Comment 23•10 years ago
|
||
Made the requested change and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a40196da87
Comment 24•10 years ago
|
||
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•10 years ago
|
||
We need to include nsRegion.h from APZCTreeManager.h. Sorry for not spotting that during review.
Comment 26•10 years ago
|
||
(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.
Assignee | ||
Comment 27•10 years ago
|
||
Re-landed with the forward-declaration https://hg.mozilla.org/integration/mozilla-inbound/rev/02c5fd1fe559
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/02c5fd1fe559
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee | ||
Updated•10 years ago
|
Comment 29•10 years ago
|
||
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•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•