494281 - Touch panning no longer works as of 5/20/2009

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Joe H]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090521 Minefield/3.6a1pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090521 Minefield/3.6a1pre (.NET CLR 3.5.30729)

Basically, with yesterday's auto-update, Windows 7 native touch panning stopped working.  It had been perfectly with every nightly build before that.

(If you're wondering what touch panning is, it's sort of like Grab and Drag, but built into Windows, and it only works when using touch, not when using the mouse.)

Reproducible: Always

Steps to Reproduce:
As long as you have a touchscreen computer / tablet that supports Windows 7 touch panning (like my Dell XT does), try and drag with your finger.
Actual Results:  
Text will end up being selected.

Expected Results:  
The page should scroll up and down.

Comment 1

[Marcia Knous [:marcia]]

Joe: Does it work using the RC3 builds on this page - http://www.mozilla.com/en-US/firefox/all-beta.html?

Unfortunately I don't have a touchscreen tablet that I can test this with?

Comment 2

[Joe H]

I've been using the 3.5 betas full time since noticing the issue and it does work properly in the newest FF 3.5 rc3; in fact it worked all alone in 3.5_ just not the newer Minefield builds.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

I'm checking this problem and the panning stopped working after bug 485101 landed. Debugging it we found that the main problem is that nsWinGesture::ShutdownLibrary is being called in the startup process, and with that we clear the pointer for the getGestureInfo function (from the Windows library), so the code no longer can interpret which kind of gesture we got at a WM_GESTURE message.

After commenting out the code inside ShutdownLibrary the panning works again (although it behaves weirdly in various scrollable places such as listviews), but I'm not sure yet why ShutdownLibrary is being called at all during startup. I imagine that some temporary window created during the process is being destroyed and thus calling it, but later windows does not call InitLibrary again so we lose the function pointer forever.

Comment 4

[Jim Mathies [:jimm]]

(In reply to comment #3)
> I'm checking this problem and the panning stopped working after bug 485101
> landed. Debugging it we found that the main problem is that
> nsWinGesture::ShutdownLibrary is being called in the startup process, and with
> that we clear the pointer for the getGestureInfo function (from the Windows
> library), so the code no longer can interpret which kind of gesture we got at a
> WM_GESTURE message.
> 
> After commenting out the code inside ShutdownLibrary the panning works again
> (although it behaves weirdly in various scrollable places such as listviews),
> but I'm not sure yet why ShutdownLibrary is being called at all during startup.
> I imagine that some temporary window created during the process is being
> destroyed and thus calling it, but later windows does not call InitLibrary
> again so we lose the function pointer forever.

That's a bug! The shutdown code should be removed. It's called on the nsWinGesture destructor, so when a window is destroyed, the library would be shut down. getGestureInfo and beginPanningFeedback are statics, so that shuts things down for all windows. Gesture input would be reinitialized the next time you open a new window (and shut down again if you closed it.)

We should definitely get a patch together and get it in with the .1 release which hopefully will be out before the end of october!

Felipe do you want to work something up?

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch rev1

Hey Jim, is this how I should fix it, or is there anything that I missed?

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> Created an attachment (id=386627) [details]
> Patch rev1
> 
> Hey Jim, is this how I should fix it, or is there anything that I missed?

This seems fine, did you test it, confirm the bug and confirm this addresses the issue? I'm not a peer for this module, so you'll need to get an r+ from someone that is.

http://www.mozilla.org/owners.html#widget-windows

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 386627 [details] [diff] [review]
Patch rev1

Could you also take a look on this one?

Comment 8

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/f32ffc825792

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 386627 [details] [diff] [review]
Patch rev1

Requesting 1.9.1.2 approval since we definitely want this before win7 comes out.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 386627 [details] [diff] [review]
Patch rev1

Jim: does this affect the mozilla-1.9.1 branch? If so, please renominate for 1.9.1.3

Comment 11

[Robert Strong (they/them - no direct email)]

It doesn't affect the mozilla-1.9.1 branch. It will be necessary to land this on mozilla-1.9.1 if bug 485101 lands on mozilla-1.9.1

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 386627 [details] [diff] [review]
Patch rev1

Actually, this particular patch does affect 1.9.1 as it addresses a bug my original implementation that caused touch input to shut down inadvertently. It's not critical since 1.9.2 is the official win7 compat release but it wouldn't hurt to get this out there so people who are running 1.9.1 on touch input displays can play with it.

Comment 13

[Jim Mathies [:jimm]]

(I should clarify, as rob pointed out, it relies on bug 485101 landing first. We've nom'd that one for 1.9.1.3 so it'd be great if we could land this right on top of it.)