Open Bug 725475 Opened 9 years ago Updated 8 years ago

some Synaptics drivers direct scrolling to visible plugin regardless of focus or mouse position

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

People

(Reporter: heycam, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

Attract scrolling messages from certain Synaptics drivers when plugins are visible (v1)
6.46 KB, patch
jimm
: review+
Details | Diff | Splinter Review
Bump the version detection for Synaptics drivers that need the scrolling workarounds.
1.08 KB, patch
Details | Diff | Splinter Review 
Followup for bug 626813.  We should try to find a way to convince the Synaptics driver to send scrolling messages to our outer browser window instead of, for example, the scroll bars within the Adobe Reader plugin.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Adding this extra hidden scroll bar seems to coax scrolling messages to our window instead of a plugin.  (I did not manage to delve deep enough into the trackpad driver code to determine exactly what heuristics are being used to select the plugin.)  The only problem is that sometimes when scrolling (especially if I do it quickly) I will run into this assertion:

ASSERTION: Received "nonqueued" message 277 during a synchronous IPC message for Window xxxx ("MozillaWindowClass"), sending it to DefWindowProc instead of the normal window procedure.

which is ipc/glue/WindowsMessageLoop.cpp:329, message 277 being WM_VSCROLL.

Sometimes I also then get a few in a row identifying various different window classes for windows we have (like "MozillaHiddenWindowClass" and "nsAppShell:EventWindowClass") with message 49544 which according to Spy++ is a registered message "PwrMgrBkGndUpdateBatteryRemainingTime".  I'm guessing that's some Thinkpad thing sending that message to all windows, but happening to cause this assertion because we're showing the assertion dialog?

Anyway, I don't understand what ipc/glue/WindowsMessageLoop.cpp is for: is it for messages bubbled up from the plugin to the browser's process?  I'm not really sure what the assertion means, or whether I should just be adding a case in that message loop's switch to pass it on to the window.  Jim, can you explain?
With this patch we can also remove the ::ShowWindow() calls that were added in bug 626813.
jimm ^
Tested on Win7 x64, Synaptics driver v15.2.20.0, nppdf32.dll v9.5.0.270:
- fg window moves correctly, even with pdf open in bg and embedded pdf in bg
- fg window with embedded pdf, only the pdf moves, no way to move the HTML page with touchpad (tested on http://people.mozilla.org/~cmccormack/misc/pdf.html)

Thanks for your efforts on this!
Danilo
Danilo, thanks for testing.  The issue with scrolling when you have the page with the PDF in the foreground tab could be that focus is not being set away from the plugin.  If the plugin has focus, then the trackpad driver is going to send it the scrolling messages.  Clicking on the page background does not take away focus from the plugin (should it? I am not sure).  If you click on say the location bar, then use the trackpad to scroll, does the page scroll or the plugin?  On my machine, that will cause the page to scroll.
Yeah, forgot to mention, but I did already test this:
- restart in safe mode
- focus the location bar -> touchpad scrolls pdf
- tab -> focus to the search bar -> touchpad scrolls pdf
- tab -> focus to the page -> touchpad scrolls pdf
- tab -> focus to the tab "tab" -> touchpad scrolls pdf
- select some text on page -> touchpad scrolls pdf

Truth be said, this is the same behaviour I also experience viewing your page with IE 9.0.8112.16421.
Re comment 6, it seems to me that if I click outside the plugin, I want that plugin to lose focus.  This is similar to how if I click outside of a text box and scroll, the page scrolls and not the text box.  (Well, most of the time -- if the mouse hovers over the text box then the text box scrolls...)  Otherwise, if I understand you correctly, it would be impossible to scroll a page containing a pdf with the mousewheel, correct?  There aren't too many pages that do that, but for instance bank pages that show your statement in a pdf with other information around it...

Thanks for your work on this -- I'm looking forward to it landing :)
Danilo, since you said you're using v15.2.x.y of the Synaptics driver, and all of the Synaptics hacks we have currently apply automatically only for versions <= 15.0.x.y, so you'll need to force it on.  Try going to about:config and setting  ui.trackpoint_hack.enabled from -1 (the default) to 1 (which should force it on), then restart the browser.
Ok, now it works as intended!

How it now works:
- fg html page with bg pdf -> fg html scrolls
- fg pdf -> pdf scrolls
- fg html page with embedded pdf (follow steps in order):
   . focus location bar -> html page scrolls
   . focus search bar, etc. -> html page scrolls
   . select page text -> html page scrolls
   . hover emb pdf area -> nothing scrolls (I suppose that now the acrobat plugin *toolbars* have focus)
   . click acrobat plugin scrollbars/pdf page -> pdf scrolls
   . hover *off* emb pdf area -> pdf scrolls
   . click anything outside pdf (html page, location bar, etc) -> html page scrolls

Only one weird thing left:
- open new tab
- go to http://people.mozilla.org/~cmccormack/misc/pdf.html
- do *not* hover mouse over emb pdf area -> pdf scrolls
- hover mouse over emb pdf area -> pdf scrolls
- click html page -> pdf scrolls
- select text on html page -> pdf scrolls
- move html page vertical scrollbar down/up -> pdf scrolls
- focus location bar -> html page scrolls
- from here on, "How it now works" behaviour resumes
Cam, are you ready to kick off review?
(In reply to Jim Mathies [:jimm] from comment #11)
> Cam, are you ready to kick off review?

I should fix the assertions I trigger in comment 1.  Can you suggest how to do that?
> Anyway, I don't understand what ipc/glue/WindowsMessageLoop.cpp is for: is
> it for messages bubbled up from the plugin to the browser's process?  I'm
> not really sure what the assertion means, or whether I should just be adding
> a case in that message loop's switch to pass it on to the window.  Jim, can
> you explain?

It's a trap for messages that might trigger ipc events. Our ipc protocol requires that when a process is waiting on a response to a sent message, it can't send another message over the wire. So whenever a process is in this situation, it sets a hook that traps and defers any incoming windowing messages until a response from the other process is received.

If your patch is triggering the non-queued warning, you should add a message handler in WindowsMessageLoop for whatever event is being received. The warning indicates the WindowsMessageLoop hook doesn't know how to handle the message.

There are a number of handlers already defined, WM_VSCROLL looks like it can use the standard DeferredSendMessage message wrapper - 

http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#203
Thanks, that seems to work.  I added a case there for WM_KEYDOWN, too, as I noticed that was sometimes being sent when doing horizontal trackpad scrolling.  I didn't get WM_KEYUP -- I suppose it got sent while we're not doing a synchronous IPC message -- but I added it too just in case.  (Otherwise we might not track key down/up properly.)
Attachment #595947 - Attachment is obsolete: true
Attachment #602577 - Flags: review?(jmathies)
Maybe we should bump the version check for the Synaptics driver to include v15.2.0.0 as well.
OK, might need to do some rebasing then.
Attachment #602577 - Flags: review?(jmathies) → review+
Since Danilo in comment 5 is reporting the need for the hack with v15.2.x.y of the driver.
Attachment #603945 - Flags: review?(masayuki)
Cameron:

I checked with the landed patch and attachment 603945 [details] [diff] [review].

However, if I load PDF in background tab (either http://www.unicode.org/charts/PDF/U0A00.pdf or http://people.mozilla.org/~cmccormack/misc/pdf.html), I cannot scroll any normal web page in foreground tab (I tested with latest Synaptics driver, http://www.synaptics.com/node/55). And then, I can see a lot of warnings in console: WARNING: Not yet implemented!: file c:/Users/toybox/mozilla/mc-a/src/dom/plugins/ipc/PluginModuleChild.cpp, line 1285

So, I think you should back out the patch.

And also the latest version of the driver is 15.2.20, so, it doesn't make sense we hardcode the latest version check. I think that we don't need the version check until the driver will fix this bug or the max version should be in prefs.
OK, that's unfortunate!  I will back out.  I've tested locally just with 15.0.18.0 (on Vista), and scrolling works fine in whether the PDF is in the foreground or background tab.  I don't get the PluginModuleChild assertions either.

I will try updating to 15.2.20 locally and see if I get the same problems as you.

If you think we should remove the version check altogether, I can do that.

I think we need to talk to someone at Synaptics to let them know what the best way they can detect our window and send scrolling messages to us is.  I don't think the current hacking and reverse engineering is really sustainable.
(In reply to Cameron McCormack (:heycam) from comment #21)
> OK, that's unfortunate!  I will back out.  I've tested locally just with
> 15.0.18.0 (on Vista), and scrolling works fine in whether the PDF is in the
> foreground or background tab.  I don't get the PluginModuleChild assertions
> either.

Oh, really? The pymake or something might have failed to rebiuld. I'm going to rebuild it cleanly.

> If you think we should remove the version check altogether, I can do that.

Please do it.

> I think we need to talk to someone at Synaptics to let them know what the
> best way they can detect our window and send scrolling messages to us is.  I
> don't think the current hacking and reverse engineering is really
> sustainable.

The driver seems to target the window under the mouse cursor rather than focused window. It's same as our behavior. So, if they find a window of us (except GeckoPluginWindow) in the ancestors of a window (or itself) under mouse cursor, they should send the message to the found our window. If so, we redirect the message to the descendant most window under the cursor *when* it's necessary.

And also, they send WM_KEYDOWN of arrow keys for horizontal scrolling. It's too bad for us because it causes dispatching DOM key events rather than DOM wheel events. So, they should use WM_MOUSEHWHEEL message for horizontal scrolling. If they're using SendInput() it's supported on Vista or latter only. Otherwise, e.g., using SendMessage(), it can be used on XP either. We're handling WM_MOUSEHWEEL on all versions of Windows.
And maybe, they might not realize yet, WM_MOUSEWHEEL and WM_MOUSEHWHEEL are not command for scrolling. They're just notification of mouse wheel. E.g., game may use it for non-scrolling action. Something like Google Map use it for zooming in/out. So, basically, scrollbar check doesn't make sense...
I built a clean debug build, but it still prints a lot of warnings.
Are they the mozilla::plugins::child::_status warning?  There might be a difference between the PDF plugins we are using.  I have no idea whether it's reasonable that mozilla::plugins::child::_status reports that it is not yet implemented, or what its behaviour might be if were implemented.
A thing I noticed from the previous bug (626813):

Adobe and Nitro PDF plug-ins showed this problem, but Foxit's plugin (for whatever reason) didn't show it (you could scroll using the touchpad even if a PDF was open in another tab). I emailed Foxit and I was told "Ours is a windowed' s OCX plugin." But, that discussion didn't move any further.
Sorry to "resurrect":

I'm on Windows 8 Consumer Preview (NOT Release Preview...yet, hehe) x64 with Synaptics 15.0.24 and Firefox 12.0 and THE ISSUE IS GONE. 

I don't know if this is normal, since the original bug 626813 was not scheduled to land until FF 13.
Unassigning myself since I haven't looked at Synaptics issues for a while.
Assignee: cam → nobody
Status: ASSIGNED → NEW
Attachment #603945 - Flags: review?(masayuki)
You need to log in before you can comment on or make changes to this bug.