Last Comment Bug 774989 - Cross-process touch coordinates are ... inconsistent
: Cross-process touch coordinates are ... inconsistent
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-17 23:05 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-19 07:33 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Workaround (2.02 KB, patch)
2012-07-17 23:42 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Transform touch-event coordinates for remote content (5.10 KB, patch)
2012-07-18 04:09 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bugs: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 23:05:06 PDT
In b2g, the DOM setup looks something like

 <xul:window fullscreen>             // @x=0, y=0 in screen space
  <html:iframe id="system">          // @x=0, y=0
    <html:iframe id="browser-app">   // @x=0, y=20
      <html:iframe id="content">     // @x=y, y=70

I'm seeing touch points inside the "content" iframe at the wrong coordinates.  Synthetic mouse events dispatched from the parent process have the correct coordinates.

If I touch and drag a little near the top-left of the content iframe, I see a sequence of events like

  touchstart = { refpoint: x=0, y=-70 }
  touchmove = { refpoint: x=0, y=0, touches: [ x=0, y=70 ] }
  ...
  touchend = { refpoint: x=0, y=0 }

The touch points always seem to be in screen space.  That would be OK, except that the refpoint is inconsistent --- if it were always the frame offset to the widget, like it is for touchstart, then I could use that to move the points into the right space.

I'm not really sure where to start here --- wesj, smaug any hints?
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 23:22:43 PDT
Random wild guess --- I wonder if the problem might be that some code is doing something like

    nsTouchEvent touchEvent(*static_cast<nsTouchEvent*>(aEvent));

and expecting a deep copy of the touch points, but in fact we're only shallowly copying the pointers to nsIDOMTouch ...
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-17 23:42:15 PDT
Created attachment 643263 [details] [diff] [review]
Workaround

This patch fixes things, but is of course horrible.

Does this give you guys any hints about where to look for a real fix?
Comment 3 Olli Pettay [:smaug] 2012-07-18 00:51:33 PDT
Felipe, don't we translate mouse event coordinates somewhere?
Comment 4 :Felipe Gomes (needinfo me!) 2012-07-18 02:00:38 PDT
In fact, we do change the mouse event coordinates to translate them to the target's coordinates before sending them, here:

http://hg.mozilla.org/mozilla-central/annotate/9799dad7067b/content/events/src/nsEventStateManager.cpp#l1738

So yeah, this adjustment is also probably needed for each touch point, which I didn't realize before

Chris, also note that you do that shallow copy here: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1045701fee9#l6.15
But if the nsIDOMTouch points are read correctly in the serializer I don't expect that copy to be a problem (as they should arrive with the right coordinates)


I would guess the difference of behavior between event types is related to these two parts:

>  if (aEvent->eventStructType != NS_TOUCH_EVENT ||
>      aEvent->message == NS_TOUCH_START) {
...
>  } else {

and

>    nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, aTargetFrame);

The coordinates translation here uses aTargetFrame, which is used in the if branch for touchstart, but for touchmove branch the target is different..?
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 02:37:40 PDT
(In reply to Felipe Gomes (:felipe) from comment #4)
> In fact, we do change the mouse event coordinates to translate them to the
> target's coordinates before sending them, here:
> 
> http://hg.mozilla.org/mozilla-central/annotate/9799dad7067b/content/events/
> src/nsEventStateManager.cpp#l1738
> 

Aha!  That's exactly what I needed, thanks.
 
> Chris, also note that you do that shallow copy here:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d1045701fee9#l6.15
> But if the nsIDOMTouch points are read correctly in the serializer I don't
> expect that copy to be a problem (as they should arrive with the right
> coordinates)
> 

Yes, this is another potential instance of that problem.  In general, any code like

  nsTouchEvent ev;
  Dispatch(ev);
  // do something with ev

that expects |ev| not to have changed, might not work.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 03:09:40 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> (In reply to Felipe Gomes (:felipe) from comment #4)

>   nsTouchEvent ev;
>   nsTouchEvent ev;

Sorry, I meant

  nsTouchEvent copy(ev);
  Dispatch(copy)
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 04:09:32 PDT
Created attachment 643313 [details] [diff] [review]
Transform touch-event coordinates for remote content
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-18 12:43:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1da77bc0bc2
Comment 9 Ed Morley [:emorley] 2012-07-19 07:33:26 PDT
https://hg.mozilla.org/mozilla-central/rev/c1da77bc0bc2

Note You need to log in before you can comment on or make changes to this bug.