Bug 14227 - Scroll does not work with IBM Thinkpad
Summary: Scroll does not work with IBM Thinkpad
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 14098 22534 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-06-18 20:45 PDT by Cameo Wood
Modified: 2010-02-03 12:58 PST (History)
13 users (show)

See Also:


Attachments
Mouse messages while scrolling FF page with a T42 Thinkpad scroll device (472.94 KB, application/xml)
2007-06-19 12:32 PDT, Cameo Wood
no flags Details
Mouse messages while scrolling Safari page with a T42 Thinkpad scroll device (907.05 KB, application/xml)
2007-06-19 12:33 PDT, Cameo Wood
no flags Details
Patch (5.13 KB, patch)
2010-02-02 15:08 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (6.86 KB, patch)
2010-02-02 17:15 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (7.17 KB, patch)
2010-02-03 10:53 PST, Brian Weinstein
sfalken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameo Wood 2007-06-18 20:45:06 PDT
I have a laptop with a "IBM ThinkPad UltraNav Pointing Device.
The driver I am using is "Synaptics" from 7/16/2004, driver version 75.17.12 on Windows XP SP2.  

The scroll does work in Word, FF, and IE.  All other mouse actions work fine.
Comment 1 Dave Hyatt 2007-06-18 22:43:04 PDT
Would be happy to fix this... just need to know what messages are sent.
Comment 2 Cameo Wood 2007-06-19 09:15:36 PDT
(In reply to comment #1)
> Would be happy to fix this... just need to know what messages are sent.
> 

Tell me how I can help, and I will! 
Comment 3 Cameo Wood 2007-06-19 12:32:39 PDT
Created attachment 15125 [details]
Mouse messages while scrolling FF page with a T42 Thinkpad scroll device
Comment 4 Cameo Wood 2007-06-19 12:33:11 PDT
Created attachment 15126 [details]
Mouse messages while scrolling Safari page with a T42 Thinkpad scroll device
Comment 5 Mark Rowe (bdash) 2007-06-19 12:35:55 PDT
Thanks Cameo!
Comment 6 Cameo Wood 2007-06-19 12:43:06 PDT
In these files, I performed the following actions in each window:

1. moved my mouse into the browser window
2 clicked the blue "scroll mouse" button on the thinkpad
3. moved my "IBM ThinkPad UltraNav Pointing Device" [TM] up, then down twice.
4. moved my mouse away from the window.

I hope this helps!  I was looking for some WM_SCROLL but I wasn't seeing that at all. 

I also found a blog post by someone solving this issue for another piece of software:

http://blogs.msdn.com/markrideout/archive/2006/01/12/512359.aspx

hope that helps!  Thanks, fellows!
Comment 7 Mark Rowe (bdash) 2007-06-19 12:49:54 PDT
<rdar://problem/5279370>
Comment 8 Dave Hyatt 2007-06-19 13:47:59 PDT
Firefox has no native scrollbar.  We should probably figure out how they managed to work with this.

Comment 9 Cameo Wood 2007-06-19 14:42:13 PDT
Let me know if I can help further.
Comment 10 Anton Mostovoy 2007-07-10 10:27:02 PDT
FYI, I have the same problem, so it's not an isolated occurrence.
Comment 11 mitz 2007-08-16 12:51:22 PDT
*** Bug 14098 has been marked as a duplicate of this bug. ***
Comment 12 David Kilzer (:ddkilzer) 2007-08-16 23:12:10 PDT
See also bug 14990.

Comment 13 Soren 2007-11-22 13:16:28 PST
(In reply to comment #1)
> Would be happy to fix this... just need to know what messages are sent.
> 

In the new 3.0.4 build for Windows, the funny thing is that I can use the trackpoint for scrolling in the Bookmarks (CTRL-ALT-B) list, but still not in on webpages. Maybe it helps you a little
Comment 14 Cameo Wood 2008-01-09 09:57:30 PST
I upgraded to the latest synaptics driver from 8/10/2007, version 7.5.17.25, and the scroll still doesn't work.
Comment 15 Anton Mostovoy 2008-01-09 10:42:24 PST
I found this info here (http://levelsofdetail.kendeeter.com/2007/06/safari_browser_on_windows.html) and it worked for me.
In my case tp4table.dat is in C:\Program Files\Synaptics\SynTP

begin quote
Edit C:\windows\system32\tp4table.dat. Towards the bottom of this file, right before the [AutoScrollTable] section, I added the following lines:

;iTunes/Safari Scroll Fix
*,*,itunes.exe,*,*,*,WheelStd,0,9
*,*,Safari.exe,*,*,*,WheelStd,0,9
end quote
Comment 16 Anton Mostovoy 2008-01-09 10:43:35 PST
(In reply to comment #15)
Correct link is http://levelsofdetail.kendeeter.com/2007/06/make_the_thinkpads_middle_butt.html
Comment 17 Cameo Wood 2008-01-09 11:08:01 PST
followed anton's instructions, and still no dice.  Not sure what's not working.  I edited the file and then rebooted, and still it doesn't work.
Comment 18 Cameo Wood 2008-01-09 11:28:46 PST
At sfalken's recommendation, I tried editing SynTPEnh.ini with:

[Safari]
FC = "WebViewWindowClass"
SF = 0x10000000
SF |= 0x00004000

Still doesn't work.
Comment 19 Cameo Wood 2008-01-09 11:36:49 PST
Also, to clarify.  This is to scroll using the middle scroll button and the pointstick, not the touchpad.
Comment 20 Maxime Britto 2008-07-10 10:44:55 PDT
It would be great to test this again on the IBM ThinkPad UltraNav Pointing Device with the changes from r35083
It seems to be the same bug as #16424 which is now fixed (except if the blue "scroll mouse" button is not assimilated as the middle mouse button) 
Comment 21 Brent Fulgham 2008-11-27 15:25:21 PST
*** Bug 22534 has been marked as a duplicate of this bug. ***
Comment 22 Brent Fulgham 2008-11-27 15:25:48 PST
This appears to be a problem somewhat specific to certain kinds of laptops. 
See "http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/webinputevent_win.cc?view=markup"
for specific information on the Chromium logic to handle this.

Basically, "ThinkPad touchpads (or trackpoints) send WM_HSCROLL messages",
which probably need to be handled PlatformMouseEventWin.cpp.
Comment 23 Kyle Huey (Mozilla Developer) 2009-10-21 06:22:37 PDT
I just went through fixing this over at https://bugzilla.mozilla.org/show_bug.cgi?id=507222 .  The trackpoint driver used a nasty hack to work on Firefox and other Mozilla products that broke as a result of some internal changes.  The patches are there for viewing and pretty straightforward, and I'm more than willing to provide advice or answer questions on what I did to get this working.
Comment 24 Brian Weinstein 2010-02-02 15:08:36 PST
Created attachment 47976 [details]
Patch
Comment 25 WebKit Review Bot 2010-02-02 15:12:45 PST
Attachment 47976 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/win/WebView.cpp:1599:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit/win/WebView.cpp:1630:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Brian Weinstein 2010-02-02 15:13:40 PST
(In reply to comment #25)
> Attachment 47976 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/win/WebView.cpp:1599:  A case label should not be indented, but line up
> with its switch statement.  [whitespace/indent] [4]
> WebKit/win/WebView.cpp:1630:  A case label should not be indented, but line up
> with its switch statement.  [whitespace/indent] [4]
> Total errors found: 2
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Every other switch in this file had the case labels indented, in this case, I feel going against the style bot is right, but could be convinced either way.
Comment 27 Brian Weinstein 2010-02-02 17:15:04 PST
Created attachment 47983 [details]
Patch
Comment 28 Steve Falkenburg 2010-02-03 00:13:54 PST
Comment on attachment 47983 [details]
Patch


> +void WebView::initializeTrackpointHackIfNecessary(HWND parentWindow)
> +{
> +    const TCHAR trackpointKeys[][50] = {L"Software\\Lenovo\\TrackPoint",
> +        L"Software\\Lenovo\\UltraNav",
> +        L"Software\\Alps\\Apoint\\TrackPoint",
> +        L"Software\\Synaptics\\SynTPEnh\\UltraNavUSB",
> +        L"Software\\Synaptics\\SynTPEnh\\UltraNavPS2"};

Mixing TCHAR and L" isn't really correct (but won't cause any issues in practice for us...) since TCHAR's size depends on whether UNICODE is defined, while L" is always UTF-16.
You could switch from L" to TEXT( or switch from TCHAR to WCHAR. Since you're passing these into Win32 APIs not suffixed with A or W, TCHAR seems more appropriate.

> +    for (int i = 0; i < 5; i++) {

We use ++i in most places.

> +        int readkeyResult = ::RegOpenKeyEx(HKEY_CURRENT_USER, (LPCWSTR)&trackpointKeys[i], 0, KEY_READ, &trackpointKey);

We don't use C style casts in our code. Please change this to a C++ style cast. And cast to LPCTSTR instead.

Strange casing of your variables here. How about:

trackPointKey
readKeyResult

> +        if (readkeyResult == ERROR_SUCCESS) {
> +            // If we detected a registry key belonging to a TrackPoint driver, then create fake trackpoint
> +            // scrollbars, so the WebView will receive WM_VSCROLL and WM_HSCROLL messages. We create one
> +            // vertical scrollbar and one horizontal to allow for receiving both types of messages.
> +            ::CreateWindow(L"SCROLLBAR", L"FAKETRACKPOINTHSCROLLBAR", WS_CHILD | WS_VISIBLE | SBS_HORZ, 0, 0, 0, 0, parentWindow, 0, gInstance, 0);
> +            ::CreateWindow(L"SCROLLBAR", L"FAKETRACKPOINTVSCROLLBAR", WS_CHILD | WS_VISIBLE | SBS_VERT, 0, 0, 0, 0, parentWindow, 0, gInstance, 0);

More L"" use in non-suffixed Windows calls. Nitpicky, since it won't really create issues unless somebody tried to build this without UNICODE.

>  HRESULT STDMETHODCALLTYPE WebView::initWithFrame( 

I don't believe the STDMETHODCALLTYPE is really required here.

>      /* [in] */ RECT frame,
> @@ -2404,6 +2495,8 @@ HRESULT STDMETHODCALLTYPE WebView::initW
>          frame.left, frame.top, frame.right - frame.left, frame.bottom - frame.top, m_hostWindow ? m_hostWindow : HWND_MESSAGE, 0, gInstance, 0);
>      ASSERT(::IsWindow(m_viewWindow));
>  
> +    initializeTrackpointHackIfNecessary(m_viewWindow);
> +

We should cache this code so we don't access the registry unnecessarily every time we create a WebView! It is unlikely this driver will get installed while we're running.
Comment 29 Brian Weinstein 2010-02-03 10:53:08 PST
Created attachment 48054 [details]
Patch
Comment 30 Steve Falkenburg 2010-02-03 12:54:22 PST
Comment on attachment 48054 [details]
Patch

> +    Frame* frame = m_page->focusController()->focusedOrMainFrame();

You can move this line down to near the end of the method (in both horizontal and vertical scrolling methods).

> +bool WebView::shouldInitializeTrackPointHack()
> +{
> +    static bool shouldCreateScrollbars = false;
> +    static bool hasRunTrackPointCheck = false;

No need to init these - they will be inited to false for you.

> +    shouldCreateScrollbars = false;

This is already false.
Comment 31 Brian Weinstein 2010-02-03 12:58:48 PST
Landed in r54293.