1122804 - Have nsDisplayResolution items adjust event coordinates for hit testing and dispatching to content

Status: RESOLVED DUPLICATE of bug 1224015

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

In bug 1120566 we discovered that when APZ sends events to Gecko, someone needs to unapply the scale-to-resolution transform (introduced in bug 1076241).

The fix in bug 1120566 was to unapply it on the Gecko side in the GeckoContentController implementation, but I think this is a hacky and suboptimal solution.

This bug is to follow-up with a nicer fix that unapplies this transform on the compositor side, such as in APZCTreeManager::GetApzcToGeckoTransform.

Comment 1

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

ni? to me so I don't forget to look at whether it's reasonable to move this into the compositor. I suspect that if we kept bug 920036 in the tree and backed out bug 1076241 things would be bad in that touch events might have the wrong coordinates while going through root-process gecko on B2G. I suspect that modifying the GetApzcToGeckoTransform to include the resolution given the current code will have the same problem.

Comment 2

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

So this is as I suspected. If I back out bug 1076241 while leaving bug 920036 in the tree, bad things happen after going to a webpage and zooming in. For example, the left margin for edge swipe gestures becomes much larger and the right margin disappears entirely.

Consider a conceptual example: you are on a webpage that has a LayoutDevice to Screen scale of 10x in the steady state, and you touch the screen at x=100 screen pixels. Without bug 1076241, the input will get untransformed down to x=10 screen pixels, and that will be the coordinate in the WidgetTouchEvent that we dispatch into gecko. Without bug 920036 the input event arrives at the APZ just before it gets sent off to the child process so this is fine (because the touch is actually at CSS coordinate x=10) but with bug 920036 this x=10 will get sent through the root process gecko code first. In the root process there might be something else at x=10 on top of the child process (in this case, the edge swipe listener) and so that will receive the input event instead.

With bug 1076241 applied the input event doesn't get untransformed by 10x in the APZ and so it behaves correctly. This means that:

(1) bug 920036 relies on behaviour that I hadn't really thought about before. That behaviour is currently provided by bug 1076241 although technically could be extracted and moved somewhere else
(2) unapplying the scale-to-resolution transform *for child processes* back into the APZ untransform is a bad idea because it will mess up the events sent into the root process gecko.
(3) unapplying the scale-to-resolution transform *for the root process* back into the APZ untransform is a good idea and will be needed in order to fix the events sent into root process gecko, if we ever have things APZ-zoomed in the root process
(4) if we allow subframe zooming things get more tricky, but I think this is important to consider because it points us in the right direction as to what the correct fix is. In a nutshell, I think we need to be unapplying the resolution as the event winds its way through the gecko hit-testing code, as part of the nsDisplayResolution display item much the same way nsDisplayTransform works.

Comment 3

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

Discussed this with Botond a bit more on IRC. Summary:

- nsDisplayResolution is subtly different from nsDisplayTransform in that the touch event coordinates seen by web content are impacted by one of these but not the other. That is, nsDisplayResolution would need to actually modify the event coordinates stored in the WidgetTouchEvent before it got dispatched in order to deal with this correctly. However there is no mechanism for display items to actually mutate events, so putting the untransform where we did in bug 1120566 is as close to correct as we can do right now.

- the point of bug 1076241 was to disassociate the APZ from the nontransient transform. as a result, the nontransient transform is conceptually owned by the presShell now. the presShell lives in the child process, so it makes sense for the child process to be unapplying the nontransient transform on incoming input events. in particular, the nontransient transform is *not* conceptually owned by the compositor any more, and the fact that the compositor is the code that actually applies the transform is a wart that we would like to remove at some point.

- before we can implement subframe zooming, we will need to find a better way to deal with all this, making sure that events get untransformed properly as they go through the gecko hit-test chain and also that they map to sane coordinates when web content sees them.

Therefore I'm moving the dependency here to block subframe zooming.

Comment 4

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

On Fennec we have a situation where in a single process we have a root document with no resolution and a root content document with a resolution. This effectively breaks in the current code because the transformation at [1] is done too early (before the hit-test is done on the root document, and before the event is dispatched into the root document). For now we're planning to work around this by using the root content document as the root from which hit-tests are done, but that can cause issues of its own (for one thing, the event dispatched into gecko will do its own hit tests using the root document; for another, addons might insert things into the root document that should get hit).

I believe implementing this bug is the correct solution to this problem.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=ae48bb7aad1b#436

Comment 5

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

Attachment: Partial sketch

I wrote this up yesterday but it's incomplete; we'll need a bit somewhere in PresShell::HandleEvent probably that divides the refpoint in the input event.

Comment 6

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1122804-Have-nsDisplayResolution-items-adjust-event-coordinates-for-hit-testing-and-dispatching-to-content-15082811-a61a7b5.patch

Comment 7

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93b8bf51006

Comment 8

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

Comment on attachment 8654270 [details] [diff] [review]
0001-Bug-1122804-Have-nsDisplayResolution-items-adjust-event-coordinates-for-hit-testing-and-dispatching-to-content-15082811-a61a7b5.patch

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

So overall I think this is promising. I have some comments on the patch below.

When I talked to Botond on Friday he mentioned that there was a potential problem with this approach, in that nsDisplayResolution is a subclass of nsSubDocumentFrame, and conceptually lives in the parent document's displaylist. This means that if we try to do a hit-test directly on the root content document, the display list built there doesn't have the nsDisplayResolution and we have to make sure we have already taken into account the resolution. This makes sense to me but I also think it's worth looking at how nsDisplayZoom (which changes the app-units-to-layout-device-pixels ratio on a subdocument) works. Conceptually the two are very similar so any time there is existing code that deals with an nsDisplayZoom (or does a ScaleToOtherAppUnits call) we potentially need to update that code to handle a resolution change as well. The list of places that does this is at http://mxr.mozilla.org/mozilla-central/ident?i=ScaleToOtherAppUnits - it's not super long but not as short as I was hoping either.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +420,2 @@
>  CSSPoint
>  APZCCallbackHelper::ApplyCallbackTransform(const CSSPoint& aInput,

I guess "CSSPoint" here doesn't really mean CSSPoint, or something. It would be nice to update the types to match what's actually being passed around now.

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +145,5 @@
>    if (!document.get()) {
>      return;
>    }
>  
>    CSSPoint point = APZCCallbackHelper::ApplyCallbackTransform(aPoint, aGuid);

Same here, the point being returned may not be a CSSPoint any more, although I'm not quite sure what to call it either.

::: layout/base/nsPresShell.cpp
@@ +7000,5 @@
>    DispatchAfterKeyboardEventInternal(chain, aEvent,
>                                       aEvent.mFlags.mDefaultPrevented);
>  }
>  
> +struct ScopedTouchEventResolution {

drop "Touch" from the name since it's used for non-touch events as well.

@@ +7021,5 @@
> +    }
> +  }
> +  ~ScopedTouchEventResolution()
> +  {
> +    if (WidgetTouchEvent* event = mEvent->AsTouchEvent()) {

I would prefer if the constructor stored an unaltered copy of the refpoints in member variables, and then you restored them here. That should prevent accumulation of rounding error if an event goes through multiple ScopedTouchEventResolution instances.

Comment 9

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Comment on attachment 8654270 [details] [diff] [review]
> 0001-Bug-1122804-Have-nsDisplayResolution-items-adjust-event-coordinates-for-
> hit-testing-and-dispatching-to-content-15082811-a61a7b5.patch
> 
> Review of attachment 8654270 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> @@ +7021,5 @@
> > +    }
> > +  }
> > +  ~ScopedTouchEventResolution()
> > +  {
> > +    if (WidgetTouchEvent* event = mEvent->AsTouchEvent()) {
> 
> I would prefer if the constructor stored an unaltered copy of the refpoints
> in member variables, and then you restored them here. That should prevent
> accumulation of rounding error if an event goes through multiple
> ScopedTouchEventResolution instances.
>

I did this approach first but discovered that touches can be removed before returning from PresShell::HandleEvents see:

https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/layout/base/nsPresShell.cpp#7378

So I went with saving the resolution and undoing the operation on what ever touches were remaining in the even at destruction time.

Comment 10

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

Ah good point. That's worth mentioning in a comment in the patch then.

Comment 11

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

Oh I guess you could also store it as a map<touchid, refpoint>. If some touch points are removed you just ignore those in the map, and restore the refpoints for whatever touch points are still left. The touch identifiers in a touch input are guaranteed to be unique.

Comment 12

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Comment on attachment 8654270 [details] [diff] [review]
> 0001-Bug-1122804-Have-nsDisplayResolution-items-adjust-event-coordinates-for-
> hit-testing-and-dispatching-to-content-15082811-a61a7b5.patch
> 
> Review of attachment 8654270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So overall I think this is promising. I have some comments on the patch
> below.
> 

I'm not sure. I'm still seeing a lot of intermittent failures with no way to reliably reproduce. For example sometimes pinch zoom works with scrollable divs and iframes and some times it does not. This is on the same page. Not being able to reliably reproduce the issues makes it difficult to even know where to start now.

> When I talked to Botond on Friday he mentioned that there was a potential
> problem with this approach, in that nsDisplayResolution is a subclass of
> nsSubDocumentFrame, and conceptually lives in the parent document's
> displaylist. This means that if we try to do a hit-test directly on the root
> content document, the display list built there doesn't have the
> nsDisplayResolution and we have to make sure we have already taken into
> account the resolution. This makes sense to me but I also think it's worth
> looking at how nsDisplayZoom (which changes the
> app-units-to-layout-device-pixels ratio on a subdocument) works.
> Conceptually the two are very similar so any time there is existing code
> that deals with an nsDisplayZoom (or does a ScaleToOtherAppUnits call) we
> potentially need to update that code to handle a resolution change as well.
> The list of places that does this is at
> http://mxr.mozilla.org/mozilla-central/ident?i=ScaleToOtherAppUnits - it's
> not super long but not as short as I was hoping either.
> 

To make sure I'm understanding, The code would go from something like:

offset += docOffset.ScaleToOtherAppUnits(currAPD, apd);

to

offset += docOffset.ScaleToOtherAppUnits(currAPD / currResolution, apd / newResolution);

?

How would I even test if the changes are needed?

> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp
> @@ +420,2 @@
> >  CSSPoint
> >  APZCCallbackHelper::ApplyCallbackTransform(const CSSPoint& aInput,
> 
> I guess "CSSPoint" here doesn't really mean CSSPoint, or something. It would
> be nice to update the types to match what's actually being passed around now.
> 
> ::: gfx/layers/apz/util/ChromeProcessController.cpp
> @@ +145,5 @@
> >    if (!document.get()) {
> >      return;
> >    }
> >  
> >    CSSPoint point = APZCCallbackHelper::ApplyCallbackTransform(aPoint, aGuid);
> 
> Same here, the point being returned may not be a CSSPoint any more, although
> I'm not quite sure what to call it either.
> 

Yes, it makes it very confusing because the CSSPoint may have the resolution applied or not depending on what level you are operating on and there doesn't seem to be a good way to know which version you currently have. I have no idea what to do about it.

> ::: layout/base/nsPresShell.cpp
> @@ +7000,5 @@
> >    DispatchAfterKeyboardEventInternal(chain, aEvent,
> >                                       aEvent.mFlags.mDefaultPrevented);
> >  }
> >  
> > +struct ScopedTouchEventResolution {
> 
> drop "Touch" from the name since it's used for non-touch events as well.
> 

Done.

> @@ +7021,5 @@
> > +    }
> > +  }
> > +  ~ScopedTouchEventResolution()
> > +  {
> > +    if (WidgetTouchEvent* event = mEvent->AsTouchEvent()) {
> 
> I would prefer if the constructor stored an unaltered copy of the refpoints
> in member variables, and then you restored them here. That should prevent
> accumulation of rounding error if an event goes through multiple
> ScopedTouchEventResolution instances.

I used your map<> suggestion. That seems to work. I used mIdentifier in the Touch class for the touchid. Is that the same thing?

https://dxr.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887/dom/events/Touch.h#80

Comment 13

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

(In reply to Randall Barker [:rbarker] from comment #12)
> I'm not sure. I'm still seeing a lot of intermittent failures with no way to
> reliably reproduce. For example sometimes pinch zoom works with scrollable
> divs and iframes and some times it does not. This is on the same page. Not
> being able to reliably reproduce the issues makes it difficult to even know
> where to start now.

Yeah I can understand how multiple issues interacting can make them really hard to diagnose. After I'm done working on bug 1197824 I'll spend some time playing around with an APZ-enabled fennec build and see if I can tease apart some of these issues and come up with reduced cases/diagnoses. Hopefully that will help you out since you should be able to tackle them one at a time.

> To make sure I'm understanding, The code would go from something like:
> 
> offset += docOffset.ScaleToOtherAppUnits(currAPD, apd);
> 
> to
> 
> offset += docOffset.ScaleToOtherAppUnits(currAPD / currResolution, apd /
> newResolution);
> 
> ?

Something like that. I'm not entirely sure myself; it's just the notion that a resolution change from one presshell to another is very similar to an app unit scale change from one presshell to another. So everywhere we are currently handling app unit scale change, we should also be handling a resolution scale change. The actual mechanics of how to do that could vary, and would probably depend on the call site.

> How would I even test if the changes are needed?

For this I would probably work on a desktop build where it's easier to exercise some of these code paths. We can hack in something to be able to set a resolution on the content presshell, and compare what happens in that scenario to what happens when a full zoom is applied instead.

> Yes, it makes it very confusing because the CSSPoint may have the resolution
> applied or not depending on what level you are operating on and there
> doesn't seem to be a good way to know which version you currently have. I
> have no idea what to do about it.

We might have to introduce a type for these things. However I'd hold off on that for now.

> I used your map<> suggestion. That seems to work. I used mIdentifier in the
> Touch class for the touchid. Is that the same thing?

Yup, that's it.

Comment 14

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1122804-Have-nsDisplayResolution-items-adjust-event-coordinates-for-hit-testing-and-dispatching-to-content-15090213-819c154.patch

Comment 15

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1122804-Have-nsDisplayResolution-items-adjust-event-coordinates-for-hit-testing-and-dispatching-to-content-15102611-75f2ea8.patch

Comment 16

[Randall Barker [:rbarker]]

Attachment: 0002-Fix-base-widget-so-that-wheel-events-from-nsDOMWindowUtil-that-are-dispatched-where-controller-thread-main-thread-are-handled-correctly-15102611-334112f.patch

Comment 17

[Randall Barker [:rbarker]]

Attachment: 0003-Fix-AsyncCompositionManager-so-that-mLayersUpdated-is-only-set-to-false-after-SyncFrameMetrics-has-been-called-15102611-f2965af.patch

Comment 18

[Randall Barker [:rbarker]]

Attachment: 0004-Promote-setIsLongpressEnabled-to-PAnZoomController-interface-so-that-casting-to-JavaPanZoomController-is-no-longer-needed-15102611-eba5004.patch

Comment 19

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

This no longer blocks APZ on fennec since we went with a different approach to solving this problem. The patches randall posted above are an early version of that approach; the patches now live on bug 1224015.

Comment 20

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

So this was pretty much taken care of in bug 1224015, although it was #ifdef'd for MOZ_SINGLE_PROCESS_APZ. I'll file another bug for removing the ifdef.