Closed Bug 898443 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

26 Branch
All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
blocking-b2g koi+
Tracking Status
firefox26 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(6 files, 4 obsolete files)

2.44 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
9.05 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
26.76 KB, patch
kats
: review+
Details | Diff | Splinter Review
6.17 KB, patch
kats
: review+
Details | Diff | Splinter Review
3.96 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
9.28 KB, patch
kats
: review+
Details | Diff | Splinter Review
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.
QA Contact: bgirard
Assignee: nobody → bgirard
QA Contact: bgirard
blocking-b2g: --- → koi?
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.
No longer blocks: b2g-central-dogfood
Keywords: smoketest
Assignee: bgirard → bugmail.mozilla
Version: 23 Branch → 26 Branch
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)
Attachment #791814 - Flags: review?(bgirard)
Attachment #791812 - Flags: review?(bgirard) → review+
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 =\
Attachment #791813 - Flags: review?(bgirard) → review+
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?
Attachment #792606 - Flags: review?(bgirard) → review+
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.
Attachment #792607 - Flags: review?(bgirard) → review+
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'
(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.
Updated with review comments
Attachment #791814 - Attachment is obsolete: true
Attachment #791814 - Flags: review?(bgirard)
Attachment #793041 - Flags: review?(bgirard)
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.
Attachment #792606 - Attachment is obsolete: true
Attachment #792607 - Attachment is obsolete: true
Attachment #793044 - Flags: review+
Decided to just change them all together.
Attachment #793045 - Flags: review?(bgirard)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #793041 - Flags: review?(bgirard) → review+
Attachment #793045 - Flags: review?(bgirard) → review+
Updated to not have apzc and mApzcForInputBlock both in use, as per review comments. Carrying r+
Attachment #793041 - Attachment is obsolete: true
Attachment #793469 - Flags: review+
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.
blocking-b2g: koi? → koi+
You need to log in before you can comment on or make changes to this bug.