845169 - Target events "properly" at overlapping app frames

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Some keyboards want to have their frames extend over content in order to show transient UI like suggestions.  In v1, we handle this by "normal" DOM dispatch in the master process.  When overlapping apps are all OOP, that doesn't work.

I propose we solve this problem by separating targeting of event series into these steps.

 Step 1: Run normal DOM dispatch in the system app.  System can consume the series; if so, done.

 Step 2: If a <browser> A (i.e. remote) is targeted and it doesn't overlap any other <browser> A, let A's TabParent capture the series.  Done.

 Step 3: If A overlaps B, send an AmbiguousEventStart() message to A.  If A preventDefault()s the touchstart, send a reply message and set A's TabParent to capture the series.  Done.

 Step 4: Let B's TabParent capture the series.  Done.

I don't really want to think about more than two overlapping frames ... but we could extend the algorithm to handle that.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we need to do DOM event dispatch in more than one process. In step 3 we can rely on CSS 'pointer-events' to determine which process contains the targeted element. I hope.

Still, this seems rather hairy since in step 3 the parent process waits for a reply and anything can happen, including a new touch event.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> I don't think we need to do DOM event dispatch in more than one process.

I think I agree ... Note that as long as the system app is HTML, we need to do a full dispatch there, but /only on touchstart/.  After we determine the target (if it's in another process), events are fast-tracked forwarded to it, as soon as we possibly can [1].  (This is a hack/tradeoff that happened to become the new spec for security reasons :D.)

For touchstart, we could end up doing up to 3 full DOM dispatches, but just for that one event.  I don't see a way around that.

> In
> step 3 we can rely on CSS 'pointer-events' to determine which process
> contains the targeted element. I hope.

Hm, I don't see how that would work.  But I'm not sure I understand what you're suggesting.

> Still, this seems rather hairy since in step 3 the parent process waits for
> a reply and anything can happen, including a new touch event.

Yep, we'll need to queue.  But I also don't see a way around that.  Suggestions welcome :).

[1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#293

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> > In step 3 we can rely on CSS 'pointer-events' to determine which process
> > contains the targeted element. I hope.
> 
> Hm, I don't see how that would work.  But I'm not sure I understand what
> you're suggesting.

Determining which element to dispatch to depends only on CSS pointer-events and the current layout. Firing a mouse event in a single process does:
1) Determine destination element/frame from the input point, taking into account frame geometry and CSS pointer-events
2) Dispatch DOM event to destination element and its ancestors
3) Do some default processing if preventDefault() was not called
There is no way for any JS processing in step 2 to make an element "transparent to events" in step 1, in the single-process case. I don't think there needs to be for the multi-process case either, assuming that we want the multi-process case to behave as much like the single-process case as possible.

Anyway, I had a different idea on my run home. How about for <browser>s with mozpasspointerevents, each time we update the layer tree we also build an event-targeting display list for the entire browser area and from that, compute a "hit-target region" comprising the parts of the viewport which are not transparent to events, and forward that region to the master process with the layer tree update? Then the master process can determine which process gets the event without doing any IPC. We'd not need to do this for <browser>s without mozpasspointerevents, since they always accept pointer events anywhere in their rectangle.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> There is no way for any JS processing in step 2 to make an element
> "transparent to events" in step 1, in the single-process case. I don't think
> there needs to be for the multi-process case either, assuming that we want
> the multi-process case to behave as much like the single-process case as
> possible.

I'm still not quite understanding what's being discussed, which probably means I'm still misunderstanding :).  I'll try to catch you on IRC later.

> Anyway, I had a different idea on my run home. How about for <browser>s with
> mozpasspointerevents, each time we update the layer tree we also build an
> event-targeting display list for the entire browser area and from that,
> compute a "hit-target region" comprising the parts of the viewport which are
> not transparent to events, and forward that region to the master process
> with the layer tree update? Then the master process can determine which
> process gets the event without doing any IPC. We'd not need to do this for
> <browser>s without mozpasspointerevents, since they always accept pointer
> events anywhere in their rectangle.

That was my first idea, but there are some problems
 - you indicated a prior unwilligness to start down the path of forwarding frame trees across processes ;)
 - layer trees live on the compositor thread, but we start event dispatch from the "content thread"
 - opens up a race condition in which event series are visually mistargeted, which could be a security issue.  Haven't thought that through much though.

We'll need to do something related for bug 775452, but there we can directly use the layer tree for hit testing because the scroll frames will correlate 1:1 with apzcs, and we don't have to worry about the passpointer complication.  We've also built all the necessary machinery for shipping events off to the compositor.  (We'll also be vulnerable to mistargeting event series there, but only within the same "window", not cross-window.)

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> That was my first idea, but there are some problems

Another problem is that this would interact badly with empty-transaction transforms.  We'd have to either deoptimize or write some fairly gross direct-update code.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> That was my first idea, but there are some problems
>  - you indicated a prior unwilligness to start down the path of forwarding
> frame trees across processes ;)

This isn't that --- all we need is a region.

>  - layer trees live on the compositor thread, but we start event dispatch
> from the "content thread"
>  - opens up a race condition in which event series are visually mistargeted,
> which could be a security issue.  Haven't thought that through much though.
> Another problem is that this would interact badly with empty-transaction
> transforms.  We'd have to either deoptimize or write some fairly gross
> direct-update code.

As long as this only needs to apply to <browser mozpasspointerevents>, i.e. keyboard apps, these seem minor.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In that scheme, we also need to animate the hit targets asynchronously on the compositor, because animated key strikes are exactly what gaia-keyboard tosses into the pointer-passing space.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

*we would also

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That would be a problem, but they don't seem to be animated to me. At least not when I'm able to touch them.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > That was my first idea, but there are some problems
> >  - you indicated a prior unwilligness to start down the path of forwarding
> > frame trees across processes ;)
> 
> This isn't that --- all we need is a region.

But can we do such region fast enough? There can be plenty of frames in a region.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see why not. For the existing keyboard app the region is pretty simple.

Comment 12

[Thinker Li [:sinker]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> compute a "hit-target region" comprising the parts of the viewport which are
> not transparent to events, and forward that region to the master process
> with the layer tree update? Then the master process can determine which

Maybe can check alpha channel of the corresponding pixel instead of forwarding "hit-target region".  Considering animation, it should be more easy than forwarding "hit-target region".

The potential issue of the alpha channel way is synchronization with compositor thread.  I think we can find the target at compositor thread.  Touch events can be aggregated and dispatching  with some additional delay, so it does not add much load to the compositor thread.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Thinker Li [:sinker] from comment #12)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> > compute a "hit-target region" comprising the parts of the viewport which are
> > not transparent to events, and forward that region to the master process
> > with the layer tree update? Then the master process can determine which
> 
> Maybe can check alpha channel of the corresponding pixel instead of
> forwarding "hit-target region".  Considering animation, it should be more
> easy than forwarding "hit-target region".

That would break pages that use elements that are transparent but handle events, or elements that are opaque and don't handle events (both are easily possible with CSS).

Comment 14

[Thinker Li [:sinker]]

Sure!  We also check if mozpasspointerevents is enabled.  So, that attribute should be with the layer when updating layer tree.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch

Send the touchable region to RenderFrameParent after each composition.

Comment 16

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 782437 [details] [diff] [review]
WIP patch

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

::: layout/base/nsDisplayList.cpp
@@ +1376,4 @@
>        for (uint32_t j = 0; j < outFrames.Length(); j++) {
>          nsIFrame *f = outFrames.ElementAt(j);
>          // Handle the XUL 'mousethrough' feature and 'pointer-events'.
> +        if (!GetMouseThrough(f) && IsFrameReceivingPointerEvents(f)) {

Hmm.. the frame matched by nsDisplayBackgroundColor also pass this. Need a way to distinguish this.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kan-Ru Chen [:kanru] from comment #16)
> Hmm.. the frame matched by nsDisplayBackgroundColor also pass this. Need a
> way to distinguish this.

I'm not sure what you mean.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 782437 [details] [diff] [review]
WIP patch

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

::: layout/base/nsDisplayList.h
@@ +827,5 @@
> +  {
> +    bool snap;
> +    nsRect bounds = GetBounds(aBuilder, &snap);
> +    aOutRegion->Or(*aOutRegion, bounds);
> +  }

Instead of adding this new method, can we just call HitTest to return a list of all event target frames, and then build the region by walking through the frames and calling nsLayoutUtils::TransformFrameRectToAncestor on each one to get a rectangle relative to the TabChild's root frame?

::: layout/base/nsPresShell.cpp
@@ +5489,5 @@
> +  {
> +    if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +      if (!mFrame || !mShell)
> +        return;
> +      if (TabChild* tabChild = GetTabChildFrom(mShell)) {

This should be limited to content processes for which the parent mozbrowser has mozpasspointerevents set. Otherwise it's just unnecessary overhead. I guess you'll need TabParent to pass that information to the TabChild.

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch 2

Use a separate array to store the remote frames. Now this patch works with the OOP keyboard.

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

This patch is just a proof of concept so many parts may not be the optimal way do it.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Comment on attachment 782437 [details] [diff] [review]
> WIP patch
> 
> Review of attachment 782437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.h
> @@ +827,5 @@
> > +  {
> > +    bool snap;
> > +    nsRect bounds = GetBounds(aBuilder, &snap);
> > +    aOutRegion->Or(*aOutRegion, bounds);
> > +  }
> 
> Instead of adding this new method, can we just call HitTest to return a list
> of all event target frames, and then build the region by walking through the
> frames and calling nsLayoutUtils::TransformFrameRectToAncestor on each one
> to get a rectangle relative to the TabChild's root frame?

Do you mean to use a root-frame-size aRect for HitTest? It's likely to return most frames so why traverse the frames list twice? 

> ::: layout/base/nsPresShell.cpp
> @@ +5489,5 @@
> > +  {
> > +    if (XRE_GetProcessType() == GeckoProcessType_Content) {
> > +      if (!mFrame || !mShell)
> > +        return;
> > +      if (TabChild* tabChild = GetTabChildFrom(mShell)) {
> 
> This should be limited to content processes for which the parent mozbrowser
> has mozpasspointerevents set. Otherwise it's just unnecessary overhead. I
> guess you'll need TabParent to pass that information to the TabChild.

Yes, the check will be added later.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kan-Ru Chen [:kanru] from comment #20)
> Do you mean to use a root-frame-size aRect for HitTest? It's likely to
> return most frames so why traverse the frames list twice?

I don't think that will be a problem in practice.

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> (In reply to Kan-Ru Chen [:kanru] from comment #20)
> > Do you mean to use a root-frame-size aRect for HitTest? It's likely to
> > return most frames so why traverse the frames list twice?
> 
> I don't think that will be a problem in practice.

okay, that works. Another update is coming soon.

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Update touch region from remote frame

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 784177 [details] [diff] [review]
Update touch region from remote frame

Now checks the presence of mozpasspointerevents. The region update isn't disabled if mozpasspointerevents was set to false later. It'll be very rare though.

Another problem is the iframe element might receive the TOUCH_START event even it is pointer-events:none. I think we have to check and dispatch to remote only.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 784177 [details] [diff] [review]
Update touch region from remote frame

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

Looks pretty good! Does it work well?

::: layout/base/nsPresShell.cpp
@@ +5489,5 @@
> +  {
> +    if (XRE_GetProcessType() == GeckoProcessType_Content) {
> +      if (!mFrame || !mShell) {
> +        return;
> +      }

Better to just write
  if (XRE_GetProcessType() != GeckoProcessType_Content ||
      !mFrame || !mShell) {
    return;
  }

@@ +5492,5 @@
> +        return;
> +      }
> +      TabChild* tabChild = GetTabChildFrom(mShell);
> +      if (tabChild && tabChild->GetUpdateTouchRegion()) {
> +        nsRegion region;

And here,
  if (!tabChild || !tabChild->GetUpdateTouchRegion()) {
    return;
  }

::: layout/ipc/RenderFrameParent.cpp
@@ +1019,5 @@
> +nsDisplayRemote::HitTest(nsDisplayListBuilder* aBuilder, const nsRect& aRect,
> +                         HitTestState* aState, nsTArray<nsIFrame*> *aOutFrames)
> +{
> +  if (mRemoteFrame->HitTest(aRect)) {
> +    aState->mRemoteFrames.AppendElement(mFrame);

I don't understand this. Why aren't we just adding the frame to aOutFrames?

::: layout/ipc/RenderFrameParent.h
@@ +114,4 @@
>    virtual bool RecvCancelDefaultPanZoom() MOZ_OVERRIDE;
>    virtual bool RecvDetectScrollableSubframe() MOZ_OVERRIDE;
>  
> +  virtual bool RecvUpdateTouchRegion(const nsRegion& aRegion) MOZ_OVERRIDE;

Let's call this RecvUpdateHitRegion.

@@ +162,5 @@
>    bool mFrameLoaderDestroyed;
>    // this is gfxRGBA because that's what ColorLayer wants.
>    gfxRGBA mBackgroundColor;
> +
> +  nsRegion mTouchRegion;

Let's call it mHitRegion. This could be used for mouse events too (on desktop).

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 784177 [details] [diff] [review]
Update touch region from remote frame

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

Yes! It works pretty well.

::: layout/base/nsDisplayList.cpp
@@ +1381,5 @@
> +          nsIFrame *f = outFrames.ElementAt(j);
> +          // Handle the XUL 'mousethrough' feature and 'pointer-events'.
> +          if (!GetMouseThrough(f) && IsFrameReceivingPointerEvents(f)) {
> +            writeFrames->AppendElement(f);
> +          }

Here.

::: layout/ipc/RenderFrameParent.cpp
@@ +1019,5 @@
> +nsDisplayRemote::HitTest(nsDisplayListBuilder* aBuilder, const nsRect& aRect,
> +                         HitTestState* aState, nsTArray<nsIFrame*> *aOutFrames)
> +{
> +  if (mRemoteFrame->HitTest(aRect)) {
> +    aState->mRemoteFrames.AppendElement(mFrame);

Because we remove frames that are pointer-events:none from aOutFrames. There is no way to distinguish them.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kan-Ru Chen [:kanru] from comment #26)
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +1019,5 @@
> > +nsDisplayRemote::HitTest(nsDisplayListBuilder* aBuilder, const nsRect& aRect,
> > +                         HitTestState* aState, nsTArray<nsIFrame*> *aOutFrames)
> > +{
> > +  if (mRemoteFrame->HitTest(aRect)) {
> > +    aState->mRemoteFrames.AppendElement(mFrame);
> 
> Because we remove frames that are pointer-events:none from aOutFrames. There
> is no way to distinguish them.

How about adding extra logic around the check of GetEffectivePointerEvents?

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Update touch region from remote frame v2

Address review comments.

Don't add BackgroundColor to list if we are pointer-events:none.
Before patch the display list looks like:
OwnLayer 0x44ad5298(Viewport(-1))
  nsDisplayTransform 0x48632860(Block(div)(42))
    WrapList 0x48632a88(FrameOuter(iframe src=app://keyboard.gaiamobile.org/index.html)(0))
      Remote 0x48632a88(FrameOuter(iframe src=app://keyboard.gaiamobile.org/index.html)(0))
      BackgroundColor 0x48632a88(FrameOuter(iframe src=app://keyboard.gaiamobile.org/index.html)(0))

After patch it's
OwnLayer 0x44ad5298(Viewport(-1))
  nsDisplayTransform 0x48632860(Block(div)(42))
    WrapList 0x48632a88(FrameOuter(iframe src=app://keyboard.gaiamobile.org/index.html)(0))
      Remote 0x48632a88(FrameOuter(iframe src=app://keyboard.gaiamobile.org/index.html)(0))

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 784900 [details] [diff] [review]
Update touch region from remote frame v2

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

This needs a test!

Comment 30

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: 0001-Bug-845169-Add-test-case.-r-roc.patch

--- a/layout/ipc/RenderFrameParent.cpp
+++ b/layout/ipc/RenderFrameParent.cpp
@@ -626,2 +626,4 @@ RenderFrameParent::RenderFrameParent(nsFrameLoader* aFrameLoader,
   }
+  // Set a default RenderFrameParent
+  mFrameLoader->SetCurrentRemoteFrame(this);
 }

I need to set the RenderFrameParent in order to run the testcase on the Desktop browser. Is this OK?

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 787391 [details] [diff] [review]
0001-Bug-845169-Add-test-case.-r-roc.patch

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

::: layout/base/tests/test_remote_passpointerevents.html
@@ +21,5 @@
> +
> +        document.body.appendChild(iframe);
> +
> +        iframe.addNextPaintListener(function() {
> +          window.setTimeout(function(){

Need this timeout because we don't know when will the async UpdateHitRegion finish.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 787391 [details] [diff] [review]
0001-Bug-845169-Add-test-case.-r-roc.patch

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

::: layout/base/tests/test_remote_passpointerevents.html
@@ +21,5 @@
> +
> +        document.body.appendChild(iframe);
> +
> +        iframe.addNextPaintListener(function() {
> +          window.setTimeout(function(){

Instead of using a single setTimeout I think you should poll frequently until the conditions you expect become true.

::: layout/ipc/RenderFrameParent.cpp
@@ +624,5 @@
>        CompositorParent::SetControllerForLayerTree(mLayersId, mContentController);
>      }
>    }
> +  // Set a default RenderFrameParent
> +  mFrameLoader->SetCurrentRemoteFrame(this);

I don't understand why this is needed. Can you explain it?

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

> ::: layout/ipc/RenderFrameParent.cpp
> @@ +624,5 @@
> >        CompositorParent::SetControllerForLayerTree(mLayersId, mContentController);
> >      }
> >    }
> > +  // Set a default RenderFrameParent
> > +  mFrameLoader->SetCurrentRemoteFrame(this);
> 
> I don't understand why this is needed. Can you explain it?

On desktop we never repaint the remote frame so the frame loader does not get a RenderFrameParent.

This is the only place we set a RenderFrameParent to it.
http://dxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#l917

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 787391 [details] [diff] [review]
0001-Bug-845169-Add-test-case.-r-roc.patch

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

I still think you should fix the setTimeout issue.

Comment 35

[Ivan Tsay (:ITsay)]

Since this block the keyboard OOP, flag it to koi+. Also add keyword in whiteboard for EPM tracking and wiki site: https://wiki.mozilla.org/FirefoxOS/SprintStatus#Systems-Platform

Comment 36

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add test case v2

Address review comment.

Comment 37

[Kan-Ru Chen [:kanru] (UTC+9)]

https://tbpl.mozilla.org/?tree=Try&rev=644ea704b8cc

Comment 38

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/ce94c5f1ad3f
https://hg.mozilla.org/integration/b2g-inbound/rev/7bb10d5dfca2

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce94c5f1ad3f
https://hg.mozilla.org/mozilla-central/rev/7bb10d5dfca2