<rdar://
<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.
Created attachment 31557 [details] Patch to enable single finger and two-finger panning with Window bounce
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 @@ > +2009-06-19 Brian Weinstein <bweinstein@apple.com> > + > + 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
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.
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.
Created attachment 31570 [details] Take 2 of Panning
Committed in r44883.
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);