Closed Bug 1202050 Opened 6 years ago Closed 6 years ago

APZ hit test transforms aren't guaranteed to be 2d.

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: mattwoodrow, Assigned: kevin.m.wern, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1120683 +++

I made a mistake in comment #3 of bug 1120683 saying that since we always apply and then unapply all transforms that could potentially be 3d, the result will always be 2d.

This really isn't true, so we need to handle this by using ProjectPoint and removing the .Is2D() assert.

Looks like there's 7 places in APZCTreeManager where we do this, all of these need to be changed.

Kevin, are you interested in doing this?
Flags: needinfo?(kevin.m.wern)
Whiteboard: [gfx-noted] [lang=c++] → [lang=c++]
Sure, Matt!

I should also be changing transforms in [Bug 1173521](https://bugzilla.mozilla.org/show_bug.cgi?id=1173521) we thought were 2D from the same premise, right?
Flags: needinfo?(kevin.m.wern)
(In reply to kevin.m.wern from comment #1)

> I should also be changing transforms in [Bug
> 1173521](https://bugzilla.mozilla.org/show_bug.cgi?id=1173521) we thought
> were 2D from the same premise, right?

Interesting...didn't know it worked that way. Bug 1173521.
Flags: needinfo?(matt.woodrow)
Yeah, looks like those could potentially be 3d too.
Flags: needinfo?(matt.woodrow)
I had found 5 uses of TransformTo working on Bug 1173521, 3 of which weren't changed because they involved either 2D transforms exclusively or unapplied and reapplied 3D transforms.  Of those, I think only one of them is in the latter category and needs to use UntransformTo. If you could double-check the last two examples in my first comment there and see if they also need to be modified, I'd appreciate it.
Flags: needinfo?(botond)
Comment on attachment 8663537 [details] [diff] [review]
Extend APZ w-coordinate check for points and rectangles

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

Thanks, Kevin! I agree that the other two call sites you mention will only have 2D transforms as well.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ -158,4 @@
>  TransformClipRect(Layer* aLayer,
>                    const Matrix4x4& aTransform)
>  {
> -  MOZ_ASSERT(aTransform.Is2D());

I've talked this over with Matt, and it turns out we can assume this transform is 2D, after all.

The reason is pretty subtle. TransformClipRect() has two call sites: TranslateShadowLayer()  applies just a translation, so that's 2D. The more interesting call site is ApplyAsyncTransformToScrollbarForContent(). Here, the matrix being passed in obtained by multiplying together a CSS transform, the inverse of an async transform, and the inverse of the CSS transform. This could be 3D *if* the CSS transform can be 3D. However, the CSS transform is that of the scrolled content, and this code is only run if the scrollbar layer is a descendant of the scrolled content; in such a case, the scrolled content will not have a 3D transform (any transform would either be on an ancestor of the scrolled content, or on a descendant in which case it would be on a sibling of the scrollbar).

Therefore, we can keep this assertion, and keep this call as TransformTo().
Attachment #8663537 - Flags: feedback+
Assigning the bug to you.
Assignee: nobody → kevin.m.wern
Flags: needinfo?(botond)
Comment on attachment 8664078 [details] [diff] [review]
Extend APZ w-coordinate check for points (revised to exclude change to TransformClipRect)

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

I just realized this patch needs some more work: UntransformTo returns a Maybe, so at each of these sites we will need to handle the case of an empty Maybe. Suggestions for how to do that:

  - APZCTreeManager::TransformCoordinateToGecko() isn't actually used by anyone
    any more (it used to be used by the Windows Metro code, but that was removed
    from the tree). Let's just remove this function.

  - For all other call sites in APZCTreeManager, return early with 
    nsEventStatus_eIgnore.

  - In AsyncPanZoomController::ConvertToGecko(), return false.
Blocks: 1168263
Did some type changes, because there is a way to assign a PointTyped to an IntPointTyped, but not a way to assign a Maybe<PointTyped> to a Maybe<IntPointTyped>.  Let me know if that was handled correctly.
Attachment #8664078 - Attachment is obsolete: true
Flags: needinfo?(botond)
Comment on attachment 8666425 [details] [diff] [review]
0001-Extend-UntransformTo-to-other-3D-untransforms.patch

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

Thanks, this looks pretty good! Just a couple of minor comments remaining.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +739,5 @@
> +          outTransform, pinchInput.mFocusPoint);
> +
> +        if (!untransformedFocusPoint) {
> +          return result;
> +        }

Please move the computation of the |outTransform|, the call to |UntransformTo|, and the check of the result, to before the call to |ReceiveInputEvent|, for consistency with the other cases (also the call to ReceiveInputEvent assigns to |result|, so it won't necessarily be |nsEventStatus_eIgnore| any more).

@@ +765,5 @@
> +          UntransformTo<ScreenPixel>(outTransform, tapInput.mPoint);
> +
> +        if (!untransformedPoint) {
> +          return result;
> +        }

Same here.
Attachment #8666425 - Flags: feedback+
Flags: needinfo?(botond)
Attachment #8666425 - Attachment is obsolete: true
Flags: needinfo?(botond)
Comment on attachment 8669267 [details] [diff] [review]
Extend UntransformTo calls, final revision

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

Looks good, thanks!
Attachment #8669267 - Flags: review+
I pushed the patch to the Try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb800055d26
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb800055d26

All clean. I will land when inbound opens.
https://hg.mozilla.org/mozilla-central/rev/1d27bae5a64e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Thanks, Kevin!
Depends on: 1212876
You need to log in before you can comment on or make changes to this bug.