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

RESOLVED FIXED in Firefox 26

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

({regression})

26 Branch
mozilla26
All
Gonk (Firefox OS)
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed)

Details

Attachments

(6 attachments, 4 obsolete attachments)

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.
Blocks: 901339
Blocks: 884399

Updated

4 years ago
QA Contact: bgirard
Assignee: nobody → bgirard
QA Contact: bgirard
Blocks: 886321

Updated

4 years ago
blocking-b2g: --- → koi?

Updated

4 years ago
Keywords: regression, smoketest
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: 884399
Keywords: smoketest
Assignee: bgirard → bugmail.mozilla
Version: 23 Branch → 26 Branch
Created attachment 791812 [details] [diff] [review]
Part 1 - Add a parent pointer to APZC
Attachment #791812 - Flags: review?(bgirard)
Created attachment 791813 [details] [diff] [review]
Part 2 - Extract input transformation calculations
Attachment #791813 - Flags: review?(bgirard)
Created attachment 791814 [details] [diff] [review]
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)
Attachment #791814 - Flags: review?(bgirard)
Created attachment 792606 [details] [diff] [review]
Part 4 - Further extract the input transformation calculations
Attachment #792606 - Flags: review?(bgirard)
Created attachment 792607 [details] [diff] [review]
Part 5 - Send events to innermost APZC containing all touch points
Attachment #792607 - Flags: review?(bgirard)

Updated

4 years ago
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?

Updated

4 years ago
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.
Created attachment 793041 [details] [diff] [review]
Part 3 - Use the same APZC until the next touch-start (v2)

Updated with review comments
Attachment #791814 - Attachment is obsolete: true
Attachment #791814 - Flags: review?(bgirard)
Attachment #793041 - Flags: review?(bgirard)
Created attachment 793043 [details] [diff] [review]
Part 4 - Further extract the input transformation calculations

Rebased
Attachment #793043 - Flags: review+
Created attachment 793044 [details] [diff] [review]
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.
Attachment #792606 - Attachment is obsolete: true
Attachment #792607 - Attachment is obsolete: true
Attachment #793044 - Flags: review+
Created attachment 793045 [details] [diff] [review]
Part 6 - Make loop variables a size_t

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.

Updated

4 years ago
Attachment #793041 - Flags: review?(bgirard) → review+

Updated

4 years ago
Attachment #793045 - Flags: review?(bgirard) → review+
Created attachment 793469 [details] [diff] [review]
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+
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.
(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
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 907977
blocking-b2g: koi? → koi+
status-firefox26: --- → fixed
You need to log in before you can comment on or make changes to this bug.