Settings app with APZ mixes up touch events

VERIFIED FIXED in Firefox 28

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: milan, Assigned: BenWa)

Tracking

unspecified
mozilla28
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Blocks: 916185
Created attachment 8335509 [details] [diff] [review]
Modified settings index.html to test with
Blocking a blocker for 1.3
blocking-b2g: --- → 1.3+
Assignee: nobody → bgirard
Flags: needinfo?(nhirata.bugzilla)
Summary: Settings app with APZ on lets you pan out and mixes up touch events → Settings app with APZ mixes up touch events and sometimes deletes part of the display port
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
Summary: Settings app with APZ mixes up touch events and sometimes deletes part of the display port → Settings app with APZ mixes up touch events
(Assignee)

Updated

5 years ago
Blocks: 944047
(Assignee)

Comment 7

5 years ago
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.
Flags: needinfo?(nhirata.bugzilla)
Excellent news!  Keeping Naoki on needinfo, he's tracking APZC related bugs and that's his preferred method.
Flags: needinfo?(nhirata.bugzilla)
(Assignee)

Comment 9

5 years ago
Ohh ok. I was wondering for a few days why he was flagged and wasn't expecting any info from him.
(Assignee)

Comment 10

5 years ago
Created attachment 8340510 [details] [diff] [review]
patch

Cleaned up. I don't see any regressions with APZC off
Attachment #8340510 - Flags: review?(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.
Attachment #8340510 - Flags: review?(botond) → review-

Updated

5 years ago
Component: Gaia → Panning and Zooming
Product: Firefox OS → Core
(Assignee)

Comment 13

5 years ago
Created attachment 8341309 [details] [diff] [review]
patch

Flagging Kats for optional feedback.
Attachment #8340510 - Attachment is obsolete: true
Attachment #8341309 - Flags: review?(botond)
Attachment #8341309 - Flags: feedback?(bugmail.mozilla)
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).
(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 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.
Attachment #8341309 - Flags: feedback?(bugmail.mozilla) → review?(bugmail.mozilla)
(Assignee)

Comment 18

5 years ago
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 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.
Attachment #8341309 - Flags: review?(bugmail.mozilla) → review-
(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.
(Assignee)

Comment 21

5 years ago
Created attachment 8341935 [details] [diff] [review]
patch v3

Great feedback so far. The coordinate conversions make sense to me now and everything works great.
Attachment #8341309 - Attachment is obsolete: true
Attachment #8341309 - Flags: review?(botond)
Attachment #8341935 - Flags: review?(bugmail.mozilla)
Attachment #8341935 - Flags: review?(botond)
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.
Attachment #8341935 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8341935 - Flags: review?(botond) → review+
(Assignee)

Comment 24

5 years ago
> 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    */
(Assignee)

Comment 25

5 years ago
Created attachment 8341962 [details] [diff] [review]
patch v4
Attachment #8341935 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
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.
Attachment #8341962 - Flags: review?(mbrubeck)

Updated

5 years ago
No longer blocks: 944047
FWIW I verified that the patch seems to work fine on Metro.
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?
Attachment #8341962 - Flags: review?(mbrubeck) → feedback-
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 on attachment 8341962 [details] [diff] [review]
patch v4

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8341962 - Flags: feedback-
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?
(Assignee)

Comment 33

5 years ago
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.
Whiteboard: sdfafsd das fsad fdsa fdsa fasd ads fds fsad fsad fasd fsa fsa fsa fsda fsa dfsa fsda fsad fsad fsad fsadf sad fdsa fdsaf sadf ads
(Assignee)

Comment 34

5 years ago
(Opps, was trying to reproduce a bug)
Whiteboard: sdfafsd das fsad fdsa fdsa fasd ads fds fsad fsad fasd fsa fsa fsa fsda fsa dfsa fsda fsad fsad fsad fsadf sad fdsa fdsaf sadf ads
(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.
Vivien, do you still want this one blocking the non-user build activation of the pref (bug 942726)?
(Assignee)

Comment 37

5 years ago
Waiting on mbrubeck to confirm patch doesn't regress metro before landing.
Flags: needinfo?(mbrubeck)
(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).
This did not break anything in Metro in my quick smoke-testing.
Flags: needinfo?(mbrubeck)
(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...
https://hg.mozilla.org/mozilla-central/rev/d93a8d72cac1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
status-firefox28: --- → fixed
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(nhirata.bugzilla)
You need to log in before you can comment on or make changes to this bug.