485101 - Implement panning feedback for touch enabled displays with win7

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Panning feedback can produce a "bump" or "pull" on the main window frame of views that are panned beyond their boundaries on Win7. It produces a nice visual effect that looks to be pretty standard for windows apps running on win7.

https://bugzilla.mozilla.org/show_bug.cgi?id=479901#c33

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

Comment 1

[Jim Mathies [:jimm]]

Attachment: pan feedback v.1

A couple notes - I've added a no defer flag to the gui event so that pan scroll events do not cause asynchronous scrolling. This addressed the problem with knowing when the view hits it's bounds, and also had a nice side effect of improving pan responsiveness.

The float casts are just some warning cleanup in the scrollable view I came across.

The win stuff is pretty straight forward.

I'll have try server builds sometime tomorrow.

Comment 2

[Jim Mathies [:jimm]]

Try server is snappy today:

https://build.mozilla.org/tryserver-builds/2009-05-06_18:[e-mail]-try-d984ce01b94/

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> also had a nice side effect of
> improving pan responsiveness.
Strange. The async scrolling was added to improve pixel scrolling responsiveness/speed especially in cases where web page has many iframes or so.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Have you tested that "bump" works well also when the page has been
zoomed in or out?

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #3)
> (In reply to comment #1)
> > also had a nice side effect of
> > improving pan responsiveness.
> Strange. The async scrolling was added to improve pixel scrolling
> responsiveness/speed especially in cases where web page has many iframes or so.

I'll do some more testing, do you know the bug number for the pixel scroll performance bug off hand?

The visual difference I noticed is that without no defer, the page's drag position lags behind the user's drag point. With no defer the two stay more in sync.

(In reply to comment #4)

> Have you tested that "bump" works well also when the page has been
> zoomed in or out?

Yes, that seems to be working fine.

Comment 6

[reedt]

I'm not seeing bounce feedback on the try build above.  I assume I'm missing something.  :)

- Reed

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #6)
> I'm not seeing bounce feedback on the try build above.  I assume I'm missing
> something.  :)
> 
> - Reed

Hmm, should be working. I just finished updating to RC and will do some final testing over the weekend to be sure. I'll fire off a new set of try builds once I'm done.

Olli - not sure why macs were having performance issues with immediate pixel scroll stuff but it's definitely not a problem on windows. I'd like to keep the no defer events because the responsiveness is better with it for panning on win7.

Comment 8

[Jim Mathies [:jimm]]

Try builds - 

https://build.mozilla.org/tryserver-builds/2009-05-10_15:[e-mail]-feedback/

Rob, any chance you could give this a spin on your tablet?

Comment 9

[Robert Strong (they/them - no direct email)]

Just tried it out and bump is working. There is at least one difference in behavior when compared with IE. IE will perform a bump even when the scrollbar is already at the end when trying to pan further whereas the try build only bumps when the scrollbar hasn't already reached the end. After this happens the try build will sometimes allow one more bump on trying to pan and then no more.

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #9)
> Just tried it out and bump is working. There is at least one difference in
> behavior when compared with IE. IE will perform a bump even when the scrollbar
> is already at the end when trying to pan further whereas the try build only
> bumps when the scrollbar hasn't already reached the end. After this happens the
> try build will sometimes allow one more bump on trying to pan and then no more.

That's expected. Feedback is triggered only after the scrollable view returns an overscroll value.

Comment 11

[reedt]

(In reply to comment #9)
> Just tried it out and bump is working. There is at least one difference in
> behavior when compared with IE. IE will perform a bump even when the scrollbar
> is already at the end when trying to pan further whereas the try build only
> bumps when the scrollbar hasn't already reached the end. After this happens the
> try build will sometimes allow one more bump on trying to pan and then no more.

I'm confused by the latter part of this comment.  If the scrollable view hasn't reached the end, shouldn't it be panning and not triggering bounce feedback?  

Regarding the first point, an important reason for the bounce feedback is informing the user they can't pan in that direction.  User research showed us many users initially try and pan in the wrong direction (mirroring the action of a mouse on the scrollbar), and bounce feedback provides a quick reminder to pan the other way.

Finally, which Win7 build are you using to dev and test these days?  I'm still not able to trigger bounce feedback using the try build Jim posted above on recent Win7 builds.

Comment 12

[Robert Strong (they/them - no direct email)]

With the try build when it was not at the end of the scrollable area it will bump for feedback that it reached the end of the scrollable area and at times it appears that it allowed one more bump. It may be that it wasn't quite at the end of the scrollable region for the additional bump.

With IE I noticed that if the area could be scrolled it will always bump for feedback at the end of the scrollable area.

Still on the beta and will upgrade sometime later this week

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #11)
> Regarding the first point, an important reason for the bounce feedback is
> informing the user they can't pan in that direction.  User research showed us
> many users initially try and pan in the wrong direction (mirroring the action
> of a mouse on the scrollbar), and bounce feedback provides a quick reminder to
> pan the other way.


> Finally, which Win7 build are you using to dev and test these days?  I'm still
> not able to trigger bounce feedback using the try build Jim posted above on
> recent Win7 builds.

(In reply to comment #12)
> With the try build when it was not at the end of the scrollable area it will
> bump for feedback that it reached the end of the scrollable area and at times
> it appears that it allowed one more bump. It may be that it wasn't quite at the
> end of the scrollable region for the additional bump.

On a view that does not have a scrollbar on an axis, that axis should not experience feedback. This is parity with IE and prevents the annoying effect of modest right-left feedback when panning a page up or down that can't scroll horizontally. 

The lack of feedback on an axis that does have a scrollbar but has simply reached it's bounds should work. I'll take a look.
  
> 
> With IE I noticed that if the area could be scrolled it will always bump for
> feedback at the end of the scrollable area.
> 
> Still on the beta and will upgrade sometime later this week

I'm working with the latest try build I posted and the RC and it's working fine. (Build 7100 w/the latest drivers from n-Trig on an hp touchsmart)

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 376120 [details] [diff] [review]
pan feedback v.1

Confirming Rob's bug.  I'll post a new patch / build here in a bit.

Generally though feedback should be working except in this one case. Reed, what type of system are you working with?

Rob, curious if we ever get that display working? If so any chance we could test the latest try build on that as well with the RC?

Comment 15

[Jim Mathies [:jimm]]

Attachment: pan feedback v.3

This addresses two issues in the original patch, a feedback oscilation problem from using client rather than screen coordinates I found last week, and Rob's bug where feedback did not occur on windows that had reached their bounds. Try server builds later today hopefully.

Comment 16

[Jim Mathies [:jimm]]

try builds - 

https://build.mozilla.org/tryserver-builds/2009-05-12_06:[e-mail]-panfeedbackv3/

Comment 17

[reedt]

Ok, the try build Jim just posted works for me.  I'm seeing boundary feedback both on panning and inertia.

One bug I noticed is occasionally I get boundary feedback in the middle of the content (can pan in either direction still).  I don't have a good repro for this.

Also, the system default is going to be left/right drags with single-finger panning to generate mouse input.  E.g. left/right yields mouse, up/down yields panning.  This allows the user to select some text on a page for example.  You can play w/ this in the RC build of IE.  If you want to mirror that behavior, you can set GC_PAN_WITH_SINGLE_FINGER_VERTICALLY (without setting the horizontal equivalent) with SetGestureConfig().

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> Ok, the try build Jim just posted works for me.  I'm seeing boundary feedback
> both on panning and inertia.
> 
> One bug I noticed is occasionally I get boundary feedback in the middle of the
> content (can pan in either direction still).  I don't have a good repro for
> this.

I'll take a look, thanks for testing.
 
> Also, the system default is going to be left/right drags with single-finger
> panning to generate mouse input.  E.g. left/right yields mouse, up/down yields
> panning.  This allows the user to select some text on a page for example.  You
> can play w/ this in the RC build of IE.  If you want to mirror that behavior,
> you can set GC_PAN_WITH_SINGLE_FINGER_VERTICALLY (without setting the
> horizontal equivalent) with SetGestureConfig().

I'll split this out as a second bug, we should add more fine grained control over these options through prefs. We currently have a general single finger input pref but that should probably be split up by axis with the defaults set to match win7 system default.

Comment 19

[Jim Mathies [:jimm]]

(In reply to comment #18)
> (In reply to comment #17)
> > Ok, the try build Jim just posted works for me.  I'm seeing boundary feedback
> > both on panning and inertia.
> > 
> > One bug I noticed is occasionally I get boundary feedback in the middle of the
> > content (can pan in either direction still).  I don't have a good repro for
> > this.
> 
> I'll take a look, thanks for testing.
> 

Hmm, unable to reproduce this, maybe Rob can try when he gets upgraded.

Comment 20

[Robert Strong (they/them - no direct email)]

I tried with both the Win 7 beta and rc without being able to reproduce it.

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #17)
> Also, the system default is going to be left/right drags with single-finger
> panning to generate mouse input.  E.g. left/right yields mouse, up/down yields
> panning.  This allows the user to select some text on a page for example.  You
> can play w/ this in the RC build of IE.  If you want to mirror that behavior,
> you can set GC_PAN_WITH_SINGLE_FINGER_VERTICALLY (without setting the
> horizontal equivalent) with SetGestureConfig().

I think this might have to wait until we get a better handle on how we'll treat gesture input in the future. Currently we simulate scroll events down in our widget code for panning. The longer term solution is to generate a new set of gesture events and expose pan input / feedback interfaces on the window.  That way the scrollable view & event state manager can control what's going on. This would allow for smarter input control. For example, switching between single finger text selection and scroll depending on the horizontal scroll state of the view.

I think I'll morph that prefs bug into this work.

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #20)
> I tried with both the Win 7 beta and rc without being able to reproduce it.

Thanks Rob. I'm going to keep testing looking for issues, but I'd like to kick off basic reviews on this patch now as we are fast approaching code freeze for the RC.

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 376917 [details] [diff] [review]
pan feedback v.3

smaug -> for changes to event state manager and scrollable view.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 376917 [details] [diff] [review]
pan feedback v.3

> nsEventStateManager::DoScrollText(nsPresContext* aPresContext,
>                                   nsIFrame* aTargetFrame,
>                                   nsInputEvent* aEvent,
>                                   PRInt32 aNumLines,
>+                                  PRInt32 * aOverflow,
>                                   PRBool aScrollHorizontal,
>-                                  ScrollQuantity aScrollQuantity)
>+                                  ScrollQuantity aScrollQuantity,
>+                                  PRBool aNoDefer)
You could change the type of aEvent to be nsMouseEvent and then there is no need to
add new aNoDefer parameter.

You could probably get rid of aScrollHorizontal parameter too. Just make
local PRBool scrollHorizontal variable which takes the value from aEvent. 

>+    if (aScrollQuantity == eScrollByPage) {
>+      scrollView->ScrollByPages(scrollX, scrollY,
>+        (aNoDefer ? NS_VMREFRESH_IMMEDIATE:NS_VMREFRESH_SMOOTHSCROLL));
>+    }
>+    else if (aScrollQuantity == eScrollByPixel) {
>+      scrollView->ScrollByPixels(scrollX, scrollY, overflowX, overflowY,
>+        (aNoDefer ? NS_VMREFRESH_IMMEDIATE:NS_VMREFRESH_DEFERRED));
>+    }
>+    else {
>+      scrollView->ScrollByLinesWithOverflow(scrollX, scrollY, overflowX, overflowY,
>+        (aNoDefer ? NS_VMREFRESH_IMMEDIATE:NS_VMREFRESH_SMOOTHSCROLL));
>+    }
Could you have some helper variable like
PRUint32 updateFlags = aNoDefer ? NS_VMREFRESH_IMMEDIATE : NS_VMREFRESH_SMOOTHSCROLL;
and use its value when calling scrollView.
And note, there is a space before and after ':' ;)

How does this behave with overflow:hidden?
Something like http://mozilla.pettay.fi/moztests/long_overflow_hidden.html

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 376917 [details] [diff] [review]
pan feedback v.3

If overflow:hidden works like in other cases, r=me with comments addressed.

Comment 26

[Jim Mathies [:jimm]]

(In reply to comment #24)
> How does this behave with overflow:hidden?
> Something like http://mozilla.pettay.fi/moztests/long_overflow_hidden.html

It doesn't scroll and provides feedback on the page as it is loaded. I assume that's what we want as it's the same thing I see with wheel scroll.

Comment 27

[Jim Mathies [:jimm]]

Attachment: pan feedback v.4

vlad -> for the windows widget stuff.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 377464 [details] [diff] [review]
pan feedback v.4


>diff --git a/widget/public/nsGUIEvent.h b/widget/public/nsGUIEvent.h

since this is touching non-windows widget code, please get sr from roc, vlad or stuart

>diff --git a/widget/src/windows/nsWinGesture.cpp b/widget/src/windows/nsWinGesture.cpp
>   sLibraryHandle = ::LoadLibraryW(kGestureLibraryName);
>+  HMODULE hTheme = ::LoadLibraryW(kThemeLibraryName);
>   if (sLibraryHandle) {
...
>+    beginPanningFeedback = (BeginPanningFeedbackPtr)GetProcAddress(hTheme, "BeginPanningFeedback");
>+    endPanningFeedback = (EndPanningFeedbackPtr)GetProcAddress(hTheme, "EndPanningFeedback");
>+    updatePanningFeedback = (UpdatePanningFeedbackPtr)GetProcAddress(hTheme, "UpdatePanningFeedback");
>   }

you should check hTheme before using it.

> 
>   if (!getGestureInfo || !closeGestureInfoHandle || !getGestureExtraArgs ||
>-      !setGestureConfig || !getGestureConfig) {
>+      !setGestureConfig || !getGestureConfig || !beginPanningFeedback ||
>+      !endPanningFeedback || !updatePanningFeedback) {
>     getGestureInfo         = nsnull;
>     closeGestureInfoHandle = nsnull;
>     getGestureExtraArgs    = nsnull;
>     setGestureConfig       = nsnull;
>     getGestureConfig       = nsnull;
>-    
>+    beginPanningFeedback   = nsnull;
>+    endPanningFeedback     = nsnull;
>+    updatePanningFeedback  = nsnull;
>     return PR_FALSE;
>   }

if we fail to load uxtheme or the feedback functions but succeed in loading the gesture functions, we should probably still accept gestures.

>+PRBool nsWinGesture::BeginPanningFeedback(HWND hWnd)
>+{
>+  if (!mAvailable)
>+    return PR_FALSE;
>+
>+  return beginPanningFeedback(hWnd);
>+}
>+
>+PRBool nsWinGesture::EndPanningFeedback(HWND hWnd)
>+{
>+  if (!mAvailable)
>+    return PR_FALSE;
>+
>+  return endPanningFeedback(hWnd, TRUE);
>+}
>+
>+PRBool nsWinGesture::UpdatePanningFeedback(HWND hWnd, LONG offsetX, LONG offsetY, BOOL fInInertia)
>+{
>+  if (!mAvailable)
>+    return PR_FALSE;
>+
>+  return updatePanningFeedback(hWnd, offsetX, offsetY, fInInertia);
>+}

Unless there's a good argument against my previous suggestion, you will need to check if we have these functions available before using them.

>     case GID_ROLLOVER:
>     {
>-      // Two finger drum roll. Defaults to right click if it falls through.
>+      // Two finger right click. Defaults to right click if it falls through.

It seems that GID_ROLLOVER has been renamed to GID_PRESSANDTAP, can you make that switch and update the comment accordingly

>@@ -362,63 +403,191 @@ nsWinGesture::ProcessPanMessage(HWND hWn
>   coord = gi.ptsLocation;
>-  coord.ScreenToClient(hWnd);
>+  // We want screen coordinates, client coordinates will change when feedback is taking place. 
>+  //coord.ScreenToClient(hWnd);

According to the comment on line 481 of nsGUIEvent.h refPoint should be in "widget relative" coordinates. My understanding is that you should keep this screen to client conversion and adjust where needed using WidgetToScreenOffset()

If I'm wrong, please delete the line rather than comment it out and adjust the comment above to explain why its okay to have screen coordinates here.

>         mPixelScrollDelta.x = mPanIntermediate.x - coord.x;
>         mPixelScrollDelta.y = mPanIntermediate.y - coord.y;
>+        
>         mPanIntermediate = coord;

nit, we don't need the extra newline

>+    mGesture.UpdatePanFeedbackY(mWnd, event, endFeedback);
>+    
>+    mGesture.PanFeedbackFinalize(mWnd, endFeedback);
>     mGesture.CloseGestureInfoHandle((HGESTUREINFO)lParam);

nit, probably don't need this one either

Comment 29

[Jim Mathies [:jimm]]

Attachment: pan feedback v.5

updated.

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 378118 [details] [diff] [review]
pan feedback v.5

adding roc for the sr.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 378118 [details] [diff] [review]
pan feedback v.5

>+PRBool nsWinGesture::EndPanningFeedback(HWND hWnd)
>+{
>+  if (!beginPanningFeedback)
>+    return PR_FALSE;
>+
>+  return endPanningFeedback(hWnd, TRUE);
>+}
>+
>+PRBool nsWinGesture::UpdatePanningFeedback(HWND hWnd, LONG offsetX, LONG offsetY, BOOL fInInertia)
>+{
>+  if (!beginPanningFeedback)
>+    return PR_FALSE;
>+
>+  return updatePanningFeedback(hWnd, offsetX, offsetY, fInInertia);
> }

nit, it may be better to test endPanningFeedback and updatePanningFeedback respectively instead of beginPanningFeedback.  Although, I may just be being paranoid.

Comment 32

[Jim Mathies [:jimm]]

(In reply to comment #31)

> nit, it may be better to test endPanningFeedback and updatePanningFeedback
> respectively instead of beginPanningFeedback.  Although, I may just be being
> paranoid.


They all get set to nsnull if any one fails, so any one will do.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 378118 [details] [diff] [review]
pan feedback v.5

Olli would be a better reviewer for this than me.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 378118 [details] [diff] [review]
pan feedback v.5

>+inline PRBool SignsEqual(PRInt32 a, PRInt32 b)
>+{
>+  if (a == 0 || b == 0) return PR_TRUE;
Hmm, why this? At least add some comment.

Could you test the case when window is deleted during panning feedback.
Something like where you call setTimeout("window.close()", 0); in DOMMouseScroll
listener. You may need to do that in a window which a script has opened.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Something like this
http://mozilla.pettay.fi/moztests/mousescroll_base.html

Comment 36

[Martijn Wargers (dead)]

Bugs with crashes when window gets removed during scroll event: bug 336587 and bug 466433.

Comment 37

[Jim Mathies [:jimm]]

Attachment: [m-c checkin: 254c5cd4978b] pan feedback v.7

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 378501 [details] [diff] [review]
[m-c checkin: 254c5cd4978b] pan feedback v.7

Don't think it's worth the risk at this point in the cycle for the comensurate value. We'll get it next time (it's on trunk, right?)

Comment 40

[Jim Mathies [:jimm]]

(In reply to comment #39)
> (From update of attachment 378501 [details] [diff] [review])
> Don't think it's worth the risk at this point in the cycle for the comensurate
> value. We'll get it next time (it's on trunk, right?)

yep.

Comment 41

[Robert Strong (they/them - no direct email)]

Comment on attachment 378501 [details] [diff] [review]
[m-c checkin: 254c5cd4978b] pan feedback v.7

This has baked for a while now on trunk with no ill affects though it wasn't working without the fix in bug 494281 which only landed as of 7/17. It is a significant usability improvement so requesting 1.9.1.3

Comment 42

[Jim Mathies [:jimm]]

Comment on attachment 378501 [details] [diff] [review]
[m-c checkin: 254c5cd4978b] pan feedback v.7

after a discussion with felipe on irc, we decided to skip off landing this on 1.9.1 due to code divergence on the 1.9.1 branch.