940706 - Settings app with APZ mixes up touch events

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

Update settings app index.html (see bug 916185) with the correct meta-viewport tag.  Turn on APZ from the dev. settings.  Restart the phone.

Go to the settings app and try to go to the dev settings again - can't get into device information; the correct field highlights, but nothing happens.

You can also get the app to disappear by scrolling around for a few seconds.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Modified settings index.html to test with

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Blocking a blocker for 1.3

Comment 3

[Benoit Girard (:BenWa)]

Alright I think there's a few problems here. On top of what is originally described there:
- APZC doesn't debounce/fire the events the same way. For something to register a click with APZC on the Settings app I need to do it quickly and it appears to only register if the touch has the EXACT same X/Y. For example the following doesn't register as a click:

(Note the delta is very small on a HiDPI screen)
I/Gecko   (  782): touch event start 440, 1336
I/Gecko   (  782): touch event end 439, 1345

Comment 4

[Benoit Girard (:BenWa)]

Ok some progress. With APZC on or off we go through different gesture detection code. I suspect they are very different:
https://bug930939.bugzilla.mozilla.org/attachment.cgi?id=827404

Comment 5

[Benoit Girard (:BenWa)]

I've been comparing the two guestures controllers are they are very different indeeded:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#27
should instead read ui.click_hold_context_menus.delay.

Likewise we don't honor ui.dragThresholdX/ui.dragThresholdY. Tomorrow I'll work on merging the two.

Comment 6

[Benoit Girard (:BenWa)]

I've taken this bug in the direction of fixing the bad touch inputs. My goal is to be able to disable the APZC reliably in the settings app. I'm cloning the |sometimes deletes part of the display port| into a different bug since I believe the patch will be different.

Comment 7

[Benoit Girard (:BenWa)]

Alright I have a patch that fixes the problem locally. Tomorrow I'll simplify the patch and make sure that it works in more complex scenarios.

For the record the unit restructuring made this much easier.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Excellent news!  Keeping Naoki on needinfo, he's tracking APZC related bugs and that's his preferred method.

Comment 9

[Benoit Girard (:BenWa)]

Ohh ok. I was wondering for a few days why he was flagged and wasn't expecting any info from him.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch

Cleaned up. I don't see any regressions with APZC off

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8340510 [details] [diff] [review]
patch

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

Benoit and I discussed this patch in person. Minusing for now as the calls to WidgetSpaceToCompensatedViewportSpace should be removed (they don't make sense in conjunction with transformToGecko) and this patch should get a review by someone from Metro as the values sent to GeckoContentController are changing and we should make sure that doesn't break Metro.

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch

Flagging Kats for optional feedback.

Comment 14

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=63e3fc9c6c06

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8341309 [details] [diff] [review]
patch

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

Looks good in general, but I'd like to see some of the comments addressed / questions answered before r+ing it. Also, I'd prefer if Kats looked at this as well to double-check that the coordinate conversions make sense.

::: dom/ipc/TabChild.cpp
@@ +1655,5 @@
> +
> +  int time = 0;
> +  DispatchSynthesizedMouseEvent(NS_MOUSE_MOVE, time, currentPoint);
> +  DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, time, currentPoint);
> +  DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, time, currentPoint);

Why is the change from DispatchMouseEvent to DispatchSynthesizedMouseEvent necessary here? I'm not saying there's anything wrong with it, I'm just trying to understand.

::: dom/ipc/TabParent.cpp
@@ +664,5 @@
> +  if (mIsDestroyed) {
> +    return false;
> +  }
> +
> +  // XXX We're mixing CSS and LayoutDevice units. This needs to be fixed.

I think we can fix this very easily in this patch, by dividing mChildProcessOffsetAtTouchStart by mDefaultScale before adding it to aPoint.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +734,5 @@
>    return nsEventStatus_eConsumeNoDefault;
>  }
>  
> +void
> +AsyncPanZoomController::ConvertToGeckoOut(const ScreenPoint& aPoint, CSSIntPoint* aOut)

Is there any reason to prefer an out parameter over a return value here?

@@ +738,5 @@
> +AsyncPanZoomController::ConvertToGeckoOut(const ScreenPoint& aPoint, CSSIntPoint* aOut)
> +{
> +  gfx3DMatrix transformToApzc;
> +  gfx3DMatrix transformToGecko;
> +  APZCTreeManager* treeManagerLocal = mTreeManager;

The reason we need a local copy of mTreeManager here is that the compositor thread could be concurrently nulling out mTreeManager. This means treeManagerLocal could be nullptr, so we need to check for that, like in CallDispatchScroll() (or the end of AttemptScroll() if you haven't pulled recently).

@@ +740,5 @@
> +  gfx3DMatrix transformToApzc;
> +  gfx3DMatrix transformToGecko;
> +  APZCTreeManager* treeManagerLocal = mTreeManager;
> +  treeManagerLocal->GetInputTransforms(this, transformToApzc, transformToGecko);
> +  gfx3DMatrix outTransform = transformToGecko;

What is the purpose of copying the matrix here?

@@ +742,5 @@
> +  APZCTreeManager* treeManagerLocal = mTreeManager;
> +  treeManagerLocal->GetInputTransforms(this, transformToApzc, transformToGecko);
> +  gfx3DMatrix outTransform = transformToGecko;
> +  gfxPoint result = outTransform.Transform(gfxPoint(aPoint.x, aPoint.y));
> +  LayoutDevicePoint layoutPoint = LayoutDevicePoint(result.x, result.y);

Elsewhere we treat the result of transforming by transformToGecko as a ScreenPoint. Can you add a comment saying why it's OK to treat it as a LayoutDevicePoint here? (My understanding of the reason is that transformToGecko takes a point into global (i.e. root layer's) screen space from Gecko's point of view (that is, not counting async transforms), and for the root layer, 1 screen pixel = 1 layout device pixel, since the root layer cannot have transforms or a resolution).

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> @@ +742,5 @@
> > +  APZCTreeManager* treeManagerLocal = mTreeManager;
> > +  treeManagerLocal->GetInputTransforms(this, transformToApzc, transformToGecko);
> > +  gfx3DMatrix outTransform = transformToGecko;
> > +  gfxPoint result = outTransform.Transform(gfxPoint(aPoint.x, aPoint.y));
> > +  LayoutDevicePoint layoutPoint = LayoutDevicePoint(result.x, result.y);
> 
> Elsewhere we treat the result of transforming by transformToGecko as a
> ScreenPoint. Can you add a comment saying why it's OK to treat it as a
> LayoutDevicePoint here? (My understanding of the reason is that
> transformToGecko takes a point into global (i.e. root layer's) screen space
> from Gecko's point of view (that is, not counting async transforms), and for
> the root layer, 1 screen pixel = 1 layout device pixel, since the root layer
> cannot have transforms or a resolution).

Actually, on second thought, there is no reason the root layer can't have a resolution, is there? Have you tested tapping on a link in the browser while zoomed in with this patch?

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8341309 [details] [diff] [review]
patch

Oh, one more thing: during our in-person discussion about this patch it came up that since this patch can potentially change the values passed to GeckoContentController::Handle*Tap(), and Metro also implements GeckoContentController and uses the values passed to those functions, we should also have this patch reviewed by someone familiar with Metro code to make sure it doesn't break anything there. I'm flagging Kats for that.

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 8341309 [details] [diff] [review]
patch

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

::: dom/ipc/TabChild.cpp
@@ +1655,5 @@
> +
> +  int time = 0;
> +  DispatchSynthesizedMouseEvent(NS_MOUSE_MOVE, time, currentPoint);
> +  DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, time, currentPoint);
> +  DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, time, currentPoint);

I'm trying use DispatchSynthesizedMouseEvent like we did pre-APZC.

::: dom/ipc/TabParent.cpp
@@ +664,5 @@
> +  if (mIsDestroyed) {
> +    return false;
> +  }
> +
> +  // XXX We're mixing CSS and LayoutDevice units. This needs to be fixed.

Right, sorry I forgot we had discussed this on Friday!

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +734,5 @@
>    return nsEventStatus_eConsumeNoDefault;
>  }
>  
> +void
> +AsyncPanZoomController::ConvertToGeckoOut(const ScreenPoint& aPoint, CSSIntPoint* aOut)

To avoid having to copy/move the CSSIntPoint. It also seem to be a common pattern:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#446

@@ +742,5 @@
> +  APZCTreeManager* treeManagerLocal = mTreeManager;
> +  treeManagerLocal->GetInputTransforms(this, transformToApzc, transformToGecko);
> +  gfx3DMatrix outTransform = transformToGecko;
> +  gfxPoint result = outTransform.Transform(gfxPoint(aPoint.x, aPoint.y));
> +  LayoutDevicePoint layoutPoint = LayoutDevicePoint(result.x, result.y);

I was actually basing this decision on this piece of code where a similar function is returning LayoutDeviceIntPoint:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#446

If you think that function is wrong then let me know and maybe I can clean that up too.

Comment 19

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

Comment on attachment 8341309 [details] [diff] [review]
patch

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

::: dom/ipc/TabChild.cpp
@@ +1645,5 @@
>    }
>  
> +  double scale = 1;
> +  GetDefaultScale(&scale);
> +  if (scale < 0) {

You should use mWidget->GetDefaultScale() instead which is cheaper because it caches the result (and also it returns a CSSToLayoutDeviceScale)

::: dom/ipc/TabParent.cpp
@@ +667,5 @@
> +
> +  // XXX We're mixing CSS and LayoutDevice units. This needs to be fixed.
> +  CSSIntPoint p = aPoint;
> +  p.x = aPoint.x + mChildProcessOffsetAtTouchStart.x;
> +  p.y = aPoint.y + mChildProcessOffsetAtTouchStart.y;

mDefaultScale is the conversion from CSS -> LayoutDevice pixels for the tabchild, so you can't use that when converting mChildProcessOffsetAtTouchStart to CSS pixels. You have to get the scale from the prescontext of the owning content, like nsEventStateManager::GetChildProcessOffset does. But regardless I don't understand why this code is adding mChildProcessOffsetAtTouchStart. Shouldn't it be subtracting it in order to make it make the coordinate relative to the child? And why is this only on single tap? We need to do it on double tap and long tap as well.

Comment 20

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

(In reply to Botond Ballo [:botond] from comment #15)
> Elsewhere we treat the result of transforming by transformToGecko as a
> ScreenPoint.

Where do we do this? Once it's in "gecko" coordinate space it should be a LayoutDevicePoint, I think.

(In reply to Benoit Girard (:BenWa) from comment #18)
> > +void
> > +AsyncPanZoomController::ConvertToGeckoOut(const ScreenPoint& aPoint, CSSIntPoint* aOut)
> 
> To avoid having to copy/move the CSSIntPoint. It also seem to be a common
> pattern:
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> APZCTreeManager.cpp#446

I would also prefer to have it a return value rather than an out-param but I don't care that much. (I would also like to change APZCTreeManager::TransformCoordinateToGecko to have a return value rather than an out-param). We can do that in a follow-up if you don't want to do it here.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: patch v3

Great feedback so far. The coordinate conversions make sense to me now and everything works great.

Comment 22

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

Comment on attachment 8341935 [details] [diff] [review]
patch v3

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

r=me with comments addressed.

::: dom/ipc/TabChild.cpp
@@ +1644,5 @@
>      return true;
>    }
>  
> +  CSSPoint p = aPoint;
> +  LayoutDevicePoint currentPoint = p * mWidget->GetDefaultScale();;

No need for local "p", just inline it

::: dom/ipc/TabParent.h
@@ +219,5 @@
> +    bool SendHandleSingleTap(const CSSIntPoint& aPoint);
> +    bool SendHandleLongTap(const CSSIntPoint& aPoint);
> +    bool SendHandleDoubleTap(const CSSIntPoint& aPoint);
> +
> +    CSSIntPoint AdjustTapToChildWidget(const CSSIntPoint& aPoint);

Make this private

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +737,5 @@
> +bool
> +AsyncPanZoomController::ConvertToGeckoOut(const ScreenPoint& aPoint, CSSIntPoint* aOut)
> +{
> +  gfx3DMatrix transformToApzc;
> +  gfx3DMatrix transformToGecko;

Move these inside the if ()

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +517,4 @@
>     */
>    void SetState(PanZoomState aState);
>  
> +  bool ConvertToGeckoOut(const ScreenPoint& aPoint, CSSIntPoint* aOut);

I'm not entirely happy with the name "ConvertToGeckoOut" - maybe just "ConvertToGecko"? I don't have a much better suggestion so I'll leave this to you.

Also add some documentation for this method here. Just say that it converts points in the coordinate space of the APZC's transient async transform to the coordinate space that Gecko understands.

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8341935 [details] [diff] [review]
patch v3

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

r=me with the comments addressed

::: dom/ipc/TabParent.cpp
@@ +666,5 @@
> +  if (!content) {
> +    return aPoint;
> +  }
> +  nsPresContext* presContext =
> +    content->OwnerDoc()->GetShell()->GetPresContext();

Do we need to check the return values of GetShell() and GetPresContext() for nullptr?

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +742,5 @@
> +  APZCTreeManager* treeManagerLocal = mTreeManager;
> +  if (treeManagerLocal) {
> +    treeManagerLocal->GetInputTransforms(this, transformToApzc, transformToGecko);
> +    gfxPoint result = transformToGecko.Transform(gfxPoint(aPoint.x, aPoint.y));
> +    LayoutDevicePoint layoutPoint = LayoutDevicePoint(result.x, result.y);

As dicussed with Kats on IRC, the result of calling transformToGecko isn't *quite* a LayoutDevicePoint, but we're calling it a LayoutDevicePoint because we don't have a name for the coordinate system it's in and it's closest to LayoutDevicePoint. Can we add a comment to that effect here?

@@ +793,4 @@
>      int32_t modifiers = WidgetModifiersToDOMModifiers(aEvent.modifiers);
> +    CSSIntPoint geckoScreenPoint;
> +    if (ConvertToGeckoOut(aEvent.mPoint, &geckoScreenPoint)) {
> +      ConvertToGeckoOut(aEvent.mPoint, &geckoScreenPoint);

Don't call ConvertToGeckoOut again here.

@@ +811,3 @@
>        int32_t modifiers = WidgetModifiersToDOMModifiers(aEvent.modifiers);
> +      CSSIntPoint geckoScreenPoint;
> +      ConvertToGeckoOut(aEvent.mPoint, &geckoScreenPoint);

Check the return value of ConvertToGeckoOut here.

Comment 24

[Benoit Girard (:BenWa)]

> Also add some documentation for this method here. Just say that it converts
> points in the coordinate space of the APZC's transient async transform to
> the coordinate space that Gecko understands.

520   /**                                                                           
521    * Convert ScreenPoint relative to this APZC to CSSIntPoint relative          
522    * to the parent document. This excludes the transient compositor transform.  
523    * NOTE: This must be converted to CSSIntPoint relative to the child          
524    * document before sending over IPC.                                          
525    */

Comment 25

[Benoit Girard (:BenWa)]

Attachment: patch v4

Comment 26

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=f5606ecf30fd

Comment 27

[Benoit Girard (:BenWa)]

Comment on attachment 8341962 [details] [diff] [review]
patch v4

Can you verify that this works well for metro? In particular I'm changing the gestures and no longer calling WidgetSpaceToCompensatedViewportSpace.

Comment 28

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

FWIW I verified that the patch seems to work fine on Metro.

Comment 29

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8341962 [details] [diff] [review]
patch v4

It looks like this will require changes in Metro similar to the ones in TabParent/TabChild, to convert the new coordinates passed to HandleSingleTap and HandleDoubleTap. The relevant Metro code is here:

http://hg.mozilla.org/mozilla-central/file/b2b20bc6576a/widget/windows/winrt/APZController.cpp#l281
http://hg.mozilla.org/mozilla-central/file/b2b20bc6576a/browser/metro/base/content/contenthandlers/Content.js#l369

Are you able to write and test a patch for that, or would you like us to do it?

Comment 30

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

In the metro case though there is only the root widget, so there's no need to translate the coordinates to be relative to anything. Or am I misunderstanding? Do you have a specific example where this patch fails?

Comment 31

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8341962 [details] [diff] [review]
patch v4

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> In the metro case though there is only the root widget, so there's no need
> to translate the coordinates to be relative to anything.

Oh, I see.  My previous comment was based just on reading the code, and I didn't understand the full context.  I'll defer to kats here, and I'll also test out a build with this patch just to double-check.

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Just as a note, my original problem doesn't reproduce anymore.  Is there an STR for what the issue we're tracking with this bug is?

Comment 33

[Benoit Girard (:BenWa)]

I can still reproduce on my dated build. Its possible on a newer build we no longer nest APZC on Settings and thus don't run into this problem.

Comment 34

[Benoit Girard (:BenWa)]

(Opps, was trying to reproduce a bug)

Comment 35

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #32)
> Just as a note, my original problem doesn't reproduce anymore.  Is there an
> STR for what the issue we're tracking with this bug is?

Without this patch I got some issues on the music app. I can not click on any of the buttons at the bottom of the app.

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Vivien, do you still want this one blocking the non-user build activation of the pref (bug 942726)?

Comment 37

[Benoit Girard (:BenWa)]

Waiting on mbrubeck to confirm patch doesn't regress metro before landing.

Comment 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #36)
> Vivien, do you still want this one blocking the non-user build activation of
> the pref (bug 942726)?

This one is really painful for some apps. In music you can not click the buttons at the bottom. In contacts, using the shortcuts navigation with letter there is sometimes some weird coordinates. So I think we need this bug and bug 943846 to land. With the others bugs that are in inbound, my feeling is that we will be good to turn it on. (maybe just tweaking pref("apz.fling_friction", "0.002"); in b2g.js too, to make it feels it it not slow).

Comment 39

[Matt Brubeck (:mbrubeck)]

This did not break anything in Metro in my quick smoke-testing.

Comment 40

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #38)
> (In reply to Milan Sreckovic [:milan] from comment #36)
> > Vivien, do you still want this one blocking the non-user build activation of
> > the pref (bug 942726)?
> 
> This one is really painful for some apps. In music you can not click the
> buttons at the bottom. In contacts, using the shortcuts navigation with
> letter there is sometimes some weird coordinates. So I think we need this
> bug and bug 943846 to land. With the others bugs that are in inbound, my
> feeling is that we will be good to turn it on. (maybe just tweaking
> pref("apz.fling_friction", "0.002"); in b2g.js too, to make it feels it it
> not slow).

Sounds good.  Let's see where we are tomorrow.  Tweaking the other prefs would be useful, and we'll probably get even more feedback once we enable this, so I expect to keep tweaking those ones...

Comment 41

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d93a8d72cac1

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d93a8d72cac1

Comment 43

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I'm sort of hesitant to mark this as verified.  There's some oddities with taps getting "eaten"

Having said that, I can now tap on certain overlays such as the music player, etc.
Gaia      67e8b2b0d150f87b4297bb2f1fbae3d4aca3e5a7
Gecko    
BuildID   20131010004040
Version   18.0
ro.build.version.incremental=eng.zxliu.20131010.001907
ro.build.date=Thu Oct 10 00:19:23 CST 2013
Buri