Closed
Bug 1115111
Opened 9 years ago
Closed 9 years ago
Ensure event regions based hit-testing is robust in multiprocess scenarios
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(2 files, 2 obsolete files)
2.97 KB,
patch
|
kats
:
review+
kats
:
checkin-
|
Details | Diff | Splinter Review |
8.68 KB,
patch
|
botond
:
review+
kats
:
checkin-
|
Details | Diff | Splinter Review |
A bug to track the things in the event regions code that need to be fixed up that block bug 920036. I've found a couple of minor issues so far but I figured they deserve a separate bug.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8540897 -
Flags: review?(botond)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8540899 -
Flags: review?(botond)
Comment 3•9 years ago
|
||
Comment on attachment 8540897 [details] [diff] [review] Part 1 - Fix obscuration computation Review of attachment 8540897 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I'll hold off on the r+ until we clear up the GetTouchSensitiveRegion() vs. GetVisibleRegion() thing. ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +518,5 @@ > ancestorTransform, aParent, next, > obscured); > > + // Each layer and its subtree obscures its previous siblings, so we augment > + // the obscured region as we loop backwards through the children. Ooh, nice catch! @@ +526,2 @@ > } else { > childRegion = child.GetVisibleRegion(); Not necessarily related to this patch, but in PrepareAPZCForLayer we use the result of GetTouchSensitiveRegion() as the hit-region when event-regions are disabled, but here we use GetVisibleRegion(). Is there a reason for this discrepancy?
Comment 4•9 years ago
|
||
Comment on attachment 8540899 [details] [diff] [review] Part 2 - Carry obscured regions across process boundaries Review of attachment 8540899 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +487,5 @@ > + // where the B2G notification bar and the keyboard get merged into a single > + // layer with a large visible region that obscures all child processes, even > + // though visually they do not. We'd probably have to check for mask layers > + // and so on in order to properly handle that case, and event regions are > + // supposed to take care of that for us. Just to make sure I understand: in the problematic case, the merged notificaticationbar+keyboard layer has a visible region that obscures child processes, but its hit region does not, making the scenario problematic only when event-regions are disabled?
Attachment #8540899 -
Flags: review?(botond) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #3) > Not necessarily related to this patch, but in PrepareAPZCForLayer we use the > result of GetTouchSensitiveRegion() as the hit-region when event-regions are > disabled, but here we use GetVisibleRegion(). Is there a reason for this > discrepancy? I think the main reason for this difference is that the area returned by GetTouchSensitiveRegion() is intersected with the "touch-sensitive region" from the GeckoContentController. Conceptually this means there's some part of the layer that's "visible" (i.e in the VisibleRegion) but doesn't accept touch input. This part of the layer should still be obscuring stuff under it, because it's visible. Honestly though I'm looking forward to throwing out all of this GetTouchSensitiveRegion stuff and making the event regions code do all the work. I think we should be able to do that now but I still need to test it. I also want to make sure we won't flip the event-regions pref back to false before I start ripping out code.
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #4) > Just to make sure I understand: in the problematic case, the merged > notificaticationbar+keyboard layer has a visible region that obscures child > processes, but its hit region does not, making the scenario problematic only > when event-regions are disabled? Yup, exactly.
Assignee | ||
Comment 7•9 years ago
|
||
Rebased on top of latest patches in bug 1109873, carrying r+ since the code is functionally the same but the comments have been updated.
Attachment #8540899 -
Attachment is obsolete: true
Attachment #8544189 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
Also rebased on top of bug 1109873
Attachment #8540897 -
Attachment is obsolete: true
Attachment #8540897 -
Flags: review?(botond)
Attachment #8544236 -
Flags: review?(botond)
Comment 9•9 years ago
|
||
Comment on attachment 8544236 [details] [diff] [review] Part 1 - Fix obscuration computation Review of attachment 8544236 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > (In reply to Botond Ballo [:botond] from comment #3) > > Not necessarily related to this patch, but in PrepareAPZCForLayer we use the > > result of GetTouchSensitiveRegion() as the hit-region when event-regions are > > disabled, but here we use GetVisibleRegion(). Is there a reason for this > > discrepancy? > > I think the main reason for this difference is that the area returned by > GetTouchSensitiveRegion() is intersected with the "touch-sensitive region" > from the GeckoContentController. Conceptually this means there's some part > of the layer that's "visible" (i.e in the VisibleRegion) but doesn't accept > touch input. This part of the layer should still be obscuring stuff under > it, because it's visible. Makes sense, thanks for the explanation!
Attachment #8544236 -
Flags: review?(botond) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Both of these patches are obsoleted by the "part 5" patches I posted to bug 1109873.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Assignee | ||
Updated•9 years ago
|
Attachment #8544189 -
Flags: checkin-
Assignee | ||
Updated•9 years ago
|
Attachment #8544236 -
Flags: checkin-
You need to log in
before you can comment on or make changes to this bug.
Description
•