870311 - Context menu events are wonky on hidpi devices

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Chris Lord [:cwiiis]]

It seems after bug 867703, touch events sometimes have the wrong coordinates or targets (as discovered by chpo). Certainly the 'contextmenu' event consistently has the wrong target (causing app arrangement to be completely broken on the homescreen) and scrolling and tapping in the browser is now impossible.

It would appear that the widget scaling factor isn't being taken into account somewhere (or multiple places).

Comment 1

[Chris Lord [:cwiiis]]

Making PuppetWidget return 96dpi/1.0 scale fixes the event issues, but also stops the browser and app pages from being scaled correctly.

My guess is that TabChild/Parent (whichever) needs to multiply coordinates by the scale somewhere...

Comment 2

[Chris Lord [:cwiiis]]

This could be that the AsyncPanZoomController or GestureEventListener needs to handle scale... Would explain why some events are ok and others aren't.

Comment 3

[Chris Lord [:cwiiis]]

So the 'contextmenu' event for the homescreen (and presumably all pages not viewed via the browser) is coming from nsEventStateManager. The coordinates of the event seem correct, but the event target isn't.

The event target for these events comes via nsPresShell, but I've not had the time to trace its origin further than that.

AsyncPanZoomController being wrong is a separate problem to this one, I think (but same rough cause).

Comment 4

[Doug Sherk (:drs) (inactive)]

http://mxr.mozilla.org/mozilla-central/search?string=SetDPI

There's a SetDPI() function within APZC that used to be called from TabChild I think, but that code is gone. It assumes a DPI of 72 if it's not set.

As for device pixel scaling, APZC should handle that fine through layers. Using a device with 96 DPI but APZC assuming 72 DPI would definitely explain some of the problems here. It would not explain the event targeting issue, though.

Comment 5

[Chris Lord [:cwiiis]]

(In reply to Doug Sherk (:drs) (:dRdR) from comment #4)
> http://mxr.mozilla.org/mozilla-central/search?string=SetDPI
> 
> There's a SetDPI() function within APZC that used to be called from TabChild
> I think, but that code is gone. It assumes a DPI of 72 if it's not set.
> 
> As for device pixel scaling, APZC should handle that fine through layers.
> Using a device with 96 DPI but APZC assuming 72 DPI would definitely explain
> some of the problems here. It would not explain the event targeting issue,
> though.

I had a look at that, but it seems that DPI isn't used for any event transformation - If it was, every device would see the issue as the default DPI is 96 (and most phones are higher still anyway).

Comment 6

[Chris Lord [:cwiiis]]

No closer to finding the cause of this - current idea is that only mouse events are affected but touch events are ok... I've not nailed a good way of debugging this yet.

Comment 7

[Chris Lord [:cwiiis]]

I've come to the conclusion that only contextmenu and AsyncPanZoomController are affected, all other events are fine.

Looking into this today and taking a more structured approach, I've found:

- Both touch and mouse events have the correct coordinates and are delivered correctly
- The event when it gets to nsEventStateManager is correct
- The generated contextmenu event from nsEventStateManager is correct
* Somewhere between the dispatch in FireContextClick and the handling in nsPresShell::HandleEventInternal, the frame, content and ref point of the generated contextmenu event all get changed.

I'm tracing the event from the dispatch in nsEventStateManager now.

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Respect scale when getting mouse events from remote tabs

This fixes the bad contextmenu events for me and restores correct behaviour on the home-screen with apparently no other side-effects.

This doesn't fix AsyncPanZoomController.

Comment 9

[Chris Lord [:cwiiis]]

wrt AsyncPanZoomController, there seem to be assumptions, in Axis.cpp at least, that CSS pixels == Screen pixels, which is incorrect. I think fixing this will account for (all?) the issues here.

Comment 10

[Chris Lord [:cwiiis]]

heh, quite handily there's mDevPixelsPerCSSPixels on the FrameMetrics - this might be easier than I thought :)

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Respect scale in AsyncPanZoomController WIP

This is not the correct solution to this, but it makes the browser *almost* usable - correcting mCompositionBounds in GetCompositionLength fixes the page jerking about all over the place, dividing the origin makes the whole page accessible (plus a tiny little bit extra it seems)

I'm asking for feedback to get some more eyes on this - the issues here seems to be that mCompositionBounds is in screen coordinates and everything else is working in CSS pixels. I tried more elaborate generic fixes, but they never worked any better than this simple patch.

Dividing the origin is just bogus, it seems the origin of mCompositionBounds is incorrect? I haven't wrapped my head around the code in ThebesLayerComposite::GetCompositionBounds yet, so I don't know if there's anything going wrong there, or if it's something else.

I'd appreciate any advice on this - I imagine I'll get to the right solution eventually, but it'd really help to get comment from someone that knows this code.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I don't understand the Axis code especially well. Perhaps drs or kats may have some insight?

Comment 13

[Michael Wu [:mwu]]

I'm confused about the fix in TabChild::RecvMouseEvent -  who called SendMouseEvent? I couldn't follow the code backwards to the source, though I wasn't using gdb either.

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Michael Wu [:mwu] from comment #13)
> I'm confused about the fix in TabChild::RecvMouseEvent -  who called
> SendMouseEvent? I couldn't follow the code backwards to the source, though I
> wasn't using gdb either.

I forgot to make notes on this, and now I can't find it either :/ This is an ipdl thing, I'm assuming it happens any time synthetic event dispatch happens on the parent, but I'm failing to see by what mechanism. I was hoping you'd know :)

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 748835 [details] [diff] [review]
Respect scale in AsyncPanZoomController WIP

I've spent more time with this code, proper fix incoming.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Take pixel scale into account in AsyncPanZoomController

I think this could be a little neater, but it seems the whole class could do with being a bit neater really :/

Anyway, pretty much everything deals with CSS pixels rather than device pixels, except for FrameMetrics.mCompositionBounds (this is already documented). This patch corrects the assumption that 1 css pixel = 1 device pixel and fixes odd panning/zooming behaviour when GetDefaultScale on nsWindow != 1.0.

Comment 17

[Chris Lord [:cwiiis]]

I may as well take this bug now, I'd be in denial to say I haven't already...

Comment 18

[Michael Wu [:mwu]]

Comment on attachment 748578 [details] [diff] [review]
Respect scale when getting mouse events from remote tabs

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

<mwu> Cwiiis: I think it comes from AsyncPanController::OnLongPress
<mwu> Cwiiis: I was assuming RecvMouseEvent was coming from SendMouseEvent, but it was actually coming from one of the other Recv calls in TabChild.cpp

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Chris Lord [:cwiiis] from comment #16)
> I think this could be a little neater, but it seems the whole class could do
> with being a bit neater really :/

I agree. I'm trying to find some time to do that.

Comment 20

[Chris Lord [:cwiiis]]

(In reply to Michael Wu [:mwu] from comment #18)
> Comment on attachment 748578 [details] [diff] [review]
> Respect scale when getting mouse events from remote tabs
> 
> Review of attachment 748578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> <mwu> Cwiiis: I think it comes from AsyncPanController::OnLongPress
> <mwu> Cwiiis: I was assuming RecvMouseEvent was coming from SendMouseEvent,
> but it was actually coming from one of the other Recv calls in TabChild.cpp

It doesn't come from OnLongPress - AsyncPanZoomController is only used by the browser, it seems.

Comment 21

[Michael Wu [:mwu]]

Ok, I poked around again and it looks like contextmenu is generated entirely within TabChild.cpp. Looks like most of the logic is in TabChild::UpdateTapState, which is called from TabChild::RecvRealTouchEvent.

Comment 22

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

Comment on attachment 749212 [details] [diff] [review]
Take pixel scale into account in AsyncPanZoomController

I've been looking at this patch but it'll take me a bit longer to wrap my head around the code to the point where I'm comfortable reviewing it. Adding Anthony and Doug to the reviewer list in case they have some cycles and can get to it sooner. Anybody r+'ing can remove the other reviewers if they are confident enough in their review.

Comment 23

[Chris Lord [:cwiiis]]

Unassigning from me and marking as dependent on bug 865735- I think if we use all the right units, this bug will resolve itself.

Comment 24

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

Moving dependency to bug 879011 which hopefully will be the actual bug that adds type info to the touch events and fixes this.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Kartikaya,

We will have to fix this on b2g18-v1.1hd branch, and :mwu told me while these dependencies will fix this bug automagically on m-c, they are not upliftable to b2g18. Do you have a better idea for that? Thanks.

Comment 26

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

I'm hoping that as I continue to add unit information to this code and the bug is fixed, it will be easy to extract a patch that does the right conversion and apply it to b2g18-v1.1hd. I suspect the patch will end up looking a lot like Chris' patch but we'll have more confidence that it's correct.

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> I'm hoping that as I continue to add unit information to this code and the
> bug is fixed, it will be easy to extract a patch that does the right
> conversion and apply it to b2g18-v1.1hd. I suspect the patch will end up
> looking a lot like Chris' patch but we'll have more confidence that it's
> correct.

Thanks!

Comment 28

[Eric Rescorla (:ekr)]

The patch above only appears to be a partial fix.

Some pages work, but clicking through google results does not.

Comment 29

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

The patches on bug 883646 might fix some of this. I have ordered a hidpi B2G device and will try to fix the rest of these issues when it arrives.

Comment 30

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

So I have a HiDPI Peak device now but there are no actual STR in this bug as to what the problem is. Based on the description in comment 0 I reproduced an issue where long-pressing on an icon on the homescreen would act as though I long-pressed on a different icon. Is that the same issue? (Specifically on the Peak, assuming the icon in the top-left is (0,0), then long-pressing on the icon at (1,1) would actually trigger the long-press action on the icon at (2,2)).

Let's limit this bug's scope to this specific problem; please file other bugs for other issues with STR.

Comment 31

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> So I have a HiDPI Peak device now but there are no actual STR in this bug as
> to what the problem is. Based on the description in comment 0 I reproduced
> an issue where long-pressing on an icon on the homescreen would act as
> though I long-pressed on a different icon. Is that the same issue?
> (Specifically on the Peak, assuming the icon in the top-left is (0,0), then
> long-pressing on the icon at (1,1) would actually trigger the long-press
> action on the icon at (2,2)).
> 
> Let's limit this bug's scope to this specific problem; please file other
> bugs for other issues with STR.

Yes, this is that bug - the browser acts similarly, if you try to press a link at 10,10, the touch will actually occur at 15,15.

Comment 32

[pancake]

Just as a note from another Peak owner:

The DPI of the Peak is wrongly defined. Even the startup image (powered by mozilla) is blurry. I don't know if this can be part of this issue or it should be reported on a separated one.

Also, longtap event is not working depending on the distance from 0,0. and the swipe events seems to be the half of it, because switching between screens requires corner to corner swiping, not like in Keon.

Comment 33

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

(In reply to Chris Lord [:cwiiis] from comment #31)
> Yes, this is that bug - the browser acts similarly, if you try to press a
> link at 10,10, the touch will actually occur at 15,15.

I have been trying to reproduce this on my grid test page (http://people.mozilla.com/~kgupta/grid.html) but have not been able to reliably do so, at any zoom level. I do see a number of issues:
1) The wrong link gets highlighted when clicking
2) Sometimes clicking does nothing (can't reproduce this when I have the debugger attached, annoyingly enough)
3) Sometimes clicking the link will update the URL bar to the right thing but the page won't scroll to the grid point
4) Open in a new tab on of the grid links will just open the new tab to the top-left of the page instead of the grid point requested

These can be broken out into separate bugs so we can focus on the homescreen issue in this one.

Comment 34

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> So I have a HiDPI Peak device now but there are no actual STR in this bug as
> to what the problem is. Based on the description in comment 0 I reproduced
> an issue where long-pressing on an icon on the homescreen would act as
> though I long-pressed on a different icon. Is that the same issue?
> (Specifically on the Peak, assuming the icon in the top-left is (0,0), then
> long-pressing on the icon at (1,1) would actually trigger the long-press
> action on the icon at (2,2)).

I debugged this a teeny tiny bit in between other things, and I basically re-discovered what Cwiiis said in comment 0, that the contextmenu event target is wrong. More specifically, I found that the contextmenu pageX and pageY are different from the touchstart pageX and pageY for a single long-press on the icon grid.

contextmenu event: 196, 214
touchstart event: 131, 143

The difference is the 1.5 widget scaling factor, which is not very surprising. So the code that generates a contextmenu event from the touch events needs to be corrected. I didn't see the APZC long-tap code getting hit so I'm not sure that's where the fault lies.

Comment 35

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #34)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> > So I have a HiDPI Peak device now but there are no actual STR in this bug as
> > to what the problem is. Based on the description in comment 0 I reproduced
> > an issue where long-pressing on an icon on the homescreen would act as
> > though I long-pressed on a different icon. Is that the same issue?
> > (Specifically on the Peak, assuming the icon in the top-left is (0,0), then
> > long-pressing on the icon at (1,1) would actually trigger the long-press
> > action on the icon at (2,2)).
> 
> I debugged this a teeny tiny bit in between other things, and I basically
> re-discovered what Cwiiis said in comment 0, that the contextmenu event
> target is wrong. More specifically, I found that the contextmenu pageX and
> pageY are different from the touchstart pageX and pageY for a single
> long-press on the icon grid.
> 
> contextmenu event: 196, 214
> touchstart event: 131, 143
> 
> The difference is the 1.5 widget scaling factor, which is not very
> surprising. So the code that generates a contextmenu event from the touch
> events needs to be corrected. I didn't see the APZC long-tap code getting
> hit so I'm not sure that's where the fault lies.

I had a patch that fixed this, will try and track it down... There was some discussion on it too if I recall correctly.

Comment 36

[Chris Lord [:cwiiis]]

So comment #8 is where I discovered this - attachment #748578 [details] [diff] [review].

Comment 37

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

(In reply to Chris Lord [:cwiiis] from comment #36)
> So comment #8 is where I discovered this - attachment #748578 [details] [diff] [review]
> [diff] [review].

So this patch looks reasonable, but without adding proper type information to the event classes I can't be sure. In particular it also looks like it will change mousemove/mousedown/mouseup events dispatched from RecvHandleSingleTap, and the point there is in CSSPixels, so dividing by the widget scale factor doesn't make any sense.

Comment 38

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> (In reply to Chris Lord [:cwiiis] from comment #36)
> > So comment #8 is where I discovered this - attachment #748578 [details] [diff] [review] [diff] [review]
> > [diff] [review].
> 
> So this patch looks reasonable, but without adding proper type information
> to the event classes I can't be sure. In particular it also looks like it
> will change mousemove/mousedown/mouseup events dispatched from
> RecvHandleSingleTap, and the point there is in CSSPixels, so dividing by the
> widget scale factor doesn't make any sense.

right, I'd only suggest using the patch as inspiration rather than taking it as is :)

Comment 39

[:gerard-majax]

This patch is fixing the issue of context menu not popping and being unable to move icons on homescreen, but scrolling in the browser is still broken (Nexus S, b12g18hd).

Disabling AsyncPanZoom via this change circumvent this last issue:
 diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
 index 2638f41..bef4907 100644
 --- a/dom/ipc/TabParent.cpp
 +++ b/dom/ipc/TabParent.cpp
 @@ -1292,7 +1292,7 @@ TabParent::UseAsyncPanZoom()
    bool asyncPanZoomEnabled =
      Preferences::GetBool("layers.async-pan-zoom.enabled", false);
    return (usingOffMainThreadCompositing && asyncPanZoomEnabled &&
 -          GetScrollingBehavior() == ASYNC_PAN_ZOOM);
 +          GetScrollingBehavior() == ASYNC_PAN_ZOOM && !IsBrowserElement());
  }
  
  void


I'm not sure whether this deserves a separate bug (or if one already exists)

Comment 40

[Robert Kaiser]

(In reply to Alexandre LISSY :gerard-majax from comment #39)
> This patch is fixing the issue of context menu not popping and being unable
> to move icons on homescreen, but scrolling in the browser is still broken
> (Nexus S, b12g18hd).

As I mentioned in bug 885613, the browser seems fixed in that regard on Peak with m-c/master now, so I guess that's a different bug.

Comment 41

[pancake]

This !IsBrowserElement() patch doesnt seems to fix anything on BRANCH=master

Comment 42

[Michael Wu [:mwu]]

Attachment: Fix coordinates on contextmenu event

With some hints from kats, I hacked together something that fixes the homescreen while using the new point types so we're more sure about what's going on.

Comment 45

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Two dup from QA means this is blocker to them. I guess.

Comment 46

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

Comment on attachment 776104 [details] [diff] [review]
Fix coordinates on contextmenu event

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

Awesome, glad you were able to track this down.

Comment 47

[pancake]

(In reply to Michael Wu [:mwu] from comment #42)
> Created attachment 776104 [details] [diff] [review]
> WIP Fix coordinates on contextmenu event
> 
> With some hints from kats, I hacked together something that fixes the
> homescreen while using the new point types so we're more sure about what's
> going on.

Thanks for the patch. I'm really happy to finally see some advances on this issue. It was really anoying and your patch seems to work as expected. Now the keyboard opens when tapping entries and longtap/drag on homescreen works (very slow, but works).

Comment 48

[Michael Wu [:mwu]]

Comment on attachment 776104 [details] [diff] [review]
Fix coordinates on contextmenu event

This was a WIP because I wanted to change SendMouseEvent to use CSSPoint. Unfortunately, it's a xpcom interface so that's not so trivial..

Comment 50

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

Comment on attachment 776104 [details] [diff] [review]
Fix coordinates on contextmenu event

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

LGTM

Comment 51

[Michael Wu [:mwu]]

https://hg.mozilla.org/projects/birch/rev/b1d0825c97cc

Comment 52

[Michael Wu [:mwu]]

The bug summary was updated to reflect the reduced scope of this bug. The rest of this bug was fixed in bug 883646.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Really didn't mean to merge this yet. Hopefully it's green.
https://hg.mozilla.org/mozilla-central/rev/b1d0825c97cc

Comment 55

[Chris Lord [:cwiiis]]

We need to get this committed to b2g18 I guess for it to turn up in the 1.1.0hd branch - are you tracking that, Michael?

Comment 56

[Ryan VanderMeulen [:RyanVM]]

(In reply to Chris Lord [:cwiiis] from comment #55)
> We need to get this committed to b2g18 I guess for it to turn up in the
> 1.1.0hd branch - are you tracking that, Michael?

It needs to land on the b2g18_v1_1_0_hd branch, not b2g18. I haven't attempted to uplift this because I assumed that b2g18_v1_1_0_hd would require a patch that was significantly different from what landed on m-c.

Comment 57

[Michael Wu [:mwu]]

Attachment: Fix coordinates on contextmenu event (1.1.0hd)

I haven't had a chance to test this patch, but this is essentially what needs to be done on 1.1.0hd.

Comment 58

[Michael Wu [:mwu]]

(In reply to Michael Wu [:mwu] from comment #57)
> Created attachment 777935 [details] [diff] [review]
> Fix coordinates on contextmenu event (1.1.0hd)
> 
> I haven't had a chance to test this patch, but this is essentially what
> needs to be done on 1.1.0hd.

This patch works. We can land it.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/caa71850224d

Comment 61

[Swaroopa Vantipalli [:swaroopa]]

Attachment: Flickering browser

Bug is still reproducible. Tested on Helix device and Build:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2bb08f2878da
Gaia   1cb40adb9478f91a7ef0f43928e2471b8fb10bf0
BuildID 20130724070202
Version 18.0

Steps:

1. Opened youtube.com and try to scroll down the list of videos and the browser starts flickering.(Attached video. Can see the flickering at 00:08)
2. I try to click on any of the videos and it takes a number of taps to open a video.

3. Open browser and from the search bar, search with any text.
4. Try to click on any of the result links. When I click on one link, another link which is below the one I selected opens. Probably still picking up the wrong coordinates.

Comment 62

[Jason Smith [:jsmith]]

Swaroopa - can you file a followup bug for what you found in comment 61?

Comment 63

[Swaroopa Vantipalli [:swaroopa]]

Filed a new bug: 897688