376679 - WM_MOUSEWHEEL and WM_MOUSEHWHEEL messages should be delivered to windowless plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Supriya Rao]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

The scrollable components in a swf that is embedded in the html as windowless mode doesn't scroll while using the mouse wheel. This is because Firefox is not sending WM_MOUSEWHEEL event to flash. 

Reproducible: Always

Steps to Reproduce:
1. open the NotWorking.html in  files submitted
2. try to scroll through the list box using the mousewheel

Actual Results:  
Doesnt scroll

Expected Results:  
should scroll, like it happens when no wmode is used (open working.html)

Comment 1

[Supriya Rao]

Attachment: The zip contains the html and swf that demonstrate the bug

Comment 2

[Erik Abair]

I think this is related to (or a duplicate of) bug 359403, which provides an additional test case.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is important bug for x64 build since x64 build for Windows will support only windoowless mode. My test patch works fine. I'll brush up them before review.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I realized that we cannot know if windowless plugin *will* handle/consume sending WM_MOUSE*WHEEL messages actually. So, for preventing double action, we should do nothing after we send the wheel messages to windowless plugin. This means that operating wheel on windowless plugin may not cause anything even if the plugin content doesn't handle wheel messages.

For preventing this issue as far as possible, we should check wheel transaction *before* sending the messages. This means that we cannot use usual path to send native message to the target plugin. I think that we should include the plugin frame as scrollable frame in WheelHandlingHelper.cpp.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=624f3745b96b

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c3ceab0881b

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Change the result of EventStateManager::ComputeScrollTarget() from nsIScrollableFrame* to nsIFrame*

The strategy to fix this bug is that we should think that sending wheel events to plugin process is one of default actions of WidgetWheelEvent. If there is a wheel transaction, we should keep scrolling even if mouse cursor is over a plugin. Even if plugin content doesn't have any scrollable content, we should send wheel events if it's a windowless plugin on Windows since we *cannot* know a plugin will handle the wheel event actually.

So, for preparing this new behavior, EventStateManager::ComputeScrollTarget() should return nsIFrame* instead of nsIScrollableFrame because nsPluginFrame isn't scrollable but we will assume that it may be scrollable.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 EventStateManager should treat plugin frame as scrollable frame if the plugin wants to handle wheel events as default action

EventStateManager::ComputScrollTarget() should return nsPluginFrame when:

* the target frame of current wheel transaction is an nsPluginFrame
* there is no active wheel transaction and the frame under mouse cursor is nsPluginFrame

But this should be restricted for computing the default action target. For computing legacy mouse scroll event target, we should keep current behavior for backward compatibility.

Finally, if the result of ComputeScrollTarget() is an nsPluginFrame, the default action should be handled in the frame.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Compute default action target frame for wheel event before deciding the action because plugin should decide what is the default action when the target is a plugin frame

If the scroll target of default action of wheel event is nsPluginFrame, EventStateManager shouldn't decide the default action (scroll vs. zoom vs. history navigation vs. none) since it should be decided by the plugin. Therefore, before deciding the action, PostHandleEvent() should check if the scroll target is an nsPluginFrame.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Manage wheel transaction at sending a wheel event to target plugin

When an nsPluginFrame handles default action of a wheel event, it should cause starting/ending wheel transaction. So, we should separate the modifying wheel transaction code in ComputeScrollTarget() to WheelTransaction. And before sending wheel event to nsPluginFrame, EventStateManager should call it.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 nsPluginInstanceOwner::ProcessEvent() should create WM_MOUSE*WHEEL message from WidgetWheelEvent data

Now, WidgetWheelEvent is sent to nsPluginFrame which has a windowless plugin at handling default of the event. In the event, native messages are not set since plugins may not support high resolution wheel scroll and we don't dispatch WidgetWheelEvent if accumulated delta values are too small to scroll the contents. Therefore, we need to generate WM_MOUSE*WHEEL message from WidgetWheelEvent. When one or more lines or pages should be scrolled, the integer value is set to WidgetWheelEvent::lineOrPageDeltaX/Y. We should generate the message only when the value is not 0.

jimm: Could you review almost all of this patch except MouseEvents.h? Note that this patch assumes that all users haven't customized the scroll amount in the system settings. Next patch fixes this issue.

smaug: Could you review the MouseEvents.h part?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 nsPluginInstanceOwner::ProcessEvent() should refer both deltaMode and system scroll amount settings when it generates WM_MOSUE*WHEEL messages

This fixes the cases of customized system settings.

If deltaMode is paged scroll, the value should be 1 or -1. We should just multiply it by WHEEL_DELTA.

If deltaMode is lined scroll, the scroll amount per WHEEL_DELTA is the result of SPI_GETWHEELSCROLLLINES or SPI_GETWHEELSCROLLCHARS.

Otherwise, we should do nothing.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I forgot to say. These patch are not APZC-aware. However, it's okay for now. This is important when we make all flash players as windowed plugin on x64 build.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> I forgot to say. These patch are not APZC-aware. However, it's okay for now.
Curious, why is it ok?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8673069 [details] [diff] [review]
part.2 EventStateManager should treat plugin frame as scrollable frame if the plugin wants to handle wheel events as default action

So I like this approach, but what guarantees we haven't already sent the event to the plugin in nsPresShellEventCB::HandleEvent?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8673073 [details] [diff] [review]
part.4 Manage wheel transaction at sending a wheel event to target plugin

> EventStateManager::DoScrollText(nsIScrollableFrame* aScrollableFrame,
>                                 WidgetWheelEvent* aEvent)
> {
>   MOZ_ASSERT(aScrollableFrame);
>   MOZ_ASSERT(aEvent);
> 
>   nsIFrame* scrollFrame = do_QueryFrame(aScrollableFrame);
>   MOZ_ASSERT(scrollFrame);
>+
>+  if (!WheelTransaction::WillHandleDefaultAction(aEvent, scrollFrame)) {
>+    return;
>+  }
>+
>   nsWeakFrame scrollFrameWeak(scrollFrame);

Could you move WillHandleDefaultAction call here, and make that method to take
nsWeakFrame&.
That way it is then clear to the caller that the frame might die.

>+WheelTransaction::WillHandleDefaultAction(WidgetWheelEvent* aWheelEvent,
>+                                          nsIFrame* aTargetFrame)
>+{
>+  nsWeakFrame targetFrameWeak(aTargetFrame);
then you wouldn't need this here

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8673069 [details] [diff] [review]
part.2 EventStateManager should treat plugin frame as scrollable frame if the plugin wants to handle wheel events as default action

So at least I would expect some MOZ_ASSERTs in nsPluginFrame::HandleEvent to make sure we don't double handle wheel events.
Or some explanation why that doesn't happen.

(It is not quite clear to me why we couldn't just handle the event there and set eventStatus to nsEventStatus_eConsumeNoDefault or something.)

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(Though, I think all EventDispatchingCallback handling should be moved to happen after system group, or during it, if possible. Calling EventDispatchingCallback between event groups is odd, old behavior. But that change would be about some other bug.)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8673069 [details] [diff] [review]
> part.2 EventStateManager should treat plugin frame as scrollable frame if
> the plugin wants to handle wheel events as default action
> 
> So I like this approach, but what guarantees we haven't already sent the
> event to the plugin in nsPresShellEventCB::HandleEvent?

On Windows, we don't set native message to WidgetWheelEvent. Therefore, wheel events are never sent to plugins. Generating the message from WidgetWheelEvent is the trick to guarantee that.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8673069 [details] [diff] [review]
part.2 EventStateManager should treat plugin frame as scrollable frame if the plugin wants to handle wheel events as default action

See the previous comment. nsPluginFrame::HandleEvent() never handles WidgetWheelEvent on Windows due to no native message.

However, PostHandleEvent() (or HandleWheelEventAsDefaultAction()) can check if WidgetGUIEvent::mPluginEvent is nullptr.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> > I forgot to say. These patch are not APZC-aware. However, it's okay for now.
> Curious, why is it ok?

Because APZC isn't enabled in release build not so soon. I (or APZC team) can work it later. (I don't understand well the design of APZC and I have some other urgent bugs for 44. So, I don't have much time.)

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 EventStateManager should treat plugin frame as scrollable frame if the plugin wants to handle wheel events as default action

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Manage wheel transaction at sending a wheel event to target plugin (r=smaug)

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note for myself:

APZC scrolls the content before dispatching wheel events from TabChild. Therefore, we need to hack TabChild::RecvMouseWheelEvent() or PrepareForSetTargetAPZCNotification(). However, I cannot explain why calling WheelEvent.preventDefault() works with this design...

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> Because APZC isn't enabled in release build not so soon. I (or APZC team)
> can work it later. (I don't understand well the design of APZC and I have
> some other urgent bugs for 44. So, I don't have much time.)
I thought apz would be enabled when e10s will be, but maybe the plans have changed.
(and I thought e10s will be on by default rather soon)
But sure, apz handling can be done in a different bug.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8673485 [details] [diff] [review]
part.2 EventStateManager should treat plugin frame as scrollable frame if the plugin wants to handle wheel events as default action

I see, and nsPluginInstanceOwner::Init seems to be rather horrible :/
But that issue is not about this bug, and it all started in Bug 55822.


+  if (NS_WARN_IF(!!aWheelEvent->mPluginEvent)) {
I would just remove '!!'

Comment 28

[Jim Mathies [:jimm]]

Comment on attachment 8673083 [details] [diff] [review]
part.5 nsPluginInstanceOwner::ProcessEvent() should create WM_MOUSE*WHEEL message from WidgetWheelEvent data

Review of attachment 8673083 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +2084,4 @@
>          pluginEvent.event = upMsgs[mouseEvent->button];
>          break;
>        }
> +      // For plugins which doesn't support high-resolution scroll, we should

s/doesn't/don't

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7208b172fb6a

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8673485 [details] [diff] [review]
> part.2 EventStateManager should treat plugin frame as scrollable frame if
> the plugin wants to handle wheel events as default action
> 
> I see, and nsPluginInstanceOwner::Init seems to be rather horrible :/
> But that issue is not about this bug, and it all started in Bug 55822.
> 
> 
> +  if (NS_WARN_IF(!!aWheelEvent->mPluginEvent)) {
> I would just remove '!!'

Hmm, without the |!!|, gcc doesn't think it as bool nor pointer.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=114fe6babfa6
http://mxr.mozilla.org/mozilla-central/source/widget/BasicEvents.h#463

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=405e72f8ca3b

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b077decb629516239e8561ffb9766f2c70b9f08f
Bug 376679 part.1 Change the result of EventStateManager::ComputeScrollTarget() from nsIScrollableFrame* to nsIFrame* r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/617fd3741cc955d890fbb3541a4490fc2a9f3003
Bug 376679 part.2 EventStateManager should treat plugin frame as scrollable frame if the plugin wants to handle wheel events as default action r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaaeec4b918b4e6a1de7b5edc63bcc79172ae095
Bug 376679 part.3 Compute default action target frame for wheel event before deciding the action because plugin should decide what is the default action when the target is a plugin frame r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/7413b0258e1db92d7e3efe62a6d36d6360b3d50b
Bug 376679 part.4 Manage wheel transaction at sending a wheel event to target plugin r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8adf2083f85b0524452e31fe592787337222b8
Bug 376679 part.5 nsPluginInstanceOwner::ProcessEvent() should create WM_MOUSE*WHEEL message from WidgetWheelEvent data r=smaug+jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/168aa78da27c9f9b54270a2850f1f6b034ca5d5e
Bug 376679 part.6 nsPluginInstanceOwner::ProcessEvent() should refer both deltaMode and system scroll amount settings when it generates WM_MOSUE*WHEEL messages r=jimm

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b077decb6295
https://hg.mozilla.org/mozilla-central/rev/617fd3741cc9
https://hg.mozilla.org/mozilla-central/rev/eaaeec4b918b
https://hg.mozilla.org/mozilla-central/rev/7413b0258e1d
https://hg.mozilla.org/mozilla-central/rev/dd8adf2083f8
https://hg.mozilla.org/mozilla-central/rev/168aa78da27c

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b077decb6295
https://hg.mozilla.org/mozilla-central/rev/617fd3741cc9
https://hg.mozilla.org/mozilla-central/rev/eaaeec4b918b
https://hg.mozilla.org/mozilla-central/rev/7413b0258e1d
https://hg.mozilla.org/mozilla-central/rev/dd8adf2083f8
https://hg.mozilla.org/mozilla-central/rev/168aa78da27c