932525 - APZC hit testing uses the wrong coordinate space

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

APZCTreeManager::GetAPZCAtPoint() does hit testing for a layer L by converting the point to be tested to L's layer space and then seeing whether that point is contained within L's visible rect.

L's visible rect has type LayerRect, and is computed in APZCTreeManager::UpdatePanZoomControllerTree() by multiplying the layer's composition bounds (which is a ScreenRect) by ScreenToLayerScale(1.0).

I believe this is incorrect because at the time of hit testing, there may be an async transform on L (which is taken into account when converting to L's layer space), but the visible rect doesn't take this async transform into account.

For example, suppose we have a scroll frame whose composition bounds are [0, 500] screen pixels vertically, but the scrolled content is longer (say, 1000 pixels). Suppose I scroll up, inducing an async transform of -200 pixels on L, and then before a repaint occurs (so when the async transform is still in effect), I put my finger down again at screen coordinate 400. Hit testing applied the inverse of L's async transform to the 400, yielding 600 in L's layer space, and the hit test fails since that's outside of [0, 500] - which is not what we want.

I think the visible rect should instead be a ScreenRect (in L's screen space), and hit testing should convert the test point to L's screen space, not L's layer space.

Comment 1

[Botond Ballo [:botond]]

Attachment: WIP (untested)

Untested WIP patch.

Comment 2

[Botond Ballo [:botond]]

Attachment: Patch + test

Updated patch to include a test. (This ended up being a bit more involved than I thought, because it turned out that the existing APZCTreeManager.GetAPZCAtPoint test wasn't really testing the things we wanted it to test.)

Comment 3

[Botond Ballo [:botond]]

Attachment: Rename 'transformToScreen' to 'transformToGecko'

I think 'aTransformToScreenOut' is a poor name for the third parameter of APZCTreeManager::GetInputTransforms(), because that matrix does not transform anything "to screen" - rather it transforms coordinates into the the coordinate space that Gecko expects, which is layer space.

The attached patch renames this parameter to 'aTransformToGeckoOut', and renames variables that derive their name from it accordingly.

Comment 4

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

Comment on attachment 825441 [details] [diff] [review]
Rename 'transformToScreen' to 'transformToGecko'

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +518,5 @@
>  }
>  
>  static already_AddRefed<AsyncPanZoomController>
>  GetTargetAPZC(APZCTreeManager* manager, const ScreenPoint& aPoint,
> +              gfx3DMatrix& aTransformToApzcOut, gfx3DMatrix& aTransformToGreenOut)

s/Green/Gecko/

@@ +523,4 @@
>  {
>    nsRefPtr<AsyncPanZoomController> hit = manager->GetTargetAPZC(aPoint);
>    if (hit) {
> +    manager->GetInputTransforms(hit.get(), aTransformToApzcOut, aTransformToGreenOut);

Ditto

Comment 5

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

Comment on attachment 825433 [details] [diff] [review]
Patch + test

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +654,5 @@
>    gfx3DMatrix ancestorUntransform = aApzc->GetAncestorTransform().Inverse();
> +
> +  // Hit testing for this layer is performed in aApzc's screen coordinates.
> +  gfxPoint hitTestPointForThisLayer = ancestorUntransform.ProjectPoint(aHitTestPoint);
> +  APZC_LOG("Untransformed %f %f to screen coordinates %f %f for APZC %p\n",

s/for APZC/for hit-testing APZC/

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +472,3 @@
>      gfx3DMatrix(),
>      gfx3DMatrix(),
>      gfx3DMatrix(),

You only need 4 transforms here, not 5.

@@ +497,5 @@
> +  metrics.mScrollId = aScrollId;
> +  nsIntRect layerBound = aLayer->GetVisibleRegion().GetBounds();
> +  metrics.mCompositionBounds = ScreenIntRect(layerBound.x, layerBound.y,
> +                                             layerBound.width, layerBound.height);
> +  metrics.mScrollableRect = aScrollableRect;

Some duplicated code here that you should be able to refactor.

@@ +551,5 @@
>    EXPECT_EQ(gfx3DMatrix(), transformToScreen);
>  
>    // Now we have a root APZC that will match the page
> +  SetScrollableFrameMetrics(root, FrameMetrics::ROOT_SCROLL_ID);
> +  manager->UpdatePanZoomControllerTree(nullptr, root, false, 0);

Huh. I'm surprised the compiler didn't warn about this before. Or maybe it did and I didn't see it?

@@ +626,5 @@
> +  SetScrollableFrameMetrics(layers[1], FrameMetrics::START_SCROLL_ID, CSSRect(0, 0, 80, 80));
> +  SetScrollableFrameMetrics(layers[3], FrameMetrics::START_SCROLL_ID + 1, CSSRect(0, 0, 80, 80));
> +
> +  manager->UpdatePanZoomControllerTree(nullptr, root, false, 0);
> +

If my calculations are correct, then at this point the following holds:

layers[0] has content from (0,0)-(200,200) clipped by bounds (0,0)-(100,100)
layers[1] has content from (10,10)-(90,90) clipped by bounds (10,10)-(50,50)
layers[2] has content from (20,60)-(100,100) clipped by bounds (10,60)-(50,100)
layers[3] has content from (20,60)-(180,140) clipped by bounds (20,60)-(100,100)

If this is correct please add it as a comment here.

@@ +640,5 @@
> +
> +  // Hit an area on the root that would be on layers[3] if layers[2]
> +  // weren't transformed.
> +  hit = GetTargetAPZC(manager, ScreenPoint(15, 75), transformToApzc, transformToScreen);
> +  EXPECT_EQ(apzcroot, hit.get());

Update the comment here to say that if layers[2] had an APZC this would hit that instead of the root, since this point is inside the composition bounds for layers[2]. Took me forever to realize why it wasn't hitting layers[2].

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> >  static already_AddRefed<AsyncPanZoomController>
> >  GetTargetAPZC(APZCTreeManager* manager, const ScreenPoint& aPoint,
> > +              gfx3DMatrix& aTransformToApzcOut, gfx3DMatrix& aTransformToGreenOut)
> 
> s/Green/Gecko/

LOL, not sure what happened there...

Comment 7

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> @@ +551,5 @@
> >    EXPECT_EQ(gfx3DMatrix(), transformToScreen);
> >  
> >    // Now we have a root APZC that will match the page
> > +  SetScrollableFrameMetrics(root, FrameMetrics::ROOT_SCROLL_ID);
> > +  manager->UpdatePanZoomControllerTree(nullptr, root, false, 0);
> 
> Huh. I'm surprised the compiler didn't warn about this before. Or maybe it
> did and I didn't see it?

I don't think I've ever seen a compiler warn about implicit conversions between bool and int. They are very commonplace because people often use 0/1 and false/true interchangeably.

Comment 8

[Botond Ballo [:botond]]

Attachment: Patch + test

Comment 9

[Botond Ballo [:botond]]

Attachment: Rename 'transformToScreen' to 'transformToGecko'

Comment 10

[Botond Ballo [:botond]]

Updated patches to address Kats' comments. Carrying r+.

Will test on metro before landing.

Comment 11

[Botond Ballo [:botond]]

Looks ok on metro.

https://hg.mozilla.org/integration/b2g-inbound/rev/4361037d9a4c
https://hg.mozilla.org/integration/b2g-inbound/rev/233e43e38854

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4361037d9a4c
https://hg.mozilla.org/mozilla-central/rev/233e43e38854

Comment 14

[Botond Ballo [:botond]]

Could we get this uplifted to b2g 1.2? Without this, panning fast is broken (specifically, if you pan by some amount and then try to pan on a region brought into view by the first pan before Gecko gets a chance to repaint, the second pan will be ignored).

(This will also a prerequisite for other APZC fixes that we may want to uplift to b2g 1.2, such as bug 935219 (which I'm currently working on fixing)).

The posted patch applies cleanly to the b2g 1.2 branch.

Comment 15

[Preeti Raghunath(:Preeti)]

Botand,

Will this fail any certification tests?

Comment 16

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #15)
> Botand,
> 
> Will this fail any certification tests?

I don't think this is relevant to any certification workflows.

If the dupe applies to b2g as well, then the impact would likely be that zoom in will fail after doing a max zoom out. I don't know if it's a regression or not though.

Comment 17

[Botond Ballo [:botond]]

(In reply to Preeti Raghunath(:Preeti) from comment #15)
> Botand,
> 
> Will this fail any certification tests?

I'm not sure what those are. Is there a test suite I should be running for those?

Comment 18

[Preeti Raghunath(:Preeti)]

QA,

Can we please help check if this issue a regression issue?

Comment 19

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #18)
> QA,
> 
> Can we please help check if this issue a regression issue?

Note for qawanted - use the dupe's STR to test this out.

Comment 20

[Matthew Vaughan]

The issue does reproduce MOST of the time when using the STR from bug 913927 on the 11/20/13 1.1 build. Sometimes the image can be zoomed in on from a max zoomed out state, but it doesn't zoom in very much at all. Double tapping on an image zooms in much further.

Also, the zoom feature in 1.1 doesn't work very well to begin with. The zoom feature in 1.2 is much better when compared to 1.1. 

- 1.1 BUILD -
Environmental Variables:
Device: Leo v1.1 COM RIL
BuildID: 20131120041201
Gaia: b585b32441fafa67f2b4582db23be5f3a2afab21
Gecko: a9fa9a04390d
Version: 18.0
RIL Version: 01.01.00.019.281

Comment 21

[Jason Smith [:jsmith]]

Sounds like this isn't a regression then, which makes this a non-blocker.