User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:2.0b6pre) Gecko/20100903 Firefox/4.0b6pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre On my laptops touchpad there are two gestures for going back and forward in page navigation (three fingers left or right). This doesn't work anymore. Instead of that it does now page up or page down. Reproducible: Always Steps to Reproduce: 1. get a build from latest trunk 2. try to use back or forward gesture on your Laptops Touchpad Actual Results: it does page up or page down Expected Results: it does page back or page forward Happens since: Built from http://hg.mozilla.org/mozilla-central/rev/3f499de2401d Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre

Seems not to happen on Synaptics Touchpads Happens on Elan Smartpad Touchpads They seem to use different ways to do the back/forward navigation.

Try updating your build because I believe this was bug 589529 which is now fixed.

Same happens here, Elan Touchpad. Kurt I am using the latest hourly from today and it is still broken.

Still happens with latest nightly.

Also happens on the Beta 5 candidate, any chance of a quick fix before release?

This is break before or after bug 589529 was fixed? Or is this also a regression from bug 130078

I think it happened before 589523 was fixed. Not sure about 130078. Should I add 130078 as block? It happens exactly since this hourly build: http://hg.mozilla.org/mozilla-central/rev/3f499de2401d

(In reply to comment #7) > I think it happened before 589523 was fixed. > Not sure about 130078. Should I add 130078 as block? > > It happens exactly since this hourly build: > http://hg.mozilla.org/mozilla-central/rev/3f499de2401d Only if you can confirm that it was caused by that bug or most likely caused by it. Can you try the 08-27 and 08-28 nightlys and see if it worked in the 27ths build and broken in the 28ths? http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010-08-27-04-mozilla-central/ http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010-08-28-04-mozilla-central/

Works on 27th and 28th nightly. It started with an hourly from 30th This is the build ID from "about:support": 20100830165706 This is what "about:buildconfig" says: Built from http://hg.mozilla.org/mozilla-central/rev/3f499de2401d

khuey, looks like bug 589523 caused problems with other trackpads that were originally working after bug 130078 landed.

Hopefully Bug 591874 will fix this.

This might not be fixable, but I guess we should block until it becomes clear it isn't.

Latest hourly which includes fixed Bug 591874, doesn't fix this Bug. It still does page up/down instead of back/forward. There are lots of people out there with an Elan Smartpad Touchpad. So hopefully this gets fixed, as this is something which makes people change their browser.

Confirmed with Morpog, swipe back and forward is still broken with the patch checked in. I am a very upset user, I am willing and able to try test builds in order to get this resolved.

I doubt there's much we can do here. The only thing I can think of is to try enabling the a11y emulation if we detect one of these drivers.

Maybe some of you developers could try to get in contact with Elantech. http://www.emc.com.tw/eng/about_elan_5.asp

Kyle, would you be able to put up a tryserver build with this emulation so I can test it? So long as it works for now, at least it is better than nothing. However a real fix would be a much better solution.

I'm also interested into a tryserver build, so we can try nad confirm if an emulation does the trick.

Alex, is there an easy way to force the HWND emulation that you added to be on?

(In reply to comment #19) > Alex, is there an easy way to force the HWND emulation that you added to be on? HWND emulation is a part of accessibility and we enable it if we detect the presence of certain screen readers. It looks like accessibility isn't wanted to make mouse gestures work. If HWND emulation is right way to fix this bug then it should be moved out from accessibility, so the answer is rather no than yes.

Is there something easy we can do to test if the emulation fixes this bug before investing more time into moving the emulation out of accessibility?

Say, flipping a pref, or a small patch we can apply that forces the emulation on for a try server build?

yes, you should make nsWinUtils::IsWindowEmulationEnabled to return true (http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsWinUtils.cpp#135) always and then start accessibility: you could run any screen reader (like NVDA nvda-project.org), AT tool (like Accessible Event Watcher) or you could use DOM inspector (by inspecting the accessible tree).

(In reply to comment #22) > Say, flipping a pref, or a small patch we can apply that forces the emulation > on for a try server build? no perf. If you mean remote testing of try server build by running mochitests then you should morph tests to start a11y, that can be done by getting nsIAccessbilityService service, btw, you could do the same locally instead of approaches described above.

Could someone who sees this bug try this try server build http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-de9af3868036/ and before testing you need to install DOM Inspector (https://addons.mozilla.org/en-US/firefox/addon/6622/) then open it from the Tools menu, then click on the dropdown icon to the left of "Document - DOM Nodes" near the top left of the window and choose Accessible Tree. Then see if the touchpad things work or not.

Just tried it. Doesn't help. Still does scrolling.

Timothy, I tried that, but the scrolling is not working. Keep the tryservers coming, I am willing and able.

Wait, is this bug about back/forward gestures not working or scrolling gestures not working?

back/forward gestures not working.

Yup, swiping left and right is not working ( three finger )

So then comment 27 was mistaken when it talked about scrolling not working?

Yep it was mistaken, I realised after you replied, but there is no way to remove posts? Sorry for the miscommunication!

Kyle, would we need to hook up event handling or something to the emulated HWNDs or would you expect this to just work if the HWNDs are present?

If I had to guess I'd think that we need to forward messages from these emulated HWNDs back up to the real HWND to be dispatched appropriately. It's possible that the drivers rely on focus/size of the various HWNDs though, in which case this is probably not fixable on our side.

Guys, I noticed that pinch and zoom is also broken. Should I make a seperate bug, or can this one be modified to reflect this issue?

Probably the same issue.

I hate to bugspam, but I am willing to test new builds and this bug seems to have been forgotten about?

This is probably different from bug 594977 since it doesn't involve Synaptics hardware/drivers, as I understand it. Can someone with this Elan Touchpad identify what drivers it uses? Is there a special page in the Mouse Control Panel? Is there any touchpad helper application running like SynTPEnh.exe for Synaptics Touchpads?

- It uses the (latest) driver shown in the attached screenshot - there is a special page in the Mouse control panel - there seems to be a touchpad helper application (etdctrl.exe)

Great thanks! Too bad I don't read German :-). Under \Program Files\Elantech, are there any text files that mention Mozilla or Firefox anywhere?

Whole Elantech directory from "Program Files" as Winrar package

Thanks! ETDApix.dll contains one occurrence of MozillaWindowClass and multiple occurrences of MozillaUIWindowClass. ETDCmds.dll contains one occurrence of MozillaUIWindowClass. So it's a safe bet that Elantech is sniffing for MozillaUIWindowClass and not finding it, thus breaking something.

Maybe we could detect Elantech and switch our toplevel window class to MozillaUIWindowClass? Morpog, can you find any registry entries for Elantech's drivers?

Yes there are alot settings in registry. Check the INF file I uploaded.

OK, I suggest adding another hack to detect Elantech in the registry and if so switch the window class name to MozillaUIWindowClass.

Robert, Elantech have released updated drivers 8.0.7.0 that fix swiping however zooming is broken. The driver is however unnofficial in respect of my laptop. So not sure if Elan are aware, but it seem they have changed their detection? Here is the link to the driver, if you want to me to post anything let me know. http://www.station-drivers.com/page/elantech.htm

Unfortunately those 8.0.7.0 drivers don't work with my Elantech Trackpad at all. Maybe they only work with new touchpad models from Elantech. I also tried those unofficial 7.0.7.0 drivers (i had last official 7.0.6.2), but those didn't fix it. That 8.0.7.0 INF File shows alot new Registry settings. Some are related to Firefox.

I'm probably not going to be able to fix this without access to hardware.

We can purchase hardware to test if needed. Can someone drop me an email indicating the exact hardware/software we need to acquire, please?

Anybody seeing this bug care to chime in with what hardware they have? Google isn't telling me much about what machines have Elantech touchpads, other than that they seem to be common among Asus netbooks.

I got an EeePC 1000H. Should be cheap to get used on ebay.

I have an Asus N61Jq, with elantech touchpad.

Nearly 2 months and nobody interested in this bug?

We just haven't had time to work on it yet. We will soon.

I got my hands on an Eee PC 1000, and can reproduce the problem. The three finger swipe gesture now seems to generate page up/down events.

This patch adds a hack that makes the three finger swipe gesture work again with v7 of the Elantech drivers. With the HWND change, the driver now dispatches Page Up and Page Down keypress messages rather than Alt+Left and Alt+Right. It does so in a way that can be differentiated from regular Page Up/Down presses though -- it sends a virtual key 0xFF WM_KEYDOWN message just before the VK_PRIOR/VK_NEXT WM_KEYDOWN/WM_KEYUP pair, and then a 0xFF WM_KEYUP afterwards. (0xFF is sometimes used for the Fn key on laptop keyboards.) The patch detects this 0xFF + Page Up/Down keypress and translates it into an Alt+Left/Right before passing it off to the event dispatch code. The patch also factors out some of the Trackpoint hack code to reuse it for this hack.

I can reproduce the problem mentioned with the pinch-to-zoom gesture on v8 of the drivers, but haven't diagnosed it yet. I can actually trigger it, but it's harder than with v7 -- I find I need to do larger and quicker pinches for the zoom to happen. The zooming is achieved by the driver sending Alt+mousewheel messages.

Is there a tryserver build for that patch?

Yes, please try this build: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cmccormack@mozilla.com-c52cb5339c36/try-w32-dbg/firefox-4.0b10pre.en-US.win32.zip

That build doesn`t work for me. A side by side error pops up after starting firefox.exe

Cameron: you'll need to make an opt build if you want anybody who doesn't have MSVC 2005 installed to test it.

Cameron, I am eager to test but as mentioned I get a side by side error.

Ah, I did not know that. Here is an opt build to try: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cmccormack@mozilla.com-b902347bc639/try-w32/firefox-4.0b10pre.en-US.win32.zip

Thanks for providing that tryserver build. Three finger swipe gesture works great now with v7 drivers on my EeePC 1000H! Pinch to zoom doesn't work well at all with v7 drivers. It's very choppy and you need to pinch alot to get it start to zoom at all.

Three finger not working here. Using the 7.0.5.10 drivers?

Comment on attachment 505285 [details] [diff] [review] Part 1: Add a hack for the Elantech touchpad so that three-finger swipe performs a browser back/forward action instead of page up/down. I have 7.0.6.0, but now am finding it's back to page up/down behaviour. Will investigate.

Recall/Morpog: can you try this build? http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cmccormack@mozilla.com-b64858cb29a1/try-w32/firefox-4.0b10pre.en-US.win32.zip It seems some versions of the driver use virtual key 0xFF (Fn) has the modifier when sending the Page Up/Down keys, while others use 0xCC (an OEM defined key).

Three finger swipe gesture still works great now with 7.0.7.0 drivers on my EeePC 1000H. Pinch to zoom still doesn't work well. It's very choppy and you need to pinch alot to get it start to zoom at all.

Hi Cameron. I just tested with 8.0.5.0 drivers and it works fine with them, zooming is good and also back forward and up down work fine. However with the officially latest drivers for my laptop which are 7.0.5.10 back forward works fine, so does up and down. However pinch and zoom fails to respond.

Great. (I haven't actually made any changes to fix the zooming yet.)

Comment on attachment 506507 [details] [diff] [review] Add a hack for the Elantech touchpad so that three-finger swipe left/right performs a browser back/forward action instead of page up/down (v2.1) Remove a line that was part of the unfinished zoom gesture fix part.

Here is a build to try that should make the zoom gesture work: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cmccormack@mozilla.com-18cc4ddb8e9c/try-w32/firefox-4.0b10pre.en-US.win32.zip Can you verify that this works for you Morpog/Recall? The zooming is somewhat dysfunctional, in that sometimes the driver helper application sends some incorrectly ordered messages -- instead of WM_KEYDOWN VK_CONTROL WM_MOUSEWHEEL WM_KEYUP VK_CONTROL it sends WM_KEYDOWN VK_CONTROL WM_KEYUP VK_CONTROL WM_MOUSEWHEEL the result of which is that sometimes when doing a pinch-to-zoom gesture, you will get some page scrolling too. It does this even for Firefox 3.6, AFAICT, so with this build the regression is resolved, but is not entirely satisfactory behaviour. (I think it should be possible to detect that these misordered messages are sent from the driver and to assume they are Ctrl+Wheel messages, so I will look at that next.)

Works alot better than before, but not as good/smooth as in other applications (like windows image viewer). Sometimes it scrools while zooming as you said.

Turns out accounting for the misordered messages isn't an easy fix. Sometimes the helper application sends more WM_KEYDOWN/WM_KEYUP pairs than WM_MOUSEWHEELs. When this happens, the WM_MOUSEWHEEL isn't sent while the 1x1 px touchpad helper window is under the mouse cursor, so it's not possible to distinguish it from a normal mouse wheel message from scrolling. In fact, it looking at the timestamps of the messages when WM_MOUSEWHEEL is sent, our window procedure gets them like this (when the problem surfaces): WM_KEYDOWN VK_CONTROL time t0 WM_KEYUP VK_CONTROL time t2 WM_MOUSEWHEEL time t1 so I don't know what's happening there. (I don't know enough about Windows message queues to know if that is reasonable.) The touchpad helper program seem to have this problem with, say, the Windows Image Viewer program, but for now I don't know why. I will trace through the helper program to see if there's an peculiarities about how it delivers us the messages.

It sounds like we could be reaching the point of diminishing returns. It sounds to me like maybe we should go with the fix in comment #74 and not block on any more work.

Yeah.

This patch searches for the window that would be underneath the 1x1 px window that the touchpad helper application places under the cursor when doing the pinch-to-zoom gesture, and ensures that the Ctrl+Wheel messages are not discarded. (The existing code looks to see what window is under the cursor to determine where to dispatch the mouse wheel event.) IsOurProcessWindow and FindOurProcessWindow are also made static in the class declaration, which masayuki confirmed they should be.

I found a way to work around the problems with the zoom gesture, I believe. Can you guys try this build? http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cmccormack@mozilla.com-778039453f9a/try-w32/firefox-4.0b10pre.en-US.win32.zip

This works very very well! I don't think it can get any better on that old and slow ATOM CPU. Nice done!

The misordering of the key/wheel messages actually was due to the way the message loop peeked the queue for UI events. Currently, all key messages are retrieved from the message queue and dispatched before all mouse messages are. This patch makes the message loop retrieve key and mouse messages in order from the queue.

As mentioned, with the particular v8 driver I've tested with, the WM_KEYDOWN/WM_KEYUP messages sent as part of the zoom gesture are sent with erroneous timestamps -- always a constant 10. They thus always get retrieved first from the message queue. This patch detects these messages and sets a flag to assume that all WM_MOUSEWHEEL messages sent before the current system time should be dispatched as events with the control key down. (It also renames the pref from ui.elantech_gesture_hack.enabled to ui.elantech_gesture_hacks.enabled since there are really two separate hacks that it enables -- one for the three-finger swiping, one for the pinch-to-zoom.)

Hi Cameron, sorry for not replying been busy with home life and work. Anyway just tested pinch zoom and back and forward on the 7.0.5.10 which are latest official for my notebook and I pinch zoom works even better than before ( smooth rather than jerky ) Also back forward also works. I think Elantech really need to sort out their driver team, as there are way too many variations in their drivers.

Excellent, thanks very much for testing.

Comment on attachment 506507 [details] [diff] [review] Add a hack for the Elantech touchpad so that three-finger swipe left/right performs a browser back/forward action instead of page up/down (v2.1) mostly nits - >+ // Elantech Touchpad Gesture Hack (Bug 593372) No need for this, hg blame will track changes. >+PRBool nsWindow::UseInputHack(const char* aPrefName, PRBool aDefaultValue) Lets get rid of PRBool aDefaultValue and just return the flag value. Also, lets change 'UseInputHack' to something a bit more friendly, like GetInputPref or something similar. > { > nsresult rv; > nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); > if (NS_SUCCEEDED(rv) && prefs) { > PRInt32 lHackValue; >- rv = prefs->GetIntPref("ui.trackpoint_hack.enabled", &lHackValue); >+ rv = prefs->GetIntPref(aPrefName, &lHackValue); Add a null check on aPrefName. > static PRBool >-IsObsoleteSynapticsDriver() >+GetRegistryKey(HKEY aRoot, LPCWSTR aKeyName, LPCWSTR aValueName, PRUnichar* aBuffer, DWORD aBufferLength) > { Null checks on passed in params before we go digging in the registry. You could also have this just return a length if aBuffer is null. Add a comment above this describing the params as well. Also, LPCWSTR and PRUnichar should be interchangeable, so try using PRUnichar for everything.

Comment on attachment 507370 [details] [diff] [review] Part 2: Ensure that Elantech driver helper window doesn't prevent zoom gestures from working >+EXTRA_DSO_LDOPTS += psapi.lib >+ >+ if (hProcess) { >+ PRUnichar path[256]; >+ if (::GetProcessImageFileName(hProcess, path, sizeof path)) { >+ path[255] = 0; According to msdn, this is only available on XP and up. We still support builds on Win2K so it looks like you're going to have to load this manually using GetProcAddress.

Comment on attachment 508144 [details] [diff] [review] Part 3: Always process mouse and keyboard events in the right order Nice cleanup. This definitely needs a try run before landing.

Comment on attachment 508149 [details] [diff] [review] Part 4: Work around the Elantech v8 driver's erroneous message time stamps for zoom gestures Looks to be a slight modification of the first patch, could you merge this together when you resubmit? (Also, please nix all the (Bug 593372) references in this one.

(In reply to comment #86) > >+ // Elantech Touchpad Gesture Hack (Bug 593372) > > No need for this, hg blame will track changes. OK. > >+PRBool nsWindow::UseInputHack(const char* aPrefName, PRBool aDefaultValue) > > Lets get rid of PRBool aDefaultValue and just return the flag value. Where would the default value come in to play, if it is not passed in to this method? (UseInputHack would have to know about particular pref names, to know what their defaults are. Passing in the default value seems simplest.) > Also, lets > change 'UseInputHack' to something a bit more friendly, like GetInputPref or > something similar. All right (though they are quite hacky!). > > { > > nsresult rv; > > nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); > > if (NS_SUCCEEDED(rv) && prefs) { > > PRInt32 lHackValue; > >- rv = prefs->GetIntPref("ui.trackpoint_hack.enabled", &lHackValue); > >+ rv = prefs->GetIntPref(aPrefName, &lHackValue); > > > Add a null check on aPrefName. OK. > > static PRBool > >-IsObsoleteSynapticsDriver() > >+GetRegistryKey(HKEY aRoot, LPCWSTR aKeyName, LPCWSTR aValueName, PRUnichar* aBuffer, DWORD aBufferLength) > > { > > Null checks on passed in params before we go digging in the registry. OK. > You could > also have this just return a length if aBuffer is null. I will make it survive null being passed in for aBuffer, and still return true/false depending on whether the registry value exists. Returning the length would either mean returning a special value like -1 to indicate non-existence, or passing in a PRUint32* in which the length would be stored. We don't need that information yet, so I don't think we need to return it at the moment. > Add a comment above > this describing the params as well. OK. > > static PRBool > >-IsObsoleteSynapticsDriver() > >+GetRegistryKey(HKEY aRoot, LPCWSTR aKeyName, LPCWSTR aValueName, PRUnichar* aBuffer, DWORD aBufferLength) > > { > > > Also, LPCWSTR and PRUnichar should be interchangeable, so try using PRUnichar > for everything. OK. (In reply to comment #87) > We still support builds > on Win2K so it looks like you're going to have to load this manually using > GetProcAddress. How about those WINCE ifdefed parts? I didn't worry about them at all. Is it OK to ignore them? (In reply to comment #88) > Nice cleanup. This definitely needs a try run before landing. I got a successful try run earlier, but I'll run another one after addressing your comments. (In reply to comment #89) > Looks to be a slight modification of the first patch, could you merge this > together when you resubmit? The rename of the pref/member variable/functions? OK. > (Also, please nix all the (Bug 593372) references > in this one. OK.

> > >+PRBool nsWindow::UseInputHack(const char* aPrefName, PRBool aDefaultValue) > > > > Lets get rid of PRBool aDefaultValue and just return the flag value. > > Where would the default value come in to play, if it is not passed in to this > method? (UseInputHack would have to know about particular pref names, to know > what their defaults are. Passing in the default value seems simplest.) > Maybe I missed something, I didn't see where aDefaultValue was being used. I thought what you needed here was a boolean result based on the pref value (true if it's set and true, false if it's false or not set). I'll wait for your next rev to take a look. > (In reply to comment #87) > > We still support builds > > on Win2K so it looks like you're going to have to load this manually using > > GetProcAddress. > > How about those WINCE ifdefed parts? I didn't worry about them at all. Is it > OK to ignore them? Ignore them. We don't actively support ce anymore.

Addressed review comments.

(In reply to comment #91) > Maybe I missed something, I didn't see where aDefaultValue was being used. I > thought what you needed here was a boolean result based on the pref value (true > if it's set and true, false if it's false or not set). I'll wait for your next > rev to take a look. The result of IsObsoleteElantechDriver() is passed in as the aDefaultValue. aDefaultValue is the true/false value to use when the pref is set to autodetect whether or not to use the hack (-1, or lack of the pref, means autodetect). (I seem to have uploaded the wrong patches, please wait.)

Comment on attachment 510502 [details] [diff] [review] Part 1: Add a hack for the Elantech touchpad so that three-finger swipe left/right performs a browser back/forward action instead of page up/down (v3) Pushed the wrong patches to tryserver, too. Will attach new patches after another try run.

Addressed review comments. * I used InitInputWorkaroundPrefDefaults as the friendlier replacement name for InitInputHackDefaults. Also s/UseInputHack/GetInputWorkaroundPref/. * Renamed the GetInputWorkaroundPref's aDefault argument to aValueIfAutomatic to make it clearer, hopefully. Also added some documentation for that function.

Cameron, before you go away can you link me to a tryserver build. I will test it for you and post my feedback here.

I think this is the try build you want http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cmccormack@mozilla.com-aec6985388a0/

(In reply to comment #101) > I think this is the try build you want > http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cmccormack@mozilla.com-aec6985388a0/ Thank Timothy! I can now confirm it works well with both my native drivers 7.0.5.10 and the latest from Asus which are 8.0.5.0.

http://hg.mozilla.org/mozilla-central/rev/87fb25283d8d http://hg.mozilla.org/mozilla-central/rev/c0ee311faca1 http://hg.mozilla.org/mozilla-central/rev/0da0c99b690d http://hg.mozilla.org/mozilla-central/rev/d2db6c6be747 My apologies for the checkin author on the last two patches. They didn't have a user name embedded, and I assumed that they did without checking based on the first two patches. :(

This broke scrolling on Alps touchpads (at least on my HP dv4t laptop w/ Win7). Firefox now zooms when I attempt to scroll using the touchpad. FWIW. The scroll gesture was working fine for me up to this point.

I was pointed to the ui.elantech_gesture_hacks.enabled pref on IRC. Setting it to 0 and restarting fixed my problem.

Ehsan, not sure why my hg exports didn't get the name in there, sorry about that. Ryan, it seems suboptimal for this patch to break scrolling on Alps touchpads with default pref settings. Maybe the autodetection logic needs to be tweaked. Can you file a followup bug? I will look into it on Monday. Thanks.

http://hg.mozilla.org/mozilla-central/rev/8ba0d8f0efb2 backed out, this broke scroll wheel functionality on windows.

There's something very very wrong with mAssumeWheelIsZoomUntil.

This also blew up thunderbird builds: e:\buildbot\win32-comm-central-build\build\mozilla\widget\src\windows\nsWindow.h(384) : warning C4244: 'return' : conversion from 'LPARAM' to 'WORD', possible loss of data e:/buildbot/win32-comm-central-build/build/mozilla/widget/src/windows/nsWindow.cpp(6732) : warning C4018: '<' : signed/unsigned mismatch e:/buildbot/win32-comm-central-build/build/mozilla/widget/src/windows/nsWindow.cpp(7739) : error C2664: 'LoadLibraryA' : cannot convert parameter 1 from 'const unsigned short [10]' to 'LPCSTR' Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast e:/buildbot/win32-comm-central-build/build/mozilla/widget/src/windows/nsWindow.cpp(7758) : error C2664: 'DWORD (HANDLE,LPTSTR,DWORD)' : cannot convert parameter 2 from 'PRUnichar [256]' to 'LPTSTR' Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast e:/buildbot/win32-comm-central-build/build/mozilla/widget/src/windows/nsWindow.cpp(7760) : error C2664: 'lstrlenA' : cannot convert parameter 1 from 'PRUnichar [256]' to 'LPCSTR' Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast e:/buildbot/win32-comm-central-build/build/mozilla/widget/src/windows/nsWindow.cpp(7762) : error C2664: 'lstrcmpiA' : cannot convert parameter 1 from 'PRUnichar *' to 'LPCSTR' Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Looks like mAssumeWheelIsZoomUntil isn't initialized. Not sure about the Thunderbird build problems. Do they compile without -DUNICODE?

(In reply to comment #110) > Looks like mAssumeWheelIsZoomUntil isn't initialized. > > Not sure about the Thunderbird build problems. Do they compile without > -DUNICODE? Yes, we need to explicitly request the ascii or wide character apis in widget. So the str apis need to be specific, and calls like LoadLibrary needs to be LoadLibraryW/A.

OK. (I had a feeling it was OK to assume -DUNICODE for some reason, but I guess I was wrong.) Another issue hidden in those warnings is that mAssumeWheelIsZoomUntil is unsigned, while the result of ::GetMessageTime() is signed. It sounds like ::GetMessageTime() values wrap back around to zero once they reach 2**31-1, while ::GetTickCount() values wrap at 2**32-1. So that timestamp wrapping checking code at the top of nsWindow::ProcessMessage probably needs checking. (http://tbpl.mozilla.org/?tree=MozillaTry&pusher=cmccormack@mozilla.com is building with the mAssumeWheelIsZoomUntil intialization.) /me enplanes

Comment on attachment 508144 [details] [diff] [review] Part 3: Always process mouse and keyboard events in the right order This always used to bug me (particularly on W2K where key and mouse events don't get thrown away after 2 seconds) - I'm used to waiting for Gecko to respond to a click so that I can be sure that it's safe to type!

Comment on attachment 510546 [details] [diff] [review] Part 2: Ensure that Elantech driver helper window doesn't prevent z oom gestures from working (v2.1) >+static HWND FindTopmostWindowAtPoint(HWND aHWND, const POINT& aPoint) (Why doesn't this use [Child]WindowFromPoint[Ex]?) Non-Unicode fixes required: >+typedef DWORD (*GetProcessImageFileNameProc)(HANDLE, LPTSTR, DWORD); LPWSTR >+ static HMODULE hPSAPI = ::LoadLibrary(L"psapi.dll"); LoadLibraryW >+ PRUnichar path[256]; WCHAR (for mingw, but also in case we turn off -Zc:wchar_t-) >+ int pathLength = lstrlen(path); lstrlenW >+ if (lstrcmpi(path + pathLength - filenameSuffixLength, filenameSuffix) == 0) { lstrcmpiW

I wasn't able to reproduce the scrolling break-age on a local build. Does anybody have any idea how to reproduce it?

(In reply to comment #115) > I wasn't able to reproduce the scrolling break-age on a local build. Does > anybody have any idea how to reproduce it? On a system that doesn't have an elantech touchpad the scroll wheel changes zoom with the patches applied.

Make the code Unicode-correct.

With mAssumeWheelIsZoomUntil initialized and the rounding check corrected (which was incorrect for reasons beside the signed-ness; i.e., it was using 0x8000000 instead of 0x80000000).

I wasn't able to reproduce the original scrolling problem in the end. I've fixed a bunch of problems with these patches and have sent them to the try server. The builds should be available in a couple of hours here: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-4f2be7d88eac> It would be great if people can test those builds to see if the mouse scroll wheel problem has been fixed. CCing some QA folks too.

This patch incorporates some other things that Cameron has fixed in his try push today <http://hg.mozilla.org/try/rev/eb5376d1b3cd>.

I also pushed another try server build with the original patches landed as part of this bug, for people to test for comparison: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-bcfa6ea99a85> If the new patches here are effective, the build in this comment should change the zoom on wheel scroll, and the build in comment 119 should not show that behavior.

I think the scrolling when unintended happens just due to mAssumeWheelIsZoomUntil being uninitialized. If it happened to start off with a high enough number in it, all mouse wheels would be processed as if control was pressed down. Thanks for driving the fixes to my patches, Ehsan!

(In reply to comment #119) > I wasn't able to reproduce the original scrolling problem in the end. I've > fixed a bunch of problems with these patches and have sent them to the try > server. The builds should be available in a couple of hours here: > > <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-4f2be7d88eac> I had an embarrassing typo in my patch which caused a build failure. Builds with the fix will hopefully be available here: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-6860167b3daf>

(In reply to comment #122) > I think the scrolling when unintended happens just due to > mAssumeWheelIsZoomUntil being uninitialized. If it happened to start off with > a high enough number in it, all mouse wheels would be processed as if control > was pressed down. > > Thanks for driving the fixes to my patches, Ehsan! That's what I suspect too. I'll do more testing tomorrow, and will also ask other people to test as well.

Ehsan, I tested all the builds you posted. All worked fine without breaking anything on my Elan Touchpad. However your last build <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-6860167b3daf> crashes at startup. Here is the log AvailableVirtualMemory: 4157898752 BuildID: 20110210220707 CrashTime: 1297443638 InstallTime: 1297443491 ProductName: Firefox SecondsSinceLastCrash: 133 StartupTime: 1297443638 SystemMemoryUsePercentage: 30 Throttleable: 1 TotalVirtualMemory: 4294836224 URL: Vendor: Mozilla Version: 4.0b12pre This report also contains technical information about the state of the application when it crashed.

(In reply to comment #125) > Ehsan, I tested all the builds you posted. All worked fine without breaking > anything on my Elan Touchpad. However your last build > <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-6860167b3daf> > crashes at startup. Hmm, does that happen every time for you? Can you try and get a stack trace please? <https://developer.mozilla.org/en/how_to_get_a_stacktrace_with_windbg> Instead of the ".sympath" line in that page, you would want to use this: .sympath SRV*c:\symbols*http://build.mozilla.org/tryserver-symbols/;SRV*c:\symbols*http://msdl.microsoft.com/download/symbols Thanks!

If I would help, I will drive to Fry's and buy some ASUS netbooks for testing. Let me know.

So far I've run the build in comment 121 (which should have the problem) and the build in comment 123 (which hopefully doesn't have the problem) about 100 times each. I have managed to reproduce the scroll wheel bug in the build from comment 121 only once, and never in the build in comment 123. I don't have any device with an Elantech touchpad... (In reply to comment #127) > If I would help, I will drive to Fry's and buy some ASUS netbooks for testing. > Let me know. That may be a good idea. I asked roc whether we have devices with Elantech touchpads, and he said that he thinks that we do, but he doesn't know where...

(In reply to comment #123) > (In reply to comment #119) > > I wasn't able to reproduce the original scrolling problem in the end. I've > > fixed a bunch of problems with these patches and have sent them to the try > > server. The builds should be available in a couple of hours here: > > > > <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-4f2be7d88eac> > > I had an embarrassing typo in my patch which caused a build failure. Builds > with the fix will hopefully be available here: > > <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-6860167b3daf> Working as expected here. Scrolling works fine. (As mentioned on irc I can't test the Elantech changes though.)

Comment on attachment 511608 [details] [diff] [review] Part 2: Ensure that Elantech driver helper window doesn't prevent z oom gestures from working (v2.1) >+static PRBool PointInWindow(HWND aHWND, const POINT& aPoint) >+ >+static HWND FindTopmostWindowAtPoint(HWND aHWND, const POINT& aPoint) >+ Neil made a good point in his review, couldn't we use ChildWindowFromPoint or RealChildWindowFromPoint in place of these?

(In reply to comment #126) > (In reply to comment #125) > > Ehsan, I tested all the builds you posted. All worked fine without breaking > > anything on my Elan Touchpad. However your last build > > <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-6860167b3daf> > > crashes at startup. > > Hmm, does that happen every time for you? Can you try and get a stack trace > please? <https://developer.mozilla.org/en/how_to_get_a_stacktrace_with_windbg> > > Instead of the ".sympath" line in that page, you would want to use this: > > .sympath > SRV*c:\symbols*http://build.mozilla.org/tryserver-symbols/;SRV*c:\symbols*http://msdl.microsoft.com/download/symbols > > Thanks! Crashes every time, also it won't download the symbols. *** ERROR: Symbol file could not be found. Defaulted to export symbols for ntdll.dll - ntdll!CsrSetPriorityClass+0x40: 00000000`76e80fb0 cc int 3 0:000> .sympath SRV*c:\symbols*SRV*c:\symbols*http://build.mozilla.org/tryserver-symbols/;SRV*c:\symbols*http://msdl.microsoft.com/download/symbols Symbol search path is: SRV*c:\symbols*SRV*c:\symbols*http://build.mozilla.org/tryserver-symbols/;SRV*c:\symbols*http://msdl.microsoft.com/download/symbols Expanded Symbol search path is: srv*c:\symbols*srv*c:\symbols*http://build.mozilla.org/tryserver-symbols/;srv*c:\symbols*http://msdl.microsoft.com/download/symbols

(In reply to comment #130) > Comment on attachment 511608 [details] [diff] [review] > Part 2: Ensure that Elantech driver helper window doesn't prevent z oom > gestures from working (v2.1) > > >+static PRBool PointInWindow(HWND aHWND, const POINT& aPoint) > >+ > >+static HWND FindTopmostWindowAtPoint(HWND aHWND, const POINT& aPoint) > >+ > > Neil made a good point in his review, couldn't we use ChildWindowFromPoint or > RealChildWindowFromPoint in place of these? Maybe we could, but I don't have a device with an Elantech touchpad, so I wanted to change as little possible to avoid the risk of breaking the fix. We could file a followup to investigate that.

So, in order to test the effect of mAssumeWheelIsZoomUntil being uninitialized, I instrumented a build to initialize its value to 0x3dcdcdcd, and with that, I get the zoom on mouse wheel bug. Therefore, I'm fairly sure that the recent version of Part 4 has fixed that issue.

No scrolling issues here with the latest tryserver build and a fresh profile.

Comment on attachment 511608 [details] [diff] [review] Part 2: Ensure that Elantech driver helper window doesn't prevent z oom gestures from working (v2.1) r+'ing for now on the assumption we'll file a follow up for more cleanup.

Recall: I've submitted another try build for you. It should be available in an hour or so: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-cc2545084754> Can you please try it to see if you see the startup crash with this one too?

(In reply to comment #135) > Comment on attachment 511608 [details] [diff] [review] > Part 2: Ensure that Elantech driver helper window doesn't prevent z oom > gestures from working (v2.1) > > r+'ing for now on the assumption we'll file a follow up for more cleanup. Filed bug 633572 for that.

I got a device with an Elantech touchpad, and I can reproduce the startup crash in the build in comment 123. Investigating...

(In reply to comment #136) > Recall: I've submitted another try build for you. It should be available in an > hour or so: > <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-cc2545084754> > > Can you please try it to see if you see the startup crash with this one too? Ok will do, will post back with results. I notice you have been able to reproduce the error.

Comment on attachment 510545 [details] [diff] [review] Part 1: Add a hack for the Elantech touchpad so that three-finger s wipe left/right performs a browser back/forward action instead of page up/down (v2.2) I think I found the cause of the crash. >+GetRegistryKey(HKEY aRoot, PRUnichar* aKeyName, PRUnichar* aValueName, PRUnichar* aBuffer, DWORD aBufferLength) The GetRegistryKey receives PRUnichar* as a pointer to a two byte array. aBufferLength is in bytes, which means that in this particular case, it's 80 (because |buf|'s size is 40.) >+{ >+ if (!aKeyName) { >+ return PR_FALSE; >+ } >+ > HKEY key; >- LONG result = ::RegOpenKeyExW(HKEY_LOCAL_MACHINE, >- L"Software\\Synaptics\\SynTP\\Install", 0, KEY_READ, &key); >+ LONG result = ::RegOpenKeyExW(aRoot, aKeyName, NULL, KEY_READ, &key); > if (result != ERROR_SUCCESS) > return PR_FALSE; > DWORD type; >- PRUnichar buf[40]; >- DWORD buflen = sizeof(buf); >- result = ::RegQueryValueExW(key, L"DriverVersion", NULL, &type, (BYTE*)buf, &buflen); >+ result = ::RegQueryValueExW(key, aValueName, NULL, &type, (BYTE*) aBuffer, &aBufferLength); > ::RegCloseKey(key); > if (result != ERROR_SUCCESS || type != REG_SZ) > return PR_FALSE; >- buf[NS_ARRAY_LENGTH(buf) - 1] = 0; >+ if (aBuffer) >+ aBuffer[aBufferLength - 1] = 0; Here, we're adding (aBufferLength - 1) to a PRUnichar* pointer. That is wrong, since aBufferLength is twice the number of items in the array. So, we end up writing to somewhere down the stack, which happens to be the top two bytes of the nsIWidget* we're trying to get from do_CreateInstance call used to create the first nsWindow object, so the QueryInterface calls ends up crashing. I'm going to have a fix soon.

Ehsan, did you cancel your build as it says nothing there?

(In reply to comment #142) > Ehsan, did you cancel your build as it says nothing there? I've pushed another build which should have fixed the crash problem. It will be available here: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-71a52bcc3e05>. It would be great if you can test it. It will be finished in a few hours I guess. Thanks!

Trybuild from Comment 143 works great with Elantech Touchpad (driver 7.0.6.4). Scrolling and zooming work both very smooth. No crash at all, tried for an hour. Can't test 8.x.x.x drivers, as they don't work with my touchpad.

(In reply to comment #143) > (In reply to comment #142) > > Ehsan, did you cancel your build as it says nothing there? > > I've pushed another build which should have fixed the crash problem. It will > be available here: > <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/eakhgari@mozilla.com-71a52bcc3e05>. > > It would be great if you can test it. It will be finished in a few hours I > guess. > > Thanks! Works fine no crashes with either 7.0.5.10 or 8.0.5.0

Thanks everyone for testing this. My local tests also show that the gestures are working correctly.

Will this make the next beta?

(In reply to comment #147) > Will this make the next beta? Yes, it will. :-)

http://hg.mozilla.org/mozilla-central/rev/720d17e28023 http://hg.mozilla.org/mozilla-central/rev/ace573ee5421 http://hg.mozilla.org/mozilla-central/rev/fa7c98cbc339 http://hg.mozilla.org/mozilla-central/rev/e9899b77c6ea

It would be really good if you guys could test tomorrow's nightly builds to make sure that this problem is indeed fixed. Thanks!

Works great in latest nightly! Scrolling and Zooming works as it should on my Elantech Touchpad. No crash at all. Also tried the latest Nightly on my Desktop PC. TThere are also no issues at all with crashes or scrolling with latest nightly. Thumbs up!

I find no problems with the nightly on the Eee PC I've got here; pinch-to-zoom and three-finger swipe back/forward both work well, and without crashing in the few minutes I've used it for. This is with both v7 and v8 of the driver.

Well done guys!

Works fine here, thanks for all the hard work guys!

Given the amount of positive feedback lets mark this bug as verified fixed.

Hello, I'm getting this bug with the Firefox 4.0 that was released today. I have the Elantech 7.0.3.12 drivers, Windows XP, Asus EEE PC 1000HE. The three-finger swipe does PageUp/PageDown, not Back/Forward

junimitsu, if you are able, can you try a more recent driver, say 7.0.5 of later? I don't think I tried this earlier version of the driver, and if upgrading helps you, I think we can avoid adding further hacks. If it doesn't, can you please file a new bug for this (and CC me)?

Yep, upgrading to the 7.0.5.16 driver fixed my issue. Thanks!

It looks like the hack used to solve these swipe issues has spawned a new problem with the Page Up/Down keys. Check the bug report and fix: https://bugzilla.mozilla.org/show_bug.cgi?id=640587#c48 The issue is being reported to be solved by setting the "ui.elantech_gesture_hacks.enabled" option to 0. I have the latest ElanTech driver Asus has for my laptop (7.0.5.12) and cannot use the three finger swipes described in this bug report, no matter the value of "elantech_gesture_hacks" nor how many times I restart Firefox, so I am not sure if the hack is still needed Can someone try to verify if having the hack set to -1 is still required to prevent the three finger gesture issues? If the latest ElanTech drivers fix the problem by themselves, I'd recommend disabling the hack by default.