Context menu events are wonky on hidpi devices

VERIFIED FIXED

Status

defect
P1
blocker
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: cwiiis, Assigned: mwu)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:hd+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 wontfix, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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).
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...
This could be that the AsyncPanZoomController or GestureEventListener needs to handle scale... Would explain why some events are ok and others aren't.
Depends on: 845182
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).
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.
(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).
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.
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.
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.
Attachment #748578 - Flags: feedback?(mwu)
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.
heh, quite handily there's mDevPixelsPerCSSPixels on the FrameMetrics - this might be easier than I thought :)
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.
Attachment #748835 - Flags: feedback?(ajones)
I don't understand the Axis code especially well. Perhaps drs or kats may have some insight?
Flags: needinfo?(bugzilla)
Flags: needinfo?(bugmail.mozilla)
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.
(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 on attachment 748835 [details] [diff] [review]
Respect scale in AsyncPanZoomController WIP

I've spent more time with this code, proper fix incoming.
Attachment #748835 - Flags: feedback?(ajones)
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.
Attachment #748835 - Attachment is obsolete: true
Attachment #749212 - Flags: review?(bugmail.mozilla)
Flags: needinfo?(bugzilla)
Flags: needinfo?(bugmail.mozilla)
I may as well take this bug now, I'd be in denial to say I haven't already...
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
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
Attachment #748578 - Flags: feedback?(mwu)
(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.
(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.
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 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.
Attachment #749212 - Flags: review?(bugzilla)
Attachment #749212 - Flags: review?(ajones)
Unassigning from me and marking as dependent on bug 865735- I think if we use all the right units, this bug will resolve itself.
Assignee: chrislord.net → nobody
Status: ASSIGNED → NEW
Depends on: 865735
Attachment #749212 - Attachment is obsolete: true
Attachment #749212 - Flags: review?(bugzilla)
Attachment #749212 - Flags: review?(bugmail.mozilla)
Attachment #749212 - Flags: review?(ajones)
blocking-b2g: --- → hd+
Moving dependency to bug 879011 which hopefully will be the actual bug that adds type info to the touch events and fixes this.
No longer depends on: 865735
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.
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
The patch above only appears to be a partial fix.

Some pages work, but clicking through google results does not.
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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #34)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
So comment #8 is where I discovered this - attachment #748578 [details] [diff] [review].
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :)
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)
(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.
This !IsBrowserElement() patch doesnt seems to fix anything on BRANCH=master
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.
Two dup from QA means this is blocker to them. I guess.
Severity: normal → blocker
Priority: -- → P1
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.
Attachment #776104 - Flags: feedback+
(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 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..
Attachment #776104 - Attachment description: WIP Fix coordinates on contextmenu event → Fix coordinates on contextmenu event
Attachment #776104 - Flags: review?(bugmail.mozilla)
Duplicate of this bug: 894202
Comment on attachment 776104 [details] [diff] [review]
Fix coordinates on contextmenu event

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

LGTM
Attachment #776104 - Flags: review?(bugmail.mozilla) → review+
Assignee: nobody → mwu
Summary: Various touch events (such as contextmenu and browser scrolling) are wonky on hidpi devices → Context menu events are wonky on hidpi devices
The bug summary was updated to reflect the reduced scope of this bug. The rest of this bug was fixed in bug 883646.
Really didn't mean to merge this yet. Hopefully it's green.
https://hg.mozilla.org/mozilla-central/rev/b1d0825c97cc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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?
Flags: needinfo?(mwu)
(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.
I haven't had a chance to test this patch, but this is essentially what needs to be done on 1.1.0hd.
Flags: needinfo?(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.
Duplicate of this bug: 895312
Posted video 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Swaroopa - can you file a followup bug for what you found in comment 61?
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Filed a new bug: 897688
Status: RESOLVED → VERIFIED
Duplicate of this bug: 896941
You need to log in before you can comment on or make changes to this bug.