Closed Bug 483136 Opened 11 years ago Closed 1 year ago

mouse wheel scrolling stops when mouse cursor is over a plugin (flash (youtube), silverlight)

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
blocking2.0 --- -

People

(Reporter: marun2, Assigned: masayuki)

References

(Blocks 1 open bug, )

Details

(Keywords: parity-chrome, Whiteboard: [see comments 204, 207] [platform-rel-Youtube][tpi:+])

Attachments

(1 file, 32 obsolete files)

87.37 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre

In this page start scrolling when the mouse cursor is over the Youtube video the page stops scrolling - to scroll use mouse scroll wheel not the Windows key equivalent (the method I used)

Reproducible: Always

Steps to Reproduce:
1. Go to site http://blog.ppcproz.com/2009/03/what-is-google-ad-auction.html
2. Make sure mouse cursor is in the text (no need to click on the text)
3. Scroll down using mouse scroll wheel
4. Wait until mouse cursor is over the Youtube video
5. Try scrolling down now
Actual Results:  
Scroll doesnt happen

Expected Results:  
Scrolling should not be interrupted by web page objects because scrolling is a function of the browser, and the site should not be able to do anything to modify or alter the standard scrolling behavior
Confirmed on Windows XP; this is a regression between Firefox 2 and 3.
Component: General → Layout
Keywords: regression
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → Trunk
Well, this seems caused by Bug 333166.
Blocks: 333166
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Some events should go to the plugin and based on bug 333166 i think this is working as expected
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Flags: wanted1.9.2?
(In reply to comment #3)
> Some events should go to the plugin and based on bug 333166 i think this is
> working as expected
Yes. Flash Player should forward the messages if it isn't interested in the wheel events.
That being said, we could stop passing the wheel messages to plug-ins if a wheel trnsaction is in progress. Nakano-san?
Same issue occurs here also

http://www.signaturecommunities.com/success-in-naples.html

The flash is big and occupies the entire mousable space, thus interfering with standard browser behavior. I expect the page to scroll normally and any flash content cannot be taking control of those standard mouse events which are function of the browser not hand over the control to the website or any flash content on it.
Same here 
http://www.houghi.org/tomtom/tomtom.php
This page has embedded Youtube.
but what is strange here, is that, in IE7, scrolling doesnt stop, even when mouse cursor is over the video itself. But in same IE7, scrolling wont happen here http://blog.ppcproz.com/2009/03/what-is-google-ad-auction.html
very strange
If this is indeed a regression between Firefox 2 and 3 with the same Flash Player plug-in (say, use the latest released Flash Player), then this should be a Firefox bug.
Duplicate of this bug: 407500
Severity: trivial → minor
Last I debugged a case like this, the plugin API involves the plugin telling the browser whether it consumed the event, and I recall flash always returning that it did consume the event.  But that was a while ago, and my memory could be wrong.
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
So basically, its not just YouTube, but its flash in general that does this.  
The Plug-In steals scrolling away from the browser even by hovering over and flash hasn't been clicked on.

I noticed it also occurs on a page with flash being used to display images, where no animation or other controls existed.  

If I stop the plugin from loading Youtube video, I have to click back on the page's white space or scrollbar to scroll again.

Maybe plugins shouldn't get automatic focus on hover over, since to interact with them, you have to click on something regardless.  And why would I want to scroll a flash video before I click on it to control it?  It makes more sense for WebGL demos to do hover/mouse event focus than flash.
Here is a different case where this page's flash content starts with focus and there is not much to scroll for the page up and down.  

Hovering does scroll the flash content sideways immediately on page load using the mouse scroll wheel: http://wonderwall.msn.com/#m=OyFjJSHIkMY

Yet when it hits the edge of the flash scroll area, it goes back to scrolling the browser window and the page shifts down, even while hovered.

Which this is not the same behavior as typical YouTube content.
I tested Silverlight and found scrolling stops too.
http://silverlight.net/learn/videos/silverlight-videos/building-a-skinnable-custom-control-part-1/
Summary: Scrolling stops when mouse cursor is over Youtube video → Scrolling stops when mouse cursor is over a plugin (flash (youtube), silverlight)
Duplicate of this bug: 518536
FYI to anyone fixing this: Bug 78714 discusses plugins having focus (including flash) and taking over keyboard events, but what is more noteworthy is the hundreds of comments and suggested behavioral changes on how FF should interact with the plugins in general, including this issue.
(In reply to comment #14)
Oops, sorry for the bugspam, that should be bug 78414.
Depends on: 497949
It appears that Firefox gives control to Flash on the mouse over even though the instance of flash does not have focus yet.  This doesn't happen on Mac nor on IE for Windows.  I don't think this is a Flash bug.
Duplicate of this bug: 524200
Confirmed on Windows 7 x86
Confirmed on Windows 7 x64
Duplicate of this bug: 536403
(In reply to comment #4)
> (In reply to comment #3)
> > Some events should go to the plugin and based on bug 333166 i think this is
> > working as expected
> Yes. Flash Player should forward the messages if it isn't interested in the
> wheel events.
> That being said, we could stop passing the wheel messages to plug-ins if a
> wheel trnsaction is in progress. Nakano-san?

Oops, sorry, I missed your comment.

I'm looking the code and trying to write a patch. Probably, we're in a wheel transaction, we shouldn't retarget the wheel event to plug-ins.

I have a strange code in nsWindow::HandleScrollingPlugins():

> 6076   nsWindow* destWindow = GetNSWindowPtr(destWnd);
> 6077   if (!destWindow || destWindow->mWindowType == eWindowType_plugin) {
...> 6087     HWND parentWnd = ::GetParent(destWnd);
> 6088     while (parentWnd) {
> 6089       nsWindow* parentWindow = GetNSWindowPtr(parentWnd);
> 6090       if (parentWindow) {
...
> 6096         if (mInScrollProcessing) {
> 6097           destWnd = parentWnd;
> 6098           destWindow = parentWindow;
> 6099         } else {
...
> 6104           mInScrollProcessing = PR_TRUE;
> 6105           if (0 == ::SendMessageW(destWnd, aMsg, aWParam, aLParam))
> 6106             aHandled = PR_TRUE;
> 6107           destWnd = nsnull;
> 6108           mInScrollProcessing = PR_FALSE;
> 6109         }
> 6110         return PR_FALSE; // break, but continue processing

Isn't this wrong? If mInScrollProcessing is TRUE, we shouldn't return here...
> I have a strange code in nsWindow::HandleScrollingPlugins():

s/have/find
Attached patch Patch v1.0 (obsolete) — Splinter Review
O.K., the most cases are our simple bug like I pointed in comment 21.

The wheel transaction principle may be good thing on widget level too, however, I don't have good idea to implement it simply.

I hope that this patch can be landed on 1.9.2.x.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #421777 - Flags: review?(VYV03354)
Attached patch Patch v1.0 (obsolete) — Splinter Review
Oops, the right patch is here.
Attachment #421777 - Attachment is obsolete: true
Attachment #421777 - Flags: review?(VYV03354)
changing the component and re-requesting wanted1.9.2. However, it's too late for 1.9.2.0. I'd like the patch to be landed 1.9.2.1 or later.
Component: Layout → Widget: Win32
Flags: wanted1.9.2- → wanted1.9.2?
QA Contact: layout → win32
This bug is a regression of bug 487245.
Blocks: 487245
No longer blocks: 333166
(In reply to comment #26)
> This bug is a regression of bug 487245.

Are you sure? Bug 487245 is after this bug was filed.
(In reply to comment #27)
> (In reply to comment #26)
> > This bug is a regression of bug 487245.
> 
> Are you sure? Bug 487245 is after this bug was filed.

Oh, right. But the patch just fix a bug of bug 487245's patch. Even if the flash player (or other plug-ins) doesn't eat the native mouse wheel events, we don't handle them after bug 487245. So, if the old version of flash player eat the message always, this bug can be reproduced before bug 487245 (i.e., regressed by another bug).
(In reply to comment #26)
> This bug is a regression of bug 487245.

I think you meant bug 507222. Bug 487245 was a code cleanup bug.
(In reply to comment #29)
> (In reply to comment #26)
> > This bug is a regression of bug 487245.
> 
> I think you meant bug 507222. Bug 487245 was a code cleanup bug.

Actually looks like that just moved the code around and did some cleanup as well. Not sure where this regressed from.
The cleaned up code is here:

>   4.5581 +    while (parentWnd) {
>   4.5582 +      nsWindow* parentWindow = GetNSWindowPtr(parentWnd);
>   4.5583 +      if (parentWindow) {
>   4.5584 +        // We have a child window - quite possibly a plugin window.
>   4.5585 +        // However, not all plugins are created equal - some will handle this message themselves,
>   4.5586 +        // some will forward directly back to us, while others will call DefWndProc, which
>   4.5587 +        // itself still forwards back to us.
>   4.5588 +        // So if we have sent it once, we need to handle it ourself.
>   4.5589 +        if (mInWheelProcessing) {
>   4.5590 +          destWnd = parentWnd;
>   4.5591 +          destWindow = parentWindow;
>   4.5592 +        } else {
>   4.5593 +          // First time we have seen this message.
>   4.5594 +          // Call the child - either it will consume it, or
>   4.5595 +          // it will wind it's way back to us, triggering the destWnd case above.
>   4.5596 +          // either way, when the call returns, we are all done with the message,
>   4.5597 +          mInWheelProcessing = PR_TRUE;
>   4.5598 +          if (0 == ::SendMessageW(destWnd, msg, wParam, lParam)) {
>   4.5599 +            result = PR_TRUE; // consumed - don't call DefWndProc
>   4.5600 +          }
>   4.5601 +          destWnd = nsnull;
>   4.5602 +          mInWheelProcessing = PR_FALSE;
>   4.5603 +        }
>   4.5604 +        return PR_FALSE; // break; // stop parent search
>   4.5605 +      }

The original code is here:

>   4.7373 -          while (parentWnd) {
>   4.7374 -            nsWindow* parentWindow = GetNSWindowPtr(parentWnd);
>   4.7375 -            if (parentWindow) {
>   4.7376 -              // We have a child window - quite possibly a plugin window.
>   4.7377 -              // However, not all plugins are created equal - some will handle this message themselves,
>   4.7378 -              // some will forward directly back to us, while others will call DefWndProc, which
>   4.7379 -              // itself still forwards back to us.
>   4.7380 -              // So if we have sent it once, we need to handle it ourself.
>   4.7381 -              if (mIsInMouseWheelProcessing) {
>   4.7382 -                destWnd = parentWnd;
>   4.7383 -                destWindow = parentWindow;
>   4.7384 -              } else {
>   4.7385 -                // First time we have seen this message.
>   4.7386 -                // Call the child - either it will consume it, or
>   4.7387 -                // it will wind it's way back to us, triggering the destWnd case above.
>   4.7388 -                // either way, when the call returns, we are all done with the message,
>   4.7389 -                mIsInMouseWheelProcessing = PR_TRUE;
>   4.7390 -                if (0 == ::SendMessageW(destWnd, msg, wParam, lParam)) {
>   4.7391 -                  result = PR_TRUE; // consumed - don't call DefWndProc
>   4.7392 -                }
>   4.7393 -                destWnd = nsnull;
>   4.7394 -                mIsInMouseWheelProcessing = PR_FALSE;
>   4.7395 -              }
>   4.7396 -              break; // stop parent search
>   4.7397 -            }

You had a mistake. You moved the code from case statement of ProcessMessage() to a new method. At that time, you need to replace some |break| to |return|. However, here is in while block, so, it should be still |break|.

I guess that if the mistake isn't there, this bug was already fixed by another bug or marked as WFM.
Status: ASSIGNED → NEW
Component: Widget: Win32 → Layout
Flags: wanted1.9.2?
Component: Layout → Widget: Win32
Flags: wanted1.9.2?
Attachment #421778 - Flags: review?(VYV03354)
Status: NEW → ASSIGNED
(In reply to comment #32)
> https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v.1.0/
> 
> Test build is here.

Hi, sorry. This fixes most instances of the bug, including youtube.com, and embedded videos, but for some reason, embedded youtube videos still stop the scrolling.
(In reply to comment #33)
> but for some reason, embedded youtube videos still stop the
> scrolling.

Please explain the detail. Where can I reproduce it? What are the steps to reproduce it?

Note that if the plug-in eats WM_MOUSEWHEEL and WM_MOUSEHWHEEL messages, this patch cannot fix the problem (maybe, comment 11's site is so). And also the case when the focus has focus is out of scope of this bug (see bug 497949).
(In reply to comment #34)
> And also the case when the focus has focus is out of scope of this bug (see bug 497949).

oops, I meant:

the case when a plug-in has focus is out of scope of this bug.
(In reply to comment #34)
> Please explain the detail. Where can I reproduce it? What are the steps to
> reproduce it?
> 
> Note that if the plug-in eats WM_MOUSEWHEEL and WM_MOUSEHWHEEL messages, this
> patch cannot fix the problem (maybe, comment 11's site is so). And also the
> case when the focus has focus is out of scope of this bug (see bug 497949).

Not entirely sure whether or not this falls into the category of bug 497949, as I'm not sure whether that plugin can cause this. You can reproduce it on http://durian.blender.org/, scroll down until your mouse is over the youtube video, and scrolling stops.
Kyle, can you dissect this?
Blocks: 507222
No longer blocks: 487245
Attachment #421778 - Flags: review?(me)
(In reply to comment #36)
> Not entirely sure whether or not this falls into the category of bug 497949, as
> I'm not sure whether that plugin can cause this. You can reproduce it on
> http://durian.blender.org/, scroll down until your mouse is over the youtube
> video, and scrolling stops.

I cannot reproduce it on the page with IntelliPoint 7.0. What's your mouse and driver?
(In reply to comment #38)
> (In reply to comment #36)
> I cannot reproduce it on the page with IntelliPoint 7.0. What's your mouse and
> driver?

Logitech MX1100, driver version 4.83.5
O.K. I can reproduce it with MX1100. I'm checking the cause.
(In reply to comment #40)
> O.K. I can reproduce it with MX1100. I'm checking the cause.

Thanks guys, this is really awesome of you. This bug's been driving me and a lot of other people crazy, so thanks a ton.
Attachment #421778 - Flags: review?(me)
Attachment #421778 - Flags: review?(VYV03354)
This is a regression from http://hg.mozilla.org/mozilla-central/rev/1aecdc720018 (Bug 487245)

The substantive change the patch makes is that when the plugin has forwarded the message back to our window we would actually forward the message to the parent window instead of dropping it.  That revision turned a return into a break.
Blocks: 487245
No longer blocks: 507222
Yeah, basically what Masayuki said in comment 31 is accurate.  Guess I should read the entire thread first :-D

Patch looks good to me but I'm not a peer.

Are we sure the remaining issues aren't Bug 207256?
I'm still not sure how Bug 487245 can be the problem when it was filed after this bug ... my guess is that the initial problems were bug 207256 but that by not duping this we managed to catch this other bug.  A little bizarre.
https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v.1.0/bug483136_v.1.0-win32.zip
This build is still showing the issue. I tested the URL in the URL field.
I'm using a Microsoft Notebook Optical mouse with one of Windows' Plug and Play drivers. HID compliant mouse v 6.0.6001.18000.
Duplicate of this bug: 539634
Ugh... There are a lot of problems in HandleScrollingPlugins().

1. |if (0 == ::SendMessageW(destWnd, aMsg, aWParam, aLParam))| is wrong if aMsg is WM_MOUSEHWHEEL.

2. mInScrollProcessing isn't referred as expected when the caller isn't the parent of the window.

3. The message will be sent to a window under mouse cursor. However, if the message will not be sent to us (its parent window), we don't handle the message.

4. Calling ProcessMessage() might create infinite loop.
Flags: wanted1.9.2?
Attached patch Patch v3.1 (obsolete) — Splinter Review
I'm testing this...
Attachment #421778 - Attachment is obsolete: true
And I found an accessibility problem.

If we handle wheel message when we receive it always, even if Flash applications process the wheel event, the page is scrolled by my patch. I think Flash applications cannot prevent the default behavior. Ideally, this is Flash's problem. However, we shouldn't ignore.

Patch v3.1 doesn't handle the wheel messages if the cursor is on focused plug-in.
Um... When I use IntelliPoint, Flash Player sends the wheel message to us. However, it doesn't send it when I use SetPoint or Alps touchpad. But it's not always. E.g., I can reproduce this bug on http://blog.ppcproz.com/2009/03/what-is-google-ad-auction.html with my patch. However, I cannot reproduce this bug on http://www.youtube.com/watch?v=K7l0a2PVhPQ&feature=player_embedded# .

I'm not sure what's the difference...
Attached patch Patch v3.3 (obsolete) — Splinter Review
fixes WM_VSCROLL/WM_HSCROLL's wrong result.
Attachment #421996 - Attachment is obsolete: true
Attached patch Patch v3.4 (obsolete) — Splinter Review
Attachment #421999 - Attachment is obsolete: true
Attached patch Patch v3.5 (obsolete) — Splinter Review
Attachment #422002 - Attachment is obsolete: true
https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v3.5/

I think that we should use v3.5 to fix this bug strictly. However, some problems cannot fixed by it. We should take another patch after v3.5, that checks the wheel transaction of ESM. Then, most problems will be hidden.
O.K. I see the difference.

When the delta value is less than 120, Flash Player doesn't eat the message, otherwise, always eats them. So, it seems that Flash Player fails unexpectedly to process the wheel messages when the delta is small.

# InteliPoint posts the messages with 30 or lager value, but SetPoint always posts them with 120 or larger value. Probably, the standard mouse driver is same.
(In reply to comment #55)
> O.K. I see the difference.
> 
> When the delta value is less than 120, Flash Player doesn't eat the message,
> otherwise, always eats them. So, it seems that Flash Player fails unexpectedly
> to process the wheel messages when the delta is small.
> 
> # InteliPoint posts the messages with 30 or lager value, but SetPoint always
> posts them with 120 or larger value. Probably, the standard mouse driver is
> same.

OK, is it possible to do anything about that, or are we stuck with whatever our mice give us? I'm assuming the latter, but hopeful!!
I changed my mind. We need to change the scroll event processing architecture.

1. We shouldn't send wheel messages to plug-in window directly.
2. We should dispatch scroll events even if mouse cursor is on plug-in window.

By this changes, we'll fire the DOM mouse wheel events, this is better thing.

3. If the DOM mouse wheel events aren't consumed, ESM processes the scrolling, this time, if the target frame is windowed plug-in, ESM should send the native message to the plug-in window via nsIWidget. And if the plug-in window consumes the message, ESM should stop the processing, otherwise, the ancestor scrollable view should be scrolled.

By this change, we can honor the mouse wheel transaction on plug-in window too.

So, this is pretty risky change, we cannot fix this bug on 1.9.2 branch.
I think that the new behavior helps to fix bug 409669.
Blocks: 409669
Could you file a separate bug only to fix a regression of bug 487245?
It's different from what was originally reported in this bug, and it should be fixed on the 192 branch.
(In reply to comment #59)
> Could you file a separate bug only to fix a regression of bug 487245?
> It's different from what was originally reported in this bug, and it should be
> fixed on the 192 branch.

Filed Bug 540510.
(In reply to comment #59)
> Could you file a separate bug only to fix a regression of bug 487245?
> It's different from what was originally reported in this bug, and it should be
> fixed on the 192 branch.

I think that there is no merit to fix the regression on branch because that cannot fix this bug on most systems.

So, if somebody thinks that we should fix the regression on branch, he/she should file a new bug.
WebKit (Google Chrome) sends mouse wheel messages only when a plug-in has focus. But this doesn't have consistency...
Attached patch Patch v4.0 (obsolete) — Splinter Review
Attachment #422004 - Attachment is obsolete: true
Attached patch Patch v4.1 (obsolete) — Splinter Review
OOPP capable.
Attachment #422346 - Attachment is obsolete: true
Attached patch Patch v4.2 (obsolete) — Splinter Review
Attachment #422360 - Attachment is obsolete: true
Attached patch Patch v4.3 (obsolete) — Splinter Review
Attachment #422367 - Attachment is obsolete: true
Comment on attachment 422478 [details] [diff] [review]
Patch v4.3

Kimura-san, would you review the nsWindow part?
And Olli, would you review the ESM part?

This patch always dispatches nsMouseScrollEvent. By this change, even if the cursor is on a plug-in window, the DOM mouse scroll events are dispatched on every element. So, JS can eat the scroll events before the native events are dispatched to the plug-in.

Only when the events are not consumed by JS, nsEventStateManager calls nsIWidget::DispatchNativeEvent() with the nsGUIEvent if the cursor is on a plug-in and we're not in a mouse wheel transaction.

If the method doesn't consume the native event, ESM try to scroll the plug-in ancestors.
Attachment #422478 - Flags: review?(VYV03354)
Attachment #422478 - Flags: review?(Olli.Pettay)
(In reply to comment #69)
> The test build of the latest patch is here.
> https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v4.3/
Win32 build doesn't show the bug anymore, but it doesn't scroll if you start scrolling in the flash object or if you hesitate too long while scrolling over the object. But this is intended I guess?
(In reply to comment #70)
> this is intended I guess?

Yes, exactly. Sorry for the not enough explanation.

When the cursor is on a plug-in, we send the native event to the plug-in. If plug-in doesn't consume it, ESM tries to scroll its ancestors. However, Flash Player always returns "processed" result, it's the cause. Unfortunately, we should believe the result because Flash Player's application can handle the wheel scroll event by onMouseWheel handler.

So, if you feels something wrong, it's the plug-in's problem (or plug-in's application's problem).
(In reply to comment #71)
> So, if you feels something wrong, it's the plug-in's problem (or plug-in's
> application's problem).

Yes, you can say that. Not only Firefox Flash Player plugin has the problem but also Internet Explorer Flash Player ActiveX since some builds ago, I don't know when it started.
(In reply to comment #7)
> If this is indeed a regression between Firefox 2 and 3 with the same Flash
> Player plug-in (say, use the latest released Flash Player), then this should be
> a Firefox bug.
Firefox 2 had *never* passed wheel events to plug-ins because of bug 333166. So I don't think this is a regression.
Keywords: regression
Interesting, bug 541309's page:
http://www.google.com/finance?q=INDEXDJX:.DJI

The pages's Flash Player doesn't process WM_MOUSEWHEEL message when it's not focused. It sounds Flash Player's application can control whether they consume the message or not.

Note that when it's not focused, the Flash Player's wndproc calls our wndproc. At that time, we don't process the message and return non-zero result. However, Flash Player returns zero to us. I think that Flash Player always returns zero at its wndproc. We should report this bug to Adobe.
I may be stating something others already know, but just in case it is helpful information the latest test build with v4.3 patch fixes a problem where Flash components that have focus did not process mouse scroll wheel events.

This is clearly evident in Google Finance's Flash charts where mouse wheel scrolling should change the chart time period.  In Firefox 3.6 final the scrolling does not work but it does work using this test build.

I'm somewhat new to the Bugzilla system and the test builds that are distributed here.  Are these builds using the 3.6 final code with just the specified patch or are they built off of a trunk containing other code changes as well?
(In reply to comment #75)
> I'm somewhat new to the Bugzilla system and the test builds that are
> distributed here.  Are these builds using the 3.6 final code with just the
> specified patch or are they built off of a trunk containing other code changes
> as well?

Those tests builds are trunk + Masayuki's changes.
(In reply to comment #76)
> (In reply to comment #75)
> > I'm somewhat new to the Bugzilla system and the test builds that are
> > distributed here.  Are these builds using the 3.6 final code with just the
> > specified patch or are they built off of a trunk containing other code changes
> > as well?
> 
> Those tests builds are trunk + Masayuki's changes.

Yeah, and it might change some web pages' behavior, so, I think that we shouldn't use the patch on 1.9.2 branch (Fx3.6.1 or later).
(In reply to comment #77)

> Yeah, and it might change some web pages' behavior, so, I think that we
> shouldn't use the patch on 1.9.2 branch (Fx3.6.1 or later).

I'm confused by what you're saying.  The noted behavior from Firefox 3.5 to 3.6 has most DEFINITELY changed, and your patch restores the expected behavior (at least on the sites I've tested).  If this patch could have other unintended side effects then why can't we go back to the same code used in Fx3.5 for the time being, or did I miss something else in this thread?
Duplicate of this bug: 541762
(In reply to comment #69)
> The test build of the latest patch is here.
> https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v4.3/

I tried the try server build v4.3 on windows 7.
I can not start scroll page by wheel scroll when mouse pointer on the flash movie.
[STR]
1. open http://www.youtube.com/watch?v=K7l0a2PVhPQ&feature=player_embedded .
2. Wait till a page is loaded and the flash movie is started automatically.
3. Left-Click on the flash movie to pause the flash movie.
4. Try scrolling by wheel scroll

[Actual]
Scroll does not happen

[Expected]
Start scroll page.

IE8, Google Chrome 4.0.249.78, Opera 10.10 and Safari 4.0.4 can start scroll  as expected.
The try server build version 4.3 can not start scroll.
> +    HWND pluginWnd = ::FindWindowExA(mWnd, NULL, "GeckoPluginWindow", NULL);
nit: Use FindWindowExW.

> +  // redirect the message to the our window
typo: to our window

> -  if (result)
> -    *aRetValue = isVertical ? 0 : TRUE;
> -  
> +  aHandled = DispatchWindowEvent(&scrollEvent);
> +  *aRetValue = MakeMessageResult(msg, aHandled);
Why did you change the code so that it may return nonzero even when the message isn't WM_MOUSEHWHEEL?

(In reply to comment #74)
> I think that Flash Player always returns zero
> at its wndproc.
I don't think it's a bug of Flash Player.
We should consider Flash Player didn't handle the message if Flash Player called our wndproc because the return value of wndproc doesn't indicate whether message is handled or not.
(In reply to comment #78)
> (In reply to comment #77)
> 
> > Yeah, and it might change some web pages' behavior, so, I think that we
> > shouldn't use the patch on 1.9.2 branch (Fx3.6.1 or later).
> 
> I'm confused by what you're saying.  The noted behavior from Firefox 3.5 to 3.6
> has most DEFINITELY changed, and your patch restores the expected behavior (at
> least on the sites I've tested).  If this patch could have other unintended
> side effects then why can't we go back to the same code used in Fx3.5 for the
> time being, or did I miss something else in this thread?
It's the reason why I proposed filing a bug in comment #59.
I still think it's worth to fix a regression from bug 487245 for 192 branch.
Same problem in http://www.bestlzone.com/news.php when you focus the chat box you can't scroll or http://www.bestlzone.com/download_the-lord-of-the-rings-the-two-towers-dvdrip.2413 try to scroll when you point the filebox video,very very frustrating and i think it's an old bug,as it's the same problem in 3.5 i don't remember if the problem is in 2.0 or 3.0,but i know it's in firefox 3.5.7 and now 3.6

Thanks,hope this helps a little.
(In reply to comment #80)
> (In reply to comment #69)
> > The test build of the latest patch is here.
> > https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v4.3/
> 
> I tried the try server build v4.3 on windows 7.
> I can not start scroll page by wheel scroll when mouse pointer on the flash
> movie.

It's intentional behavior. See comment 70 and comment 71.
(In reply to comment #81)
> > -  if (result)
> > -    *aRetValue = isVertical ? 0 : TRUE;
> > -  
> > +  aHandled = DispatchWindowEvent(&scrollEvent);
> > +  *aRetValue = MakeMessageResult(msg, aHandled);
> Why did you change the code so that it may return nonzero even when the message
> isn't WM_MOUSEHWHEEL?

Excuse me, I'm not sure what you think a problem. MakeMessageResult() makes a valid message result for the boolean param.

Current ESM *always* consumes the event, so, the |if| condition is always TRUE. However, the new ESM may return FALSE to aHandled. Then, we should return non-zero for WM_MOUSEWHEEL and zero for WM_MOUSEHWHEEL.
> Then, we should return non-zero for WM_MOUSEWHEEL and zero for WM_MOUSEHWHEEL.
We should always return zero for WM_MOUSEWHEEL unless we call DefWindowProc or CallWindowProc. Why do you think to return non-zero for WM_MOUSEWHEEL?
(In reply to comment #84)
> (In reply to comment #80)
> > (In reply to comment #69)
> > > The test build of the latest patch is here.
> > > https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v4.3/
> > 
> > I tried the try server build v4.3 on windows 7.
> > I can not start scroll page by wheel scroll when mouse pointer on the flash
> > movie.
> 
> It's intentional behavior. See comment 70 and comment 71.
I do not understand,
IE8, Google Chrome 4.0.249.78, Opera 10.10 and Safari 4.0.4 can start scroll as expected.
I file a new bug after this bug was fixed.
(In reply to comment #86)
> > Then, we should return non-zero for WM_MOUSEWHEEL and zero for WM_MOUSEHWHEEL.
> We should always return zero for WM_MOUSEWHEEL unless we call DefWindowProc or
> CallWindowProc. Why do you think to return non-zero for WM_MOUSEWHEEL?

http://msdn.microsoft.com/ja-jp/library/ms645617%28en-us,VS.85%29.aspx
Why shouldn't we return non-zero when we don't consume it? If so, why are we checking the SendMessage result? I think the result is important for senders.

(In reply to comment #87)
> (In reply to comment #84)
> > (In reply to comment #80)
> > > (In reply to comment #69)
> > > > The test build of the latest patch is here.
> > > > https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-bug483136_v4.3/
> > > 
> > > I tried the try server build v4.3 on windows 7.
> > > I can not start scroll page by wheel scroll when mouse pointer on the flash
> > > movie.
> > 
> > It's intentional behavior. See comment 70 and comment 71.
> I do not understand,
> IE8, Google Chrome 4.0.249.78, Opera 10.10 and Safari 4.0.4 can start scroll as
> expected.
> I file a new bug after this bug was fixed.

*Our* wheel scrolling processing rules are:

1. Scrolls two or more scrollable view by one scroll event.
2. Always honors the mouse cursor position rather than focus.

I don't think that we should change our rules.

If Flash Player said that the message was processed, we shouldn't process the message ourselves. However, next patch checks whether our wndproc was called or not. So, it might improve the behavior in some cases.
> 1. Scrolls two or more scrollable view by one scroll event.

oops, NOT scrolls ... I meant.
(In reply to comment #82)
> (In reply to comment #78)
> > (In reply to comment #77)
> > 
> > > Yeah, and it might change some web pages' behavior, so, I think that we
> > > shouldn't use the patch on 1.9.2 branch (Fx3.6.1 or later).
> > 
> > I'm confused by what you're saying.  The noted behavior from Firefox 3.5 to 3.6
> > has most DEFINITELY changed, and your patch restores the expected behavior (at
> > least on the sites I've tested).  If this patch could have other unintended
> > side effects then why can't we go back to the same code used in Fx3.5 for the
> > time being, or did I miss something else in this thread?
> It's the reason why I proposed filing a bug in comment #59.
> I still think it's worth to fix a regression from bug 487245 for 192 branch.

No, comment 59's suggestion cannot fix the bug on most systems which are not using IntelliPoint. However, it's going to be fixed by bug 531341.

Davison:

We already released 3.6.0. Web developers should check their web sites with it and they might changes their web sites for 3.6.0. If we land the patch at 3.6.x, it might break such web sites.
Attached patch Patch v4.5 (obsolete) — Splinter Review
Attachment #422478 - Attachment is obsolete: true
Attachment #422478 - Flags: review?(VYV03354)
Attachment #422478 - Flags: review?(Olli.Pettay)
Preparing the latest patch's test builds, wait a moment.

For the Kimura-san's review, it doesn't consume if our wndproc is called from plug-in. By this change, we can scroll web pages even if cursor is on Flash Player. However, on some Flash Applications which handle onMouseWheel(), the page is scrolled by wheel events even because they didn't prevent the default.
# Bug 541309's page prevents the page's scrolling if the Flash Player has focus.
# So, I guess that they can prevent it. And they should do it themselves.
Attached patch Patch v4.5.1 (obsolete) — Splinter Review
Attachment #423307 - Attachment is obsolete: true
Attachment #423309 - Flags: review?(VYV03354)
Hello,
I came to report a bug but found this one. It's almost a year old, is this ever going to be fixed? 

e.g. http://news.bbc.co.uk/1/hi/world/middle_east/8480569.stm

use the middle mouse button and it stops over the flash video. Chrome works fine, even IE works fine.

I'm running 3.6 on Windows XP.

Thanks.
I documented the wheel transaction system.
https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling
Severity: minor → normal
(In reply to comment #88)
> *Our* wheel scrolling processing rules are:
> 
> 1. Scrolls two or more scrollable view by one scroll event.
> 2. Always honors the mouse cursor position rather than focus.
> 
> I don't think that we should change our rules.
True, but you can't force Windows operating system to obey our rule.
aRetValue (that is, LRESULT) will be returned to the operating system. Per MSDN, that value supposed to be zero for WM_MOUSEWHEEL.
> Return Value
>    If an application processes this message, it should return zero. 
Note that this does NOT mean "Otherwise, it should return nonzero".
Read it as follows:
If an application processes this message, it should return zero [without calling DefWindowProc/CallWindowProc]. [Otherwise, it should return the return value from DefWindowProc or CallWindowProc.]

Only a few messages (such as WM_NCHITTEST) use the LRESULT for some meaningful purposes. Calling previous wndproc is the only way to tell that we didn't handle the message. It's the Win32 event model.

See also an wndproc implementation of our test plug-in.
http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/testplugin/nptest_windows.cpp#446

> If Flash Player said that the message was processed, we shouldn't process the
> message ourselves. However, next patch checks whether our wndproc was called or
> not. So, it might improve the behavior in some cases.
Therefore this change looks good for me.
I pushed bug 531341's patch. So, the regression of bug 487245 was fixed by it.
Attached patch Patch v5.0 (obsolete) — Splinter Review
I see. Thank you for the long explanation. MakeMessageProcessedResult() is always making "processed" state result for the message.
Attachment #423309 - Attachment is obsolete: true
Attachment #423990 - Flags: review?(VYV03354)
Attachment #423309 - Flags: review?(VYV03354)
Hello,
What is technically correct my the O/S is irrelevant. What matters is the consistency of the user experience.

Ask anyone who reads a web page and is forced to navigate the mouse around the screen in order to continue scrolling (after all the web page is largely a scrolling reading interface) and they'll tell you it's daft, counter-intuitive, wrong and very annoying.

Oddly enough a few months ago (i.e. in a slightly earlier version of Firefox I was using) this bug appeared to disappeared and I thought you'd fixed it.
hello,
This fault was fixed in 3.5.6 and now in 3.6 it is back to the **** way it was (scrolling stops when your mouse is over flash).

If I reinstall 3.5.6 all is well, if I upgrade to 3.6 it fails.
Comment on attachment 423990 [details] [diff] [review]
Patch v5.0

This patch could no longer apply. Could you update the patch to latest trunk?
(In reply to comment #100)
(In reply to comment #101)
neil, your problem will be fixed by bug 531341.
Attached patch Patch v5.0 unbitrotted (obsolete) — Splinter Review
I've resolved a conflict manually.
Please verify whether this is correct.
Attachment #423990 - Attachment is obsolete: true
Attachment #424671 - Flags: review?(masayuki)
Attachment #423990 - Flags: review?(VYV03354)
Comment on attachment 424671 [details] [diff] [review]
Patch v5.0 unbitrotted

> +  if (IsMessageProcessed(sSendingMessage, ret) && !sOurWndProcWasCalled) {
We don't have to check the return value here. The return value may be unreliable because targetWnd may not be under our control. Moreover, MSDN is inconsistent about the return value for WM_MOUSEHWHEEL.
Attachment #424671 - Flags: review?(masayuki)
(In reply to comment #104)
> Created an attachment (id=424671) [details]
> Patch v5.0 unbitrotted
> 
> I've resolved a conflict manually.
> Please verify whether this is correct.

Yeah, ok. Thanks.

(In reply to comment #105)
> (From update of attachment 424671 [details] [diff] [review])
> > +  if (IsMessageProcessed(sSendingMessage, ret) && !sOurWndProcWasCalled) {
> We don't have to check the return value here. The return value may be
> unreliable because targetWnd may not be under our control. Moreover, MSDN is
> inconsistent about the return value for WM_MOUSEHWHEEL.

I agree, I posted the mistake to MSKK, however, they've never replied...
Attached patch Patch v5.1 (obsolete) — Splinter Review
Attachment #424671 - Attachment is obsolete: true
Attachment #424729 - Flags: review?(VYV03354)
Comment on attachment 424729 [details] [diff] [review]
Patch v5.1

Firefox freezed with OOPP enabled and the patch applied.
Steps to reproduce:
1. Navigato to http://www.google.com/finance?q=INDEXDJX:.DJI
2. Click the stock chart.
Firefox did no longer freeze when OOPP was disabled or the patch was reverted.
(In reply to comment #57)
I don't understand why we need to change the event handling order.
When a plug-in have the focus and it consumes a wheel message, DOM mouse wheel event will not be generated because we have no chance to see the message. However, if a plug-in does not have the focus, DOM mouse wheel event will be generated first. It doesn't look consistent to me.
Could you define a new event to query a wheel transaction state? We could suppress re-targetting if the wheel transaction is in progress.
(In reply to comment #108)
> (From update of attachment 424729 [details] [diff] [review])
> Firefox freezed with OOPP enabled and the patch applied.
> Steps to reproduce:
> 1. Navigato to http://www.google.com/finance?q=INDEXDJX:.DJI
> 2. Click the stock chart.
> Firefox did no longer freeze when OOPP was disabled or the patch was reverted.

Ugh... If WM_NULL is sent to us, my patch makes a new bug, probably. Otherwise, it doesn't make sense. My patch should cause it.

(In reply to comment #109)
> (In reply to comment #57)
> I don't understand why we need to change the event handling order.
> When a plug-in have the focus and it consumes a wheel message, DOM mouse wheel
> event will not be generated because we have no chance to see the message.
> However, if a plug-in does not have the focus, DOM mouse wheel event will be
> generated first. It doesn't look consistent to me.
> Could you define a new event to query a wheel transaction state? We could
> suppress re-targetting if the wheel transaction is in progress.

I hate the new query event creating approach. I wrote such patch first, but it doesn't look smart. And I think that widget shouldn't need to know the wheel transaction. It's just an XP level function. widget code shouldn't depend on such functions.

I agree that the inconsistent isn't good thing but I think that it is not a problem. Isn't there the issue? E.g., mouse move and mouse click.

The new behavior makes consistence between other DOM elements.
> Ugh... If WM_NULL is sent to us, my patch makes a new bug, probably. Otherwise,
> it doesn't make sense. My patch should cause it.

My patch should *not* cause it...
I tested the hang-up bug on trunk build. I cannot reproduce at clicking, however, I can reproduce it on other situations even if the tab is in background. I believe that it's not a bug of my patch. But I need to check WM_NULL case.
Comment on attachment 424729 [details] [diff] [review]
Patch v5.1

Sorry, STR was a bit incomplete. Add the following step after STR in comment #108.
3. Rotate a wheel.
So this is definitely caused by the patch. This is 100% reproducable for me. I don't know why you can't reproduce a problem, but I can't give r+ without fixing this problem.
Attachment #424729 - Flags: review?(VYV03354) → review-
(In reply to comment #113)
> (From update of attachment 424729 [details] [diff] [review])
> Sorry, STR was a bit incomplete. Add the following step after STR in comment
> #108.
> 3. Rotate a wheel.

Ah, O.K. It makes sense. Then, my patch is sending message to the plug-in window which is separated process. Currently, we don't send the message to it. So, I can guess that it's the cause.

> So this is definitely caused by the patch. This is 100% reproducable for me. I
> don't know why you can't reproduce a problem, but I can't give r+ without
> fixing this problem.

No, I've not tested because I went to my home town last weekend, I'll check it ASAP.
Apply this on top of Patch v5.1.
This fixed a freeze issue, but I'm not sure whether this is a right fix.
Duplicate of this bug: 544931
Attached patch Patch v6.0 (obsolete) — Splinter Review
This still has the hang-up bug.

Here causes the hang-up:
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#496
> 496     while (1) {
> 497       // Wait until we have a message in the queue. MSDN docs are a bit unclear
> 498       // but it seems that windows from two different threads (and it should be
> 499       // noted that a thread in another process counts as a "different thread")
> 500       // will implicitly have their message queues attached if they are parented
> 501       // to one another. This wait call, then, will return for a message
> 502       // delivered to *either* thread.
> 503       DWORD result = MsgWaitForMultipleObjects(0, NULL, FALSE, INFINITE,
> 504                                                QS_ALLINPUT);

So, MsgWaitForMultipleObjects() didn't return. Do you understand the cause?

Changes from the previous patch:
1. Renamed nsIWidget::DispatchNativeEvent() to nsIWidget::DispatchEventToPlugin()
2. When our plug-in widget dispatches the wheel events, we don't set native messages to the pluginEvent because if we send it to the plug-in window, the plug-in process twice same message, probably.
Attachment #424729 - Attachment is obsolete: true
With IntelliPoint, the wheel message was posted to "GeckoPluginWindow" window, then, it was sent to our plug-in widget and its parent. And this time, nsWindow::OnWheel and nsWindow::DispatchEventToPlugin are not called.
(In reply to comment #118)
> And this time,
> nsWindow::OnWheel and nsWindow::DispatchEventToPlugin are not called.

Ignore this. I missed the method name. nsWindow::OnMouseWheel was called. However, it seems that nsWindow::DispatchEventToPlugin() and SendMessage() are not cause.
Attached patch Patch v7.0 (obsolete) — Splinter Review
This takes Kimura-san's hack.

I have no idea of the actual cause. I guess that the another process sends messages to us and waiting. Then, we return to the process via nsObjectFrame and NPAPI. Maybe, it's one of the cause.

This patch doesn't call next wndproc when we processed mouse wheel related messages. Because we're retargets the handler to the cursor positioned window. So, our window shouldn't handle the messages twice and more. However, if we don't process the messages, we should send them to embedder directly.
Attachment #425818 - Attachment is obsolete: true
Attachment #425950 - Attachment is obsolete: true
Attached patch Patch v7.1 (obsolete) — Splinter Review
Kimura-san, I found a way to reproduce the hang-up bug.
1. Open http://www.google.com/finance?q=INDEXDJX:.DJI
2. Open source viewer
3. Click the flash player of the background window

I think focus events or activate event is going to plug-in process, it causes the hang-up.

So, I think that it's good choice that we take your hack temporary.
Attachment #425983 - Attachment is obsolete: true
Attachment #426002 - Flags: review?(VYV03354)
Attached patch Patch v7.2 (obsolete) — Splinter Review
Fix a bug which sent to the embedder of *retargetted* window. We should send them to the embedder of the window whose ProcessMessage was called.
Attachment #426002 - Attachment is obsolete: true
Attachment #426007 - Flags: review?(VYV03354)
Attachment #426002 - Flags: review?(VYV03354)
Attached patch Patch v7.2.2 (obsolete) — Splinter Review
Fix bustage on WinCE.
Attachment #426007 - Attachment is obsolete: true
Attachment #426058 - Flags: review?(VYV03354)
Attachment #426007 - Flags: review?(VYV03354)
(In reply to comment #110)
I have doubt about whether we should consider wheel message handling as a part of "default action."
Other mouse messages are handled as if there are DOM mouse event handlers.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#4383
Is it possible to handle wheel messages here?

(In reply to comment #121)
> Kimura-san, I found a way to reproduce the hang-up bug.
> 1. Open http://www.google.com/finance?q=INDEXDJX:.DJI
> 2. Open source viewer
> 3. Click the flash player of the background window
It occurs even without a patch, so it will not be our fault.
(In reply to comment #124)
> (In reply to comment #110)
> I have doubt about whether we should consider wheel message handling as a part
> of "default action."
> Other mouse messages are handled as if there are DOM mouse event handlers.
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#4383
> Is it possible to handle wheel messages here?

Yes, it's possible. However, we're scrolling any elements as default action. So, if we should change it, we should do/discuss it in another bug.
Comment on attachment 426058 [details] [diff] [review]
Patch v7.2.2

r=me about a widget part.
Attachment #426058 - Flags: review?(VYV03354) → review+
Comment on attachment 426058 [details] [diff] [review]
Patch v7.2.2

Thank you, Kimura-san.

Olli:

Would you review the XP part?

Even when the mouse cursor is on plug-in widget, we should prefer the wheel transaction if any DOMMouseScroll handlers didn't consume it.

For that this patch sends the native events to plug-in widget via ESM.
Attachment #426058 - Flags: review?(Olli.Pettay)
I filed bug 547136 for the hanging-up bug.
Attached patch Patch v7.3 (obsolete) — Splinter Review
Attachment #426058 - Attachment is obsolete: true
Attachment #427885 - Flags: review?(Olli.Pettay)
Attachment #426058 - Flags: review?(Olli.Pettay)
Attached patch Patch v7.3.1 (obsolete) — Splinter Review
fix a nit, sorry for the spam.
Attachment #427885 - Attachment is obsolete: true
Attachment #427889 - Flags: review?(Olli.Pettay)
Attachment #427885 - Flags: review?(Olli.Pettay)
It's hard for me to tell the status of the patch from reading the notes, so apologies if this is not applicable:

Please keep in mind here that the critical requirement here is to return to the old scrolling behavior (i.e. window scrolls when the user mousewheels/gestures, even when mouse is over flash movie or flash movie has focus). This is a major interaction issue and breaking behavior from all other major browsers (IE, Safari, Chrome) due to the peculiarities of your plugin architecture is not acceptable. Unfortunately, there's no web standard for how this should work; enforcing your own standard just get us back to the Bad Old Days.

Put another way, if you allow Flash to steal mouseScroll events, web designers will hate you. Personally, I will have to special-case all of my code just for Firefox.
(In reply to comment #131)
> (i.e. window scrolls when the user mousewheels/gestures,
Web pages can always prevent the default handling of mousewheel events, and
that would prevent scrolling.


Masayuki, I'll review this soon, but any chance for tests. We do have the
testplugin which you could use in the testcases, I think.
(In reply to comment #132)
> Masayuki, I'll review this soon, but any chance for tests. We do have the
> testplugin which you could use in the testcases, I think.

There are some issues:
1. There is no API which generate a native mouse wheel event from JS.
2. There is no API which moves the actual mouse cursor of the system.
3. The patch doesn't affect to the behavior on Mac and Linux.
Ah, ctypes might be more useful than new APIs.
(In reply to comment #133)
> (In reply to comment #132)
> > Masayuki, I'll review this soon, but any chance for tests. We do have the
> > testplugin which you could use in the testcases, I think.
> 
> There are some issues:
> 1. There is no API which generate a native mouse wheel event from JS.
> 2. There is no API which moves the actual mouse cursor of the system.

SendNativeMouseEvent can do these things.

> 3. The patch doesn't affect to the behavior on Mac and Linux.

It's OK to write a test that only runs on Windows.

layout/generic/test/plugin_focus_helper.html is a good test to look at.
great, thanks.
Masayuki, are you going to update the patch to contain tests?

(In reply to comment #135)
> SendNativeMouseEvent can do these things.
Probably also DispatchDOMEventViaPresShell
Yeah, please wait a couple of days.
Attached patch Patch v7.4 (obsolete) — Splinter Review
Ugh... the tests sometimes hang up when IPC is enabled and passWheelMessagesToParent is true. Similar to bug 547136, but looks like different bug is there.
Attachment #427889 - Attachment is obsolete: true
Attachment #427889 - Flags: review?(Olli.Pettay)
Attached patch Patch v7.5 (obsolete) — Splinter Review
One of the causes is SendInput() API. This patch works better than the previous.

However, sometimes still hang up. At that time, gecko tries to update the plug-in by scrolling. So, the tests are not stable if IPC is enabled. I think we shouldn't take testcases for this until OOPP becomes stable.
Attachment #433243 - Attachment is obsolete: true
Smaug? What do you think about the test failure problem?

there are two ways:

* land the new tests on the tree, but disable the new tests with OOPP.
* land the new tests on the tree, and ignore the random hang-up by OOPP.
(In reply to comment #141)
> * land the new tests on the tree, and ignore the random hang-up by OOPP.
We must *not* do this.


(In reply to comment #140)
> I think
> we shouldn't take testcases for this until OOPP becomes stable.
Does the problem have anything to do with OOPP being stable, or is it more that
OOP Plugin does not expect Gecko to send some events to it?

Anyway, IIRC, we white list which plugins can be OOP. Could be have non-OOP test plugin?
(In reply to comment #142)
> (In reply to comment #140)
> > I think
> > we shouldn't take testcases for this until OOPP becomes stable.
> Does the problem have anything to do with OOPP being stable, or is it more that
> OOP Plugin does not expect Gecko to send some events to it?

I'm not sure. The hang-up is sometimes happened when gecko tries to update the invalidated area by scrolling. So, that's not related to this bug.

> Anyway, IIRC, we white list which plugins can be OOP. Could be have non-OOP
> test plugin?

Oh, sounds good if we can do it.
looks like the checking white list is in OOPPluginsEnabled():
http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsNPAPIPlugin.cpp#316

however, probably this is called only once at the first time loading of each plug-in, e.g., I cannot switch flush plug-in's mode by toggling dom.ipc.plugins.enabled at loading a page.

And I tried this code before opening the test window:

> var prefService = Components.classes["@mozilla.org/preferences-service;1"].
>                              getService(Components.interfaces.nsIPrefBranch2);
> prefService.setBoolPref("dom.ipc.plugins.enabled", false);
> var pluginHost = Components.classes["@mozilla.org/plugin/host;1"].
>                             getService(Components.interfaces.nsIPluginHost);
> pluginHost.reloadPlugins(false);

But if I loaded the test plug-in on another tab first, the loaded plug-in on the test is in OOPP mode too.
(In reply to comment #144)
> > var prefService = Components.classes["@mozilla.org/preferences-service;1"].
> >                              getService(Components.interfaces.nsIPrefBranch2);
> > prefService.setBoolPref("dom.ipc.plugins.enabled", false);
> > var pluginHost = Components.classes["@mozilla.org/plugin/host;1"].
> >                             getService(Components.interfaces.nsIPluginHost);
> > pluginHost.reloadPlugins(false);
> 
> But if I loaded the test plug-in on another tab first, the loaded plug-in on
> the test is in OOPP mode too.

Sorry, this works fine if I closed the another tab first. So, if there are one or more loaded plug-ins, reloadPlugins() cannot reload the plug-in.

I'll post a new patch.
Attached patch Patch v7.6 (obsolete) — Splinter Review
ok, this patch can disable OOPP only during the tests.
Attachment #433269 - Attachment is obsolete: true
Attachment #434589 - Flags: review?(Olli.Pettay)
Masayuki, can you get stack traces for the plugin and browser process, while in the hang please?  Probably put them in a new bug report.
(In reply to comment #147)
> Masayuki, can you get stack traces for the plugin and browser process, while in
> the hang please?  Probably put them in a new bug report.

Sure, but unfortunately, I cannot check it now, I'll do it this weekend or later.
>  	user32.dll!7572ce75() 	
>  	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
>  	user32.dll!7572ce75() 	
>  	user32.dll!75732d56() 	
>>	xul.dll!nsWindow::Update()  Line 2264 + 0xd bytes	C++
>  	xul.dll!nsViewManager::UpdateWidgetsForView(nsView * aView=0x07a5b470)  Line 1396	C++
>  	xul.dll!nsViewManager::ForceUpdate()  Line 1580	C++
>  	xul.dll!nsViewManager::Composite()  Line 507	C++
>  	xul.dll!nsViewManager::UpdateViewAfterScroll(nsIView * aView=0x07a5b470, const nsRegion & aUpdateRegion={...})  Line 575	C++
>  	xul.dll!nsGfxScrollFrameInner::ScrollVisual(nsIntPoint aPixDelta={...})  Line 1724	C++
>  	xul.dll!nsGfxScrollFrameInner::ScrollToImpl(nsPoint aPt={...})  Line 1801	C++
>  	xul.dll!nsGfxScrollFrameInner::AsyncScrollCallback(nsITimer * aTimer=0x0722b2c0, void * anInstance=0x072c782c)  Line 1356	C++
>  	xul.dll!nsTimerImpl::Fire()  Line 427 + 0xe bytes	C++
>  	xul.dll!nsTimerEvent::Run()  Line 521	C++
>  	xul.dll!nsThread::ProcessNextEvent(int mayWait=0x00000000, int * result=0x0035d504)  Line 527 + 0x19 bytes	C++
>  	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00785848, int mayWait=0x00000000)  Line 250 + 0x16 bytes	C++
>  	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x00789b00)  Line 118 + 0xe bytes	C++
>  	xul.dll!MessageLoop::RunInternal()  Line 217	C++
>  	xul.dll!MessageLoop::RunHandler()  Line 200	C++
>  	xul.dll!MessageLoop::Run()  Line 174	C++
>  	xul.dll!nsBaseAppShell::Run()  Line 180	C++
>  	xul.dll!nsAppShell::Run()  Line 239 + 0x9 bytes	C++
>  	xul.dll!nsAppStartup::Run()  Line 182 + 0x1c bytes	C++
>  	xul.dll!XRE_main(int argc=0x00000005, char * * argv=0x00778278, const nsXREAppData * aAppData=0x0077f2f8)  Line 3548 + 0x25 bytes	C++
>  	firefox.exe!NS_internal_main(int argc=0x00000005, char * * argv=0x00778278)  Line 158 + 0x12 bytes	C++
>  	firefox.exe!wmain(int argc=0x00000005, wchar_t * * argv=0x0085fb40)  Line 120 + 0xd bytes	C++
>  	firefox.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
>  	firefox.exe!wmainCRTStartup()  Line 403	C
>  	kernel32.dll!76b73677() 	
>  	ntdll.dll!774c9d72() 	
>  	ntdll.dll!774c9d45() 	

The stack of the main thread. I'll file a bug for this after this bug is fixed because the test needs for reproduce.
> because the test needs for reproduce.
because the test is needed for reproduce.
Smaug:

Would you review the latest patch? I think that this bug should be fixed on Lorentz too. See bug 552163.
OK, I will, trying to do it still today.
(I've had exceptionally lots of things to review lately, but sorry for the delay.)
Attachment #434589 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 434589 [details] [diff] [review]
Patch v7.6


> nsresult
>+nsEventStateManager::DoScrollPlugin(nsIFrame* aTargetFrame,
>+                                    nsMouseScrollEvent* aMouseEvent,
>+                                    nsEventStatus* aStatus)
>+{
>+  NS_ENSURE_ARG(*aStatus != nsEventStatus_eConsumeNoDefault);
>+
>+  // If we're in a transaction, override the target frame.
>+  nsIFrame* lastScrollFrame = nsMouseWheelTransaction::GetTargetFrame();
>+  if (lastScrollFrame) {
>+    aTargetFrame = lastScrollFrame;
>+  }
I don't understand this. Why do we want to override the target frame?


> 
>   aMouseEvent->scrollOverflow = numLines;
> 
>+  if (aMouseEvent->message == NS_MOUSE_SCROLL) {
>+    // We shouldn't prevent the default behavior if the event is
>+    // NS_MOUSE_SCROLL and scrolled no frame because Win32 native window
>+    // should call next wndproc when we don't use this event.
>+    *aStatus = nsEventStatus_eConsumeDoDefault;
Try to avoid using nsEventStatus_eConsumeDoDefault if possible.
Could you use nsEventStatus_eIgnore here?

Someone else needs to review windows specific parts, probably jimm.
And I'd like to look at the patch again, after I've actually tested this a bit.
I'll try to test this tomorrow.
Did you test the patch on all platforms?
(In reply to comment #153)
> > nsresult
> >+nsEventStateManager::DoScrollPlugin(nsIFrame* aTargetFrame,
> >+                                    nsMouseScrollEvent* aMouseEvent,
> >+                                    nsEventStatus* aStatus)
> >+{
> >+  NS_ENSURE_ARG(*aStatus != nsEventStatus_eConsumeNoDefault);
> >+
> >+  // If we're in a transaction, override the target frame.
> >+  nsIFrame* lastScrollFrame = nsMouseWheelTransaction::GetTargetFrame();
> >+  if (lastScrollFrame) {
> >+    aTargetFrame = lastScrollFrame;
> >+  }
> I don't understand this. Why do we want to override the target frame?

This is a point of the patch. When the user scrolled a scrollable element and the transaction is not finished, we can assume that the user want to scroll the last scrollable element rather than the current element which is under the cursor. Therefore, we should override the target here.
https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Mouse_wheel_transaction

> >   aMouseEvent->scrollOverflow = numLines;
> > 
> >+  if (aMouseEvent->message == NS_MOUSE_SCROLL) {
> >+    // We shouldn't prevent the default behavior if the event is
> >+    // NS_MOUSE_SCROLL and scrolled no frame because Win32 native window
> >+    // should call next wndproc when we don't use this event.
> >+    *aStatus = nsEventStatus_eConsumeDoDefault;
> Try to avoid using nsEventStatus_eConsumeDoDefault if possible.
> Could you use nsEventStatus_eIgnore here?

O.K. nsEventStatus_eConsumeDoDefault is going to be removed?

> Someone else needs to review windows specific parts, probably jimm.

It's already done.

(In reply to comment #154)
> Did you test the patch on all platforms?

No, I'll test on Linux and Mac.
(In reply to comment #155)
> (In reply to comment #154)
> > Did you test the patch on all platforms?
> 
> No, I'll test on Linux and Mac.

I tested. I cannot confirm any differences between non-patched build and patched build.

I checked both platforms' widget code.

Mac (Cocoa):
Looks like cocoa event is handled via DOM event of the embed/object frame. On Mac, the plug-ins are always windowless mode, so, if we need to improve the behavior on Mac, we need to apply the mouse wheel transaction system to the windowless mode too. That isn't the scope of this bug.

Mac (Carbon):
Carbon event is sent after we dispatched the wheel event, so, nsIWidget::DispatchEventToPlugin() can be used for the carbon event easily.

Linux:
Looks like if the plug-in dispatches the wheel event, our window don't override the event. I guess that it can be possible that we override the event handler always and we dispatch the event from nsIWidget::DispatchEventToPlugin().
Attached patch Patch v7.7 (obsolete) — Splinter Review
Attachment #434589 - Attachment is obsolete: true
Attachment #436134 - Flags: review?(Olli.Pettay)
Attached patch Patch v7.7.1 (obsolete) — Splinter Review
fix a nit.
Attachment #436134 - Attachment is obsolete: true
Attachment #436137 - Flags: review?(Olli.Pettay)
Attachment #436134 - Flags: review?(Olli.Pettay)
Comment on attachment 436137 [details] [diff] [review]
Patch v7.7.1

Ugh... with today's trunk, nsIPluginHost::reloadPlugins() failed on tinderbox, but I cannot reproduce the failure on my environment...
Attachment #436137 - Flags: review?(Olli.Pettay)
Attached patch Patch v8.0 (obsolete) — Splinter Review
This patch passes the tests on tryserver too. The cause is that reloadPlugin() abort because there are no changes in plug-in list. I'm not sure why the failure doesn't happen on my environment.

I added a change to nsPluginHost. If IPC settings have been changed after previous plug-in list creation, reloadPlugin() reloads.

The widget part was already reviewed by Kimura-san and Jim. After your review, I'll request a review to josh for plug-in part. And also I'll request a sr.
Attachment #436137 - Attachment is obsolete: true
Attachment #436480 - Flags: review?(Olli.Pettay)
This addon can fix the problem until it's a official fix, https://addons.mozilla.org/en-US/firefox/addon/64764
Smaug? Would you review the latest patch?
I will I will, I've been a bit busy with other reviews.
(In reply to comment #160)
> The widget part was already reviewed by Kimura-san and Jim. After your review,
> I'll request a review to josh for plug-in part. And also I'll request a sr.

I was confused with another bug. The patch wasn't reviewed by Jim. It was only reviewed by Kimura-san who has worked on the wheel message handling. But widget part of the patch has grown bigger than the Kimura-san's review. I'll request a review to Jim after bug 552163. I'll need to merge the patch if that will be landed before this bug.
Attached patch Patch v8.0.1 (obsolete) — Splinter Review
updating for latest trunk.
Attachment #436480 - Attachment is obsolete: true
Attachment #437772 - Flags: review?(Olli.Pettay)
Attachment #436480 - Flags: review?(Olli.Pettay)
The patch doesn't seem to work on Linux. Is that expected?
(In reply to comment #166)
> The patch doesn't seem to work on Linux. Is that expected?

Yes. We need to implement nsIWidget::DispatchEventToPlugin() on Linux after this bug.
Comment on attachment 437772 [details] [diff] [review]
Patch v8.0.1

> nsMouseWheelTransaction::UpdateTransaction(PRInt32 aNumLines,
>                                            PRBool aScrollHorizontal)
> {
>   nsIScrollableFrame* sf = GetTargetFrame()->GetScrollTargetFrame();
>-  NS_ENSURE_TRUE(sf, PR_FALSE);
>-
>-  if (!CanScrollOn(sf, aNumLines, aScrollHorizontal)) {
>+  if (sf && !CanScrollOn(sf, aNumLines, aScrollHorizontal)) {
>     OnFailToScrollTarget();
>     // We should not modify the transaction state when the view will not be
>     // scrolled actually.
>     return PR_FALSE;
>+  } else if (!sf) {
else after return


>+  // The target frame isn't an object frame, we need to do nothing.
>+  if (aTargetFrame->GetType() != nsGkAtoms::objectFrame) {
>+    return NS_OK;
>+  }
>+
>+  nsIObjectFrame* objectFrame = do_QueryFrame(aTargetFrame);
>+  NS_ENSURE_TRUE(objectFrame, NS_ERROR_FAILURE);
You could combine these.
Just do 
nsIObjectFrame* objectFrame = do_QueryFrame(aTargetFrame);
if (!objectFrame) {
  return NS_OK;
}

>+  if (!aMouseEvent->pluginEvent) {
>+    // If we're in a transaction, we should prevent the default processing.
Could you explain here why we should prevent the default processing.


>+  if (!targetFrame) {
>+    // The target frame might be destroyed by the native event.
>+    if (lastScrollFrame) {
Could you add a comment here that lastScrollFrame may point to deleted object here, 
and it shouldn't really be used.

I read only the ESM part. Jimm, or someone else familiar with OOPP needs to review the rest.
Please file a followup to fix this also on linux.
Attachment #437772 - Flags: review?(Olli.Pettay) → review+
No longer blocks: 552163
Depends on: 552163
Attached patch Patch v8.1 (obsolete) — Splinter Review
Thank you, smaug.

This is updated for the latest review and bug 552163.

jimm: Would you review the widget part? Kimura-san reviewed the wheel related part of widget but I added some changes for automated tests.

All wheel events are processed by the mouse wheel transaction system of nsEventStateManager by this patch. The old behavior is that nsWindow redirects the wheel messages to plug-in window when the cursor is on a plug-in. By the behavior, even if an user want to scroll the page, when a plug-in comes under the cursor by scrolling and it consumes wheel messages, the scrolling is stopped by the plug-in. But by this patch, the user can continue to scroll the page scrolling until timed out the mouse wheel transaction (see https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Mouse_wheel_transaction ).

nsWindow always dispatches our mouse wheel event rather than redirecting the message. nsEventStateManager calls nsIWidget::DispatchEventToPlugin() if the wheel event should go to a plug-in (currently windowed mode only). If the wheel message is consumed (i.e., our wndproc which is ancestor of the plug-in window wasn't called), DispatchEventToPlugin() returns nsEventStatus_eConsumeNoDefault. Then, nsEventStateManager stops the handling. Otherwise, nsEventStateManager tries to scroll ancestor scrollable element of the plug-in.

And I changed nsWindow::SynthesizeNativeMouseEvent() for automated tests of this bug. But I don't use ::SendInput() for MOUSEEVENTF_WHEEL because if I *send* wheel message to OOPP enabled plug-in, we make deadlock. So, I use PostMessage() for it.
Attachment #437772 - Attachment is obsolete: true
Attachment #439743 - Flags: review?(jmathies)
Comment on attachment 439743 [details] [diff] [review]
Patch v8.1

Josh:
Would you review the plug-in host part and test plug-in part?

plug-in host part is needed for preventing OOPP deadlock bug during automated tests. See comment 149 for the stack trace.

I want to disable OOPP mode of the test plug-in during my tests. However, there is no way to change the OOPP mode of each plug-in directly because it's decided when each plug-in is loaded first. So, I need to reload the plug-ins but if there are no changes, nsIPluginHost::realodPlugins() just returns NS_ERROR_PLUGINS_PLUGINSNOTCHANGED. So, I want it to reload always if OOPP related settings are changed after current plug-in list was created.

And I added setPassWheelMessagesToParent to the test plug-in. If it's true, WM_MOUSEWHEEL and WM_MOUSEHWHEEL are passed to its parent window. Otherwise, they are consumed by the plug-in.
Attachment #439743 - Flags: review?(joshmoz)
This works pretty well, the behavior is nice when you have multiple plugins dispersed within a page.

Note, changes to base widget will also need sr? from roc.


+    if (!OnMouseWheel(msg, wParam, lParam, PR_TRUE, IsPluginWindow())) {
+      // If OnMouseWheel() didn't consume the message, we should send the
+      // message to the embedder directly because we'll never call next wndproc.
+    }
+    // We must not call next wndproc because our window redo same things,
+    // it means we'll dispatches same events.
+    result = PR_TRUE;
+    *aRetValue = MakeMessageProcessedResult(msg);


Can you expand on these comments to explain why OnMouseWheel wouldn't consume the message, better define 'embedder' (browser/main app window), and clarify why we would "redo same things"..

+void nsWindow::SendMessageToEmbedder(UINT msg, WPARAM wParam, LPARAM lParam)

Can you comment what this method does with a comment block?

+      // No our non-plugin window under the cursor.

"No mozilla windows are currently under the mouse cursor."

This is certainly cleaner than the last patch we applied.
Attached patch Patch v8.1.1 (obsolete) — Splinter Review
> Note, changes to base widget will also need sr? from roc.

Yes, of course.
Attachment #439743 - Attachment is obsolete: true
Attachment #440401 - Flags: review?(jmathies)
Attachment #439743 - Flags: review?(joshmoz)
Attachment #439743 - Flags: review?(jmathies)
Attachment #440401 - Flags: review?(joshmoz)
Attached patch Patch v8.1.2Splinter Review
updated for latest trunk
Attachment #440401 - Attachment is obsolete: true
Attachment #440697 - Flags: review?(joshmoz)
Attachment #440697 - Flags: review?(jmathies)
Attachment #440401 - Flags: review?(joshmoz)
Attachment #440401 - Flags: review?(jmathies)
Masayuki, we're experiencing a lot of deadlocks with OOPP and wheel events sent from plugins. We're going to add a generic ReplyMessage for these events in the moz_ipc handler - 

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#3804

Curious if you have some ideas about how that might effect the work your doing here?
Jim, I don't think it's enough because plug-ins can send messages directly, right?

When you experience the deadlock on trunk?
And also we always call ReplyMessage() before dispatching our mouse scroll event now, so, if there are deadlock bug, it means that we receive other messages from the plug-in during the mouse wheel message handling on it.
(In reply to comment #176)
> And also we always call ReplyMessage() before dispatching our mouse scroll
> event now, so, if there are deadlock bug, it means that we receive other
> messages from the plug-in during the mouse wheel message handling on it.

Here's the stack:

bug 561117

The child process sends wm_mousewheel to parent, which ends up in our main window procedure down in widget. So we've added a ReplyMessage to this to free the child.

child:

user32.dll!_SendMessageW@16()  
user32.dll!_DefWindowProcW@16() 
NPSWF32.dll!729611e3()
xul.dll!mozilla::plugins::PluginInstanceChild::PluginWindowProc

parent:

xul.dll!nsWindow::OnMouseWheel
(ReplyMessage)
xul.dll!nsWindow::WindowProc
xul.dll!mozilla::plugins::PluginInstanceParent::PluginWindowHookProc
xul.dll!PluginWndProc
Looks like after my patch applied, that shouldn't be reproduced. See my patch's nsWindow::ProcessMessage().

And why didn't you add other 3 messages to the patch? I.e., WM_MOUSEHWHEEL, WM_VSCROLL, WM_HSCROLL.
(In reply to comment #178)
> Looks like after my patch applied, that shouldn't be reproduced. See my patch's
> nsWindow::ProcessMessage().

This was really targeted at 3.6.4, but it had to land on m-c before it could be approved. Feel free to modify the changes as needed in your patch for trunk.

> 
> And why didn't you add other 3 messages to the patch? I.e., WM_MOUSEHWHEEL,
> WM_VSCROLL, WM_HSCROLL.

Would those potentially be forwarded by the child window? Sounds like the plugin would never forward those to it's parent - 

The WM_VSCROLL message is sent to a window when a scroll event occurs in the window's standard vertical scroll bar. This message is also sent to the owner of a vertical scroll bar control when a scroll event occurs in the control.

The WM_HSCROLL message is sent to a window when a scroll event occurs in the window's standard horizontal scroll bar. This message is also sent to the owner of a horizontal scroll bar control when a scroll event occurs in the control.
> Would those potentially be forwarded by the child window?

I think so, they are sent from some mouse drivers and some capturing utilities too. So, if a plug-in don't handle them because they don't assume the messages come, they will come via CallNextWndProc().
I confirmed there are still some deadlock bugs only with your patch. So, we still need to call ReplyMessage() before dispatching our events. I have a question, how to reproduce the bug 561117?
(In reply to comment #181)
> I confirmed there are still some deadlock bugs only with your patch.

Oops, sorry. I took a mistake, I'll retest it. Please ignore this.
I retested. When I remove ReplyMessage()s from OnMouseWheel() and OnScroll(), it works fine for WM_MOUSEWHEEL. So, I should remove them in next patch. However, I can reproduce the deadlock when I use tilt wheel, so, you should care the other 3 messages.
(In reply to comment #180)
> > Would those potentially be forwarded by the child window?
> 
> I think so, they are sent from some mouse drivers and some capturing utilities
> too. So, if a plug-in don't handle them because they don't assume the messages
> come, they will come via CallNextWndProc().

So these events would be sent from the plugin-container.exe process via SendMessage to the parent process? Would either of these two events cause an ipc call to the child?
(In reply to comment #181)
> I confirmed there are still some deadlock bugs only with your patch. So, we
> still need to call ReplyMessage() before dispatching our events. I have a
> question, how to reproduce the bug 561117?

We don't have STR, just hang stacks from crash stats.
(In reply to comment #184)
> (In reply to comment #180)
> > > Would those potentially be forwarded by the child window?
> > 
> > I think so, they are sent from some mouse drivers and some capturing utilities
> > too. So, if a plug-in don't handle them because they don't assume the messages
> > come, they will come via CallNextWndProc().
> 
> So these events would be sent from the plugin-container.exe process via
> SendMessage to the parent process? Would either of these two events cause an
> ipc call to the child?

These messages cause dispatching mouse scroll events. When the events go to the plug-in process, it causes deadlock if the messages sent by the plug-in process.
By bug 561495 change, the patch will be broken. The child process will use ReplyMessage() before calling the wndproc of the plug-in. So, I need to redesign the architecture of the patch. We cannot return the result of nsIWidget::DispatchEventToPlugin() synchronously, so, the method should just dispatch the native event to plug-in. And child process need to notify after it called the plug-in wndproc. Then, nsWindow can know whether the message was consumed or not by tricky way. Then, nsWindow should dispatch a new event.
Depends on: 561495
Attachment #440697 - Flags: review?(joshmoz)
Attachment #440697 - Flags: review?(jmathies)
Duplicate of this bug: 568009
Ugh, there is another problem. Mouse drivers may post two or more messages at once. Then, the second WM_MOUSE*WHEEL message may come before the plug-in finishes handling the first message.

If we can wait the end of handling in plug-in (by ::Sleep() ?), the story becomes simple. But if it causes new dead lock bug by ::SendMessage() by the plug-in, I need to look for another approach.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
i am not sure if this is related but after clicking on a plug-in control (e.g. Play), mouse will not scroll even if you move out of the plug-in area.
I'm wondering why this is blocking.  I don't see any comments to indicate why it is, and it hasn't blocked any of the previous releases.  Anyone?
Also, I should say that I don't think this should block.
It's a very old regression that we should do our utmost to fix. If we need to unblock on it, we can, but for now we should act as if it's a blocker.
Well, this used to work in Firefox3.0, so I wouldn't call this a very old regression.
(In reply to comment #194)
> Well, this used to work in Firefox3.0, so I wouldn't call this a very old
> regression.

It didn't work in 3.0.  See comment 2.  Again, I don't think this should be a blocker.  I'd rather have people working on the already large list of blockers that, well, are real.
Whiteboard: parity-chrome
I'm not seeing this issue any more. Can anyone else confirm?
http://www.youtube.com/watch?v=K7l0a2PVhPQ&feature=player_embedded

It is not yet fixed.
http://hg.mozilla.org/mozilla-central/rev/b1d56e69f4d9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100920 Firefox/4.0b7pre ID:20100920042006
That is weird. With your link, I see the problem, but when I go to http://www.youtube.com/watch?v=fAVRNA1l1wM&feature=sub, I don't see the problem. I'm using the same build that you are.

(In reply to comment #197)
> http://www.youtube.com/watch?v=K7l0a2PVhPQ&feature=player_embedded
> 
> It is not yet fixed.
We really have more important fish to fry, it's not a new regression and it doesn't block.
blocking2.0: betaN+ → -
Summary: Scrolling stops when mouse cursor is over a plugin (flash (youtube), silverlight) → mouse wheel scrolling stops when mouse cursor is over a plugin (flash (youtube), silverlight)
I've tried Firefox 4, beta 7 and the same fault happens. Why not just close this fault as after almost 2 years it's clear it will never be fixed.
I'm the instigator that has caused this bug to be revisited.  
It happens on Windows -and- Mac, on Firefox 4 beta 7 -and- beta 8.  It is easily reproducible, and does -not- occur in Firefox 3.  And it needs to be fixed before we ship Firefox 4.  Why?  Because it makes us look bad; no other browser has this behavior, including all other versions of Firefox.  

And it's REALLY F***ING ANNOYING.  You're happily scrolling down a page, and then scrolling just stops.  You can't scroll up.  You can't scroll down.  You can't scroll sideways.  Is the browser hung?  Is my computer hung?  Is my internet connection broken?  No, you just need to manually move your cursor out of the embedded Flash object.  Tough luck for you if it is as wide as the window.
(In reply to comment #200)
> I've tried Firefox 4, beta 7 and the same fault happens. Why not just close
> this fault as after almost 2 years it's clear it will never be fixed.

Because it's worthy of fixing.  If it weren't, it would be closed WONTFIX.  There are bugs in this bugzilla that have been around since 1998, if they haven't been fixed yet it's because there are higher priority bugs to fix.

(In reply to comment #201)
> I'm the instigator that has caused this bug to be revisited.  

This attitude isn't really appropriate here.

> It happens on Windows -and- Mac, on Firefox 4 beta 7 -and- beta 8.  It is
> easily reproducible, and does -not- occur in Firefox 3.  And it needs to be
> fixed before we ship Firefox 4.  Why?  Because it makes us look bad; no other
> browser has this behavior, including all other versions of Firefox.  

Based on when it was filed, it's presumably in 3.6.  Comments indicate that it's in 3.0 as well, so this isn't new.  I'd test myself but I don't have a mouse ;-)

Also, if this occurs on Mac it's in the wrong component.

> And it's REALLY F***ING ANNOYING.  You're happily scrolling down a page, and
> then scrolling just stops.  You can't scroll up.  You can't scroll down.  You
> can't scroll sideways.  Is the browser hung?  Is my computer hung?  Is my
> internet connection broken?  No, you just need to manually move your cursor out
> of the embedded Flash object.  Tough luck for you if it is as wide as the
> window.

Nobody disputes that this leads to a bad user experience.  If you have new information that should lead to a retriaging of this bug, this post it and set the proper annotations (I think we're using [d?] in the whiteboard for this for Fx4).
I'm changing the platform field because it occurs on Mac and Windows.
I have tested, and can verify that it does not occur 3.6.  This has been verified by multiple people.
OS: Windows XP → All
Depends on: 611223
Don't change the platform. I know what same bug is on the other platforms. But we need patch for every platform. I tried to fix this bug on Windows first. But unfortunately, the way for fixing this bug has become very difficult due to OOPP. See the patch and comment 189.


Please don't post any unnecessary comments here.
It would seem to me that this bug and bug 78414 are generally related -- that plugins are allowed to capture all controls when in focus.

Is there some way to create some sort of input handling "proxy" that merely shadows control inputs to the focused plugin, instead of handing it all over?
IU:

This bug is really different bug. On windows, with typical mouse wheel utility, the WM_MOUSE*WHEEL message is sent to our window because we have native focus except that plugins create its owning window themselves. If mouse cursor is on a plugin window, we redirect the message to the window like subframe scrolling by overflow:auto;. And if the plugin does NOT process the message, i.e., the message is back to our window again, we're handling it. Then, our scrollable view is scrolled by the mouse wheel even if the mouse cursor is on plugin content. On the other hand, some Flash Player applications listen mousewheel event even if it does NOT need to handle the event. In this case, Flash Player always consumes the WM_MOUSE*WHEEL message for such application (Flash Player thinks that the application processes (consumes) the mousewheel event because it listens the event), i.e., it does NOT send back the message to us. Then, users look nothing happens by their mouse wheel operation. This is the cause of this bug.
My current idea is, we can define our internal messages for WM_MOUSEWHEEL and WM_MOUSEHWHEEL such as WM_MOZ_MOUSEWHEEL and WM_MOZ_MOUSEHWHEEL.

If we receives WM_MOUSE*WHEEL messages and cursor is NOT on plugin window, we should process the messages directly.

If we receives WM_MOUSE*WHEEL messages and cursor is on plugin window, we should redirect the message to plugin window. And our plugin-container's procedure should send back WM_MOZ_MOUSE*WHEEL to us instead of WM_MOUSE*WHEEL. Then, we can know whether the message has been processed by plugin already.

If we receives WM_MOZ_MOUSE*WHEEL messages, we should dispatch our internal mousewheel event with a flag which means plugin already processed the event.

If plugin consumes the mouse wheel messages, plugin-container should send back WM_MOZ_MOUSEWHEEL_CONSUMED to our window. Then, we can know the fact. Then, ESM can know current mouse wheel transaction is on the plugin.

However, there is a problem. DispatchEventToPlugin() cannot return the result synchronously. I need to redesign this method.

Anyway, we need big patch for doing this. So, it's too late for Fx4.
(In reply to comment #207)
> If we receives WM_MOZ_MOUSE*WHEEL messages, we should dispatch our internal
> mousewheel event with a flag which means plugin already processed the event.

Of course, this shouldn't cause MozMouseWheel DOM event being dispatched.
Duplicate of this bug: 625805
Hello,
I'm running FF 4 b10 and the fault still happens. You can see it here:
http://supermeatboy.com/

Scroll down to the first video.

Hope this helps.
Whiteboard: parity-chrome → parity-chrome [see comments 204, 207]
Filed Bug 631872 for a new regression in nightly trunk for stealing a mouse wheel scroll focus outside the plugin area.
bug confirmed on Win 7 too, plz change that in bug details
Keywords: common-issue?
From the Flash Player's perspective with the current Firefox 4 and earlier releases, this bug occurs when the Flash Player content is registered for the mousewheel event. Whether or not the content uses the event, the Flash Player assumes it has done something useful with the event because the content registered for it (and this is a hole in the ActionScript event handling spec).

YouTube is the primary Flash content that registers for the mousewheel event but does not do anything with it. Adobe has asked YouTube to fix this, and they have done so in their ActionScript 3 content. Unfortunately, their ActionScript 2 content is frozen and won't be fixed.

Hope this helps a bit.
This happens on Mac OS X as well.
Just an idea for hacking this bug. If we think that it is default action to events to be handled by a plugin, we can put off posting events to plugin to ESM::PostHandleEvent(). If there is a wheel transaction, the events shouldn't be posted to plugins. Otherwise, should notify it to widget.

When widget receives it, the widget should send a native event to the plugin (Windows) or dispatch a new plugin event (Mac). After that if widget can know whether the event is handled by the plugin and it's not consumed, widget should dispatch a new internal mouse wheel event which is only handled by ESM.

If a mouse wheel event were delayed for long time by plugins, it would work awkwardly. But that is already existing on current build.

A merit of this hack is, we can make ESM not notify it to widget when the plugin doesn't have focus. If user can choose the behavior by prefs, some users will really love it.
Anyway, I need to fix bug 672175 first.
Depends on: 672175
Easy fix for people see http://pastebin.com/wVtSRszM ,sorry for posting it here i hope you understand how much frustrating is this.
What's the reason for this defect still being open.  Is it being blocked by something, or just de-prioritised?

Thanks
Now, I cannot reproduce this bug with youtube video and other URLs mentioned in above comments too.

Does somebody have an example of this bug?
platform-rel: --- → ?
Whiteboard: parity-chrome [see comments 204, 207] → parity-chrome [see comments 204, 207] [platform-rel-Youtube]
Priority: -- → P2
Whiteboard: parity-chrome [see comments 204, 207] [platform-rel-Youtube] → parity-chrome [see comments 204, 207] [platform-rel-Youtube][tpi:+]
platform-rel: ? → ---
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: parity-chrome [see comments 204, 207] [platform-rel-Youtube][tpi:+] → [see comments 204, 207] [platform-rel-Youtube][tpi:+]

Plugins are no longer a thing, closing.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.