Closed Bug 823619 Opened 12 years ago Closed 12 years ago

Make mouse events synthesized from touch events comply with spec (in apps)

Categories

(Core :: DOM: Events, defect, P1)

Product:
Core
Component:
DOM: Events
Platform:
All
Gonk (Firefox OS)
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
B2G C4 (2jan on)
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: cjones, Assigned: cjones)

References

Details

(Whiteboard: [LOE:S])

Attachments

(4 files, 23 obsolete files)

event-test.html
1.20 KB, text/html
Details
link to patch on github
108 bytes, text/html
djf
: review+
Details
Patch for landing on mozilla-central
35.22 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Patch for landing on aurora/b2g18
35.48 KB, patch
cjones
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #774458 +++

Updating this to track reality.  This blocks numerous blockers.  We need to make a decision based on below statim.  Leaving blocking+ for decision.

I think we should very seriously consider this.  To do that we need to estimate the work to convert gaia.

Left on the table if we decide not to do this is
 - breaking touch event standard and being stuck with that "forever".  Fixing that is currently blocking+.
 - compatibility fallout from that
 - different behavior in "browser" and "app" content, visible to authors
 - bug 814252 and several blocking+ followups that require it
 - bug 819593, blocker, and bug 819595, soft-blocker currently

So my initial idea was to make a shim that implements the necessary part of the pointer events proposal (not much).  This is a straightforward change and is somewhat forwards compatible.

My concern is that the gesture detector is overkill for the users who just need sane down/move*/up events for the primary touch point, and it moves more of platform "look and feel" into gaia.  I know we've been losing that battle bigtime for v1, but I don't want to give more ground ;).

We have 54 references to mousemove, which probably means <27 real users.  That's a lot.

The remaining data I'm going to gather is how painful converting a few to pointer events would be.  Let's say homescreen and lockscreen.

djf, if you want to do the same with gesture_detector, please do.  That's another option we can consider.
Attached file wip synthetic mouse event shim library (obsolete) — 
This is a complete work in progress, but I'll go ahead and try to test it.
Attachment #694494 - Flags: feedback?(jones.chris.g)
FTR, I think we all agree at this point that directly converting all the gaia apps that rely on the buggy current gecko behavior to spec compliance is infeasible for v1.
Comment on attachment 694494 [details]
wip synthetic mouse event shim library

This is pretty much what I had, except using pointer.  This approach will result in a smaller gaia patch so let's do it.
Attachment #694494 - Flags: feedback?(jones.chris.g) → feedback+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> https://bugzilla.mozilla.org/attachment.cgi?id=694092 is a WIP of what we
> need here.

Correction: https://bug766066.bugzilla.mozilla.org/attachment.cgi?id=693969
Attached patch WIP-TabChild-mouse-events.patch (obsolete) — — Splinter Review 
This patch was originally attached to bug 766066.
This patch was originally attached to bug 766066.
For reference, here is the Pointer Events spec that Microsoft submitted to the W3C:
http://www.w3.org/Submission/pointer-events/
(In reply to David Flanagan [:djf] from comment #2)
> Created attachment 694494 [details]
> wip synthetic mouse event shim library

mbrubeck suggested we ignore multitouch events (event.touches.length > 1). It avoids some shim complexity and most people have only one mouse. :) He is taking this approach for the Firefox Metro port:

https://hg.mozilla.org/projects/elm/rev/b36e74e1a195#l10.240

He also wondered whether calling preventDefault() on touchstart or touchmove might break panning. Just something to watch out for.

re mouseover/mouseenter/mouseout/mouseleave: grepping through Gaia's apps, external-apps, and shared js shows:

1 mouseover (pdfjs)
0 mouseenters
2 mouseouts (pdfjs and sms)
3 mouseleaves (contacts, dialer, and value_selector)
Attached file event-test.html — 
Here is a simple test I used compare browsers' touch and mouse event behavior.
I'm attaching an updated version of my synthetic events shim. This one actually works.

Here's what I've learned so far.

1) cpeterson's TabChild mouse events patch (attached to this bug) does not apply cleanly to today's m-c, but it is trivial to fix the rejected hunk.

2) Touch events don't work correctly in today's m-c tree, but with these patches applied, they do work:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7f3548011c and https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbe2966ec8a

3) With those three patches applied, I cannot pan the homescreen anymore. This is expected

4) If I add my synthetic_mouse_events.js shim to apps/homescreeen/index.html, then I can pan the homescreen

5) contextevents are not working in my build of gecko. I don't know if that is the fault of cpeterson's patch or the other two patches I applied, or if they are broken for some other reason. But it means that I can't test the homescreen code that drags app icons around the screen.

6) With these gecko patches applied, I can't set an alarm in the Clock app: I can't swipe up and down on the numbers to select the time I want.  This is expected.

7) If I add my synthetic_mouse_events.js shim to apps/clock/index.html, then the numbers start moving again.  But they move much, much too slowly. The reason is that the clock code is computing the velocity of my swipe by comparing the timestamps of the mousedown and mouseend events. Real gecko events have timestamps in milliseconds. But synthetic ones (created with document.createEvent()) use microseconds.  So the time interval comes out 1000 times too big.  I work around this in gesture detector.js.  The clock app will need to include a similar workaround to determine the timestamp units so that it works both on the desktop (with native mouse events) and on the phone (with synthetic mouse events).
Attachment #694494 - Attachment is obsolete: true
(In reply to David Flanagan [:djf] from comment #11)
> Created attachment 694562 [details]
> synthetic mouse event shim, wip version 2
> 
> 5) contextevents are not working in my build of gecko. I don't know if that
> is the fault of cpeterson's patch or the other two patches I applied, or if
> they are broken for some other reason.

contextmenu is synthesized based on waiting for a delay between mousedown and mouseup.

Hmmmmmmm ... that's a bad one.  I think we'd need to fix cpeterson's patch to do that.

cpeterson, is that feasible?

Not looking good here ... :(
More experimental results:

8) The FM Radio app uses the same basic code as the Clock does for the radio tuner, so it also needs the divide the time interval by 1000 fix that the clock needs

9) The Music app listens for mousemove events for seeking within the song. But it accepts any mousemove not just mousemoves followed by clicks. So on desktop it responds to the mouse moving over it, and on the phone, you have to tap.  Taps work both with and without these patches.  When I add my library nothing seems to change. I'm not sure whether anything needs to be fixed for this app or not.

10) The video app doesn't allow you to drag the play head to seek through the video with these patches applied. Adding synthetic_mouse_events.js seems to fix it.

11) The camera and gallery can also play videos, but the UI they use is based on gesture_detector.js and it seems to work okay without changes.
David, can emitEvent() fudge the MouseEvent time like this?

    var synthetic = document.createEvent('MouseEvents');
    synthetic.timeStamp /= 1000;
(In reply to Chris Peterson (:cpeterson) from comment #14)
> David, can emitEvent() fudge the MouseEvent time like this?
> 
>     var synthetic = document.createEvent('MouseEvents');
>     synthetic.timeStamp /= 1000;

I see the answer is no. :\
More:

12) With cpeterson's patch, there is no way to answer an incoming call from the lock screen, because we can't swipe up.  But with my shim it works.
(In reply to Chris Peterson (:cpeterson) from comment #15)
> (In reply to Chris Peterson (:cpeterson) from comment #14)
> > David, can emitEvent() fudge the MouseEvent time like this?
> > 
> >     var synthetic = document.createEvent('MouseEvents');
> >     synthetic.timeStamp /= 1000;
> 
> I see the answer is no. :\

The way I worked around this in gesture detector is to assume that the timestamp is actually based on the current time. Then if e.timestamp > 2*Date.now() divide it by 1000.

It seems to work, though I should probably double-check my assumption that these timestamps are about 1000*Date.now()
Comment on attachment 694502 [details] [diff] [review]
WIP-TabChild-mouse-events.patch

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>+  // If the user single-taps (touchstart followed by touchend), then send
>+  // synthetic mousemove, mousedown, and mouseup events. mousedown will
>+  // trigger a focus event. mouseup will trigger a click event.
>+  if (aEvent.message == NS_TOUCH_END && mLastTouchEvent == NS_TOUCH_START) {
>+    nsIntPoint refPoint(0, 0);
>+    if (aEvent.touches.Length()) {
>+      refPoint = aEvent.touches[0]->mRefPoint;
>+    }
>+
>+    DispatchSynthesizedMouseEvent(NS_MOUSE_MOVE, aEvent.time, refPoint);
>+    DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, aEvent.time, refPoint);
>+    DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, aEvent.time, refPoint);

If the duration that elapses between the touchstart and touchend is greater than ui.click_hold_context_menus.delay, then I /think/ we can just also dispatch a contextmenu event here.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> If the duration that elapses between the touchstart and touchend is greater
> than ui.click_hold_context_menus.delay, then I /think/ we can just also
> dispatch a contextmenu event here.

Building this into EventStateManager behind a pref seems like a valid thing to do too. Doing it with mouseevents is already there behind a similar pref.
I really wish we had done that a long time ago, but I'm afraid that would require teaching a lot of ESM about touch events.
This new attachment is a link to a github pull request that includes the shared/js/synthetic_mouse_events.js shim and changes to these apps to make them use it:

homescreen
video
clock
fm radio
dialer (incoming calls)

I still need to update the shim to do mouseover/out and mouseenter/leave events.

There are still issues with scrolling not working and contextmenu events not working.  And the gesture detector-based Gallery app is getting double clicks when I just tap once and I haven't tried to figure that out yet.

It would be interesting to know if those bugs are side effects of cpeterson's patch or if they are distinct bugs in m-c.
Attachment #694562 - Attachment is obsolete: true
If I apply the github pull request in the attachment to a nightly b2g18 build from pvtbuilds.mozilla.org I find that the apps modified to work with the events shim still seem to work. 

But, and this is a big bug: the shim breaks context menus and scrolling in the apps that use it.

In this b2g18 build, contextmenu events work in apps (like Music) that do not use the shim and scrolling works in apps (like Gallery) that do not use the shim.

But the contextmenu event does not work in Homescreen (which uses the shim) and scrolling does not work in Video (which uses the shim).

So the fact that the shim calls preventDefault() on all of the touch events it processes means that the apps using it don't get duplicate mouse events. But it also means that other important side effects of those events get prevented.

So: it is not safe to land synthetic_mouse_events.js in gaia without landing the gecko change at the same time.
As we discussed on IRC, we need bug 814252 to fix the scrolling.
I've applied the patch from bug 814252 and it indeed fixes scrolling for apps that do not use my shim.  But it looks like apps that do use my shim have broken scrolling.

So calling preventDefault on the touch events may not be the right thing to do.

Does the code in 814252 handle the events after content handles them and does it honor defaultPrevented?

My goal was to prevent gecko from generating mouse events for taps, so I've called preventDefault() on everything.  But maybe I should be not prevent any events, and only emit mouse events when there is a touchmove?  If no move, then just let gecko handle it the mouse events. Otherwise, if there is a touch move, then generate all the events myself.  Does that sound like it could work?
(In reply to David Flanagan [:djf] from comment #24)
> I've applied the patch from bug 814252 and it indeed fixes scrolling for
> apps that do not use my shim.  But it looks like apps that do use my shim
> have broken scrolling.

To be clear, is this with cpeterson's patch applied?

> Does the code in 814252 handle the events after content handles them and
> does it honor defaultPrevented?

It honors preventDefault().  It should run after content handlers, but I'm not 100% sure.

> My goal was to prevent gecko from generating mouse events for taps, so I've
> called preventDefault() on everything.  But maybe I should be not prevent
> any events, and only emit mouse events when there is a touchmove?  If no
> move, then just let gecko handle it the mouse events. Otherwise, if there is
> a touch move, then generate all the events myself.  Does that sound like it
> could work?

After cpeterson's patch, there shouldn't be any mouse events (except for tap).
1. Here is a WIP patch that synthesizes an NS_CONTEXTMENU event from TabChild.cpp. I compare the elapsed touch time and the "ui.click_hold_context_menus.delay" pref value.

2. WARNING! Unfortunately, I have not actually been able to test the context menu with my patch! When I press and hold on the homescreen, the homescreen app crashes and restarts. I suspect the problem is a regression from the bug 814252 patch I applied because the homescreen still crashes if I comment out my NS_CONTEXTMENU changes.

3. btw, "ui.click_hold_context_menus.delay" pref's default value is 1000 ms, which feels like a really long time. Android's default long press delay is 500 ms: https://github.com/android/platform_frameworks_base/blob/a454767b09ecb7d25d00beae0e5a1fdd48605c63/core/java/android/view/ViewConfiguration.java#L80
Attachment #694502 - Attachment is obsolete: true
Attachment #694669 - Flags: feedback?(jones.chris.g)
Attachment #694669 - Flags: feedback?(dflanagan)
Comment on attachment 694669 [details] [diff] [review]
WIP-TabChild-mouse-events-v3.patch

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

::: dom/ipc/TabChild.cpp
@@ +1380,5 @@
> +
> +        DispatchSynthesizedMouseEvent(NS_MOUSE_MOVE, time, refPoint);
> +
> +        if (aEvent.time > mLastTouchStartTime + mContextMenuDelay) {
> +          DispatchSynthesizedMouseEvent(NS_CONTEXTMENU, time, refPoint);

I don't think you can wait for the touchend event like this. Contextmenu has to fire after 1000ms (or whatever the interval is) even if the user's finger is still down, right?

It seems like it would be a lot easier to do in js than in c++, but I don't understand the layers of gecko and chrome well enough to know why we can't just modify the code that is currently generating the contextmenu event.
Attachment #694669 - Flags: feedback?(dflanagan) → feedback-
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> (In reply to David Flanagan [:djf] from comment #24)
> > I've applied the patch from bug 814252 and it indeed fixes scrolling for
> > apps that do not use my shim.  But it looks like apps that do use my shim
> > have broken scrolling.
> 
> To be clear, is this with cpeterson's patch applied?
> 

Yes.

> > Does the code in 814252 handle the events after content handles them and
> > does it honor defaultPrevented?
> 
> It honors preventDefault().  It should run after content handlers, but I'm
> not 100% sure.

Okay, so calling preventDefault() is the wrong thing to do. I'll take that out.

> > My goal was to prevent gecko from generating mouse events for taps, so I've
> > called preventDefault() on everything.  But maybe I should be not prevent
> > any events, and only emit mouse events when there is a touchmove?  If no
> > move, then just let gecko handle it the mouse events. Otherwise, if there is
> > a touch move, then generate all the events myself.  Does that sound like it
> > could work?
> 
> After cpeterson's patch, there shouldn't be any mouse events (except for
> tap).

Right, but my shim duplicates those events, so I was calling preventDefault hoping that I could make cpeterson's mouse events go away.  I realize now that I was completely misunderstanding the layers of event code in gecko.  There's no way I can cancel those events so I need to not generate them.

It makes your suggestion that we use pointer events seem like an appealing approach.  But I'll try again with this shim and see if I can modify it to just supply the missing mouse events
Ugh.  cpeterson's code is correct.  I think the gecko bowels are wrong here.  preventDefault()ing the touchend should work though.
I've simplified my synthetic_mouse_events.js shim and pushed a new version of the PR to github.  This one does not try to prevent default anything, and only generates mouse events when there is a touchmove.  So touchstart/touchend are handled by gecko.  If there is a touchmove in between, then the shim kicks in and supplies the mouse events.

With the TabChild patch and the scrolling patch applied, this shim makes the homescreen, clock, fm radio and video apps work as before, and it also allows scrolling to work.
Oh, actually we should be checking |nsIPresShell::gPreventMouseEvents| and "default prevented".  So the gecko bowels should be ok.
The latest version of my shim supports mouseover/mouseout events. No apps use mouseover, but the sms app uses mouse out.  I've just tested, and it works as it should with the shim.  (Though the use of mouseout is kind of bogus and the code should probably be changed.)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> Oh, actually we should be checking |nsIPresShell::gPreventMouseEvents| and
> "default prevented".  So the gecko bowels should be ok.

Are you saying that if I call preventDefault() on a touchstart event in content, that should get back to the TabChild.cpp code and prevent the generation of the mouse events?

If so, I wonder if my shim could call preventDefault() on touchstart to prevent the mouse events there but could still have scrolling work even with that initial touchstart event prevented...

The SMS app tries to use a mousedown event to highlight a button, to show that it is active.  It would work in the desktop model, but with the TabChild patch, the mousedown doens't get generated until after the touchend, so the highlight never shows up.  If my shim could cancel the tab child stuff without breaking scrolling, then the shim could send a mousedown event right after the touchstart event.
Okay, if I call preventDefault() on the touchstart() event then it does seem to actually stop Gecko from sending mouse events.  (It makes it very, very hard to click on icons from the homescreen, but once in 20 times I can actually launch an app; I'm not sure what is happening, but it does seem that preventDefault does the right thing here.)

Unfortunately, calling preventDefault() on that initial touchstart also prevents scrolling, so I can't use that approach to always send a mousedown right after the touchstart.
We got an alternate fix for bug 814252.  That changes the calculus here significantly, since its tree of blockers are no longer blocked by this.

cpeterson's patch has some issues I'm going to take about an hour to fix.  If closing this out looks like more than a person-day of work (including gaia) with neglible risk, we might need to cut our losses for bug 819595.
https://github.com/mozilla-b2g/gaia/pull/7120 now includes two versions of my shim.

The first version is in shared/js/synthetic_mouse_events.js. It only synthesizes events when it sees a mousemove, and lets Gecko send synthetic events for tap gestures. It never calls preventDefault() on anything.  This first version has been tested with cpeterson's first version of the ChildTab patch.

The second version is shared/js/synthetic_mouse_events2.js.  It calls preventDefault() on touchend events that do not have a touchmove before them, and in doing so tries to get Gecko not to send synthetic events.  Since it does this, it synthesizes its own mouse events for all touch events. This means that it can send a mousedown as soon as it sees the touchstart. It doesn't need to wait for the touchend to know whether it is a tap or not.  In this case, the shim has to decide whether or not to send a click event, since it won't be automatically generated from the synthetic mousedown/mouseup pair. This second version has not been tested, but is intended for use with cjones's modification of the ChildTab patch.

The github pull request also includes modifications of some gaia apps to make them use the shim.

Neither version of the shim does mouseenter/mouseleave events yet, but I can add that pretty easily if we need it.
We're possibly looking at a backout of bug 766066, so the calculus changes again.

I was testing this while those bugs were being filed.  Environment
 - build from 42213ca59024 (that is, without bug 766066)
 - apply 8cbe2966ec8a
 - apply cd7f3548011c

The patches here fix all the bugs that this blocks.

But it's not all roses.  There are regressions with both the v1 and v2 synthetic_mouse_event files (oddly).

 1. homescreen fast-pan.  Pan between pages as fast as you can, nothing happens.  Major user-perceived perf issue.
 2. dialer key long-press.  No tone, no visual indicator.  Certification blocker.
 3. as expected, tap detection is bad with the heuristics in synthetic_mouse_event
 5. bug 821583
 6. bug 822590
 7. bug 822592

My guess is that (1) is a timestamp issue.  That's scary.

The rest look like broken gaia code that should be easily fixable.

I think we can spare a few (but no more!) more person-hours on quick analysis of above.  If (1) can't be fixed, we abort, for sure.  I'll try to look before I collapse.
calling prevent default on touchend should not do anything as per the touchevents spec.
Chris,

Looks like the homescreen panning issues is apps/homesscreen/js/grid/js:207
Add a divide by 1000 and I bet it works.  My fix in the other apps that have this issue is to test the timestamp to see if it is much bigger than Date.now(). If so, divide by 1000.

cpeterson suggests a utility function bundled with the shim for getting timestamps from events.  Now that we need that in at least 3 places in the code, I think it is a good idea.
Note, if you use event.timestamp information for anything, .timestamp will change at some point.
Gecko has had buggy .timestamp since the beginning and so far it hasn't been too critical to fix
(in other words, there has always been something more important).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #39)
> We're possibly looking at a backout of bug 766066, so the calculus changes
> again.
> ...
> I think we can spare a few (but no more!) more person-hours on quick
> analysis of above.  If (1) can't be fixed, we abort, for sure.  I'll try to
> look before I collapse.

What is the current thinking? Should I back out bug 766066?
(In reply to Wesley Johnston (:wesj) from comment #40)
> calling prevent default on touchend should not do anything as per the
> touchevents spec.

Do you know what the motivation for that is?  If it's important, then I don't believe we'll be able to make a working mouseevent shim.
Attached patch Rollup (obsolete) — — Splinter Review 

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #44)
> (In reply to Wesley Johnston (:wesj) from comment #40)
> > calling prevent default on touchend should not do anything as per the
> > touchevents spec.
> 
> Do you know what the motivation for that is?  If it's important, then I
> don't believe we'll be able to make a working mouseevent shim.

The primary goal of the touch events v1 spec was just to document the behavior of the iPhone's browser. This is one of those "quirks" of it. TBH, I'd be fine with removing this restriction for us, although its non-trivial and might lead to larger delays between taps turning into clicks (since we'd have to wait for content to make that decision).

The iPhone only allows preventDefault on touchstart or the first touchmove associated with a point, presumably to keep pages from blocking async panning.

    
    

    
  
To set up

$ hg up -r 42213ca59024
$ hg qimport -n touch-hell https://bugzilla.mozilla.org/attachment.cgi?id=694933

then rebuild.

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #44)
> (In reply to Wesley Johnston (:wesj) from comment #40)
> > calling prevent default on touchend should not do anything as per the
> > touchevents spec.
> 
> Do you know what the motivation for that is?  If it's important, then I
> don't believe we'll be able to make a working mouseevent shim.

I don't know the motivation. I wouldn't want to deviate from spec on the touchdown or touchmove events.  But it seems safer to deviate for touchend.

If we decide to abide by the spec we can still create a shim that works for most apps. But mousedown events will be delayed until after a touchmove or touchend.  This means that longpress in the dialer won't work and we'll have to manually convert that app to use touch events.  Shouldn't be too hard.  The shim will still work for swipes, because they get move events right away.

    
    

    
  
Remaining questions

 1. Why is the homescreen fast-panning not working
 2. Why are dialer long-presses not working
 3. Why is the gallery edit-mode slider not working

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> To set up
> 
> $ hg up -r 42213ca59024
> $ hg qimport -n touch-hell
> https://bugzilla.mozilla.org/attachment.cgi?id=694933
> 
> then rebuild.

Also, I needed to add

 $ hg qpush

    
    

    
  
One more difficult test case (bug 821583)

STR:
====================
  0. 2012/12/14 mozilla-central build.
  1. Add a new alarm (the time picker is still working).
  2. Reset that alarm.
  3. When entering the time picker page that contains a "Delete" button below.

Actual:
====================
  When scrolling the timer picker, the whole page will be scrolled, thus failing to set the time.

Expected:
====================
  The whole page shouldn't be scrolled.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rollup (obsolete)
        — 
          — Splinter Review
      

    


  
qparent, dammit.

        Attachment #694933 -
        Attachment is obsolete: true

    
    

    
  
I can't reproduce the alarm bug in comment 51.  Last time I used the clock, I saw what you meant about the alarm button off the bottom of the screen. But now everything fits on the screen, so there is nothing to scroll.

With the latest version of my shim I do see another bug however: each alarm is getting set twice.  So there must be a duplicate event.


(In reply to Chris Jones [:cjones] [:warhammer] from comment #49)
> Remaining questions
> 
>  1. Why is the homescreen fast-panning not working

Calling elementFromPoint() on each touchmove event is making the frame rate substantially slower.

The shim could make that optional, but it still might need to be ported to use touch events natively for maximum speed.

>  2. Why are dialer long-presses not working

Long presses are working for me.  But, as with the clock above, dialing is very difficult because I'm getting duplicate events.  More shim debugging is needed.

>  3. Why is the gallery edit-mode slider not working

The exposure slider and the crop controls all use mouse events (for desktop compatibility).  But Gallery also uses gesture_detector, and including the shim along with gesture detector seems to cause problems.

So that code might need to be ported by hand.  Or I need to make the shim and the gesture detector play nicely together.

    
    

    
  
Argh. Its stopImmediatePropagation(), not preventImmediatePropagation()! That would explain all the double clicks

    
    

    
  
I've fixed the double alarm and double dial problems by using stopImmediatePropagation.

Dialer is one of the apps that depends on mouseleave events and the shim doesn't send those yet. So it is back to the state where if you start a long press and then slide your finger to another key and release, the tone doesn't stop sounding.  Bad bug, but we know how to fix it.

    
    

    
  
Once again, I've updated my pull request at https://github.com/mozilla-b2g/gaia/pull/7120

The shim has a new name: shared/js/mouse_event_shim.js

All of the modified apps should now be using that new version of this shim. I've left the old versions around for reference in case they work better.

This new version applies cpeterson's trick of capturing and discarding the real mouse events from gecko.

This new version also defines a global MouseEventShim object.  Set MouseEventShim.trackMouseMoves to false if you don't want the overhead of elementFromPoint().  You should be able to flip this switch between any two events but that is not tested.

Use MouseEventShim.getEventTimestamp() to portably get a timestamp value in milliseconds.  The pull request uses this utility in homescreen, clock and fm radio

Other updates:

This shim and gesture detector do seem to work okay together afterall.  Adding the shim to gallery fixes the slider and crop handles without seeming to break anything.  This is the best I've seen gallery working.

Long presses are working in dialer, though it still needs mouseleave support in the shim, or the dialer needs to be converted to use mouseout instead

I can't reproduce the alarm clock bug.  The clock app might need to call preventDefault on the mousedown event it handles, and that should now get passed on.

Moving icons around on the homescreen isn't working right. This may be a scrolling while dragging issue... Maybe a preventDefault() call would fix that. The shim does not propagate preventDefault calls on mousemove events to the touchmove. I think maybe it needs to do this.

And with that, I'm out of time.  Back at work full time on the 27th.

    
    

    
  
We're already in for about 2 person-days investment here, and in exchange we're looking at 5 (maybe more) gaia blockers that we probably just can't fix otherwise.  Engineering time on hacks has been proven to fail twice in the current setup, so probably not worth investing.  Spec compliance is also on the table.  Thankfully, sunk costs in backed-out hackery have been incorporated here.

djf's mouse events shim is as minimally invasive as could be hoped for common cases.  That experiment was a success.

What remains to be demonstrated is that important perf targets can be fixed.  I'll invest my 0-person-days weekend time in that tomorrow.  If we prove perf, I think we should do this.

    
    

    
  
add C3 milestone as this bug blocks other C3 bugs.
Target Milestone: --- → B2G C3 (12dec-1jan)

    
    

    
  
I just tested mouse_event_shim.js in the UI Tests keyboard panel and in the feedback app, because I was worried that stopping the propagation of the mouse events gecko generates would stop the focus event from being delivered and prevent the keyboard from popping up.

But it doesn't seem to do that.  Apps that use the keyboard can apparently use this shim safely.

    
    

    
  
Comment on attachment 694669 [details] [diff] [review]
WIP-TabChild-mouse-events-v3.patch

Has some issues, fixed in updated patch.

        Attachment #694669 -
        Flags: feedback?(jones.chris.g)

    
    

    
  


  
Note the XXX comments.

        Attachment #694503 -
        Attachment is obsolete: true

        Attachment #694669 -
        Attachment is obsolete: true

        Attachment #694721 -
        Attachment is obsolete: true

        Attachment #694726 -
        Attachment is obsolete: true

        Attachment #694973 -
        Attachment is obsolete: true

        Attachment #696179 -
        Flags: review?(mwu)

        Attachment #696179 -
        Flags: feedback?(wjohnston)

    
    

    
  


  
This is Shih-Chiang's patch from 814252, slightly cleaned up.  There's a bug in it in which click events are being eaten for remote content on tap gestures.

        Attachment #696181 -
        Flags: review?(jones.chris.g)

    
    

    
  
Comment on attachment 696181 [details] [diff] [review]
Use touch event for scrolling if available

>diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js

>   onTouchEnd: function cp_onTouchEnd(evt) {

>+    this.isScrolling = false;
> 
>     this.onTouchMove(evt);

>+    let click = (this.watchedEventsType == 'touch') ? true : evt.detail;
>     if (this.target && click && (this.panning || this.preventNextClick)) {

The issue here is that if we detect |this.isScrolling|, we don't have
to block the next click because a click event would never be
synthesized.  By definition !tap <=> isScrolling.

In this code however, we reset isScrolling before onTouchMove, which
means we can't fix this if() condition.  Just moving it below the if()
block fixes that.

        Attachment #696181 -
        Flags: review?(jones.chris.g)

    
    

    
  


  
Address cjones's review comment ;).

        Attachment #696185 -
        Flags: review+

        Attachment #696181 -
        Attachment is obsolete: true

    
    

    
  
Also, the call here to onTouchMove() in onTouchEnd() is completely useless, because we reset dragging = false before doing it.  (That existed before bug 814252.)  So I'm going to remove that too.

    
    

    
  


  
Ran into an issue that required some relatively nontrivial changes, so requesting review.  Shih-Chiang, I would recommend looking at the interdiff between this and v2.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #63)
> Comment on attachment 696181 [details] [diff] [review]
> Use touch event for scrolling if available
> 
> By definition !tap <=> isScrolling.
>

This is true, except that BrowserElementScrolling doesn't attempt to detect isScrolling if there's not a potential scroll target.  So we don't always know for sure whether we want to block clicks.

The big change here is using the KineticPanning gesture detector to figure out if a touch series is a click or not.  We use that in onTouchEnd() when we're watching touch events.

The second change was a small simplification to the isScrolling logic.  It's now explicitly, "is detecting scroll".

        Attachment #696207 -
        Flags: review?(schien)

        Attachment #696185 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #61)
> Created attachment 696179 [details] [diff] [review]
> part 1: Make TabChild dispatch spec-compliant compat mouse events
> 
> Note the XXX comments.

The current TabChild event code is firing contextmenu, but I don't understand how.  All that logic may be unnecessary.

    
    

    
  
Aha, this is because the apzc gesture detector already detects long-tap.  But nobody does it for sync-scrolled content, so we still need that logic.

    
    

    
  
Comment on attachment 696207 [details] [diff] [review]
part 2: Use touch event for scrolling if available, v3

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

::: dom/browser-element/BrowserElementScrolling.js
@@ +132,5 @@
>      KineticPanning.record(new Point(0, 0), evt.timeStamp);
>    },
>  
>    onTouchEnd: function cp_onTouchEnd(evt) {
> +    if (!this.dragging || 

nit: remove tailing space.

@@ +198,5 @@
> +        // We're going to drive synchronously scrolling an inner frame.
> +        var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> +        os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);
> +      } else {
> +        // Let AsyncPanZoomController handle the scrolling gesture.

Due to the definition change of |this.detectingScrolling|, we'll remove the scrollCallback while we reach the end of scrollable region before receiving touchend event. However, we should be able to scroll the previous selected frame to different direction without re-tap on the touch screen.

@@ +203,5 @@
> +        this.scrollCallback = null;
> +        return;
> +      }
> +    }
> +      

nit: remove tailing space.

@@ +211,5 @@
>        this.panning = true;
>        this._resetActive();
>      }
> +
> +    if (this.panning) {

|this.panning| will remain false if delta is too small, however, these touch/mouse event were already used for scrolling. I think we also need to consider |isScroll| here for determining if events are consumed by scrolling.

        Attachment #696207 -
        Flags: review?(schien)

    
    

    
  
Need to hit the sack.  Below are the regressions found with fairly extensive testing.  The ones I think are gecko problems I've noted (GECKO), and I have a fix (I think).

The rest are gaia regressions that need synthetic_mouse_event love.

== Homescreen ==
 - tap events generated when panning starting from icon X and ending on icon X

== Dialer ==
 - keypad: long-press on 0 to make + doesn't work
 - keypad: enter long number, long-press <=, doesn't delete all numbers
 - (GECKO) keypad: keys not highlighting on press
 - in-call keypad: tones keep playing when finger moves off key

== Gallery ==
 - drag far on image, release on same image => click (Ed: looks like same issue as homescreen)

== Contacts ==
 - contact-image expose doesn't work, can't "pull down"

== SMS ==
 - "Send" button doesn't work --- some hacky workaround?

== Settings ==
 - can't drag "Sound" sliders
 - (GECKO) rows not highlighted on tap

== Music ==
 - scrubber doesn't slide (Ed: works fine in video ...)

    
    

    
  
Chris,

For these regressions had you applied my gaia patch with the shim and app modifications to use the shim?

    
    

    
  
That's right; working with synthetic_mouse_events branch rebased on 12b722515af8df6eabad59ac8ecd19d8b0b24734.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rollup (obsolete)
        — 
          — Splinter Review
      

    


  
Applies on top of gecko-18 b8d7310e257ab133e9c7b1eb5425e1b2cf819398.

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #70)
> Need to hit the sack.  Below are the regressions found with fairly extensive
> testing.  The ones I think are gecko problems I've noted (GECKO), and I have
> a fix (I think).
> 
> The rest are gaia regressions that need synthetic_mouse_event love.
> 
> == Homescreen ==
>  - tap events generated when panning starting from icon X and ending on icon
> X
> 
> == Dialer ==
>  - keypad: long-press on 0 to make + doesn't work
>  - keypad: enter long number, long-press <=, doesn't delete all numbers
>  - (GECKO) keypad: keys not highlighting on press
>  - in-call keypad: tones keep playing when finger moves off key
> 
> == Gallery ==
>  - drag far on image, release on same image => click (Ed: looks like same
> issue as homescreen)
> 
> == Contacts ==
>  - contact-image expose doesn't work, can't "pull down"
> 
> == SMS ==
>  - "Send" button doesn't work --- some hacky workaround?

I should clarify: this worked just fine when I tested on the 21st.  So something in the sms app has changed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rollup v2 (obsolete)
        — 
          — Splinter Review
      

    


  
This fixes all the gecko bugs listed above.  Working on top of gecko-18 d4c41e8c1e4dd8cfa222b7a0cdda59641d25225d and djf's branch merged with gaia 85b6f11cda451d5d5e66aef344a40592d970a4b6.

Updated triage

Behavioral change: "long-tap", i.e. context menu, gestures no longer
generate taps.  (Even on buttons, which is a very difficult bug.)

== Homescreen ==
 - tap events generated when panning starting from icon X and ending on icon X

== Gallery ==
 - drag far on image, release on same image => click (same issue as above)

== Dialer ==
 - keypad: keys stay highlighted when finger moves off
 - in-call keypad: tones keep playing when finger moves off key (same-ish bug as above)

== Contacts ==
 - contact-image expose doesn't work, can't "pull down"

== SMS ==
 - send to existing contact: "Send" button doesn't work --- some hacky workaround?
 - enter number into search, write message, "Send": gives
E/GeckoConsole( 3027): [JavaScript Error: "Error: Permission denied to access property 'dispatchEvent'" {file: "app://sms.gaiamobile.org/shared/js/mouse_event_shim.js" line: 191}]

== Settings ==
 - can't drag "Sound" or "Display" sliders

== Music ==
 - scrubber doesn't slide (Ed: works fine in video ...)

        Attachment #696344 -
        Attachment is obsolete: true

    
    

    
  
djf: the thing that worries me most is the permission error above.  I'm going to take a quick look, but I think that due to the organization of the communications app, we end up trying to dispatch an event cross-domain :|.

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #75)
> == SMS ==
>  - send to existing contact: "Send" button doesn't work --- some hacky
> workaround?
>  - enter number into search, write message, "Send": gives
> E/GeckoConsole( 3027): [JavaScript Error: "Error: Permission denied to
> access property 'dispatchEvent'" {file:
> "app://sms.gaiamobile.org/shared/js/mouse_event_shim.js" line: 191}]

The behavior here is strangely intermittent.  I see three different behaviors, trying to sort them out
 - "Send" button does nothing, no security error
 - "Send" button does nothing, security error
 - "Send" button "clicks", but no sms is sent

    
    

    
  
Heh, the problem here is the event shim itself.  The SMS app's usage of mouse events is legit even in a touch environment.  If I remove the shim, everything works perfectly, and I scroll the thread without dismissing the keyboard etc.

    
    

    
  
So remaining work
 - get the gecko patches r+'d
 - make homescreen and gallery not get (clicks?) when panning
 - fix keys on dialer keypad
 - shim the settings sliders
 - make the music scrubber behave like the video scrubber

    
    

    
  
also
 - don't regress homescreen perf

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] from comment #69)
> Comment on attachment 696207 [details] [diff] [review]
> part 2: Use touch event for scrolling if available, v3
> 
> Review of attachment 696207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementScrolling.js
> @@ +132,5 @@
> >      KineticPanning.record(new Point(0, 0), evt.timeStamp);
> >    },
> >  
> >    onTouchEnd: function cp_onTouchEnd(evt) {
> > +    if (!this.dragging || 
> 
> nit: remove tailing space.
> 

Done.

> @@ +198,5 @@
> > +        // We're going to drive synchronously scrolling an inner frame.
> > +        var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> > +        os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);
> > +      } else {
> > +        // Let AsyncPanZoomController handle the scrolling gesture.
> 
> Due to the definition change of |this.detectingScrolling|, we'll remove the
> scrollCallback while we reach the end of scrollable region before receiving
> touchend event. However, we should be able to scroll the previous selected
> frame to different direction without re-tap on the touch screen.

OK, I understand.  Nice catch! :)  Let me revert this change.

> 
> @@ +203,5 @@
> > +        this.scrollCallback = null;
> > +        return;
> > +      }
> > +    }
> > +      
> 
> nit: remove tailing space.
> 

Done.

> @@ +211,5 @@
> >        this.panning = true;
> >        this._resetActive();
> >      }
> > +
> > +    if (this.panning) {
> 
> |this.panning| will remain false if delta is too small, however, these
> touch/mouse event were already used for scrolling. I think we also need to
> consider |isScroll| here for determining if events are consumed by scrolling.

The problem here is that we can't disrupt platform tap or contextmenu detection.  BrowserElementScrolling only starts "consuming" touchmove events (uses them to scroll) if |this.panning| becomes true.  If |this.panning| never becomes true, then the gesture was a tap and BrowserElementScrolling didn't interfere.

If we preventDefault() touchmove events when |this.panning| is false, then the following bug occurs
 - touchstart on target X
 - touchmove a distance within the tap threshold
   * BrowserElementScrolling preventDefault()s the touchmove
   * Gecko blocks sending synthetic click (per spec AFAIK)
 - touchend
   * no click is generated on X

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #81)
> (In reply to Shih-Chiang Chien [:schien] from comment #69)
> > Comment on attachment 696207 [details] [diff] [review]
> > @@ +198,5 @@
> > > +        // We're going to drive synchronously scrolling an inner frame.
> > > +        var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> > > +        os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);
> > > +      } else {
> > > +        // Let AsyncPanZoomController handle the scrolling gesture.
> > 
> > Due to the definition change of |this.detectingScrolling|, we'll remove the
> > scrollCallback while we reach the end of scrollable region before receiving
> > touchend event. However, we should be able to scroll the previous selected
> > frame to different direction without re-tap on the touch screen.

Actually, no, I don't see the problem here.  detectingScrolling always goes from true -> false on the first touchmove.  If that touchmove was below the pan threshold, we hand off to the apzc (as the previous code did).  If it was above the pan threshold, then BrowserElementScrolling doesn't set the scrollCallback to null and owns panning from there one.

After BrowserElementScrolling owns panning, it's perfectly fine for the scrollCallback to return false.  BrowserElementScrolling still continues to own the panning.

In other words, this detection code only runs once per pan series.  That was something that confused me a bit in your previous patch, which is why I attempted to change it :).

Let me know if I missed something.

    
    

    
  


  
I'm afraid v4 picks up one more level of complexity.  Sorry Shih-Chiang :(.

The problem with the previous patch is that it broke :active handling.  If we had done things right from the beginning and taught EventStateManager about touch events, this all would have mostly Just Worked.  But we didn't.

So this patch duplicates yet more of that logic, but not completely correctly.  The logic looks like

 - touchstart
   * save the "real" event.target as |this.pointerDownTarget|
   * if we have a pannable |this.target|, then set a timer off which to activate |pointerDownTarget|
   * otherwise, immediately activate |pointerDownTarget|
 - touchmove
   * if we detected a pan, cancel the timer and reset activation
 - [timer expires]
   * activate pointerDownTarget
 - touchend
   * cancel everything, reset activation

        Attachment #696207 -
        Attachment is obsolete: true

        Attachment #696437 -
        Flags: review?(schien)

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #79)
>  - make homescreen and gallery not get (clicks?) when panning

The homescreen grid doesn't listen for clicks on elements so I'm not quite sure what's up.  Looks like a problem with removing event listeners or the order of event delivery.

The gallery isn't doing anything wrong either, it's just relying on the current weird BES behavior.  Is mouse_events_shim even needed for the main pannable area?  It looks like things should Just Work without it.  We would still need it for the sliders, of course.

    
    

    
  
I'm on vacation right now, though I may be able to review after new years if no other reviewer is available sooner.

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #82)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #81)
> > (In reply to Shih-Chiang Chien [:schien] from comment #69)
> > > Comment on attachment 696207 [details] [diff] [review]
> > > @@ +198,5 @@
> > > > +        // We're going to drive synchronously scrolling an inner frame.
> > > > +        var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> > > > +        os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);
> > > > +      } else {
> > > > +        // Let AsyncPanZoomController handle the scrolling gesture.
> > > 
> > > Due to the definition change of |this.detectingScrolling|, we'll remove the
> > > scrollCallback while we reach the end of scrollable region before receiving
> > > touchend event. However, we should be able to scroll the previous selected
> > > frame to different direction without re-tap on the touch screen.
> 
> Actually, no, I don't see the problem here.  detectingScrolling always goes
> from true -> false on the first touchmove.  If that touchmove was below the
> pan threshold, we hand off to the apzc (as the previous code did).  If it
> was above the pan threshold, then BrowserElementScrolling doesn't set the
> scrollCallback to null and owns panning from there one.
> 
> After BrowserElementScrolling owns panning, it's perfectly fine for the
> scrollCallback to return false.  BrowserElementScrolling still continues to
> own the panning.
> 
> In other words, this detection code only runs once per pan series.  That was
> something that confused me a bit in your previous patch, which is why I
> attempted to change it :).
> 
> Let me know if I missed something.
Now I understand the usage of |this.detectingScrolling| in your patch, thanks for the explanation. :)

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #79)
> So remaining work
>  - get the gecko patches r+'d
>  - make homescreen and gallery not get (clicks?) when panning
>  - fix keys on dialer keypad
>  - shim the settings sliders
>  - make the music scrubber behave like the video scrubber

In an unrelated code review for the music app, I noticed that the music scrubber is using the layerX property of the mouseevent rather than pageX or clientX. The shim doesn't emulate layerX, so that is why it is breaking.  It should be easy to patch.

Chris: do you want to break these gaia work items out as separate bugs and assign them to me?  Or should we just work on them here?

    
    

    
  
However you prefer; I think so far the gaia patching has been easy enough that we can have two patches, one for the shim and one for users.  All this has to land together so we might as well do it here.

    
    

    
  
My heart sings when I see the list of bugs that this bug will positively impact. :)

    
    

    
  
Comment on attachment 696437 [details] [diff] [review]
part 2: Use touch event for scrolling if available, v4

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

::: dom/browser-element/BrowserElementScrolling.js
@@ +6,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  const ContentPanning = {
>    init: function cp_init() {
> +    ['mousedown', 'mouseup', 'mousemove', 'touchstart', 'touchend', 'touchmove'].forEach(function(type) {

Some people do use:

try {
  content.document.createEvent('TouchEvent');
  
  var events = ['touchstart', 'touchmove', touchend'];
} catch(e) {
  var events = ['mousedown', 'mousemove' 'mouveup'];
}

events.forEach(...);

That would let you get rid of the need for _filterEvent.

@@ +78,5 @@
> +      if (touches[i].identifier === this.primaryPointerId) {
> +        return touches[i];
> +      }
> +    }
> +    return undefined;

return null?

@@ +85,2 @@
>    onTouchStart: function cp_onTouchStart(evt) {
> +    let target, screenX, screenY;

target seems unused.

@@ +96,5 @@
> +      screenY = firstTouch.screenY;
> +    } else {
> +      this.pointerDownTarget = evt.target;
> +      screenX = evt.screenX;
> +      screenY = evt.screenY;

You may be able to move some of this logic to the switch case directly if you pass evt.changedTouches[0] to a 

case 'touchstart':
  this.onTouchStart(evt.changedTouches[0]);
  break;
case 'mousedown':
  this.onTouchStart(evt);
  break;

I also feel that you can move your findPrimaryPointer calls to handleEvent as well by decoupling the mousemove/touchmove and the mouseup/touchend cases.

@@ +109,5 @@
>      // this case, and if we are using async panning and zooming on the parent
>      // frame, inform the pan/zoom controller that it should not attempt to
>      // handle any touch events it gets until the next batch (meaning the next
>      // time we get a touch end).
>      if (this.target != null && ContentPanning._asyncPanZoomForViewportFrame) {

nit: ContentPanning -> this

@@ +128,5 @@
> +      if (this.target === null) {
> +        this.notify(this._activationTimer);
> +      } else {
> +        this._activationTimer.initWithCallback(this,
> +                                               /*XXXpref?*/ 50 /*ms*/,

50 seems really small on a device.

@@ +219,5 @@
> +      // Stop async-pan-zooming if the subframe is really scrolled.
> +      if (isScroll) {
> +        // We're going to drive synchronously scrolling an inner frame.
> +        var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> +        os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);

Services.os.notifyObservers(...)

@@ +226,5 @@
> +        this.scrollCallback = null;
> +        return;
> +      }
> +    }
> +      

nit: extra spaces

@@ +384,5 @@
> +  get _activationTimer() {
> +    delete this._activationTimer;
> +    return this._activationTimer = Cc["@mozilla.org/timer;1"]
> +                                     .createInstance(Ci.nsITimer);
> +  },

I guess in this case a _activationTimer: Cc["@mozilla.org/timer;1"]
                                           .createInstance(Ci.nsITimer); would be enough

@@ +390,3 @@
>    _resetActive: function cp_resetActive() {
> +    let elt = this.target || this.pointerDownTarget;
> +    // assert elt

Is it a leftover or do you want to assert here?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +592,5 @@
>    bool mHandlingTouchQueue;
>  
> +  // Flag used to determine whether or not we should try scrolling by
> +  // BrowserElementScrolling first. If set, this means we should pend touch move
> +  // event, which not be cosumed by GestureListener. This flag will be reset

nit: cosumed -> consumed

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #90)
> Some people do use:
> try {
>   content.document.createEvent('TouchEvent');
>   var events = ['touchstart', 'touchmove', touchend'];
> } catch(e) {
>   var events = ['mousedown', 'mousemove' 'mouveup'];
> }

We're really trying to discourage people from writing sites or apps that won't work with touch and mouse simultaneously right now. We're already seeing lots of bugs from it with Windows 8 Metro (where users can choose to switch between mouse and touch at any time based on which they prefer).

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #90)
> Comment on attachment 696437 [details] [diff] [review]
> part 2: Use touch event for scrolling if available, v4
> 
> Review of attachment 696437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementScrolling.js
> @@ +6,5 @@
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  const ContentPanning = {
> >    init: function cp_init() {
> > +    ['mousedown', 'mouseup', 'mousemove', 'touchstart', 'touchend', 'touchmove'].forEach(function(type) {
> 
> Some people do use:
> 
> try {
>   content.document.createEvent('TouchEvent');
>   
>   var events = ['touchstart', 'touchmove', touchend'];
> } catch(e) {
>   var events = ['mousedown', 'mousemove' 'mouveup'];
> }
> 
> events.forEach(...);
> 
> That would let you get rid of the need for _filterEvent.
> 

Works for me.

> @@ +78,5 @@
> > +      if (touches[i].identifier === this.primaryPointerId) {
> > +        return touches[i];
> > +      }
> > +    }
> > +    return undefined;
> 
> return null?
> 

Sure.

> @@ +85,2 @@
> >    onTouchStart: function cp_onTouchStart(evt) {
> > +    let target, screenX, screenY;
> 
> target seems unused.

Yep, good call.

> @@ +96,5 @@
> > +      screenY = firstTouch.screenY;
> > +    } else {
> > +      this.pointerDownTarget = evt.target;
> > +      screenX = evt.screenX;
> > +      screenY = evt.screenY;
> 
> You may be able to move some of this logic to the switch case directly if
> you pass evt.changedTouches[0] to a 
> 

IDOMTouch doesn't have all the information we need, like timestamps e.g.

> I also feel that you can move your findPrimaryPointer calls to handleEvent
> as well by decoupling the mousemove/touchmove and the mouseup/touchend cases.

I'm not quite sure I follow ... you're suggesting to move the pointer-detection code into handleEvent() in order to share it between touchmove and touchend?

> @@ +109,5 @@
> >      // this case, and if we are using async panning and zooming on the parent
> >      // frame, inform the pan/zoom controller that it should not attempt to
> >      // handle any touch events it gets until the next batch (meaning the next
> >      // time we get a touch end).
> >      if (this.target != null && ContentPanning._asyncPanZoomForViewportFrame) {
> 
> nit: ContentPanning -> this
> 

Changed.

> @@ +128,5 @@
> > +      if (this.target === null) {
> > +        this.notify(this._activationTimer);
> > +      } else {
> > +        this._activationTimer.initWithCallback(this,
> > +                                               /*XXXpref?*/ 50 /*ms*/,
> 
> 50 seems really small on a device.

It's about three frames.  I chose it based on fairly extensive testing with the settings app.  If it's too high, then we don't fix bug 805908.  If it's too low, then we don't get a "click" feedback when entering a menu.

This is something we can easily tune though.

> @@ +219,5 @@
> > +      // Stop async-pan-zooming if the subframe is really scrolled.
> > +      if (isScroll) {
> > +        // We're going to drive synchronously scrolling an inner frame.
> > +        var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> > +        os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);
> 
> Services.os.notifyObservers(...)
> 

Changed.

> @@ +226,5 @@
> > +        this.scrollCallback = null;
> > +        return;
> > +      }
> > +    }
> > +      
> 
> nit: extra spaces
> 

Removed.

> @@ +384,5 @@
> > +  get _activationTimer() {
> > +    delete this._activationTimer;
> > +    return this._activationTimer = Cc["@mozilla.org/timer;1"]
> > +                                     .createInstance(Ci.nsITimer);
> > +  },
> 
> I guess in this case a _activationTimer: Cc["@mozilla.org/timer;1"]
>                                            .createInstance(Ci.nsITimer);
> would be enough
> 

Sorry, I don't understand this comment.

> @@ +390,3 @@
> >    _resetActive: function cp_resetActive() {
> > +    let elt = this.target || this.pointerDownTarget;
> > +    // assert elt
> 
> Is it a leftover or do you want to assert here?
> 

I want to assert; I left this in for documentation.  But I guess it's pretty obvious that we'll throw below if it's null.  Removed.

> ::: gfx/layers/ipc/AsyncPanZoomController.h
> @@ +592,5 @@
> >    bool mHandlingTouchQueue;
> >  
> > +  // Flag used to determine whether or not we should try scrolling by
> > +  // BrowserElementScrolling first. If set, this means we should pend touch move
> > +  // event, which not be cosumed by GestureListener. This flag will be reset
> 
> nit: cosumed -> consumed

Fixed up this comment a bit.

    
    

    
  
(In reply to Wesley Johnston (:wesj) from comment #91)
> (In reply to Vivien Nicolas (:vingtetun) from comment #90)
> > Some people do use:
> > try {
> >   content.document.createEvent('TouchEvent');
> >   var events = ['touchstart', 'touchmove', touchend'];
> > } catch(e) {
> >   var events = ['mousedown', 'mousemove' 'mouveup'];
> > }
> 
> We're really trying to discourage people from writing sites or apps that
> won't work with touch and mouse simultaneously right now. We're already
> seeing lots of bugs from it with Windows 8 Metro (where users can choose to
> switch between mouse and touch at any time based on which they prefer).

This is code deep within the bowels of gecko that controls auto-scrolling of content.

That said, I really think we should be investing in something like the pointer events spec.  It's just incredibly hard to do anything nontrivial while having to support both mousemove and touchmove simultaneously.

    
    

    
  


  

        Attachment #696437 -
        Attachment is obsolete: true

        Attachment #696437 -
        Flags: review?(schien)

        Attachment #696841 -
        Flags: review?(schien)

        Attachment #696841 -
        Flags: review?(21)

    
    

    
  
Comment on attachment 696841 [details] [diff] [review]
part 2: Use touch event for scrolling if available, v5

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

::: dom/browser-element/BrowserElementScrolling.js
@@ +11,4 @@
>    init: function cp_init() {
> +    var events;
> +    try {
> +      content.document.createEvent('TouchEvent');

In desktop build, we'll not be able to scroll because touch event can still be created.

@@ +153,1 @@
>      if (this.target && click && (this.panning || this.preventNextClick)) {

For touch event, the !KineticPanning.isPan() and this.panning will always be the opposite value, therefore, the unexpected click will still be triggered while swiping on screen.

@@ +200,5 @@
> +    if (!this.scrollCallback) {
> +      return;
> +    }
> +
> +    let isScroll = this.scrollCallback(delta.scale(-1));

I think we should only perform scrolling while |KinecticPanning.isPan() == true| since we're using KinecticPanning for distinguishing panning and click.

@@ +206,5 @@
> +      this.detectingScrolling = false;
> +      // Stop async-pan-zooming if the subframe is really scrolled.
> +      if (isScroll) {
> +        // We're going to drive synchronously scrolling an inner frame.
> +        Services.os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);

typo: should be Services.obs

        Attachment #696841 -
        Flags: review?(schien) → review-

    
    

    
  
Chris: Mwu is on PTO until the 14th, so we need a different reviewer for part 1.
Assignee: nobody → jones.chris.g

    
    

    
  
Comment on attachment 696179 [details] [diff] [review]
part 1: Make TabChild dispatch spec-compliant compat mouse events

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

This all seems sane to me. The interaction of some of these things with :active is a bit strange for us, but its probably better handled somewhere else...

::: dom/ipc/TabChild.cpp
@@ +1465,5 @@
> +void
> +TabChild::FireContextMenuEvent()
> +{
> +  MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0);
> +  // XXX do we need to fire mousemove+mousedown here?

We don't do that on Fennec and I've never seen any problems from it.

        Attachment #696179 -
        Flags: feedback?(wjohnston) → feedback+

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] from comment #95)
> Comment on attachment 696841 [details] [diff] [review]
> part 2: Use touch event for scrolling if available, v5
> 
> Review of attachment 696841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementScrolling.js
> @@ +11,4 @@
> >    init: function cp_init() {
> > +    var events;
> > +    try {
> > +      content.document.createEvent('TouchEvent');
> 
> In desktop build, we'll not be able to scroll because touch event can still
> be created.

A simple test gives me

[13:08:11.337] NotSupportedError: Operation is not supported @ file:///home/cjones/Desktop/test.html:37

but I'll double-check in a b2g desktop build.

> @@ +153,1 @@
> >      if (this.target && click && (this.panning || this.preventNextClick)) {
> 
> For touch event, the !KineticPanning.isPan() and this.panning will always be
> the opposite value, therefore, the unexpected click will still be triggered
> while swiping on screen.

There's no synthetic click generated when KineticPanning.isPan() is true and we're using touch events.

> @@ +200,5 @@
> > +    if (!this.scrollCallback) {
> > +      return;
> > +    }
> > +
> > +    let isScroll = this.scrollCallback(delta.scale(-1));
> 
> I think we should only perform scrolling while |KinecticPanning.isPan() ==
> true| since we're using KinecticPanning for distinguishing panning and click.

That makes sense, good catch.

> @@ +206,5 @@
> > +      this.detectingScrolling = false;
> > +      // Stop async-pan-zooming if the subframe is really scrolled.
> > +      if (isScroll) {
> > +        // We're going to drive synchronously scrolling an inner frame.
> > +        Services.os.notifyObservers(docShell, 'cancel-default-pan-zoom', null);
> 
> typo: should be Services.obs

Oops!

    
    

    
  
(In reply to Wesley Johnston (:wesj) from comment #97)
> Comment on attachment 696179 [details] [diff] [review]
> part 1: Make TabChild dispatch spec-compliant compat mouse events
> 
> Review of attachment 696179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all seems sane to me. The interaction of some of these things with
> :active is a bit strange for us, but its probably better handled somewhere
> else...
> 
> ::: dom/ipc/TabChild.cpp
> @@ +1465,5 @@
> > +void
> > +TabChild::FireContextMenuEvent()
> > +{
> > +  MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0);
> > +  // XXX do we need to fire mousemove+mousedown here?
> 
> We don't do that on Fennec and I've never seen any problems from it.

Good to know, thanks.

    
    

    
  


  
Will try to find an alternate reviewer in the meantime.

        Attachment #696179 -
        Attachment is obsolete: true

        Attachment #696179 -
        Flags: review?(mwu)

        Attachment #697281 -
        Flags: review?(mwu)

    
    

    
  


  
Addresses Shih-Chiang's comments.  We were enabling touch events in b2g desktop builds; this patch turns that off.

        Attachment #696841 -
        Attachment is obsolete: true

        Attachment #696841 -
        Flags: review?(21)

        Attachment #697282 -
        Flags: review?(schien)

        Attachment #697282 -
        Flags: review?(21)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rollup v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #696416 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #100)
> Created attachment 697281 [details] [diff] [review]
> part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1

jlebar, your review queue seems to be pretty substantial, but do you think you could fill in for mwu here?
Flags: needinfo?(justin.lebar+bug)

    
    

    
  
Comment on attachment 697281 [details] [diff] [review]
part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1

Sure, I can look at this.

        Attachment #697281 -
        Flags: review?(mwu) → review?(justin.lebar+bug)
Flags: needinfo?(justin.lebar+bug)

    
    

    
  
Comment on attachment 697282 [details] [diff] [review]
part 2: Use touch event for scrolling if available, v6

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

looks good to me, except for one little problem.

::: dom/browser-element/BrowserElementScrolling.js
@@ +218,5 @@
> +        // Let AsyncPanZoomController handle the scrolling gesture.
> +        this.scrollCallback = null;
> +        return;
> +      }
> +    }

The expressions above mean APZC will be enable if our first scrolling gesture moves only a tiny distance, i.e., we cannot scroll subframe while we scroll the web page very slowly. However, this defect is not perceptible under regular user operation IMO.

        Attachment #697282 -
        Flags: review?(schien) → review+

    
  
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)

    
    

    
  
Comment on attachment 697282 [details] [diff] [review]
part 2: Use touch event for scrolling if available, v6

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

::: dom/browser-element/BrowserElementScrolling.js
@@ +116,5 @@
> +      if (this.target === null) {
> +        this.notify(this._activationTimer);
> +      } else {
> +        this._activationTimer.initWithCallback(this,
> +                                               /*XXXpref?*/ 50 /*ms*/,

I tried it on a device and I bet this is too short. What about adding a pref right now so UX will be able to tweak it easily in Gaia?

@@ +154,5 @@
> +    // event.  That's OK however, because we won't fire a synthetic
> +    // click when we're using touch events and this touch series
> +    // wasn't a "tap" gesture.
> +    let click = (this.watchedEventsType == 'mouse') ?
> +      evt.detail : !KineticPanning.isPan();

This code will still fire a click if you move your finger back to the original position.
Also it seems like the plaform generates a click when you pan but the target is still under your finger.

    
    

    
  
Vivien points out that these patches break text selection in app processes.  (It's already broken in browser processes, and it continues to work in the browser UI.)  That's not fixable in the scope of v1, so we're going to have to declare bankruptcy on that regression.

    
    

    
  
Comment on attachment 697282 [details] [diff] [review]
part 2: Use touch event for scrolling if available, v6

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

I will be fine to r+ this code once the click issue is resolved.

Steps to reproduce:
 - Go to the UI tests app
 - Choose 'Select tests'
 - Choose the second select where the content is scrollable
 - Click on 'basecamp' and scroll it to the top keeping the finger on it

Actual result:
 - 'basecamp' is selected even if you have pan more than what is expected for a click

Expected result:
 - no click

        Attachment #697282 -
        Flags: review?(21) → review-

    
    

    
  
Is the b2g process drawing that UI?  If so I think I know what the problem is.
Flags: needinfo?(21)

    
    

    
  
Have fix, but it ain't gonna be pretty.
Flags: needinfo?(21)

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #107)
> Comment on attachment 697282 [details] [diff] [review]
> part 2: Use touch event for scrolling if available, v6
> 
> Review of attachment 697282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementScrolling.js
> @@ +116,5 @@
> > +      if (this.target === null) {
> > +        this.notify(this._activationTimer);
> > +      } else {
> > +        this._activationTimer.initWithCallback(this,
> > +                                               /*XXXpref?*/ 50 /*ms*/,
> 
> I tried it on a device and I bet this is too short. What about adding a pref
> right now so UX will be able to tweak it easily in Gaia?
> 

I keep meaning to add the pref, will do.  I tuned this in the settings app: when we tap a category header, we want to see it highlight when we transition into the subview.  At 100ms, I didn't ever really see highlight.

The second case is panning around the categories list.  With 50ms I mostly don't see the activation highlight when starting a pan.  At 25ms, it was the opposite, I almost always saw it.

> @@ +154,5 @@
> > +    // event.  That's OK however, because we won't fire a synthetic
> > +    // click when we're using touch events and this touch series
> > +    // wasn't a "tap" gesture.
> > +    let click = (this.watchedEventsType == 'mouse') ?
> > +      evt.detail : !KineticPanning.isPan();
> 
> This code will still fire a click if you move your finger back to the
> original position.
> Also it seems like the plaform generates a click when you pan but the target
> is still under your finger.

This doesn't happen in content processes, but you're right, it occurs in the (still non-compliant) master process.  My fix/hack takes care of this.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fixes on top of v6 (obsolete)
        — 
          — Splinter Review
      

    


  
Interdiff is going to fall over.  Addresses review comments and adds hack for b2g process.

        Attachment #697637 -
        Flags: review?(21)

    
    

    
  


  

        Attachment #697282 -
        Attachment is obsolete: true

        Attachment #697283 -
        Attachment is obsolete: true

    
    

    
  
Testing on http://people.mozilla.com/~cjones/events.html with a "tap" gesture, here's a samping of behaviors

             fennec: touchstart touchend mousemove mousedown mouseup click
                b2g: touchstart touchend mousemove mousedown mouseup click
  chrome-android-JB: touchstart touchend mousemove mousedown mouseup click
 android-browser-JB: mousemove touchstart touchend mousedown mouseup click
  mobilesafari-ios5: touchstart touchend

All behaviors comply with spec.  Yay for great specs!  I think it's most important for us to align with fennec here, and now we do.  chrome is also in our camp, and humorously, a different one from the stock android browser.  Although TBH I like the behavior of android-browser ...

    
    

    
  
I think we've got most of the known gaia regressions fixed.

Current status is at https://etherpad.mozilla.org/touch-regressions

I've been pushing fixes to my synthetic_mouse_events branch on github: https://github.com/mozilla-b2g/gaia/pull/7120

Fixes to the shim include: a setCapture() helper, mouseenter/mouseleave events, and not sending click events if the touch moves more than 25px.

Its probably time to start reviewing the gaia side of this patch.  Who wants to do it?  cjones? cpeterson? vingtetun?

    
    

    
  
Note that becuase of bug 825840 I have not rebuilt gecko with the latest rollup patch, so I've been working on the gaia side using a Vivien's build from this morning.  We'll need to test again with the latest version of the gecko patches.

Things that remain on my todo list include grepping for mousemove in all apps to make sure that we haven't overlooked any that need the shim.

    
    

    
  
(In reply to David Flanagan [:djf] from comment #117)
> I think we've got most of the known gaia regressions fixed.
> 
> Current status is at https://etherpad.mozilla.org/touch-regressions
> 
> I've been pushing fixes to my synthetic_mouse_events branch on github:
> https://github.com/mozilla-b2g/gaia/pull/7120
> 
> Fixes to the shim include: a setCapture() helper, mouseenter/mouseleave
> events, and not sending click events if the touch moves more than 25px.
> 
> Its probably time to start reviewing the gaia side of this patch.  Who wants
> to do it?  cjones? cpeterson? vingtetun?

I can do it.

    
    

    
  
Comment on attachment 697640 [details] [diff] [review]
part 2: Use touch event for scrolling if available, v7

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

I feel like some parts of the code could be written differently to make it easier to read/maintain the code but nothing that should prevent this to land. Nice work!

        Attachment #697640 -
        Flags: review+

    
    

    
  
There is still a small bug with UI tests:

Steps to reproduce:
 - Go to the UI tests app
 - Choose 'Select tests'
 - Choose the second select where the content is scrollable
 - Click on 'basecamp' and scroll it to the top
 - Click on 'Gecko-2' and start to pan to the bottom

Actual result:
 - The scrollable area jump to the top and scroll from here

Expected result:
 - The scrollable area scroll from the current point

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #121)
> There is still a small bug with UI tests:
> 
> Steps to reproduce:
>  - Go to the UI tests app
>  - Choose 'Select tests'
>  - Choose the second select where the content is scrollable
>  - Click on 'basecamp' and scroll it to the top
>  - Click on 'Gecko-2' and start to pan to the bottom
> 
> Actual result:
>  - The scrollable area jump to the top and scroll from here
> 
> Expected result:
>  - The scrollable area scroll from the current point

It seems like this.position.set(eventX, eventY) is not set correctly and so the first onTouchMove call generate a wrong delta.

    
    

    
  
Comment on attachment 697281 [details] [diff] [review]
part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1

>+  nsIntPoint currentPoint = trackedTouch->mRefPoint;
>+  int64_t time = aEvent.time;
>+  switch (aEvent.message) {
>+  case NS_TOUCH_MOVE:
>+    if (abs(currentPoint.x - mGestureDownPoint.x) > sDragThreshold.width ||
>+        abs(currentPoint.y - mGestureDownPoint.y) > sDragThreshold.height) {

It seems weird to use L-infnity distance instead of L2 distance here.  I guess
this is OK because the drag threshold is small, so one won't notice the
difference?

>+void
>+TabChild::FireContextMenuEvent()
>+{
>+  MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0);
>+  RecvHandleLongTap(mGestureDownPoint);
>+  CancelTapTracking();

This can fire (long) after TabChild::ActorDestroy.  It looks kosher to call
RecvHandleLongTap at this point, but I wanted to check.

(Perhaps you should cancel the timer from ActorDestroy anyway, since the timer
will keep the TabParent alive, which could, at the very least, cause false
mochitest leaks.)

        Attachment #697281 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #122)
    > (In reply to Vivien Nicolas (:vingtetun) from comment #121)
    > > There is still a small bug with UI tests:
    > > 
    > > Steps to reproduce:
    > >  - Go to the UI tests app
    > >  - Choose 'Select tests'
    > >  - Choose the second select where the content is scrollable
    > >  - Click on 'basecamp' and scroll it to the top
    > >  - Click on 'Gecko-2' and start to pan to the bottom
    > > 
    > > Actual result:
    > >  - The scrollable area jump to the top and scroll from here
    > > 
    > > Expected result:
    > >  - The scrollable area scroll from the current point
    > 
    > It seems like this.position.set(eventX, eventY) is not set correctly and so
    > the first onTouchMove call generate a wrong delta.

    Sorry for the spam it seems like I fail to merged all.js in my build and so the code is failing with an exception on reading the new preference for the 'active' callback.  I'm rebuilding but I bet it will work now.

    
    

    
  
Comment on attachment 694591 [details]
link to patch on github

Vivien,

Thanks for agreeing to review this.  The patch is still a bunch of separate commits right now, but I will of course rebase and merge into one commit before landing.  I can do that right away if it makes your review easier.

I'll continue to look for gaia-side regressions and there is a chance I may add more commits, but I think it is basically ready for review now.

        Attachment #694591 -
        Flags: review?(21)

    
    

    
  
(In reply to David Flanagan [:djf] from comment #125)
> Comment on attachment 694591 [details]
> link to patch on github
> 
> Vivien,
> 
> Thanks for agreeing to review this.  The patch is still a bunch of separate
> commits right now, but I will of course rebase and merge into one commit
> before landing.  I can do that right away if it makes your review easier.
> 
> I'll continue to look for gaia-side regressions and there is a chance I may
> add more commits, but I think it is basically ready for review now.

I have added my review comments on github already. Thanks timezone shift!

(And I have spent time to look for regressions and didn't find new ones)

    
    

    
  
(In reply to Vivien Nicolas (:vingtetun) from comment #121)
> There is still a small bug with UI tests:
> 
> Steps to reproduce:
>  - Go to the UI tests app
>  - Choose 'Select tests'
>  - Choose the second select where the content is scrollable
>  - Click on 'basecamp' and scroll it to the top
>  - Click on 'Gecko-2' and start to pan to the bottom
> 
> Actual result:
>  - The scrollable area jump to the top and scroll from here
> 
> Expected result:
>  - The scrollable area scroll from the current point

I can't reproduce this.  I hope you can't in your updated build :).

    
    

    
  
(In reply to Justin Lebar [:jlebar] from comment #123)
> Comment on attachment 697281 [details] [diff] [review]
> part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1
> 
> >+  nsIntPoint currentPoint = trackedTouch->mRefPoint;
> >+  int64_t time = aEvent.time;
> >+  switch (aEvent.message) {
> >+  case NS_TOUCH_MOVE:
> >+    if (abs(currentPoint.x - mGestureDownPoint.x) > sDragThreshold.width ||
> >+        abs(currentPoint.y - mGestureDownPoint.y) > sDragThreshold.height) {
> 
> It seems weird to use L-infnity distance instead of L2 distance here.  I
> guess
> this is OK because the drag threshold is small, so one won't notice the
> difference?
> 

This code is taken verbatim from nsEventStateManager, so consistency is the goal.  This is a discussion to have elsewhere.

> >+void
> >+TabChild::FireContextMenuEvent()
> >+{
> >+  MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0);
> >+  RecvHandleLongTap(mGestureDownPoint);
> >+  CancelTapTracking();
> 
> This can fire (long) after TabChild::ActorDestroy.  It looks kosher to call
> RecvHandleLongTap at this point, but I wanted to check.

We'll never see TabChild::ActorDestroy in release builds.  Separately, yes it's kosher for this timer to fire afterwards.

> (Perhaps you should cancel the timer from ActorDestroy anyway, since the
> timer
> will keep the TabParent alive, which could, at the very least, cause false
> mochitest leaks.)

Do we clear timer listeners on xpcom-shutdown or whichever?

    
    

    
  
I agree that we should cancel tracking taps at ActorDestroy().  I don't think it would cause any problems not to, but it's pointless to continue doing so (since we won't receive any more input events) and could just lead to badness.

    
    

    
  
In fact, we should never receive ActorDestroy if we're tracking taps except in emergencies (parent-process crash).

    
    

    
  


  

        Attachment #697281 -
        Attachment is obsolete: true

        Attachment #697637 -
        Attachment is obsolete: true

        Attachment #697640 -
        Attachment is obsolete: true

        Attachment #697641 -
        Attachment is obsolete: true

        Attachment #698182 -
        Flags: review+

    
    

    
  
Gonk widget backend remains broken.
Summary: Make mouse events synthesized from touch events comply with spec (in gonk widget backend and cross-process code) → Make mouse events synthesized from touch events comply with spec (in apps)

    
    

    
  
Comment on attachment 694591 [details]
link to patch on github

Vivien reviewed and approved this patch, but forgot to mark r+ here.

        Attachment #694591 -
        Flags: review?(21) → review+
Depends on: 827783
Depends on: 827969
Blocks: 826401
No longer depends on: 826401
Blocks: 829336
Depends on: 844326
Depends on: 834584

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: