Closed Bug 376679 Opened 13 years ago Closed 4 years ago

WM_MOUSEWHEEL and WM_MOUSEHWHEEL messages should be delivered to windowless plugins

Categories

(Core :: Plug-ins, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: Supriya.Rao, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 2 obsolete files)

130.73 KB, application/zip
Details
12.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.27 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.44 KB, patch
jimm
: review+
smaug
: review+
Details | Diff | Splinter Review
5.03 KB, patch
jimm
: review+
Details | Diff | Splinter Review
11.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.29 KB, patch
Details | Diff | Splinter Review
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)
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
I think this is related to (or a duplicate of) bug 359403, which provides an additional test case.
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 359403
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.
Assignee: nobody → masayuki
Blocks: 1201904
Status: RESOLVED → REOPENED
Ever confirmed: true
OS: Windows XP → Windows
Resolution: DUPLICATE → ---
Summary: Firefox does not send up WM_MOUSEWHEEL events in windowless mode wmode=transparent → WM_MOUSEWHEEL and WM_MOUSEHWHEEL messages should be delivered to windowless plugins
Version: unspecified → Trunk
Status: REOPENED → ASSIGNED
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.
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.
Attachment #8673066 - Flags: review?(bugs)
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.
Attachment #8673069 - Flags: review?(bugs)
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.
Attachment #8673072 - Flags: review?(bugs)
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.
Attachment #8673073 - Flags: review?(bugs)
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?
Attachment #8673083 - Flags: review?(jmathies)
Attachment #8673083 - Flags: review?(bugs)
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.
Attachment #8673084 - Flags: review?(jmathies)
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.
Hardware: x86 → Unspecified
(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?
Attachment #8673066 - Flags: review?(bugs) → review+
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?
Attachment #8673083 - Flags: review?(bugs) → review+
Attachment #8673072 - Flags: review?(bugs) → review+
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
Attachment #8673073 - Flags: review?(bugs) → review+
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.)
Attachment #8673069 - Flags: review?(bugs) → review-
(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.)
(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 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.
Attachment #8673069 - Flags: review- → review?(bugs)
(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.)
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...
(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 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 '!!'
Attachment #8673485 - Flags: review?(bugs) → review+
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
Attachment #8673083 - Flags: review?(jmathies) → review+
Attachment #8673084 - Flags: review?(jmathies) → review+
(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
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
No longer depends on: 1211824
Duplicate of this bug: 560964
You need to log in before you can comment on or make changes to this bug.