898443 - Figure out a good focus model for input events to multi-APZC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

With the patches in bug 866232, I am able to scroll subdocuments (well, iframes anyway) on B2G. However, if my finger goes outside the bounds of the subdocument and onto the parent document, the hit testing code in APZCTreeManager starts delivering the events to the parent document's APZC, and starts scrolling that one instead. This is unexpected from a user's point of view. Instead, the APZCTreeManager should have some notion of focus, and should send the entire block of input events to the same APZC.

Comment 1

[Jason Smith [:jsmith]]

I'm going to pull this off the tracker bug for big moz central + gaia master bugs since I haven't seen this present in our smoketests. However, if this isn't right, feel free to reset the flags.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a parent pointer to APZC

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Extract input transformation calculations

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Use the same APZC until the next touch-start

The MULTITOUCH_START event is sent whenever a new pointer goes down, even if it's not the first one (see http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#177)

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Further extract the input transformation calculations

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Send events to innermost APZC containing all touch points

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 791813 [details] [diff] [review]
Part 2 - Extract input transformation calculations

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

Reluctant r+

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +559,5 @@
> +  aTransformToApzcOut = ancestorUntransform;
> +  // aTransformToScreenOut is initialized to LA.Inverse() * MC * NC * OC
> +  aTransformToScreenOut = asyncUntransform * aApzc->GetAncestorTransform();
> +
> +  for (AsyncPanZoomController* parent = aApzc->GetParent(); parent; parent = parent->GetParent()) {

You're doing quite a bit more computation to calculate this transform with this patch =\

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 791814 [details] [diff] [review]
Part 3 - Use the same APZC until the next touch-start

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +201,5 @@
>    gfx3DMatrix transformToScreen;
>    switch (aEvent.mInputType) {
>      case MULTITOUCH_INPUT: {
>        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
> +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {

I wonder if we should be catching the touch end to make sure we don't keep a stale mAPZC? Particularly on shutdown we might keep the reference to the APZC much longer I think.

@@ +204,5 @@
>        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
> +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> +        apzc = GetTargetAPZC(ScreenPoint(multiTouchInput.mTouches[0].mScreenPoint),
> +                             transformToApzc, transformToScreen);
> +        mApzcForInputBlock = apzc;

We should only use mApzcForInputBlock if it's identical to apzc always.

@@ +272,5 @@
> +        }
> +      }
> +      if (apzc) {
> +        MultiTouchInput inputForApzc(touchEvent);
> +        for (int i = inputForApzc.mTouches.Length() - 1; i >= 0; i--) {

Any reason why this is iterating backwards?

@@ +278,5 @@
> +        }
> +
> +        gfx3DMatrix outTransform = transformToApzc * transformToScreen;
> +        nsTouchEvent* outEvent = static_cast<nsTouchEvent*>(aOutEvent);
> +        for (int i = outEvent->touches.Length() - 1; i >= 0; i--) {

same?

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 792607 [details] [diff] [review]
Part 5 - Send events to innermost APZC containing all touch points

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +270,5 @@
>          apzc = GetTargetAPZC(ScreenPoint::FromUnknownPoint(gfx::Point(point.x, point.y)));
> +        for (size_t i = 1; i < touchEvent.touches.Length(); i++) {
> +          point = touchEvent.touches[i]->mRefPoint;
> +          nsRefPtr<AsyncPanZoomController> apzc2 =
> +            GetTargetAPZC(ScreenPoint::FromUnknownPoint(gfx::Point(point.x, point.y)));

Can't we use ScreenPoint here?

@@ +601,5 @@
>    }
>  }
>  
> +AsyncPanZoomController*
> +APZCTreeManager::CommonAncestor(AsyncPanZoomController* aApzc1, AsyncPanZoomController* aApzc2)

We could come up with an algorithm that has better BigO runtime. Say O(len(Apzc1, CommonAnchestor) + len(Apzc2, CommonAnchestor)) but I don't think it we will run into cases with the ancestor is far from the root of the tree so this will likely perform better.

@@ +618,5 @@
> +  for (AsyncPanZoomController* parent = aApzc2; parent; parent = parent->GetParent()) {
> +    depth2++;
> +  }
> +
> +  int minDepth = depth1 < depth2 ? depth1 : depth2;

// Find the first ancestor that has the same depth for both APZC.

@@ +628,5 @@
> +    depth2--;
> +    aApzc2 = aApzc2->GetParent();
> +  }
> +
> +  while (true) {

// With both pointers at the same depth, recurse up to find the same first common ancestor.

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 792607 [details] [diff] [review]
Part 5 - Send events to innermost APZC containing all touch points

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +604,5 @@
> +AsyncPanZoomController*
> +APZCTreeManager::CommonAncestor(AsyncPanZoomController* aApzc1, AsyncPanZoomController* aApzc2)
> +{
> +  if (aApzc1 == nullptr) {
> +    return aApzc2;

IMO these should return null:
What's the ancestor of Apzc1 and 'Dont Know' => 'Dont Know'

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Benoit Girard (:BenWa) from comment #7)
> You're doing quite a bit more computation to calculate this transform with
> this patch =\

Yeah, we might have to cache some of this math to speed it up.

(In reply to Benoit Girard (:BenWa) from comment #8)
> >        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
> > +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> 
> I wonder if we should be catching the touch end to make sure we don't keep a
> stale mAPZC? Particularly on shutdown we might keep the reference to the
> APZC much longer I think.

I was debating whether or not I should bother. I can put that it if you prefer, but I think the right condition to clear out variable would be "type == CANCEL || (type == MULTITOUCH_END && event.touches.length() == 1)". that way we don't clear it out prematurely if you have two fingers down and lift one of them.

> @@ +204,5 @@
> >        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
> > +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> > +        apzc = GetTargetAPZC(ScreenPoint(multiTouchInput.mTouches[0].mScreenPoint),
> > +                             transformToApzc, transformToScreen);
> > +        mApzcForInputBlock = apzc;
> 
> We should only use mApzcForInputBlock if it's identical to apzc always.

I don't understand this comment. Can you clarify?

> > +        MultiTouchInput inputForApzc(touchEvent);
> > +        for (int i = inputForApzc.mTouches.Length() - 1; i >= 0; i--) {
> 
> Any reason why this is iterating backwards?
> > +        for (int i = outEvent->touches.Length() - 1; i >= 0; i--) {
> 
> same?

Not really. I'll make them iterate forward instead. I default my loops to backwards because it's more efficient but I realize here it's actually bad because the int should be a size_t.

(In reply to Benoit Girard (:BenWa) from comment #9)
> > +          nsRefPtr<AsyncPanZoomController> apzc2 =
> > +            GetTargetAPZC(ScreenPoint::FromUnknownPoint(gfx::Point(point.x, point.y)));
> 
> Can't we use ScreenPoint here?

I guess we can, yes. At the time I wrote the original line of code it wasn't always a ScreenPoint coming in here. (I'm not 100% sure it always is, actually, but going forward it better well be).

> // Find the first ancestor that has the same depth for both APZC.
> 
> // With both pointers at the same depth, recurse up to find the same first
> common ancestor.

I'll add these.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Use the same APZC until the next touch-start (v2)

Updated with review comments

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Further extract the input transformation calculations

Rebased

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Send events to innermost APZC containing all touch points

Addresses review comments. I just took out the early nullptr checks to handle the "Unknown APZC" cases in CommonAncestor because it will fall through the code gracefully anyway.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 6 - Make loop variables a size_t

Decided to just change them all together.

Comment 16

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> (In reply to Benoit Girard (:BenWa) from comment #7)
> > You're doing quite a bit more computation to calculate this transform with
> > this patch =\
> 
> Yeah, we might have to cache some of this math to speed it up.
> 
> (In reply to Benoit Girard (:BenWa) from comment #8)
> > >        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
> > > +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> > 
> > I wonder if we should be catching the touch end to make sure we don't keep a
> > stale mAPZC? Particularly on shutdown we might keep the reference to the
> > APZC much longer I think.
> 
> I was debating whether or not I should bother. I can put that it if you
> prefer, but I think the right condition to clear out variable would be "type
> == CANCEL || (type == MULTITOUCH_END && event.touches.length() == 1)". that
> way we don't clear it out prematurely if you have two fingers down and lift
> one of them.

Right, debouncing is fine.

> 
> > @@ +204,5 @@
> > >        const MultiTouchInput& multiTouchInput = aEvent.AsMultiTouchInput();
> > > +      if (multiTouchInput.mType == MultiTouchInput::MULTITOUCH_START) {
> > > +        apzc = GetTargetAPZC(ScreenPoint(multiTouchInput.mTouches[0].mScreenPoint),
> > > +                             transformToApzc, transformToScreen);
> > > +        mApzcForInputBlock = apzc;
> > 
> > We should only use mApzcForInputBlock if it's identical to apzc always.
> 
> I don't understand this comment. Can you clarify?

Right now you have both 'apzc' and 'mApzcForInputBlock'. They are both always the same (at least from the point of view of this hunk. If possible you should remove 'apzc' which will avoid a needless add/release ref but mostly make the code easier to understand.
> 
> > > +        MultiTouchInput inputForApzc(touchEvent);
> > > +        for (int i = inputForApzc.mTouches.Length() - 1; i >= 0; i--) {
> > 
> > Any reason why this is iterating backwards?
> > > +        for (int i = outEvent->touches.Length() - 1; i >= 0; i--) {
> > 
> > same?
> 
> Not really. I'll make them iterate forward instead. I default my loops to
> backwards because it's more efficient but I realize here it's actually bad
> because the int should be a size_t.

Talked about this on IRC. Normally people iterate backwards for a reason so I was wondering what that was.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Use the same APZC until the next touch-start (v3)

Updated to not have apzc and mApzcForInputBlock both in use, as per review comments. Carrying r+

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 793045 [details] [diff] [review]
Part 6 - Make loop variables a size_t

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +71,5 @@
>                                  aIsFirstPaint, aFirstPaintLayersId,
>                                  &apzcsToDestroy);
>    }
>  
> +  for (size_t i = 0; i < apzcsToDestroy.Length(); i++) {

Note that all those Length()s are uint32_ts, not size_ts.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to :Ms2ger from comment #18)
> Note that all those Length()s are uint32_ts, not size_ts.

True. The unsigned part is the important bit with respect to my loop condition, though.

Try run is clear: https://tbpl.mozilla.org/?tree=Try&rev=59d3fe2a7945, landed on b2g-inbound:
https://hg.mozilla.org/integration/b2g-inbound/rev/4010fb95eace
https://hg.mozilla.org/integration/b2g-inbound/rev/5b560d547e07
https://hg.mozilla.org/integration/b2g-inbound/rev/6b1ab9364062
https://hg.mozilla.org/integration/b2g-inbound/rev/8ba0e4588b64
https://hg.mozilla.org/integration/b2g-inbound/rev/3818dcc57d74
https://hg.mozilla.org/integration/b2g-inbound/rev/baaa2bb6a192

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4010fb95eace
https://hg.mozilla.org/mozilla-central/rev/5b560d547e07
https://hg.mozilla.org/mozilla-central/rev/6b1ab9364062
https://hg.mozilla.org/mozilla-central/rev/8ba0e4588b64
https://hg.mozilla.org/mozilla-central/rev/3818dcc57d74
https://hg.mozilla.org/mozilla-central/rev/baaa2bb6a192