Bug 26488 – No Support for Single Finger or Two Finger Panning in Windows 7

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brian Weinstein]

<rdar://

[Brian Weinstein]

<rdar://problem/6781050>

WebKit is currently unable to scroll using one or two finger panning on Windows 7. WM_GESTURE support will need to be added, and I am debating whether to reuse some of the WheelEventWin/EventHandler::handleWheelEvent code, or if I want to write new handling code that handles panning. Currently, I am leaning towards reusing the WheelEventWin and EventHandler::handleWheelEvent code, but would be open to discussion.

[Brian Weinstein]

Created attachment 31557 [details]
Patch to enable single finger and two-finger panning with Window bounce

[Steve Falkenburg]

Comment on attachment 31557 [details]
Patch to enable single finger and two-finger panning with Window bounce

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 44858)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Brian Weinstein  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +        
> +        https://bugs.webkit.org/show_bug.cgi?id=26488

Would be nice to include a short description of the bugzilla bug w/ the URL

> Index: WebCore/platform/PlatformWheelEvent.h
> ===================================================================
> --- WebCore/platform/PlatformWheelEvent.h	(revision 44832)
> +++ WebCore/platform/PlatformWheelEvent.h	(working copy)
> @@ -99,6 +99,7 @@ namespace WebCore {
>  
>  #if PLATFORM(WIN)
>          PlatformWheelEvent(HWND, WPARAM, LPARAM, bool isMouseHWheel);
> +        PlatformWheelEvent(HWND, float, float, float, float);

We include parameter names when it isn't obvious from the type.  4 floats strung together isn't understandable for someone to know how to call this.
Also, it seems odd to call this PlatformWheelEvent if we're not actually calling this in response to a wheel event.

>  #endif
>  
>  #if PLATFORM(WX)
> Index: WebCore/platform/win/WheelEventWin.cpp
> ===================================================================
> --- WebCore/platform/win/WheelEventWin.cpp	(revision 44832)
> +++ WebCore/platform/win/WheelEventWin.cpp	(working copy)
> @@ -62,6 +62,29 @@ static int verticalScrollLines()
>      return scrollLines;
>  }
>  
> +PlatformWheelEvent::PlatformWheelEvent(HWND hWnd, float deltaX, float deltaY, float xLoc, float yLoc)
> +    : m_isAccepted(false)
> +    , m_shiftKey(false)
> +    , m_ctrlKey(false) // FIXME: Do we need modifier keys for gestures?

Does the control key do something special for gestures?

> +    , m_altKey(false)
> +    , m_metaKey(false)
> +{
> +    // Scroll down scrolling in x direction to make it less finicky
> +    m_deltaX = deltaX / 3;

The divide by 3 here looks out of place - I think you copied it from the other PlatformWheelEvent ctor - would be nice to combine the common code for these.

> +    m_deltaY = deltaY;
> +
> +    m_wheelTicksX = m_deltaX;
> +    m_wheelTicksY = m_deltaY;
> +
> +    // Global Position is just x, y location of event
> +    POINT point = {xLoc, yLoc};
> +    m_globalPosition = (IntPoint) point;

We like to avoid C style casts. I think you can just remove the cast completely, since there's a constructor for IntPoint that takes a POINT:

   m_globalPosition = point;

> +
> +    // Position needs to be translated to our client
> +    ScreenToClient(hWnd, &point);
> +    m_position = (IntPoint) point;

Same problem here. I'd recommend doing this without a cast.




> Index: WebKit/win/WebView.cpp
> ===================================================================
> --- WebKit/win/WebView.cpp	(revision 44832)
> +++ WebKit/win/WebView.cpp	(working copy)
> @@ -95,6 +95,7 @@
>  #include <WebCore/ResourceHandle.h>
>  #include <WebCore/ResourceHandleClient.h>
>  #include <WebCore/ScriptValue.h>
> +#include <WebCore/Scrollbar.h>
>  #include <WebCore/ScrollbarTheme.h>
>  #include <WebCore/SecurityOrigin.h>
>  #include <WebCore/SelectionController.h>
> @@ -127,6 +128,18 @@
>  #include <ShlObj.h>
>  #include <tchar.h>
>  #include <windowsx.h>
> +#include "SoftLinking.h"
> +#include "WindowsTouch.h"

Given the current include sorting in this file, I'd recommend moving these 2 new lines up with the other includes for files within WebKit.


>  
> +bool WebView::gestureNotify(WPARAM wParam, LPARAM lParam)
> +{
> +    GESTURENOTIFYSTRUCT* gn = (GESTURENOTIFYSTRUCT*) lParam;

You should use a C++ style cast.

> +
> +    Frame* coreFrame = core(m_mainFrame);
> +    if (!coreFrame)
> +        return false;
> +
> +    ScrollView* view = (ScrollView*) coreFrame->document()->view();

This should not require a cast, since view() returns a FrameView* and FrameView is derived from ScrollView.
Also, I think this can be coreFrame->view().

> +    if (!view)
> +        return false;
> +
> +    // If we don't have this function, we shouldn't be receiving this message
> +    if (!SetGestureConfigPtr())
> +        return false;

I think this can just be an assert - I don't think you could end up with the WM_GESTURENOTIFY if the lib wasn't present.

> +
> +    DWORD dwPanWant;
> +    DWORD dwPanBlock; 
> +
> +    if (gn->ptsLocation.x > view->visibleWidth()) {
> +        //We are in the scrollbar, turn off panning
> +        dwPanWant = GC_PAN  | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER;
> +        dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY | GC_PAN_WITH_SINGLE_FINGER_VERTICALLY;	
> +    } else {
> +        //We aren't in the scrollbar, turn on panning
> +        dwPanWant = GC_PAN  | GC_PAN_WITH_SINGLE_FINGER_VERTICALLY | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER;
> +        dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY;	
> +    }
> +
> +    GESTURECONFIG gc = { GID_PAN, dwPanWant , dwPanBlock } ;
> +    UINT uiGcs = 1;

Not necessary to stick this in a local temporarily. I'm not sure the name uiGcs adds a lot of value.

> +    BOOL bResult = SetGestureConfigPtr()(m_viewWindow, 0, uiGcs, &gc, sizeof(GESTURECONFIG));
> +
> +    return bResult;

could just return the result of the call directly. No need to assign it to a temporary local.

> +}
> +
> +bool WebView::gesture(WPARAM wParam, LPARAM lParam) 
> +{
> +    GESTUREINFO gi = {0};
> +
> +    BOOL eventValue = false;
> +
> +    PlatformWheelEvent* wheelEvent;
> +    long currentX;
> +    long currentY;
> +    long deltaX;
> +    long deltaY;

Can define these closer to their first use.

> +
> +    // We want to bail out if we don't have either of these functions
> +    if (!GetGestureInfoPtr() || !CloseGestureInfoHandlePtr())
> +        return false;
> +
> +    gi.cbSize = sizeof(GESTUREINFO);

Could just move the gi definition directly above this line. That would make its initial state more clear.

> +
> +    BOOL bResult = GetGestureInfoPtr()((HGESTUREINFO)lParam, (PGESTUREINFO) &gi);
> +
> +    if (bResult){

Early return would be better here.

Don't need bResult. Could just do if (GetGesture...
You need a space between ){

> +        switch (gi.dwID){

Space between ){

> +            case GID_BEGIN:
> +                m_lastPanX = gi.ptsLocation.x;
> +                m_lastPanY = gi.ptsLocation.y;
> +
> +                CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);

C style cast - shouldn't use this.

> +                break;
> +            case GID_ZOOM:    
> +                // FIXME: Do we want to implement a new kind of zoom here? <rdar://problem/6980304>    
> +                CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);

C style cast - shouldn't use this.

> +                break;
> +            case GID_PAN: {
> +                Frame* coreFrame = core(m_mainFrame);
> +                if (!coreFrame)
> +                    return false;
> +
> +                ScrollView* view = (ScrollView*) coreFrame->document()->view();

I don't think this cast is needed.

> +                if (!view)
> +                    return false;
> +
> +                Scrollbar* vertScrollbar = view->verticalScrollbar();
> +                if (!vertScrollbar)
> +                    return true;	//No panning of any kind when no vertical scrollbar, matches IE8

Doesn't this leak the gesture info?

> +
> +                // Where are the fingers currently?
> +                currentX = gi.ptsLocation.x;
> +                currentY = gi.ptsLocation.y;
> +                
> +                // How far did we pan in each direction?
> +                deltaX = currentX - m_lastPanX;
> +                deltaY = currentY - m_lastPanY;
> +                
> +                // Calculate the overpan for window bounce
> +                m_yOverpan -= m_lastPanY - currentY;
> +                m_xOverpan -= m_lastPanX - currentX;
> +                
> +                // Update our class variables with updated values
> +                m_lastPanX = currentX;
> +                m_lastPanY = currentY;
> +
> +                // Represent the pan gesture as a mouse wheel event
> +                wheelEvent = new PlatformWheelEvent(m_viewWindow, deltaX, deltaY, currentX, currentY);

It looks like you're leaking the wheelEvent. Can you just make this stack allocated?

> +                eventValue = coreFrame->eventHandler()->handleWheelEvent(*wheelEvent);
> +
> +                // FIXME: Soon we'll have horizontal window bounce too, just haven't implemented yet
> +                if (vertScrollbar->currentPos() == 0) {
> +                    // FIXME: Is something else besides 0 here? 0 worked in tests, and window bounce worked
> +                    if (UpdatePanningFeedbackPtr()) {

Is it useful to check for all of these functions separately? Seems like if 1 is available, they'd all be there.

> +                        UpdatePanningFeedbackPtr()(m_viewWindow, 0, m_yOverpan, gi.dwFlags & GF_INERTIA);
> +                    }
> +                } else if (vertScrollbar->currentPos() >= vertScrollbar->maximum()) {
> +                    if (UpdatePanningFeedbackPtr()) {
> +                        UpdatePanningFeedbackPtr()(m_viewWindow, 0, m_yOverpan, gi.dwFlags & GF_INERTIA);
> +                    }
> +                }
> +                if (gi.dwFlags & GF_BEGIN) {
> +                    if (BeginPanningFeedbackPtr()) {
> +                        BeginPanningFeedbackPtr()(m_viewWindow);
> +                        m_yOverpan = 0;
> +                    }
> +                } else if (gi.dwFlags & GF_END) {
> +                    if (EndPanningFeedbackPtr()) {
> +                        EndPanningFeedbackPtr()(m_viewWindow, true);
> +                        m_yOverpan = 0;
> +                    }
> +                }
> +
> +                CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);

C style cast - shouldn't use this.

> +                return eventValue;

Is it correct to close the gesture info if we could return false?

> +
> +                break;
> +            }
> +            default:
> +                // You have encountered an unknown gesture - return false to pass it to DefWindowProc
> +                CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);

C style cast - shouldn't use this.

> +                return false;
> +                break;
> +        }
> +        CloseGestureInfoHandlePtr()((HGESTUREINFO)lParam);
> +    } else {
> +        DWORD dwErr = GetLastError();
> +        ASSERT(dwErr <= 0);			//dwErr > 0 is an error condition

Not sure this is a useful assert- seems odd since in this code path, we actually expect an error.


> Index: WebKit/win/WebView.h
> ===================================================================
> --- WebKit/win/WebView.h	(revision 44832)
> +++ WebKit/win/WebView.h	(working copy)
> @@ -742,6 +742,8 @@ public:
>      bool onUninitMenuPopup(WPARAM, LPARAM);
>      void performContextMenuAction(WPARAM, LPARAM, bool byPosition);
>      bool mouseWheel(WPARAM, LPARAM, bool isMouseHWheel);
> +    bool gesture(WPARAM, LPARAM);
> +    bool gestureNotify(WPARAM, LPARAM);
>      bool execCommand(WPARAM wParam, LPARAM lParam);
>      bool keyDown(WPARAM, LPARAM, bool systemKeyDown = false);
>      bool keyUp(WPARAM, LPARAM, bool systemKeyDown = false);
> @@ -911,6 +913,12 @@ protected:
>      HWND m_topLevelParent;
>  
>      OwnPtr<HashSet<WebCore::String> > m_embeddedViewMIMETypes;
> +
> +	//Variables needed to store gesture information
> +	long m_lastPanX;
> +	long m_lastPanY;
> +	long m_xOverpan;
> +	long m_yOverpan;

You need to initialize these in the WebView ctor.

>  };
>  
>  #endif
> Index: WebKit/win/WindowsTouch.h
> ===================================================================
> --- WebKit/win/WindowsTouch.h	(revision 0)
> +++ WebKit/win/WindowsTouch.h	(revision 0)

> +#ifndef WindowsTouch_H
> +#define WindowsTouch_H

I think our coding guidelines call for _h instead of _H.

> +#endif
> \ No newline at end of file

You should add a newline.

> 
> Property changes on: WebKit/win/WindowsTouch.h
> ___________________________________________________________________
> Added: svn:executable
>    + *

For new files you should:

svn pd svn:executable
svn ps svn:eol-style native

[Brian Weinstein]

Dave Hyatt, Adam Roben and I discussed that a good way to handle this bug would be to "override" the PlatformWheelEvent, similar to how continuous scrolling works on the Mac, so that is why I made the decision I did to use a MouseWheelEvent, because it had so much of the scrolling logic built in.

[Brian Weinstein]

The modifier keys don't do anytihng special for gestures, but I wasn't sure if I needed to declare those all as false for safety, just to make sure some other classes aren't trying to call an undefined variable. 

[Brian Weinstein]

Created attachment 31570 [details]
Take 2 of Panning

[Brian Weinstein]

Committed in r44883.

[Adam Roben (:aroben)]

Comment on attachment 31570 [details]
Take 2 of Panning

> +        PlatformWheelEvent(HWND, float deltaX, float deltaY, float xLoc, float yLoc);

It would be even better for the signature to look like this:

PlatformWheelEvent(HWND, const FloatSize& delta, const FloatPoint& location);