Closed Bug 507222 Opened 15 years ago Closed 14 years ago

Thinkpad Trackpoint middle button mouse scroll does not scroll window up and down [Minefield]

Categories

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

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
blocking2.0 --- alpha1+
status1.9.2 --- beta1-fixed

People

(Reporter: bemguard-bugzilla, Assigned: khuey)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: regression)

Attachments

(5 files, 9 obsolete files)

Winspector View of Minefield Window Structure
26.53 KB, image/png
Details
Winspector Display of Working Window Arrangement
11.60 KB, image/png
Details
Winspector Display of Strategically Injected Scrollbars
57.10 KB, image/png
Details
Patch w/nits nits picked
16.49 KB, patch
khuey
: review+
Details | Diff | Splinter Review
Patch to remove window class testing
1.56 KB, patch
roc
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090729 Minefield/3.6a1pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090729 Minefield/3.6a1pre (.NET CLR 3.5.30729)

If I try to scroll a window using my Trackpoint's middle button the window does not pan.    The cursor does activate the pointer icon associated with up-and-down panning.

Last build that worked properly:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090721044139

First build that stopped scrolling:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090722 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090722042136


Reproducible: Always

Steps to Reproduce:
1.Start Minefield
2.Open www.slashdot.org   (or something that will go off the viewport)
3.Press Trackpad middle button to scroll the view
4.Push up and down on Trackpad
Actual Results:  
Window does not scroll.

Expected Results:  
Window should scroll.

Lenovo-supplied Synaptics driver 11.1.21.0   (the current Trackpoint driver)
Version: unspecified → Trunk
Sorry, that should obviously be...
4. Push up and down on Trackpoint
That is a mouse driver issue if it works with a "normal" mouse.
In this case it could depend on the Window name that is different (minefield vs Firefox).
Both the previously working and now not working versions are called Minefield, so it's definitely something in the code that changed between 20090721 and 20090722.
I have a thinkpad, will test later today.
Having a thinkpad as well, I'm having problems scrolling a webpage via trackpount and the normal scrollbar since build 20090730. Doesn't work anymore
On LXR I see the following patches were made to mozilla-central during that time period that would make the most sense as being the source:
Bug 503892 -  Remove window proc hook from nsToolkit
Bug 352093 -  (widget-removal) Remove child widgets from content area

cc'ing Rob, since that seems to be the most likely place to start to my non-programmer eyes.
The normal scrollbar problem is Bug 507380 , which apparently was backed out.
This Trackpoint problem started days before that one.
This is certainly confirmed.  Moving to what's likely to be the right component and nominating to get on radar.
Status: UNCONFIRMED → NEW
Component: General → Widget: Win32
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → win32
Flags: blocking1.9.2?
Keywords: regression
Tested 20090730041801 with another IBM 3-button mouse with an additional up-and-down scrolling Trackpoint-ish "wheel stick" on the same machine.   It uses the standard Microsoft HID and is working properly, so it is specific to just the Trackpoint drivers and this code.

Trackpoing scrolling is working properly on same machine in other applications  (IE, Chrome, Notepad, etc.)

Mike Kaply might know more about the details how the Trackpoint driver works,
since he said he talked to the developers about getting rid of some Mozilla
hacks in  Bug 253115 .
Chris, can you reproduce this?

I fear that we're the victim of some driver-specific hack that no longer works since we changed our usage of native widgets.
Blocks: widget-removal
(In reply to comment #10)
> Chris, can you reproduce this?

Yes. If I press the middle trackpad button, I get the "scroll" cursor. I can scroll using nipple movement, but nut using trackpad movement.
Perhaps this comment from widget/src/os2/nsWindow.cpp is relevant:

// This is ugly. The Thinkpad TrackPoint driver checks to see whether or
// not a window actually has a scroll bar as a child before sending it
// scroll messages. Needless to say, no Mozilla window has real scroll
// bars. So if you have the "os2.trackpoint" preference set, we put an
// invisible scroll bar on every child window so we can scroll. Woohoo!
Workaround:
Add
*,*,*,*,*,MozillaWindowClass,WheelStd,0,9
to the top of C:\Program Files\Lenovo\TrackPoint\tp4table.dat
and then either of
- Restart Windows
- Log off and log on
- Kill tp4serv.exe and run C:\Program Files\Lenovo\TrackPoint\tp4serv.exe
What does that mean? How can we work around this issue in our code?
(In reply to comment #14)
> What does that mean? How can we work around this issue in our code?

Well I would say get Lenovo/IBM release a driver that doesn't suck, but googling tp4table.dat seems to indicate that this is a problem for dozens of applications and they haven't bothered to fix it.

Comment 12 seems to indicate that if we create a fake invisible scrollbar it will work.  I'm working on an actual regression changeset and should have that for you in a few hours.
(In reply to comment #14)
> What does that mean? How can we work around this issue in our code?

I meant workaround for users of Trackpoint, not for our code.
Well, that makes sense. It's a change we need to make, though, so we just have to find a way to work around the driver.
Thanks for finding the regressing changeset, by the way.
Yeah, I know the changeset info is not terribly useful.

What causes scrolling to break is that the driver no longer sends WM_MOUSEWHEEL
messages after that changeset.  The post ff084019260e builds do not get any
scroll messages from the driver.  I tried the invisible scrollbar trick from
the OS/2 build, but that just gets us WM_VSCROLL and WM_HSCROLL messages which
we don't implement.  The workaround posted by Atsushi gets us the WM_MOUSEWHEEL
messages again.

So either we need to find a hack that gets us WM_MOUSEWHEEL messages again or
we need to use the OS/2 invisible scrollbar trick and implement WM_V/HSCROLL.
Also interesting, my tp4table.dat has these hacks for everything from Netscape 6 and Opera to Office 2007.  I assume the trackpoint driver ships with these hacks because I've never used most of the applications in it.
Couldn't we also just change the name of the window class so that this hack in the driver would no longer apply? No one should really be relying on that anyways.
No, changing the window class name doesn't do anything.  The driver somehow guesses what messages to send.  What I would imagine happened was back when WM_MOUSEWHEEL was brand new they programmed the driver to inject WM_V/HSCROLL messages into windows with scrollbars so that the middle button scroll would work even in applications not designed for it.  Of course, now that's completely unnecessary and causes all sorts of problems for windows that don't use native widgets like us.

The workaround posted earlier just tells it to send WM_MOUSEWHEEL messages to all windows of the class "MozillaWindowClass".  

For reference, IE uses native widgets, Chrome uses the invisible scrollbar trick combined with processing the WM_H/VSCROLL messages, and Safari doesn't work either.
In case I wasn't clear right there, I tried compiling using "MozillaWindowClass2" as the window class name and it doesn't change anything.
Thanks for the analysis Kyle!

I wonder if we can just add ourselves to tp4table.dat automatically, during install perhaps?
OK, some more analysis from decompiling the driver.  There is a small hack in it that is "MozillaWindowClass" specific but it only deals with drawing the little Trackpoint scrolling icon, and not with the actual messages.  I'm trying to see if I can figure out how it decides what messages to send to applications.  I've found where it fires WM_SCROLLWHEEL messages and am trying to track it back into the decision code.
(In reply to comment #25)
> I wonder if we can just add ourselves to tp4table.dat automatically, during
> install perhaps?

I'm afraid that re-install or upgrading the driver will reverts tp4table.dat.
So adding will be needed not only during install.

(In reply to comment #26)
> There is a small hack in
> it that is "MozillaWindowClass" specific but it only deals with drawing the
> little Trackpoint scrolling icon, and not with the actual messages.

When I change "MozillaWinddowClass" in tp4serv.exe to another string, Fx3.5 does not scroll with trackpoint.
This shows actual messages likely depend on "MozillaWinddowClass".
(In reply to comment #27)
> When I change "MozillaWinddowClass"

Just typo, "MozillaWindowClass" is correct.
(In reply to comment #27)
> (In reply to comment #25)
> > I wonder if we can just add ourselves to tp4table.dat automatically, during
> > install perhaps?
> 
> I'm afraid that re-install or upgrading the driver will reverts tp4table.dat.
> So adding will be needed not only during install.

We could probably do it on browser startup. Checking for the existence of C:\Program Files\Lenovo\TrackPoint\tp4table.dat should be really cheap.
(In reply to comment #29)
> We could probably do it on browser startup. Checking for the existence of
> C:\Program Files\Lenovo\TrackPoint\tp4table.dat should be really cheap.
You can't change files under Program Files if UAC is enabled or you are logging on as Restricted User. Anyway it's a horrible idea to change other app's settings silently.
I agree with Masatoshi.  It would be so brittle and prone to failure that it's not worth it.  We're far better off IMO convincing IBM to write the hack into tp4table.dat so we can just tell users to upgrade drivers or finding a hack in our code.

s/WM_SCROLLWHEEL/WM_MOUSEWHEEL in my last comment btw.
(In reply to comment #27)
> (In reply to comment #26)
> > There is a small hack in
> > it that is "MozillaWindowClass" specific but it only deals with drawing the
> > little Trackpoint scrolling icon, and not with the actual messages.
> 
> When I change "MozillaWinddowClass" in tp4serv.exe to another string, Fx3.5
> does not scroll with trackpoint.
> This shows actual messages likely depend on "MozillaWinddowClass".

I can confirm this.  Editing the binary tp4serv.exe to match "MuzillaWindowClass" breaks Trackpoint scrolling on 3.5
OK so the problem is that the binary is looking for "MozillaWindowClass" windows and applying the correct messages to them.  On Minefield the window it looks at is of type "MozillaContentWindowClass" and it fails.
This is the Winspector view of the Minefield window structure with the window tp4serv.exe is GetClassName()ing indicated.
It actually tries to match the window and its parent both to "MozillaWindowClass" so simply flipping the defines for MozillaWindowClass and MozillaContentWindowClass doesn't do anything.
So in your screenshot, is it checking that 00070D2C and 00050CC2 are both MozillaWindowClass? Or is it checking 0050CC2 and 0040D80?

(Thanks for all this info!)
I think accessibility tools depend on these class names.
It is checking 00070D2C and 00050CC2 to see if they are both "MozillaWindowClass".
This is what Winspector shows for the same page in 3.5  The trackpoint driver checks the class names of both 00160610 and 00290B54 before deciding that this is really Mozilla and firing the messages correctly.
Thanks, that's what I expected.

One workaround would be to insert a MozillaWindowClass window into the hierarchy under each MozillaUIWindowClass/MozillaContentWindowClass. But that's horrible and difficult.

Maybe we should do the WM_HSCROLL/WM_VSCROLL thing.
Yeah I'm inclined to think that implementing WM_HSCROLL/WM_VSCROLL is the way to go, especially since I think we would need to insert two MozillaWindowClass windows underneath a MozillaContentWindowClass for the driver to be happy.

I'm going to have a go at a patch for this.
Depends on: 315727
Attached patch Patch V1 (obsolete) — — Splinter Review 
Pure brute force.  I've implemented Michael Kaply's OS/2 hack from several years ago on Windows and added some code to handle WM_VSCROLL.  There's a lot of room for improvement here.  First I've injected scrollbars under every window when I probably only need to do it under MozillaContentWindowClass windows.  Second there is no handling of HSCROLLs yet (though I'm not sure that the Trackpoint Driver sends HSCROLL messages.  Something to test).  Third, I think we're going to run into a lot of the same problems that Masayuki has hit on Bug 315727 for handling plugins since we're basically doing the same thing here.

Also, is this something that should be prefed?  (The window creation, not the scroll handling).  I think we could set a hidden preference that defaults to off and then at startup or install see if tp4srv is running and if so set the pref on.  Not sure if it's worth the added complexity though.
Assignee: nobody → me
Status: NEW → ASSIGNED
 // Deal with scrollbar messages (actually implemented only in nsScrollbar)

This comment is no longer correct.

> First I've injected scrollbars under every window when I probably only need to
> do it under MozillaContentWindowClass windows.

Actually, every window that's not MozillaWindowClass *or* has a parent that's not MozillaWindowClass, right? That's almost all windows these days, so probably not worth optimizing here.

Bug 315727 comment #12 suggests we need horizontal scrolling for some devices.

We can probably get away without preffing this, especially if it's needed for a range of devices or tools as bug 315727 seems to suggest.

Would it be slightly simpler and perhaps more efficient to use WS_HSCROLL|WS_VSCROLL when creating the window?
(In reply to comment #44)
>  // Deal with scrollbar messages (actually implemented only in nsScrollbar)
> 
> This comment is no longer correct.

Indeed.

> 
> > First I've injected scrollbars under every window when I probably only need to
> > do it under MozillaContentWindowClass windows.
> 
> Actually, every window that's not MozillaWindowClass *or* has a parent that's
> not MozillaWindowClass, right? That's almost all windows these days, so
> probably not worth optimizing here.

I don't think so.  The two in a row MozillaWindowClass stuff only deals with the driver's internal attempt to match us as Mozilla.  If we go the invisible scrollbar route, we're just trying to be seen as an application with a scrollbar so I think we can get away with just attaching fake scrollbars to MozillaContentWindowClass windows.

> 
> Bug 315727 comment #12 suggests we need horizontal scrolling for some devices.
> 

Yeah.  Means we really need an OnHScroll and an OnVScroll or an extra parameter, but that's not a big deal.

> We can probably get away without preffing this, especially if it's needed for a
> range of devices or tools as bug 315727 seems to suggest.

That's true.

> 
> Would it be slightly simpler and perhaps more efficient to use
> WS_HSCROLL|WS_VSCROLL when creating the window?

Nope.  Doesn't work for the driver and hideously deforms Firefox with a bunch of broken scrollbars.
(In reply to comment #45)
> I think we can get away with just attaching fake scrollbars to
> MozillaContentWindowClass windows.
Actually it may be *a child of* MozillaContentWindowClass which will want a scrool event.
(In reply to comment #46)
> (In reply to comment #45)
> > I think we can get away with just attaching fake scrollbars to
> > MozillaContentWindowClass windows.
> Actually it may be *a child of* MozillaContentWindowClass which will want a
> scrool event.

OK, I guess for creating the invisible scrollbar we should do whatever makes the Thinkpad driver work.
(In reply to comment #45)
> I think we can get away with just attaching fake scrollbars to
> MozillaContentWindowClass windows.

Not only Content, also UI might require fake scrollbars.
For example, go to Tools -> Options -> Applications.
It can be scrolled by Trackpoint on Fx3.5 because <deck> and also <scrollbox> have native "MozillaWindowClass" window, but can't be scrolled on Minefield because they are not "MozillaWindowClass" now.
We're still reporting this issue to IBM, even if we work around it, right?
I think the correct upstream vendor would be Synaptics and/or Lenovo, not IBM, but yeah, we should definitely send it upstream.

And Masatoshi and Atsushi are both correct in their comments.  Second go at it later today.
Attached patch Patch V2 (obsolete) — — Splinter Review 
Version 2.  Adds support for HSCROLL messages (though I'm still not sure whether or not the Trackpoint driver sends them).  Also makes scrolling work correctly on plugins, scrollable elements on a page (think IFrames) and in dialog boxes and such.  The plugin code was more or less lifted from OnMouseWheel().  Still no optimization on creation of fake scrollbars.
Attachment #392208 - Attachment is obsolete: true
> +  GetCursorPos(&point);
::GetMessagePos will be better which is used throughout nsWindow.
> +  nsWindow* destWindow = GetNSWindowPtr(destWnd);
(the rest is omitted)
Could you share the code with OnMouseWheel instead of copy & pasting?
(In reply to comment #52)
> > +  GetCursorPos(&point);
> ::GetMessagePos will be better which is used throughout nsWindow.

Yes, that would be better.

> > +  nsWindow* destWindow = GetNSWindowPtr(destWnd);
> (the rest is omitted)
> Could you share the code with OnMouseWheel instead of copy & pasting?

Yeah it should be broken out into a HandleScrollingPlugins() function.

Thanks for the feedback!
(In reply to comment #37)
> I think accessibility tools depend on these class names.

James, does NVDA rely on window names?
I forgot to cc Jamie.
(In reply to comment #54)
> James, does NVDA rely on window names?
NVDA does rely on window class names for some Mozilla support code, although not always. In what places would the window class name be changed?
Attached patch Patch V3 (obsolete) — — Splinter Review 
Patch V3.  This is the first version I think is reviewable.  I have split the duplicate code into the function "HandleScrollingPlugins" and restricted creation of fake scrollbars to windows underneath windows of class "MozillaContentWindowClass" and "MozillaDialogClass".  

There may be a better way to handle strings with the string classes than what I have in the window creation part, but I couldn't figure out how to do it.  Comments and suggestions appreciated!
Attachment #392324 - Attachment is obsolete: true
Attachment #393432 - Flags: review?(roc)
(In reply to comment #58)
> There may be a better way to handle strings with the string classes than what I
> have in the window creation part, but I couldn't figure out how to do it. 
> Comments and suggestions appreciated!
What about using parent->GetWindowType() and parent->GetContentType()?
Note that you will have to add GetContentType() method because it is not present yet.
Attached patch Patch V3.1 (obsolete) — — Splinter Review 
Fixed the string handling.  I was missing the SetLength call the other day.  I think this method is preferable to the parent->GetContentType() method because in every case where we want to create a fake scrollbar we are getting an nsNativeWidget, not an nsIWidget.  To get the parent window we'd have to use GetNSWindowPtr before calling GetContentType(), and it seems easier and probably faster just to use the native API.
Attachment #393432 - Attachment is obsolete: true
Attachment #393705 - Flags: review?(roc)
Attachment #393432 - Flags: review?(roc)
+  if(!HandleScrollingPlugins(msg, wParam, lParam, result))

if (

+  switch(LOWORD(wParam))

switch (

       // check for the incoming nsWindow handle to be null in which case
       // we assume the message is coming from a horizontal scrollbar inside
       // a listbox and we don't bother processing it (well, we don't have to)

Is this comment still accurate? To be honest, I don't really understand it

+  nsAutoString parentClass;
+  PRUnichar* pBuffer;
+  parentClass.GetMutableData(&pBuffer, kMaxClassNameLength);
+  parentClass.SetLength(::GetClassNameW(parent, (LPWSTR)pBuffer, kMaxClassNameLength));
+  if(parentClass.Equals(kClassNameContent) || parentClass.Equals(kClassNameDialog))
+ 	::CreateWindowW(L"SCROLLBAR", L"FAKETRACKPOINTSCROLLBAR", WS_CHILD | WS_VISIBLE, 0,0,0,0, mWnd, NULL, nsToolkit::mDllInstance, NULL);
+  

I wouldn't even use nsAutoString here. You can just use
  PRUnichar buffer[kMaxClassNameLength];
  ::GetClassNameW(parent, buffer, kMaxClassNameLength);
  nsDependentString bufStr(buffer);
Excuse me, does the patch fire the wheel events by WM_*SCROLL? If so, it may be bad. They are just scrolling messages, so, they are not wheel messages. The nsMouseScrollEvent will generate the DOM wheel events. So, your patch may create unexpected behavior for their message using tools.

# I'm trying to fix the conflicted requirements (Tools want to use them vs. Some mouses want to use them) in bug 315727. Please don't change the WM_*SCROLL message behavior in this bug.
(In reply to comment #61)
> +  if(!HandleScrollingPlugins(msg, wParam, lParam, result))
> 
> if (
> 
> +  switch(LOWORD(wParam))
> 
> switch (
> 
>        // check for the incoming nsWindow handle to be null in which case
>        // we assume the message is coming from a horizontal scrollbar inside
>        // a listbox and we don't bother processing it (well, we don't have to)
> 
> Is this comment still accurate? To be honest, I don't really understand it
> 

No clue to be honest, I just copied and pasted what was already there.

> +  nsAutoString parentClass;
> +  PRUnichar* pBuffer;
> +  parentClass.GetMutableData(&pBuffer, kMaxClassNameLength);
> +  parentClass.SetLength(::GetClassNameW(parent, (LPWSTR)pBuffer,
> kMaxClassNameLength));
> +  if(parentClass.Equals(kClassNameContent) ||
> parentClass.Equals(kClassNameDialog))
> +     ::CreateWindowW(L"SCROLLBAR", L"FAKETRACKPOINTSCROLLBAR", WS_CHILD |
> WS_VISIBLE, 0,0,0,0, mWnd, NULL, nsToolkit::mDllInstance, NULL);
> +  
> 
> I wouldn't even use nsAutoString here. You can just use
>   PRUnichar buffer[kMaxClassNameLength];
>   ::GetClassNameW(parent, buffer, kMaxClassNameLength);
>   nsDependentString bufStr(buffer);

OK

(In reply to comment #62)
> Excuse me, does the patch fire the wheel events by WM_*SCROLL? If so, it may be
> bad. They are just scrolling messages, so, they are not wheel messages. The
> nsMouseScrollEvent will generate the DOM wheel events. So, your patch may
> create unexpected behavior for their message using tools.
> 
> # I'm trying to fix the conflicted requirements (Tools want to use them vs.
> Some mouses want to use them) in bug 315727. Please don't change the WM_*SCROLL
> message behavior in this bug.

That's going to be a bigger problem then, because that is exactly what this patch does.
(In reply to comment #62)
> Excuse me, does the patch fire the wheel events by WM_*SCROLL? If so, it may be
> bad. They are just scrolling messages, so, they are not wheel messages. The
> nsMouseScrollEvent will generate the DOM wheel events. So, your patch may
> create unexpected behavior for their message using tools.

You mean because DOM mouse wheel events could be dispatched instead, and do something like zooming?

Maybe we should have a new flag, e.g. nsMouseScrollEvent::kScrollOnly to prevent these events from being interpreted as other than scrolling by the event state manager?

> # I'm trying to fix the conflicted requirements (Tools want to use them vs.
> Some mouses want to use them) in bug 315727. Please don't change the WM_*SCROLL
> message behavior in this bug.

I think we basically have to fix bug 315727 here.
(In reply to comment #64)
> (In reply to comment #62)
> > Excuse me, does the patch fire the wheel events by WM_*SCROLL? If so, it may be
> > bad. They are just scrolling messages, so, they are not wheel messages. The
> > nsMouseScrollEvent will generate the DOM wheel events. So, your patch may
> > create unexpected behavior for their message using tools.
> 
> You mean because DOM mouse wheel events could be dispatched instead, and do
> something like zooming?
> 
> Maybe we should have a new flag, e.g. nsMouseScrollEvent::kScrollOnly to
> prevent these events from being interpreted as other than scrolling by the
> event state manager?

Yes, I planed as:

Normally, the WM_*SCROLL messages should not be used for mouse wheel events. They should be used for operations between processes. So, I think that we should prefer the utility's behaviors which use the WM_*SCROLL messages.

I think nsCommandEvent should have new scroll events. And they can scroll only the *focused* scrollable box or only root element's scrollable box. So, they shouldn't check the scrollable box hierarchy from the mouse cursor position. I.e., the events must not prefer the mouse cursor position.

But it's not useful for the WM_*SCROLL messages using mouse drivers. For them, we should implement the wheel events generating mode which is same as the posted patch. And the users can switch the mode from pref.

I think that the users who use both such tools and such mouse are not many. My plan cannot make them happy, but other most users will be made happy.
Um, I meant nsContentCommandEvent, not nsCommandEvent. Sorry.
(In reply to comment #65)
> Normally, the WM_*SCROLL messages should not be used for mouse wheel events.
> They should be used for operations between processes. So, I think that we
> should prefer the utility's behaviors which use the WM_*SCROLL messages.
> 
> I think nsCommandEvent should have new scroll events. And they can scroll only
> the *focused* scrollable box or only root element's scrollable box. So, they
> shouldn't check the scrollable box hierarchy from the mouse cursor position.
> I.e., the events must not prefer the mouse cursor position.

I think this is really the key issue --- whether the behaviour of these events should depend on the mouse cursor position or not. I agree that for tools they should not and for these trackpad events they should.

Is there any way we can distinguish those events, say by values in wParam or lParam?
(In reply to comment #67)
> Is there any way we can distinguish those events, say by values in wParam or
> lParam?

When wParam has non-SB_LINEDOWN/UP, we can decide easy... but...

http://msdn.microsoft.com/en-us/library/bb787577%28VS.85%29.aspx
How about we take this patch for SB_LINEDOWN/SB_LINEUP only, then? It seems unlikely that tools would use those.
(In reply to comment #69)
> How about we take this patch for SB_LINEDOWN/SB_LINEUP only, then? It seems
> unlikely that tools would use those.

Perhaps, it's not so, see bug 315272 comment 0.
(In reply to comment #70)
> (In reply to comment #69)
> > How about we take this patch for SB_LINEDOWN/SB_LINEUP only, then? It seems
> > unlikely that tools would use those.
> 
> Perhaps, it's not so, see bug 315272 comment 0.

oops, bug 315727 comment 0.
hrm, if we have processed WM_MOUSEWHEEL or WM_MOUSEHWHEEL in the process, the mouse should use them. Then, we can assume that the WM_*SCROLL messages are not come from the mouse driver. However, some tools which creators know this hack may post the messages for their behavior. At that time, if the mouse drive uses WM_*SCROLL messages, the mouse cannot scroll the pages after the user uses the tool.

And of course, the tools should be used after first wheel events, if not so, the users will look the different behavior, it causes the confusing...
Another random thought ... I suspect mouse drivers post these messages (PostMessage), but tools would send them (SendMessage). If that's true, then we could use InSendMessage.
yeah, it's good point. the tools should not use PostMessage, I agree.
We can't just take the patch for only SB_LINEDOWN/SB_LINEUP because the Trackpoint driver sends SB_PAGEDOWN/SB_PAGEUP too, and without those the scrolling feels awful.  We also can't use InSendMessage because the Trackpoint driver uses SendMessage.

Masayuki's idea from comment 72 though is interesting.  What if we had a flag in nsWindow we set the first time we got a WM_MOUSEWHEEL message, and after that flag was set we no longer treated WM_*SCROLL messages as if they came from a driver.  That doesn't handle multiple input devices, but it does get us 90% there.
So the Thinkpad Driver pretends to be the fake scrollbar when it sends the scroll messages, so the lParam of the WM_*SCROLL messages is the fake window we created.  There's no reason we can't store that HWND in nsWindow when we create it and compare lParam against it.  As long as the scroll capture apps don't use the same incredibly ugly and stupid hack, we should be able to distinguish between the two cases.
(In reply to comment #76)
> So the Thinkpad Driver pretends to be the fake scrollbar when it sends the
> scroll messages, so the lParam of the WM_*SCROLL messages is the fake window we
> created.  There's no reason we can't store that HWND in nsWindow when we create
> it and compare lParam against it.  As long as the scroll capture apps don't use
> the same incredibly ugly and stupid hack, we should be able to distinguish
> between the two cases.

Sounds reasonable. But I want to know whether the alps's and hp's drivers are same behavior, or not. Would you post a new patch which only processes the lParam is the scrollbar? I'll post the patch to tryserver, and I want to wait the reply in bug 315727 for checking the hp driver. If there are no problem (or no reply from the users), we should land it.
Attached patch Patch for Tryserver (obsolete) — — Splinter Review 
Patch as requested for the try server build.
> patching file widget/src/windows/nsWindow.cpp
> Hunk #5 FAILED at 4831.
> Hunk #6 FAILED at 5627.
> 2 out of 6 hunks FAILED -- saving rejects to file widget/src/windows/nsWindow.cpp.rej
> patching file widget/src/windows/nsWindow.h
> Hunk #2 succeeded at 301 with fuzz 2.
> Hunk #3 succeeded at 395 (offset 1 line).

The patch cannot be applied to the current tree.
Hello, I’m the author of CaptureWiz, one of the “scroll programs”, and I’d like to try and help. I can answer questions about how scroll programs work, particularly how they send WM_HSCROLL and WM_VSCROLL messages to Firefox’s windows.

CaptureWiz sends WM_HSCROLL and WM_VSCROLL messages to various windows within the user’s selected screen area until CaptureWiz finds a window that responds.

CaptureWiz uses PostMessage, not SendMessage, because it has many other tasks to perform and can’t afford to wait till SendMessage returns. Of course, we could create a thread to call SendMessage instead. We’ve found it’s sometimes necessary to send several WM_HSCROLL or WM_VSCROLL messages before target applications respond.

Here are the parameters sent with the WM_HSCROLL and WM_VSCROLL message:

hWnd
Handle to the window CaptureWiz found that responds to WM_HSCROLL or WM_VSCROLL messages.
 
Msg
WM_HSCROLL or WM_VSCROLL.

wParam
SB_TOP, SB_PAGERIGHT, SB_LINELEFT, SB_LINERIGHT, SB_LINEUP SB_LINEDOWN or SB_ENDSCROLL.

lParam
Window of the specific scroll bar CaptureWiz wants to scroll. Note that some windows contain several separate scrolling areas.


Generally, CaptureWiz sends packets of messages. It repeats a few commands, like SB_LINEDOWN, followed by SB_ENDSCROLL.

For example:

for i:= 1 to StepSize
{
  PostMessage(VscrollReciever, WM_VSCROLL, SB_LINEDOWN, VScrollBar)
}
PostMessage(VscrollReciever, WM_VSCROLL, SB_ENDSCROLL, VScrollBar)

Hope that helps!
Dave Eisler
(In reply to comment #80)
> lParam
> Window of the specific scroll bar CaptureWiz wants to scroll. Note that some
> windows contain several separate scrolling areas.

Hrm, this is bad for the current idea of comment 76...
(In reply to comment #81)
> (In reply to comment #80)
> > lParam
> > Window of the specific scroll bar CaptureWiz wants to scroll. Note that some
> > windows contain several separate scrolling areas.
> 
> Hrm, this is bad for the current idea of comment 76...

Yep, back to square one then.
Allow me to provide some good news and bad news about the lParam of the WM_VSCROLL message with respect to Firefox. The good news is, when scroll capturing Firefox, the lParam is ALWAYS ZERO.

That's because Firefox does not appear to use separate windows to display scrolling areas. When I say "window", I mean a Windows component provided by the Windows API CreateWindow() routine and who's Z-order is managed by Windows and is discoverable by the EnumWindows API call.

For example, please see this simple page with two scrolling areas:
http://pixelmetrics.com/Test/TwoScrollingAreas.html

External applications, like CaptureWiz, only see one window. They do not see that there are two separate scrolling areas inside the window.

The good news is, in Firefox the lParam is unused and ALWAYS ZERO. The bad news is the two scrolling areas can not be scroll captured.

Hope that helps!
Dave Eisler
(In reply to comment #83)
> Allow me to provide some good news and bad news about the lParam of the
> WM_VSCROLL message with respect to Firefox. The good news is, when scroll
> capturing Firefox, the lParam is ALWAYS ZERO.
> 
> That's because Firefox does not appear to use separate windows to display
> scrolling areas. When I say "window", I mean a Windows component provided by
> the Windows API CreateWindow() routine and who's Z-order is managed by Windows
> and is discoverable by the EnumWindows API call.
Is it still true even if the patch is applied? We are going to add a dummy native "SCROLLBAR" class window by CreateWindowEx() to satisfy Trackpoint driver.
We really need a tryserver build to verify that. Kyle, could you update a patch?
Attached patch Patch for Tryserver (obsolete) — — Splinter Review 
Unbitrotted patch.  Applies cleanly to current trunk.
Attachment #393832 - Attachment is obsolete: true
thank you, I posted the patch to tryserver.
alps's touchpad's horizontal scrolling doesn't work fine on the build.
Tryserver build from comment #87 solves the touchpad scrolling issues on my Thinkpad Z60m (Synaptics 11.1.21.2). Thanks!
(In reply to comment #88)
> alps's touchpad's horizontal scrolling doesn't work fine on the build.

Does the alps's touchpad work with the patch I posted for review? (if you just change if(lParam) to if(true) in nsWindow:OnScroll it's essentially the same)
Attachment #393705 - Attachment is obsolete: true
Attachment #393705 - Flags: review?(roc)
(In reply to comment #90)
> (In reply to comment #88)
> > alps's touchpad's horizontal scrolling doesn't work fine on the build.
> 
> Does the alps's touchpad work with the patch I posted for review? (if you just
> change if(lParam) to if(true) in nsWindow:OnScroll it's essentially the same)

Probably, no. Because there are no WM_*SCROLL messages from the driver.
So I managed to get access to an Alps touchpad computer (Dell Inspiron 9300) and the Alps touchpad scrolling areas work fine with 3.5, 3.6a1, and the tryserver build above.  What exactly is supposed to be broken?
(In reply to comment #92)
> So I managed to get access to an Alps touchpad computer (Dell Inspiron 9300)
> and the Alps touchpad scrolling areas work fine with 3.5, 3.6a1, and the
> tryserver build above.  What exactly is supposed to be broken?

Isn't it vertical scroll? My VAIO's touch pad's *horizontal* scrolling doesn't work.
Ah I didn't try the horizontal one, I'll give that a go later today.
So the Alps touchpad doesn't work in 3.5 or this tryserver build, correct?  It would seem that they use different behavior then(In reply to comment #93)
> (In reply to comment #92)
> > So I managed to get access to an Alps touchpad computer (Dell Inspiron 9300)
> > and the Alps touchpad scrolling areas work fine with 3.5, 3.6a1, and the
> > tryserver build above.  What exactly is supposed to be broken?
> 
> Isn't it vertical scroll? My VAIO's touch pad's *horizontal* scrolling doesn't
> work.

Yeah, horizontal scrolling doesn't work on 3.5 or on the tryserver build on the Alps touchpad I have access to.
(In reply to comment #95)
> Yeah, horizontal scrolling doesn't work on 3.5 or on the tryserver build on the
> Alps touchpad I have access to.

Yes, unfortunately, we need to different fake for it, but it should be in another bug.
Can the scrolling behaviour used by the driver and/or tools be turned into some sort of test suite testcase?  I've no idea what the capabilities of that tool set are.
Attached patch Patch for review (obsolete) — — Splinter Review 
I already implemented the first round of roc's suggestions from several weeks ago.  Based on the feedback I believe casing the scroll handling as we have done is a suitable solution.  This is essentially the tryserver patch with some changes to the string handling used to create the scrollbar.

For posterity, MSDN docs provide that the LPARAM is always either the scrollbar HWND or NULL.  As our scrollbar is fake it never sends us HWNDs, and the Trackpoint driver sends us malformed scroll messages with LPARAM equal to the HWND of the window to be scrolled (not the scrollbar control).  Since there is no legitimate case where the LPARAM is not 0, this patch handles all messages with non-zero LPARAMs, allowing the other bug to eventually handle LPARAMs of 0 (i.e. legitimate scrolling messages from other applications) as needed.
Attachment #394543 - Attachment is obsolete: true
Attachment #397441 - Flags: review?(roc)
The actual behaviour looks great. Just a few nits:

+ 	::CreateWindowW(L"SCROLLBAR", L"FAKETRACKPOINTSCROLLBAR", WS_CHILD | WS_VISIBLE, 0,0,0,0, mWnd, NULL, nsToolkit::mDllInstance, NULL);

80 char lines please

+PRBool nsWindow::HandleScrollingPlugins(UINT msg, WPARAM wParam, LPARAM lParam, PRBool& result)

Parameters are supposed to start with 'a', so aMsg, sWParam, aLParam, aResult. But call aResult "aHandled" instead.

+    if (destWindow) {
+	  LRESULT aRetValue;
+      destWindow->ProcessMessage(msg, wParam, lParam, &aRetValue);
+	  result = PR_TRUE;
+      return PR_FALSE; // return result immediately
+    }

Fix indent

+PRBool nsWindow::OnScroll(UINT msg, WPARAM wParam, LPARAM lParam)

aMsg, aWParam, aLParam

+  PRBool                  HandleScrollingPlugins(UINT msg, WPARAM wParam, LPARAM lParam
+                                                 , PRBool& result);

Move comma to the previous line.

+  if (lParam)
+  {
+    // Handle fake scroll messages

This terminology is a bit confusing. I think it would be clearer to say something like
 // Scroll message generated by Trackpad Trackpoint driver or similar. Handle
 // it like mousewheel input.
And below, instead of "// Handle legitimate scroll messages", I think we should say
 // Scroll message generated by external application
 // XXX Handle this by scrolling the window by the desired amount (bug 315727)

It seems that the if (lParam) test is currently redundant since we only call OnScroll if lParam is nonzero in ProcessMessage. But we should probably get rid of those tests in ProcessMessage since the commented reason for the tests has been irrelevant for a long time. That would be part of bug 315727 though.
Thanks for the comments roc, I'll try to have a nitted version up later tonight or tomorrow at the latest.

(In reply to comment #97)
> Can the scrolling behaviour used by the driver and/or tools be turned into some
> sort of test suite testcase?  I've no idea what the capabilities of that tool
> set are.

As far as I know, there will be no way to test this completely without reimplementing the driver.  We could test other aspects of the hack, but I don't think any of the testing frameworks provide any way to use the Windows API, so it would have to be a compiled test.
Attached patch Patch w/nits (obsolete) — — Splinter Review 

        Attachment #397441 -
        Attachment is obsolete: true

        Attachment #397608 -
        Flags: review?(roc)

        Attachment #397441 -
        Flags: review?(roc)

    
    

    
  
(In reply to comment #99)
> It seems that the if (lParam) test is currently redundant since we only call
> OnScroll if lParam is nonzero in ProcessMessage. But we should probably get rid
> of those tests in ProcessMessage since the commented reason for the tests has
> been irrelevant for a long time. That would be part of bug 315727 though.

Yes, you are correct, it is redundant.  It is worth noting that the mechanism used to dispatch scroll messages in ProcessMessage only works in exactly this case.  The Trackpoint driver sends WM_*SCROLL messages with the lParam set to the HWND of the window to scroll (when according to MSDN it should be the HWND of the control asking for the scrolling to happen).  Bug 315727 will have to rework that mechanism slightly.

    
    

    
  
Comment on attachment 397608 [details] [diff] [review]
Patch w/nits

Just fix the indent here and then we can get this checked in:

+    // Scroll message generated by Thinkpad Trackpoint Driver or similar
+	// Treat as a mousewheel message and scroll appropriately

        Attachment #397608 -
        Flags: review?(roc) → review+

    
    

    
  
(In reply to comment #103)
> (From update of attachment 397608 [details] [diff] [review])
> Just fix the indent here and then we can get this checked in:
> 
> +    // Scroll message generated by Thinkpad Trackpoint Driver or similar
> +    // Treat as a mousewheel message and scroll appropriately

Ugh I thought I got them all.  Fixed version coming up in a few.  Does this need sr or review from anyone in accessibility?

    
    

    
  


  
Final nits picked per roc's review.

        Attachment #397608 -
        Attachment is obsolete: true

        Attachment #397723 -
        Flags: review+

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/6e0f37f902b7

Thanks a ton!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 397723 [details] [diff] [review]
Patch w/nits nits picked

We'll need this in 1.9.2

        Attachment #397723 -
        Flags: approval1.9.2?

    
    

    
  
Could this have broken All-in-One Gestures?

    
    

    
  
(In reply to comment #109)
> Could this have broken All-in-One Gestures?

I'm not sure. Does All-in-One Gestures use the mouse wheel?

    
    

    
  
No, right and left clicks.

    
    

    
  
(In reply to comment #111)
> No, right and left clicks.

Sounds like this is not the cause. You should file a bug and find the regression range.

    
    

    
  
Thinkpad scrolling is now working again for me in:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090903 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20090903045002

Thanks, y'all.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for MozillaUIWindowClass (obsolete)
        — 
          — Splinter Review
      

    


  
Children of MozillaUIWindowClass need fake scrollbars too (most notably for the addons window).  This will need to be taken on 1.9.2 with the big patch.

        Attachment #398567 -
        Flags: review?(roc)

        Attachment #398567 -
        Flags: approval1.9.2?

        Attachment #398567 -
        Flags: review?(roc)

        Attachment #398567 -
        Flags: approval1.9.2?

    
    

    
  


  
Per discussion with roc on IRC, instead of trying to enumerate all the classes which need the hack, just do it everywhere.

        Attachment #398567 -
        Attachment is obsolete: true

        Attachment #398576 -
        Flags: review?(roc)

        Attachment #398576 -
        Flags: approval1.9.2?

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/c390b81acd94
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a1
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1

    
    

    
  
This was already set to a blocker of fx3.6, so, the patches can be landed to 1.9.2 branch. Why isn't this still landed to 1.9.2 branch? If Kyle cannot land them, I'll land them. Or, is this waiting some fix of the regressions?

    
    

    
  
Bug 514927 needs to be fixed and then I'll make a combined patch.

    
    

    
  
(In reply to comment #118)
> Bug 514927 needs to be fixed and then I'll make a combined patch.

You shouldn't combine the patches, you should land each patches at once. And note that after this patch, I need to land the patch of bug 516654 before you land bug 514927's patch if they conflict.

    
    

    
  
These patches don't need approval to land on 1.9.2 - this bug is a 1.9.2 blocker.

        Attachment #397723 -
        Flags: approval1.9.2?

        Attachment #398576 -
        Flags: approval1.9.2?

    
    

    
  
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091007042048

Something broke this again in the Trunk nightly.  Busted for main view and Add-on window.
Status: RESOLVED → REOPENED
blocking2.0: --- → ?
Resolution: FIXED → ---

    
    

    
  
Bug 514927 is the likely cause of this grief.   The search doesn't seem to be getting my T61's Trackpoint Registry keys and I'm set for ui.trackpoint_hack.enabled to -1.

    
    

    
  
OK, where is your Trackpoint driver mentioned in the registry?

    
    

    
  
Some other places you can search for hints on this T61...

HKEY_LOCAL_MACHINE\SOFTWARE\Lenovo\ThinkPad UltraNav Utility
HKEY_LOCAL_MACHINE\SOFTWARE\Lenovo\ProdCenter\Client\UltraNav
HKEY_LOCAL_MACHINE\SOFTWARE\Lenovo\ThinkPad FnF8
HKEY_CURRENT_USER\Software\Lenovo\PRDCTR\Client\UltraNav
HKEY_CURRENT_USER\Software\Synaptics\SynTPEnh\UltraNavPS2

    
    

    
  
So, is Bug 514927 the cause for renewed bustage?  What's the story?  Need to know since this is blocking beta.  Down to the last bug here.

    
    

    
  
(In reply to comment #126)
> So, is Bug 514927 the cause for renewed bustage?  What's the story?  Need to
> know since this is blocking beta.  Down to the last bug here.

We had to pref this hack in Bug 514927 because it breaks other utilities.  The code to detect whether or not to turn the hack on isn't quite finished yet, but that doesn't need a beta.  Resolving again and we'll add registry keys to the detection code in 514927.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
No longer depends on: 528267
Depends on: 529638

    
  
Depends on: 483136

    
    

    
  
Marking the 14 bugs that are both:
 * nominated for blocking1.9.3:?
 * fixed on the 1.9.2 branch (according to status1.9.2)
as blocking1.9.3:alpha1, so that we don't have to go through the nominations individually.  They're all fixed already (so there's no work to do), and being fixed on 1.9.2 means they probably do block 1.9.3.
blocking2.0: ? → alpha1
Depends on: 541836

    
    

    
  
This just got busted again somehow on Windows XP.

Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre

I'm guessing this was caused by Bug 589529.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Please file a new bug and include hardware/driver information.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED

    
    

    
  
Opened Bug 592173.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: