Closed Bug 479901 Opened 11 years ago Closed 11 years ago

[MSFT-9354] [MSFT-9360] Add WM_GESTURE support for Windows 7

Categories

(Core :: Widget: Win32, defect, P1)

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: vlad, Assigned: jimm)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 13 obsolete files)

95.36 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
64.26 KB, patch
Details | Diff | Splinter Review
Windows 7 adds touch/gesture support; we should get support for the high-level WM_GESTURE message in.  It should be able to tie directly in to the nsIDOMSimpleGestureEvent stuff added for OSX multitouch in bug 412486.

The multitouch gestures API is described at:

http://msdn.microsoft.com/en-us/library/dd371578(VS.85).aspx
http://msdn.microsoft.com/en-us/library/dd353239(VS.85).aspx

With some example code/etc. at http://code.msdn.microsoft.com/WindowsTouch

At a later point we may want to support the full multitouch input system and pass the raw data down, but it's not needed for now.

Note that we shouldn't add a dependency on the Win7 SDK for this, so we'll have to copy in any necessary #defines and check for the functions dynamically at runtime.
Flags: wanted1.9.1+
Note that the firefox side was in bug 456520 for the OSX work; from looking at that, as long as WM_GESTURE sends similar SimpleGestureEvent events, this should Just Work (tm).
Blocks: win7support
(In reply to comment #1)
> Note that the firefox side was in bug 456520 for the OSX work; from looking at
> that, as long as WM_GESTURE sends similar SimpleGestureEvent events, this
> should Just Work (tm).

For the most part yes, although just initially looking at what's currently in there, I think I'd like to add a pan event, and maybe see if we could update the delta value related to rotation to degrees or radians.

Pan seems much more useful than swipe, since on a desktop a two-fingered pan could be used to scroll the content view or window around arbitrarily.  Swipe seems pretty limited in use and would probably be better limited to mobile apps. (Which isn't to say I can't implement it, I could generate both events and people could listen for whatever they happened to be interested in.) 

As far as rotation goes, an arbitrary number of negative min -> positive max is going to be hard to translate into a specific rotation. I wonder if the OSX code could be fine tuned to provide more accurate degree data.

Magnification looks like it'll match up nicely.

On Windows we also have a tap event, I could add something like that but it doesn't look like trackpad gestures in OSX would support it. We also have a "roll over" gesture on Window, but I haven't found a definitive description of what that does. Not sure if we need to worry about that at this point until we get better documentation.
> I did some experimentation early on to determine that the units for rotation
> are "degrees."  I couldn't reach a similar conclusion for the pinch gesture.

Found this in bug 456520, so maybe rotation will just work if I hand in a degree value.
Related:

Bug 456520 - Firefox should support Multi-Touch Gestures on Mac OS X
Bug 461376 - Allow customization of multi-touch actions by preference
Depends on: 412486
(In reply to comment #2)
> On Windows we also have a tap event, I could add something like that but it
> doesn't look like trackpad gestures in OSX would support it. We also have a
> "roll over" gesture on Window, but I haven't found a definitive description of
> what that does. Not sure if we need to worry about that at this point until we
> get better documentation.
The "roll over" gesture is a 2-finger gesture that goes primary-down, secondary-down, secondary-up, [optional] primary-up. Typically performed like a right click with your fingers, but it's also recognized in any orientation. In the Win7Beta this is treated like a right-mouse-click by default (if the WM_GESTURE is unhandled).
(In reply to comment #5)
> (In reply to comment #2)
> > On Windows we also have a tap event, I could add something like that but it
> > doesn't look like trackpad gestures in OSX would support it. We also have a
> > "roll over" gesture on Window, but I haven't found a definitive description of
> > what that does. Not sure if we need to worry about that at this point until we
> > get better documentation.
> The "roll over" gesture is a 2-finger gesture that goes primary-down,
> secondary-down, secondary-up, [optional] primary-up. Typically performed like a
> right click with your fingers, but it's also recognized in any orientation. In
> the Win7Beta this is treated like a right-mouse-click by default (if the
> WM_GESTURE is unhandled).

I see so it's like a two finger drum roll. That might work well for flipping between tabs.

Is there any public information out there yet on proposed gesture mappings for ie8?
FYI, we use rotation as tab switching right now. (I've locally switch swipe right/left to tab switch.)
In general, the UX guidance for gesture mapping for apps is:
- Zoom, pan, rotate map to those actions if they make sense in the app
- Two finger tap (two fingers tapping at the same time) maps to restoring the view
- Press and tap (a.k.a. rollover, see Scott's description) maps to right click

For Firefox, zoom and pan make sense to hook up to the existing scrolling/zoom functionality.  If you want consistency w/ the Mac trackpad gestures, rotate can be hooked up to switching tabs.  Two finger tap can restore the view to 100% zoom.  

Note all the gestures contain position data as well.  For example, you can do zooms around the center of the gesture, or two finger tap could restore the view to 100% and center the page on the gesture.

Press&Tap probably makes sense to leave as right click.  If you want that behavior, it's already the default legacy behavior in the OS.  Just let GID_BEGIN, GID_END, and the GID for Press&Tap be unhandled and hit defwndproc - the OS's legacy behavior will kick in after that.
Yep, that makes sense.. for rotate, I don't actually think we should hook it up -- the use cases here are slightly different than on OSX.  On the mac, a user is still using a trackpad; for this, presumably the user will be using a touchscreen and can just tap the tab directly.

However, what we should probably do is pass down all the gestures, and if any are unhandled (as DOM events) then go into defwndproc; this means that in the firefox side, we'd have to modify the code slightly to distinguish between gesture-on-trackpad vs. gesture-on-screen for the OSX case.
Attached patch win7 simple gesture support v.1 (obsolete) — Splinter Review
Here's the first rev. I still have some things to work out with fall through on dom events, but I thought I'd push this up here in the event that display landed out in mtn. view.
I went ahead and implemented the other two events - MozRolloverGesture and MozTapGesture and should have the dom event issues straightened out tomorrow. I'll post another patch once that's omplete.

After some tested we can figure out what default commands we want to map these too. My personal take would be - 

pan = scroll the main view side to side or up and down

(The back button is large enough to be pressed directly and I don't think forward gets much use, so I'm not a fan of our current swipe.)

magnification = page magnification

double tap = restore the last change within the view, like an undo

rollover = right click

rotate = a counter clockwise rotate is sort of like turning a tap off, so maybe close tab? a clockwise rotate might work well for spinning a tab off into a new window.
(In reply to comment #11)
> Try builds - 
> https://build.mozilla.org/tryserver-builds/2009-02-25_16:43-jmathies@mozilla.com-try-2116ecbd02b/

Hey Jim, i just took your drop above and installed it on my Dell XT (Windows 7 Touch enabled). Panning does not seem to be working, I am still seeing mouse drags instead of Panning; working exactly the same as it does in 3.0.
(In reply to comment #10)
> Created an attachment (id=364207) [details]
> win7 simple gesture support v.1
> 
> Here's the first rev. I still have some things to work out with fall through on
> dom events, but I thought I'd push this up here in the event that display
> landed out in mtn. view.

+  config[2].dwWant = GC_PAN_WITH_SINGLE_FINGER_VERTICALLY|GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY;
+  config[2].dwID = GID_PAN;
+  config[2].dwBlock = GC_PAN_WITH_GUTTER|GC_PAN|GC_PAN_WITH_INERTIA;
I think you want GC_PAN in the dwWant field.

In your message handler, for GID_BEGIN & GID_END, you should always pass those to DefWndProc.

+        // XXX If I start a pan on the desktop, cross over the foreground window and off of it,
+        // does the foreground window 'own' the events for the entire pan?
No, the desktop will get all of the pan events.

+        mZoomStartDistance = (PRUint64) gi.ullArguments; // XXX 64 bit, what units?
+        mZoomIntermediate = mZoomStartDistance;
The low 32 bits are the distance. They're in pixels.

+        // XXX Is this always 0 or does it contain an initial angle based
+        // on orientation? (I'm assuming the latter)
The latter.
So your delta in the GID_ROTATE GF_END handler looks wrong. In the GF_BEGIN message, the angle is an absolute orientation, otherwise it is a delta.
(In reply to comment #14)
> (In reply to comment #10)
> > Created an attachment (id=364207) [details] [details]
> > win7 simple gesture support v.1
> > 
> > Here's the first rev. I still have some things to work out with fall through on
> > dom events, but I thought I'd push this up here in the event that display
> > landed out in mtn. view.
> 
> +  config[2].dwWant =
> GC_PAN_WITH_SINGLE_FINGER_VERTICALLY|GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY;
> +  config[2].dwID = GID_PAN;
> +  config[2].dwBlock = GC_PAN_WITH_GUTTER|GC_PAN|GC_PAN_WITH_INERTIA;
> I think you want GC_PAN in the dwWant field.

Did I miss interpret those two constants? If we simply want left/right and up/down swipe data, wouldn't this be appropriate?

> 
> In your message handler, for GID_BEGIN & GID_END, you should always pass those
> to DefWndProc.

That might explain why it's not working for amish!

> 
> +        // XXX If I start a pan on the desktop, cross over the foreground
> window and off of it,
> +        // does the foreground window 'own' the events for the entire pan?
>
> No, the desktop will get all of the pan events.

Alright good to know, thanks.

> 
> +        mZoomStartDistance = (PRUint64) gi.ullArguments; // XXX 64 bit, what
> units?
> +        mZoomIntermediate = mZoomStartDistance;
> The low 32 bits are the distance. They're in pixels.
> 
> +        // XXX Is this always 0 or does it contain an initial angle based
> +        // on orientation? (I'm assuming the latter)
> The latter.
> So your delta in the GID_ROTATE GF_END handler looks wrong. In the GF_BEGIN
> message, the angle is an absolute orientation, otherwise it is a delta.


Thanks for feedback guys!
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #10)
> > +  config[2].dwWant =
> > GC_PAN_WITH_SINGLE_FINGER_VERTICALLY|GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY;
> > +  config[2].dwID = GID_PAN;
> > +  config[2].dwBlock = GC_PAN_WITH_GUTTER|GC_PAN|GC_PAN_WITH_INERTIA;
> > I think you want GC_PAN in the dwWant field.
> 
> Did I miss interpret those two constants? If we simply want left/right and
> up/down swipe data, wouldn't this be appropriate?
GC_PAN enables two-finger panning (and panning in general). GC_PAN_WITH_SINGLE_FINGER_VERTICALLY says that you specifically also want single-finger-panning in the vertical direction. There is no way to say that you only want single-finger panning (not that I think you'd want that here anyway).

> > 
> > In your message handler, for GID_BEGIN & GID_END, you should always pass those
> > to DefWndProc.
> 
> That might explain why it's not working for amish!
This is actually necessary for default handling of gestures to work -- i.e. future-proofing. It isn't the cause of panning not working (much more likely is the GC_PAN issue I mentioned).
Attached patch win7 simple gesture support v.2 (obsolete) — Splinter Review
- added tab and rollover events
- addressed various comments
- fixed dom fall through issue
Attachment #364207 - Attachment is obsolete: true
FYI:
Hey Jim, i took a drop from above and ran it on my Win7 Dell XT w/ multi-touch enabled. Still no panning support, getting mousedrags.
(In reply to comment #19)
> FYI:
> Hey Jim, i took a drop from above and ran it on my Win7 Dell XT w/ multi-touch
> enabled. Still no panning support, getting mousedrags.

What were you attempting to do with panning? Currently it's hooked up to the mac forward and back gestures. If you were trying to pan the content view, that wont work.

Overall though I don't have the display / win7 so we'll have to wait till someone in Mtn View can do some debugging. If there are any issues they should be small. I tested the functionality using various test messages and everything was working fine. 

Rob, Vlad did you guys get any chances to play with this?
Panning should map to scrolling the page.  The flick gestures (short strokes in one of 8 directions, generally map to appcommands) handle forward and backward navigation already, and are very similar to the 3-finger swipe gesture Apple's trackpads have.  (For historical accuracy, note Flicks were introduced in Vista and pre-date the trackpads.  ;)  )

Adding to what Amish said, I installed the build and I'm not getting obvious responses to press&tap (aka rollover) or two-finger tap.
(In reply to comment #21)
> Panning should map to scrolling the page.  The flick gestures (short strokes in
> one of 8 directions, generally map to appcommands) handle forward and backward
> navigation already, and are very similar to the 3-finger swipe gesture Apple's
> trackpads have.  (For historical accuracy, note Flicks were introduced in Vista
> and pre-date the trackpads.  ;)  )
> 
> Adding to what Amish said, I installed the build and I'm not getting obvious
> responses to press&tap (aka rollover) or two-finger tap.

So WM_TABLET_FLICK messages super cede WM_GESTURE messages? If that's the case we probably want to add support for flicks as well and map those to simple gestures. For example would a "flick" to the right with your finger come in as a WM_TABLET_FLICK and not generate a WM_GESTURE pan message or would be generated?
(In reply to comment #22)
> 
> So WM_TABLET_FLICK messages super cede WM_GESTURE messages? If that's the case
> we probably want to add support for flicks as well and map those to simple
> gestures. For example would a "flick" to the right with your finger come in as
> a WM_TABLET_FLICK and not generate a WM_GESTURE pan message or would be
> generated?

err ^^ "or would _both_ be generated?"
This is what the user will expect from the rest of Windows7 and IE. 

TabletFlicks
Back and Foward = page foward and back navigation 

WM_GESTURE: 
Panning: Smooth scolling (up and down)
Press and Tap - right click (hand back to defwinproc and let the system  generate the event)
TwoFingerTap - smart zoom
Zoom - zoom
Left to Right drags - mouse down drag / selection 

The Touch screen was send out last monday so should be in MtnView by now. I encourage you check out the interaction in IE. Let me know if you need anymore guidence on the UX.
(In reply to comment #20)
>...
> Rob, Vlad did you guys get any chances to play with this?
Not yet... I should have time to do so tomorrow.
To answer Jim's questions, you don't need to do anything to handle flicks.  I believe you're already handling the appcommand for navigate forward/backwards, which is what an unhandled WM_TABLET_FLICK message gets promoted to in the left/right flick case.  The default handling for flick unaware apps should be working - you shouldn't need to do anything there.

For the other question, a user can both flick and pan.  A flick is a short, fast, straight stroke, and the set of flicks are a subset of the set of pans.  We intentionally disable up/down flicks on pannable surfaces b/c they conflict too often with the common case of vertical panning, but the diagonals (editing flicks) and the left/right ones are enabled.  If an app wants to disable flicks entirely on their window, they can using WM_TABLET_QUERYSYSTEMGESTURESTATUS or the MICROSOFT_TABLETPENSERVICE_PROPERTY window property.
To answer Jim's questions, you don't need to do anything to handle flicks.  I believe you're already handling the appcommand for navigate forward/backwards, which is what an unhandled WM_TABLET_FLICK message gets promoted to in the left/right flick case.  The default handling for flick unaware apps should be working - you shouldn't need to do anything there.

For the other question, a user can both flick and pan.  A flick is a short, fast, straight stroke, and the set of flicks are a subset of the set of pans.  We intentionally disable up/down flicks on pannable surfaces b/c they conflict too often with the common case of vertical panning, but the diagonals (editing flicks) and the left/right ones are enabled.  If an app wants to disable flicks entirely on their window, they can using WM_TABLET_QUERYSYSTEMGESTURESTATUS or the MICROSOFT_TABLETPENSERVICE_PROPERTY window property.
Took a look at the V.2 changes linked above.  Some quick comments:
* I think I misspoke about press and tap (rollover).  It looks like it's working in the try build above, and from the code looks like it's getting handed off to defwndproc to handle correctly.
* It doesn't look like anything is hooked up for two-finger tap.  It looks like it's getting handed off to defwndproc currently.  There's no default OS action for this gesture, so if you want it to do something it needs to be mapped to an action.  You don't have to use it, but if you do, the UX guidelines recommend restoring the view to 100% or toggling some sort of smart zoom.  Happy to discuss this in more depth offline if you want.
* Zoom looks like it's working.
* Panning should scroll the page.

Let me know if you have questions - happy to have a quick conf call or email exchange.  :)
(In reply to comment #28)
> Took a look at the V.2 changes linked above.  Some quick comments:
> * I think I misspoke about press and tap (rollover).  It looks like it's
> working in the try build above, and from the code looks like it's getting
> handed off to defwndproc to handle correctly.
> * It doesn't look like anything is hooked up for two-finger tap.  It looks like
> it's getting handed off to defwndproc currently.  There's no default OS action
> for this gesture, so if you want it to do something it needs to be mapped to an
> action.  You don't have to use it, but if you do, the UX guidelines recommend
> restoring the view to 100% or toggling some sort of smart zoom.  Happy to
> discuss this in more depth offline if you want.
> * Zoom looks like it's working.
> * Panning should scroll the page.
> 
> Let me know if you have questions - happy to have a quick conf call or email
> exchange.  :)

Thanks reedt, that all sound about right based on the way I hooked it up. The actions aren't mapped to much at this point, we'll sort that out here in a bit once we're sure the core chunk of code is hooked up right. I think we'll probably get one of our own usability folks in here too to discuss how we want things mapped out.
Attached patch latest merged (obsolete) — Splinter Review
Attachment #364375 - Attachment is obsolete: true
Just an FYI, no new try builds for today as I'm still working on things. All in all gestures are working fine I just need to get things completely hooked up right.
Hey guys, have a question - is there a trick to getting the main window frame to do that bump when you reach the end of a scroll? I noticed this in a number of apps but for some reason Fx doesn't exhibit the behavior.
Blocks: 485016
Yes, you should use the boundary feedback functions.  http://msdn.microsoft.com/en-us/library/dd371419(VS.85).aspx

Basically, call BeginPanningFeedback() when a pan takes you beyond the boundary of your page.  UpdatePanningFeedback() in response to panning events that are still outside the boundaries.  EndPanningFeedback() to finish or cancel the bounce effect.

For example, if you get a message requesting a 50px pan, and you have 20px left of content, you would pan 20px and hand the remaining 30px off to UpdatePanningFeedback().  As the user continues to pan, keep calling UPF() in response until they're back within the boundaries of the content or they stop panning.

Hope that helps.  Let me know if you have questions,
- Reed
(In reply to comment #33)
> Yes, you should use the boundary feedback functions. 
> http://msdn.microsoft.com/en-us/library/dd371419(VS.85).aspx
> 
> Basically, call BeginPanningFeedback() when a pan takes you beyond the boundary
> of your page.  UpdatePanningFeedback() in response to panning events that are
> still outside the boundaries.  EndPanningFeedback() to finish or cancel the
> bounce effect.
> 
> For example, if you get a message requesting a 50px pan, and you have 20px left
> of content, you would pan 20px and hand the remaining 30px off to
> UpdatePanningFeedback().  As the user continues to pan, keep calling UPF() in
> response until they're back within the boundaries of the content or they stop
> panning.
> 
> Hope that helps.  Let me know if you have questions,
> - Reed

Thanks. I think I'll peel this off into another bug and get what we have landed. We're starting to run out of time here.
Blocks: 485101
Attached patch win7 multitouch v.1 (obsolete) — Splinter Review
Unfortunately our try server has been experiencing some problems over the last 24 hours, so I still don't have try builds for testing. I've finished up testing this on a release build on the hp though and things look to be in pretty good shape.
Attachment #366854 - Attachment is obsolete: true
Attachment #369322 - Flags: review?(vladimir)
Attached patch win7 multitouch v.1 (obsolete) — Splinter Review
Sorry for the spam, I did a little code cleanup in browser.js.
Attachment #369322 - Attachment is obsolete: true
Attachment #369322 - Flags: review?(vladimir)
Attached patch win7 multitouch v.3 (obsolete) — Splinter Review
Updated browser tests for simple gestures.
Attachment #369340 - Attachment is obsolete: true
Attachment #369373 - Flags: review?(vladimir)
Attachment #369373 - Flags: review?(jonas)
Comment on attachment 369373 [details] [diff] [review]
win7 multitouch v.3

vlad -> browser and widget
jonas -> dom and content

Please switch out reviewers if you don't think you can get to this. The goal is to get it into 3.5.
I should add a few points - we support the following gestures -

two finger drag -> pan of the main view w/inertia
two finger rotate -> flip tabs
two finger pinch -> zoom in/out
two finger tap -> zoom reset

The rollover gesture falls through to the default. Flicks messages currently also fall though, I'll be hooking those up to simple gestures later.

In testing try builds, make sure and start with a fresh profile. I had to tweak some prefs on a per platform basis.
Comment on attachment 369373 [details] [diff] [review]
win7 multitouch v.3

Events stuff so over to smaug
Attachment #369373 - Flags: review?(jonas) → review?(Olli.Pettay)
Comment on attachment 369373 [details] [diff] [review]
win7 multitouch v.3

> interface nsIDOMSimpleGestureEvent : nsIDOMMouseEvent
...
>+  /* Handled flag
>+   * 
>+   * 'handled' indicates whether or not the gesture event was handled
>+   * by a listener. On systems that support default behavior for
>+   * gestures, handled indicates whether the gesture event should fall
>+   * fall through and trigger default system behavior or be ignored.
>+   */
>+  attribute boolean handled;
Why do you need this? All the dom events have .preventDefault() method for this.

>+PRBool nsWindow::ProcessGestureMessage(WPARAM wParam, LPARAM lParam)
>+{
>+  // Treatment for pan events which translate into pixel scrolling:
Yeah, this is the right thing to do, but it would be great to get
also a mouse scroll event, similar way what happens on OSX.
Otherwise things like zoom in/out in Google maps don't work (IIRC, it uses
DOMMouseScroll events for zoom).
Mouse scroll should happen only if user has panned "enough".
Does Windows provide some information how many pixels is one
"wheel roll"? That could be used to convert pan to mouse scroll.

A testcase for pixel/mouse scrolling is this http://mozilla.pettay.fi/moztests/gmaps.html
One should be able to zoom in/out the map without scrolling the iframe.
(Test fails on Safari 3.x/OSX)

>+    if ( !mGesture.ProcessPanMessage(mWnd, wParam, lParam, eventX, eventY) )d
extra space after '(' and before ')'
The method name ProcessPanMessage is a bit strange. It doesn't process anything,
it just copies data from native event to gecko event.
Maybe ConvertPanToPixelScrollEvent()?

>+  if ( !mGesture.ProcessGestureMessage(mWnd, wParam, lParam, event) ) {
Also here, extra space after '(' and before ')'

Tests! You could probably extend current gesture tests to handle new events.

r- because there are no tests and because this adds (as far as I see) useless .handled.

And it would be really really great if mouse scroll events could be dispatched when
needed, but if Windows doesn't provide necessary information, we either have to skip
that feature, or add some hack that if user is panning more than X pixels, dispatch
also a mouse scroll event.
Attachment #369373 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #42)
> (From update of attachment 369373 [details] [diff] [review])
> > interface nsIDOMSimpleGestureEvent : nsIDOMMouseEvent
> ...
> >+  /* Handled flag
> >+   * 
> >+   * 'handled' indicates whether or not the gesture event was handled
> >+   * by a listener. On systems that support default behavior for
> >+   * gestures, handled indicates whether the gesture event should fall
> >+   * fall through and trigger default system behavior or be ignored.
> >+   */
> >+  attribute boolean handled;
> Why do you need this? All the dom events have .preventDefault() method for
> this.

Ah, that's exactly what I was looking for with the return value but couldn't find, will update.

> 
> >+PRBool nsWindow::ProcessGestureMessage(WPARAM wParam, LPARAM lParam)
> >+{
> >+  // Treatment for pan events which translate into pixel scrolling:
> Yeah, this is the right thing to do, but it would be great to get
> also a mouse scroll event, similar way what happens on OSX.
> Otherwise things like zoom in/out in Google maps don't work (IIRC, it uses
> DOMMouseScroll events for zoom).
> Mouse scroll should happen only if user has panned "enough".
> Does Windows provide some information how many pixels is one
> "wheel roll"? That could be used to convert pan to mouse scroll.
> 
> A testcase for pixel/mouse scrolling is this
> http://mozilla.pettay.fi/moztests/gmaps.html
> One should be able to zoom in/out the map without scrolling the iframe.
> (Test fails on Safari 3.x/OSX)

I'm not sure I understand why we would want to send mouse scroll events when the user is panning the view. When I pan a google maps view around with two fingers, I don't want the zoom to change, I want the map to pan. Or am I misinterpreting what you're asking for? 

> 
> >+    if ( !mGesture.ProcessPanMessage(mWnd, wParam, lParam, eventX, eventY) )d
> extra space after '(' and before ')'
> The method name ProcessPanMessage is a bit strange. It doesn't process
> anything,
> it just copies data from native event to gecko event.
> Maybe ConvertPanToPixelScrollEvent()?

will do.

> 
> >+  if ( !mGesture.ProcessGestureMessage(mWnd, wParam, lParam, event) ) {
> Also here, extra space after '(' and before ')'
> 
> Tests! You could probably extend current gesture tests to handle new events.

will do.
Hey Jim, your patch is working really well and I have just a couple of nitpicky observations

1. Single finger panning should be enabled so users that tend to prefer that method don't have to switch when going from another app that supports single finger panning to Firefox. I think it is going to be extremely important to duplicate as much as possible so we don't confuse people's motor memory.

2. I don't think it is related to the patch but the first time bringing up the context menu via tap + hold displays a different (perhaps a subset) of the context menu. Every subsequent context menu displays correctly including new tabs / windows.

3. rotate works great but it may need to be a tad more sensitive.

4. I noticed that IE doesn't have flick up / down perform drag up / down. Not sure if we should as well but it is something to think about so we don't confuse people's motor memory.

Thanks!
Looks good so far.  I'm seeing some of the same things Robert is:

1) Single-finger panning (SFP) isn't hooked up.  From a user research perspective, we see most users use SFP over two-finger panning when it's available.  (Deep diving a bit here, but once SFP is hooked up, interacting w/ AJAX stuff [like map sites] becomes trickier.  Without changes to the sites, most of the time you'll get panning messages, when the AJAX wants mouse messages.  I assume your preferred solution here would be AJAX stuff uses your DOM gesture extensions to handle those events instead of Firefox handling them.)
2) No boundary bounce feedback - I think Jim said this was going to be tracked by another bug.
3) Zoom: Might want to update the view more often for the user.  Good visual feedback about how far they've zoomed helps with gesture reliability and confidence.  For comparison, zoom with ctrl-scrollwheel on the mouse - it feels faster.
4) Rotate: I found the speed at which it cycles through tabs a bit fast.  I think that's the opposite of Robert's feeling if I'm reading his comment correctly.  :)  Might want to do a usability study (or run it by a few users) and tune that appropriately.

- Reed
(In reply to comment #45)
> messages.  I assume your preferred solution here would be AJAX stuff uses your
> DOM gesture extensions to handle those events instead of Firefox handling
> them.)

FYI for everyone: Current policy is that web content should not be able to see the gesture events because gesture interface has not been truly finalized in any way. See roc's comment (#61) in bug 412486.  Firefox calls stopPropagation from a capturing phase event listener to enforce this.  This policy may need to be revisited, but will require some thought as to what the API should look like for content as to the API I devised, which really is nearly a one-to-one map to the Mac OS X API.
s/as to/as opposed to/
Attached patch win7 multitouch v.4 (obsolete) — Splinter Review
Ok, here's an updated patch. Olli, would you mind taking a look at the event state manager stuff?

Try builds just got fired off so I'll post those tomorrow if they make it.

Single finger scrolling is now hooked up, nswingesture and nswindow code is revamped a bit and I've added some tests.

Google maps however is still not reacting to pan. I'm thinking this has nothing to do with this implementation and is instead related to the handling of pixel and line scrolling events farther down. I'll try to dig further into that tomorrow. 

The bump feedback stuff is in another bug (bug 485101), I'll flip over to that as soon as this is finished.
Attachment #369373 - Attachment is obsolete: true
Attachment #369618 - Flags: review?(Olli.Pettay)
Attachment #369373 - Flags: review?(vladimir)
Comment on attachment 369618 [details] [diff] [review]
win7 multitouch v.4

never mind, just found the google problem. new patch coming up here in a bit.
Attachment #369618 - Flags: review?(Olli.Pettay)
Attached patch win7 multitouch v.5 (obsolete) — Splinter Review
Attachment #369618 - Attachment is obsolete: true
Attachment #369625 - Flags: review?(Olli.Pettay)
>+class nsPointWin : public nsIntPoint
>+{
>+public:
>+   nsPointWin operator=(const POINTS& aPoint) {
>+     x = aPoint.x; y = aPoint.y;
>+     return *this;
>+   }
>+   nsPointWin operator=(const POINT& aPoint) {
>+     x = aPoint.x; y = aPoint.y;
>+     return *this;
>+   }
>+   nsPointWin operator+=(const POINTS& aPoint) {
>+     x += aPoint.x; y += aPoint.y;
>+     return *this;
>+   }
>+   nsPointWin operator=(int val) {
>+     x = y = val;
>+     return *this;
>+   }

Minor C++ nit: Assignment operators should return a reference to the object and not a copy.
Attachment #369625 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 369625 [details] [diff] [review]
win7 multitouch v.5

>         return this._setupGesture(aEvent, "twist", def(25, 0), "right", "left");
>       case "MozMagnifyGestureUpdate":
>       case "MozRotateGestureUpdate":
>+        aEvent.preventDefault();
>         return this._doUpdate(aEvent);
>+      case "MozTapGesture":
>+        aEvent.preventDefault();
>+        return this._doAction(aEvent, ["tap"]);
>+      case "MozTapGesture":
>+      // Fall through to default behavior
>+      return;
Why you have "MozTapGesture" twice?

>+          if (abs(mPixelScrollDeltaX) >= lineHeight) {
Nit, PR_ABS. Same also elsewhere.

>+  // Pixel scroll accumulation for synthetic line scrolls
>+  nscoord mPixelScrollDeltaX;
>+  nscoord mPixelScrollDeltaY;
These are tricky. Someone may scroll some scrollable area just few pixels, then
scroll some other scrollable area few pixels. The code in ESM would end up firing
mouse scroll event, even though nothing was scrolled that much.
Perhaps you could clear the values in nsMouseWheelTransaction::EndTransaction()
(in that case it might be ok if the variables are global).
And add NS_MOUSE_PIXEL_SCROLL handling to nsMouseWheelTransaction::OnEvent.

>+class nsWinGesture
...
>+  //PRBool PanDeltaToScrollX(nsMouseScrollEvent& evt);
>+  //PRBool PanDeltaToScrollY(nsMouseScrollEvent& evt);
Why these?

Question, how does Single-finger panning work on Win7? What is the difference
to Two finger panning? What does the patch do with single finger panning and what
does it do with 2-finger panning?
(In reply to comment #52)
> (From update of attachment 369625 [details] [diff] [review])
> >         return this._setupGesture(aEvent, "twist", def(25, 0), "right", "left");
> >       case "MozMagnifyGestureUpdate":
> >       case "MozRotateGestureUpdate":
> >+        aEvent.preventDefault();
> >         return this._doUpdate(aEvent);
> >+      case "MozTapGesture":
> >+        aEvent.preventDefault();
> >+        return this._doAction(aEvent, ["tap"]);
> >+      case "MozTapGesture":
> >+      // Fall through to default behavior
> >+      return;
> Why you have "MozTapGesture" twice?

Typo, that should be MozRolloverGesture so we can let it fall trough without preventDefault. I could also just pull it out of the observer list but figured having it in there insure people new it existed.

> 
> >+          if (abs(mPixelScrollDeltaX) >= lineHeight) {
> Nit, PR_ABS. Same also elsewhere.
> 
> >+  // Pixel scroll accumulation for synthetic line scrolls
> >+  nscoord mPixelScrollDeltaX;
> >+  nscoord mPixelScrollDeltaY;
> These are tricky. Someone may scroll some scrollable area just few pixels, then scroll some other scrollable area few pixels. The code in ESM would end up
firing mouse scroll event, even though nothing was scrolled that much.
> Perhaps you could clear the values in nsMouseWheelTransaction::EndTransaction()
> (in that case it might be ok if the variables are global).
> And add NS_MOUSE_PIXEL_SCROLL handling to nsMouseWheelTransaction::OnEvent.

I was a little unsure of that myself but I didn't see any other way to cache the change in delta. I'll update per your comments.

> >+class nsWinGesture
> ...
> >+  //PRBool PanDeltaToScrollX(nsMouseScrollEvent& evt);
> >+  //PRBool PanDeltaToScrollY(nsMouseScrollEvent& evt);
> Why these?

They save us from having to create and init two mouse scroll events in nsWindow on pixel scroll events, seemed like it would be faster.

> Question, how does Single-finger panning work on Win7? What is the difference
> to Two finger panning? What does the patch do with single finger panning and
> what does it do with 2-finger panning?

As long as the GestureConfig flags request single finger pans, the two generate the same events.

Getting this to work was nice although it had a negative effect on sites like google maps. With SFP disabled, single finger interaction translated into mouse down and mouse move events.  On a site like gmaps, this allowed the user to grab and pan the map around using one finger and change zoom with two. With SFP enabled, both single and double finger pans translate to zoom so there's no way to pan the map. 

On the flip side html views work as you would expect them to - with SFP enabled both single a double finger pans scroll the page. With SFP _disabled_ single finger pans cause content selection.
Jim, SFP is working very well! Couple of more notes:

1. The context menu issue appears to be in part due to selection though today it also showed a couple of different subsets of the context menu as follows:
Copy, Select All, View Selection Source, Properties
and
Copy, Select All, View Selection Source

I was also able too get these context menu to show as well as the correct context menu with these same items when doing a real right click so I think this is a pre-existing bug.

2. Rotate seems to switch tabs at approximately 45 degrees. I'm curious if you or Reed are seeing something different?

3. I agree with Reed that zoom should be a tad more sensitive.


Do you know if panning behaves differently with smooth scrolling turned on? It seems to be the same for me.
Attached patch win7 multitouch v.6 (obsolete) — Splinter Review
Attachment #369625 - Attachment is obsolete: true
Attached patch win7 multitouch v.6 (obsolete) — Splinter Review
clean up, removed flicks placeholder code.
Attachment #369717 - Attachment is obsolete: true
Attachment #369722 - Flags: review?(Olli.Pettay)
(In reply to comment #54)
> Jim, SFP is working very well! Couple of more notes:
> 
> 1. The context menu issue appears to be in part due to selection though today
> it also showed a couple of different subsets of the context menu as follows:
> Copy, Select All, View Selection Source, Properties
> and
> Copy, Select All, View Selection Source
> 
> I was also able too get these context menu to show as well as the correct
> context menu with these same items when doing a real right click so I think
> this is a pre-existing bug.

Sounds right. This code wouldn't be involved with that, maybe spin it off in another bug and cc me..

> 
> 2. Rotate seems to switch tabs at approximately 45 degrees. I'm curious if you
> or Reed are seeing something different?
> 
> 3. I agree with Reed that zoom should be a tad more sensitive.
> 

Would you mind testing different values and tell me what you prefer? The prefs are:

browser.gesture.pinch.threshold
browser.gesture.twist.threshold

Pinch to me seemed pretty good.

> 
> Do you know if panning behaves differently with smooth scrolling turned on? It
> seems to be the same for me.

Doesn't seem to for me. I'll take a look with the latest event code changes once I get release builds.
Comment on attachment 369722 [details] [diff] [review]
win7 multitouch v.6

>+        if (msEvent->scrollFlags & nsMouseScrollEvent::kIsVertical) {
>+          gPixelScrollDeltaX += msEvent->delta;
>+
>+          if (PR_ABS(gPixelScrollDeltaX) >= lineHeight) {
>+            nsWeakFrame weakFrame(aTargetFrame);
>+            SendLineScrollEvent(aTargetFrame, msEvent, aPresContext, aStatus,
>+              (PRInt32)ceil((float)gPixelScrollDeltaX/(float)lineHeight));
>+            NS_ENSURE_STATE(weakFrame.IsAlive());
>+
>+            gPixelScrollDeltaX = 0;
Here and with gPixelScrollDeltaY you probably shouldn't set the value to 0, but
leave the leftover pixels to the variable.

Other than that, r=me, but I can't really review the windows/ parts.
Attachment #369722 - Flags: review?(Olli.Pettay) → review+
Attached patch win7 multitouch v.7 (obsolete) — Splinter Review
Attachment #369722 - Attachment is obsolete: true
Comment on attachment 369850 [details] [diff] [review]
win7 multitouch v.7

I should have try builds here later tonight and will post them. 

I also promised Olli more tests which I'll try to get done the next couple days.
Attachment #369850 - Flags: review?(vladimir)
I think I've gotten used to the current behavior. Rotate appears to switch tabs at 30% rotation and zoom in / out seems sensitive enough. I'm curious if I misinterpreted Reed's comment about zooming (e.g. visual notification as in having the current zoom level in the status bar or something else).

I think panning is working good with and without smooth scrolling.
Regarding zoom, it appears Firefox doesn't re-render until the fingers come up or stop moving.  If possible, giving the user visual feedback during the gesture is preferable - otherwise it's hard to determine how far you're zooming in/out.  From playing w/ ctrl-scrollwheel, it looks like Firefox renders a page quickly enough you could zoom during the gesture to give interim feedback.

I also see a number of references to "rollover".  That gesture has been renamed to "Press and Tap" in WM_GESTURE docs after beta and dev feedback.  Folks generally found rollover a confusing term.

The other gestures look good.  :)
(In reply to comment #63)
> Regarding zoom, it appears Firefox doesn't re-render until the fingers come up
> or stop moving.
Is this perhaps because of bug 462935? Maybe we want to have different
behavior on Win7 than on OSX.
(In reply to comment #63)
> Regarding zoom, it appears Firefox doesn't re-render until the fingers come up
> or stop moving.  If possible, giving the user visual feedback during the
> gesture is preferable - otherwise it's hard to determine how far you're zooming
> in/out.  From playing w/ ctrl-scrollwheel, it looks like Firefox renders a page
> quickly enough you could zoom during the gesture to give interim feedback.
> 
> I also see a number of references to "rollover".  That gesture has been renamed
> to "Press and Tap" in WM_GESTURE docs after beta and dev feedback.  Folks
> generally found rollover a confusing term.
> 
> The other gestures look good.  :)

(In reply to comment #64)
> (In reply to comment #63)
> > Regarding zoom, it appears Firefox doesn't re-render until the fingers come up
> > or stop moving.
> Is this perhaps because of bug 462935? Maybe we want to have different
> behavior on Win7 than on OSX.

Reed, are you testing with a fresh build and new profile? The prefs created in the patch Smaug mentions (bug 462935) had things setup so that pinch would only make one change to zoom. In the latest patch, I customized this (the latched pref) so that win will do dynamic zooming. Last time I checked this was working. I'll check again to be sure here in a bit as I'm in the middle of a fresh test build.
(In reply to comment #63)
> I also see a number of references to "rollover".  That gesture has been renamed
> to "Press and Tap" in WM_GESTURE docs after beta and dev feedback.  Folks
> generally found rollover a confusing term.

Also - can I get a definitive explanation of how I trigger this press and hold? I' ve never quite gotten it down. :)  I'll update the final patch to reflect the name change.
I did a fresh install today on a machine that's never had Firefox installed and saw the same behavior.  Happy to test again with a fresh profile though.  What's the easiest way to ensure I've got a new profile?

There are two right click gestures: Press and hold and press and tap.  Press and hold is pretty straightforward - just put a finger down on the screen (don't move it), wait for the ring animation to finish, and then remove your finger.  You should get a right click.  For press and tap, it's a bit harder to explain in text - put one finger down (and keep it down), and then tap with another finger (down + up).  

The first video here (http://blogs.msdn.com/e7/archive/2009/03/25/touching-windows-7.aspx) has a demo of press and tap and press and hold about halfway through.  Hopefully that will make it clearer.  :)

- Reed
Attached patch win7 multitouch v.8 (obsolete) — Splinter Review
A couple touch-ups

- fixed a compile error on linux
- filtered some window types out when enabling gesture input (java, plugin, toplevel)
- fixed a minor bug on views that only accept line scroll events

As far as pinch goes, its working on my end. I get multiple zooms for a single pinch.
Attachment #369850 - Attachment is obsolete: true
Attachment #370444 - Flags: review?(vladimir)
Attachment #369850 - Flags: review?(vladimir)
(In reply to comment #67)
> I did a fresh install today on a machine that's never had Firefox installed and
> saw the same behavior.  Happy to test again with a fresh profile though. 
> What's the easiest way to ensure I've got a new profile?

Simplest way for me is to remove any app data by deleting or renaming - 

C:\Users\username\AppData\Roaming\Mozilla

..or fire up the profile manager - 

firefox.exe -ProfileManager
try builds:
https://build.mozilla.org/tryserver-builds/2009-04-01_07:51-jmathies@mozilla.com-multitouch-v8/

That's pretty much the final at this point.
Comment on attachment 370444 [details] [diff] [review]
win7 multitouch v.8

This looks fine to me -- I agree that press-and-tap sounds slightly more intuitive than rollover, so I'd suggest we make that name change, but I'm fine with it either way.
Attachment #370444 - Flags: review?(vladimir) → review+
(In reply to comment #71)
> (From update of attachment 370444 [details] [diff] [review])
> This looks fine to me -- I agree that press-and-tap sounds slightly more
> intuitive than rollover, so I'd suggest we make that name change, but I'm fine
> with it either way.

Sorry spaced the event name change. Just fired off a final to try, will do some testing tomorrow morning and if all looks good I'll land it on mc and nominate for 1.9.1.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b4
Attachment #370710 - Flags: approval1.9.1?
Target Milestone: mozilla1.9.1b4 → mozilla1.9.2a1
Created a fresh profile and I see the same issues w/ zoom.

It looks like it depends on the complexity of the page.  I was testing on cnn.com - zoom gestures don't feel very responsive at all.  (I assume due to re-laying out taking longer.)  On a very lightweight page, like google.com, zoom seems pretty responsive.
(In reply to comment #74)
> Created a fresh profile and I see the same issues w/ zoom.
> It looks like it depends on the complexity of the page.  I was testing on
> cnn.com - zoom gestures don't feel very responsive at all.  (I assume due to
> re-laying out taking longer.)  On a very lightweight page, like google.com,
> zoom seems pretty responsive.

Thanks Reed, I'll look into that. I've seen something similar on initial zoom operations, after the first zoom set things seem to speed up. We might have a performance issue there.

Overall, just wanted to say - thanks for all the help, feedback, and testing you provided. We really appreciate it.
I wonder if one and two finger pan should generate different events.
Perhaps one-finger-pan should always just scroll, and two-finger-pan could
send mousescroll event and then scroll. If this makes sense, a followup bug
could be filed.
Thanks Jim - You guys have been a pleasure to work with as well.  :)

Regarding differentiating one vs two finger panning, our general UX advice has been that they should map to the same panning behavior for the user.  From a platform point of view, they send the same WM_GESTURE panning messages, but a developer could distinguish them by inspecting the distance between the fingers in the arguments.
Jim/Vlad: is it possible to get tests for this?
Flags: in-testsuite?
See the tests we have for gesture support on OS X on bug 456520 too.
Mike, the patch that landed has tests... specifically using the same test from bug 456520 modified to include the added gestures.

(In reply to comment #37)
> Created an attachment (id=369373) [details]
> win7 multitouch v.3
> 
> Updated browser tests for simple gestures.
Flags: in-testsuite? → in-testsuite+
Comment on attachment 370710 [details] [diff] [review]
pushed to mc

a191=beltzner
Attachment #370710 - Flags: approval1.9.1? → approval1.9.1+
Depends on: 477863
Attached patch mozilla-1.9.1 patch (obsolete) — Splinter Review
This is dependent on the 1.9.1 patch in Bug 477863, so depending on what happens there, I may need to roll another one.
Attachment #371943 - Attachment is obsolete: true
Keywords: fixed1.9.1
Do we need dev-docs for this feature?
(In reply to comment #85)
> Do we need dev-docs for this feature?

I think we want to keep this internal at this point. I think we're still trying to figuring out how we will handle gesture and touch input in a more generic way that can also be exposed to content.
No longer blocks: win7support
Blocks: 488715
Summary: Add WM_GESTURE support for Windows 7 → [MSFT-9354] [MSFT-9360] Add WM_GESTURE support for Windows 7
Depends on: 491925
You need to log in before you can comment on or make changes to this bug.