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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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 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+
(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.
(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.
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+
Also rebased on top of bug 1109873
Attachment #8540897 - Attachment is obsolete: true
Attachment #8540897 - Flags: review?(botond)
Attachment #8544236 - Flags: review?(botond)
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+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: