1250050 - Add a pref to control whether wheel events should be sent to plug-ins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Masatoshi Kimura [:emk]]

This is intended a temporary workaround until bug 1234958 gets fixed in the proper manner.

Comment 1

[Masatoshi Kimura [:emk]]

And this pref will not be enabled by default, of course.

Nakano-san, do you have a bandwidth to write a patch? If not, I will take this.

Comment 2

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

Wait a moment, I'm now trying to fix bug 1234958.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=227072aa00d6

Comment 4

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

Attachment: Add a pref to disable supporting mouse wheel of windowless plugins on Windows

I guess that for fixing bug 1234958, we need to wait a fix of Adobe. So, temporarily, we should add a pref to disable mouse wheel support on windowless plugins on Windows.

Now, whole swf contents are performed in windowless on Firefox for Windows x64. So, bug 1234958 becomes more serious bug for x64 users. This is the reason why we should add this pref.

Comment 5

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

Comment on attachment 8722343 [details] [diff] [review]
Add a pref to disable supporting mouse wheel of windowless plugins on Windows

So this effectively backs out  Bug 376679, right? I mean not the code, but behavior.

>+bool EventStateManager::WheelPrefs::sIsSupportingWheelEvents = true;
A bit odd name, same also with  EventStateManager::WheelPrefs::IsSupportingWheelEvents()
Perhaps, WheelEventsEnabledOnPlugins?



+  if ((aOptions & INCLUDE_PLUGIN_AS_TARGET) &&
+      !WheelPrefs::IsSupportingWheelEvents()) {
+    aOptions = static_cast<ComputeScrollTargetOptions>(
+      static_cast<uint32_t>(aOptions) & ~INCLUDE_PLUGIN_AS_TARGET);
+  }
Hmm, what is evil. After that aOptions has value which isn't defined in the relevant enum.
But looks like we use similar setup elsewhere, but without that extra cast to uint32_t, so drop that?

And file a followup bug to fix ComputeScrollTargetOptions. We shouldn't use enum variables for random values, IMO.

Comment 6

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

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #5)
> Comment on attachment 8722343 [details] [diff] [review]
> Add a pref to disable supporting mouse wheel of windowless plugins on Windows
> 
> So this effectively backs out  Bug 376679, right? I mean not the code, but
> behavior.

No, this is just a way to disable mouse wheel feature on windowless plugins. Do you think that it's better to disable it in default settings?

Comment 7

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

> +  if ((aOptions & INCLUDE_PLUGIN_AS_TARGET) &&
> +      !WheelPrefs::IsSupportingWheelEvents()) {
> +    aOptions = static_cast<ComputeScrollTargetOptions>(
> +      static_cast<uint32_t>(aOptions) & ~INCLUDE_PLUGIN_AS_TARGET);
> +  }
> Hmm, what is evil. After that aOptions has value which isn't defined in the relevant enum.
> But looks like we use similar setup elsewhere, but without that extra cast to uint32_t, so drop that?
> 
> And file a followup bug to fix ComputeScrollTargetOptions. We shouldn't use enum variables for random values, IMO.

Hmm, I don't like to make it uint32_t or something because defining with enum make other developers really easy to understand what value should be set.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a16b78ffe0

Comment 9

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6) Add a pref to disable supporting mouse wheel of windowless plugins on Windows
> > 
> > So this effectively backs out  Bug 376679, right? I mean not the code, but
> > behavior.
> 
> No, this is just a way to disable mouse wheel feature on windowless plugins.
How is that different than to what I said? Bug 376679 made us send wheel events to windowless plugins, and
this bug disables it.


> Do you think that it's better to disable it in default settings?
I don't understand this question.



> Hmm, I don't like to make it uint32_t or something because defining with
> enum make other developers really easy to understand what value should be
> set.
Well, add a new enum value which doesn't have INCLUDE_PLUGIN_AS_TARGET? That way ~INCLUDE_PLUGIN_AS_TARGET would still lead to valid enum value.

Comment 10

[Tony Mechelynck [:tonymec]]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #9)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> Add a pref to disable supporting mouse wheel of windowless plugins on Windows
> > > 
> > > So this effectively backs out  Bug 376679, right? I mean not the code, but
> > > behavior.
> > 
> > No, this is just a way to disable mouse wheel feature on windowless plugins.
> How is that different than to what I said? Bug 376679 made us send wheel
> events to windowless plugins, and
> this bug disables it.

No, this bug adds a pref letting the user to choose between passing the mouse wheel event to the plugin or not. According to comment #1, with the pref in its default state the events would still be passed (and the fix for bug 376679 would remain in force) while toggling the pref would insulate the plugin from the mouse wheel (in practice "reversibly" backing out that fix at runtime).

> 
> 
> > Do you think that it's better to disable it in default settings?
> I don't understand this question.
> 
> 
> 
> > Hmm, I don't like to make it uint32_t or something because defining with
> > enum make other developers really easy to understand what value should be
> > set.
> Well, add a new enum value which doesn't have INCLUDE_PLUGIN_AS_TARGET? That
> way ~INCLUDE_PLUGIN_AS_TARGET would still lead to valid enum value.

For a pref of this kind, and depending on the name of the pref, I would expect a BoolPref, passing the events in one state and not in the other state. Alternatively, another type of pref (maybe a URL i.e. a StringPref) would allow passing the mouse events to some plugin instances and not to others.

Just my 0.02 € of course.

Comment 11

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

(In reply to Tony Mechelynck [:tonymec] from comment #10)
> (In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #9)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> > Add a pref to disable supporting mouse wheel of windowless plugins on Windows
> > > > 
> > > > So this effectively backs out  Bug 376679, right? I mean not the code, but
> > > > behavior.
> > > 
> > > No, this is just a way to disable mouse wheel feature on windowless plugins.
> > How is that different than to what I said? Bug 376679 made us send wheel
> > events to windowless plugins, and
> > this bug disables it.
> 
> No, this bug adds a pref letting the user to choose between passing the
> mouse wheel event to the plugin or not. According to comment #1, with the
> pref in its default state the events would still be passed (and the fix for
> bug 376679 would remain in force) while toggling the pref would insulate the
> plugin from the mouse wheel (in practice "reversibly" backing out that fix
> at runtime).

Exactly. I meant that my patch will NOT change the behavior in default settings and most users won't change hidden prefs only in about:config. So, this is really different from backing out of the patches of bug 376679 for most users.

My question was, whether we should or should not disable supporting mouse wheel on windowless plugins *in default settings*.

I think that even if it causes some inconvenience, we should keep supporting mouse wheel events on windowless plugins in default settings since some swf application may become unusable. I know some applications use mouse wheel for zoom and/or scroll. So, I think that disabling it in the default settings would make some swf applications stop working well on Windows x64 build.

>> Hmm, I don't like to make it uint32_t or something because defining with
>> enum make other developers really easy to understand what value should be
>> set.
> Well, add a new enum value which doesn't have INCLUDE_PLUGIN_AS_TARGET? That way ~INCLUDE_PLUGIN_AS_TARGET would still lead to valid enum value.

Ah, I see. I'll modify the patch.

Comment 12

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

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

Comment 13

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

Attachment: Add a pref to disable supporting mouse wheel of windowless plugins on Windows

Improving around ComputeScrollTargetOptions.

Comment 14

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

oh, right, the default value is still to be broken or something. Then I don't understand why we want to add yet another pref which no one knows about.

Comment 15

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

So either we don't take the patch, or modify the patch so that pref is false by default.

Comment 16

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

during the lunch I changed my mind :) Maybe we could take the patch and think about the default value in some other bug.

Comment 17

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

Comment on attachment 8723402 [details] [diff] [review]
Add a pref to disable supporting mouse wheel of windowless plugins on Windows

># HG changeset patch
># User Masayuki Nakano <[e-mail]>
># Parent  d848a5628d801a460a7244cbcdea22d328d8b310
>Bug 1250050 Add a pref to disable supporting mouse wheel of windowless plugins on Windows r=smaug
>
>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
>--- a/dom/events/EventStateManager.cpp
>+++ b/dom/events/EventStateManager.cpp
>@@ -273,16 +273,17 @@ nsWeakPtr EventStateManager::sPointerLoc
> // Reference to the document which requested pointer lock.
> nsWeakPtr EventStateManager::sPointerLockedDoc;
> nsCOMPtr<nsIContent> EventStateManager::sDragOverContent = nullptr;
> TimeStamp EventStateManager::sLatestUserInputStart;
> TimeStamp EventStateManager::sHandlingInputStart;
> 
> EventStateManager::WheelPrefs*
>   EventStateManager::WheelPrefs::sInstance = nullptr;
>+bool EventStateManager::WheelPrefs::sWheelEventsEnabledOnPlugins = true;
> EventStateManager::DeltaAccumulator*
>   EventStateManager::DeltaAccumulator::sInstance = nullptr;
> 
> EventStateManager::EventStateManager()
>   : mLockCursor(0)
>   , mLastFrameConsumedSetCursor(false)
>   , mPreLockPoint(0,0)
>   , mCurrentTarget(nullptr)
>@@ -2362,16 +2363,21 @@ EventStateManager::ComputeScrollTarget(n
> // and before any actual motion
> nsIFrame*
> EventStateManager::ComputeScrollTarget(nsIFrame* aTargetFrame,
>                                        double aDirectionX,
>                                        double aDirectionY,
>                                        WidgetWheelEvent* aEvent,
>                                        ComputeScrollTargetOptions aOptions)
> {
>+  if ((aOptions & INCLUDE_PLUGIN_AS_TARGET) &&
>+      !WheelPrefs::WheelEventsEnabledOnPlugins()) {
>+    aOptions = RemovePluginFromTarget(aOptions);
>+  }
>+
>   if (aOptions & PREFER_MOUSE_WHEEL_TRANSACTION) {
>     // If the user recently scrolled with the mousewheel, then they probably
>     // want to scroll the same view as before instead of the view under the
>     // cursor.  WheelTransaction tracks the frame currently being
>     // scrolled with the mousewheel. We consider the transaction ended when the
>     // mouse moves more than "mousewheel.transaction.ignoremovedelay"
>     // milliseconds after the last scroll operation, or any time the mouse moves
>     // out of the frame, or when more than "mousewheel.transaction.timeout"
>@@ -5510,16 +5516,19 @@ EventStateManager::WheelPrefs::OnPrefCha
>   sInstance->Reset();
>   DeltaAccumulator::GetInstance()->Reset();
> }
> 
> EventStateManager::WheelPrefs::WheelPrefs()
> {
>   Reset();
>   Preferences::RegisterCallback(OnPrefChanged, "mousewheel.", nullptr);
>+  Preferences::AddBoolVarCache(&sWheelEventsEnabledOnPlugins,
>+                               "plugin.mousewheel.enabled",
>+                               true);
> }
> 
> EventStateManager::WheelPrefs::~WheelPrefs()
> {
>   Preferences::UnregisterCallback(OnPrefChanged, "mousewheel.", nullptr);
> }
> 
> void
>@@ -5729,16 +5738,26 @@ EventStateManager::WheelPrefs::GetUserPr
> {
>   Index index = GetIndexFor(aEvent);
>   Init(index);
> 
>   *aOutMultiplierX = mMultiplierX[index];
>   *aOutMultiplierY = mMultiplierY[index];
> }
> 
>+// static
>+bool
>+EventStateManager::WheelPrefs::WheelEventsEnabledOnPlugins()
>+{
>+  if (!sInstance) {
>+    GetInstance(); // initializing sWheelEventsEnabledOnPlugins
>+  }
>+  return sWheelEventsEnabledOnPlugins;
>+}
>+
> bool
> EventStateManager::WheelEventIsScrollAction(WidgetWheelEvent* aEvent)
> {
>   return aEvent->mMessage == eWheel &&
>          WheelPrefs::GetInstance()->ComputeActionFor(aEvent) == WheelPrefs::ACTION_SCROLL;
> }
> 
> void
>diff --git a/dom/events/EventStateManager.h b/dom/events/EventStateManager.h
>--- a/dom/events/EventStateManager.h
>+++ b/dom/events/EventStateManager.h
>@@ -504,16 +504,22 @@ protected:
> 
>     /**
>      * IsOverOnePageScrollAllowed*() checks whether wheel scroll amount should
>      * be rounded down to the page width/height (false) or not (true).
>      */
>     bool IsOverOnePageScrollAllowedX(WidgetWheelEvent* aEvent);
>     bool IsOverOnePageScrollAllowedY(WidgetWheelEvent* aEvent);
> 
>+    /**
>+     * WheelEventsEnabledOnPlugins() returns true if user wants to use mouse
>+     * wheel on plugins.
>+     */
>+    static bool WheelEventsEnabledOnPlugins();
>+
>   private:
>     WheelPrefs();
>     ~WheelPrefs();
> 
>     static void OnPrefChanged(const char* aPrefName, void* aClosure);
> 
>     enum Index
>     {
>@@ -567,16 +573,18 @@ protected:
>     /**
>      * action values overridden by .override_x pref.
>      * If an .override_x value is -1, same as the
>      * corresponding mActions value.
>      */
>     Action mOverriddenActionsX[COUNT_OF_MULTIPLIERS];
> 
>     static WheelPrefs* sInstance;
>+
>+    static bool sWheelEventsEnabledOnPlugins;
>   };
> 
>   /**
>    * DeltaDirection is used for specifying whether the called method should
>    * handle vertical delta or horizontal delta.
>    * This is clearer than using bool.
>    */
>   enum DeltaDirection
>@@ -657,30 +665,43 @@ protected:
>   enum ComputeScrollTargetOptions
>   {
>     // At computing scroll target for legacy mouse events, we should return
>     // first scrollable element even when it's not scrollable to the direction.
>     COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET     = 0,
>     // Default action prefers the scrolled element immediately before if it's
>     // still under the mouse cursor.  Otherwise, it prefers the nearest
>     // scrollable ancestor which will be scrolled actually.
>+    COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN  =
>+      (PREFER_MOUSE_WHEEL_TRANSACTION |
>+       PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS |
>+       PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS),
>     // When this is specified, the result may be nsPluginFrame.  In such case,
>     // the frame doesn't have nsIScrollableFrame interface.
>     COMPUTE_DEFAULT_ACTION_TARGET                =
>-      (PREFER_MOUSE_WHEEL_TRANSACTION |
>-       PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS |
>-       PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS |
>+      (COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN |
>        INCLUDE_PLUGIN_AS_TARGET),
>     // Look for the nearest scrollable ancestor which can be scrollable with
>     // aEvent.
>     COMPUTE_SCROLLABLE_ANCESTOR_ALONG_X_AXIS     =
>       (PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS | START_FROM_PARENT),
>     COMPUTE_SCROLLABLE_ANCESTOR_ALONG_Y_AXIS     =
>       (PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS | START_FROM_PARENT)
>   };
>+  static ComputeScrollTargetOptions RemovePluginFromTarget(
>+                                      ComputeScrollTargetOptions aOptions)
>+  {
>+    switch (aOptions) {
>+      case COMPUTE_DEFAULT_ACTION_TARGET:
>+        return COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN;
>+      default:
>+        MOZ_ASSERT(!(aOptions & INCLUDE_PLUGIN_AS_TARGET));
>+        return aOptions;
>+    }
>+  }
>   nsIFrame* ComputeScrollTarget(nsIFrame* aTargetFrame,
>                                 WidgetWheelEvent* aEvent,
>                                 ComputeScrollTargetOptions aOptions);
> 
>   nsIFrame* ComputeScrollTarget(nsIFrame* aTargetFrame,
>                                 double aDirectionX,
>                                 double aDirectionY,
>                                 WidgetWheelEvent* aEvent,
>diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
>--- a/modules/libpref/init/all.js
>+++ b/modules/libpref/init/all.js
>@@ -3230,16 +3230,19 @@ pref("plugin.scan.Quicktime", "5.0");
> 
> // Locate and scan the Window Media Player installation directory for plugins with a minimum version
> pref("plugin.scan.WindowsMediaPlayer", "7.0");
> 
> // Locate plugins by the directories specified in the Windows registry for PLIDs
> // Which is currently HKLM\Software\MozillaPlugins\xxxPLIDxxx\Path
> pref("plugin.scan.plid.all", true);
> 
>+// Whether sending WM_MOUSEWHEEL and WM_MOUSEHWHEEL to plugins on Windows.
>+pref("plugin.mousewheel.enabled", true);
>+
> // Help Windows NT, 2000, and XP dialup a RAS connection
> // when a network address is unreachable.
> pref("network.autodial-helper.enabled", true);
> 
> // Switch the keyboard layout per window
> pref("intl.keyboard.per_window_layout", false);
> 
> #ifdef NS_ENABLE_TSF

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e8a495ded43f7ef0bf8e9dedc58ddc6c47c82c
Bug 1250050 Add a pref to disable supporting mouse wheel of windowless plugins on Windows r=smaug

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/e7e8a495ded4

Comment 20

[Masatoshi Kimura [:emk]]

Could you consider uplift this?

Comment 21

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

Comment on attachment 8723402 [details] [diff] [review]
Add a pref to disable supporting mouse wheel of windowless plugins on Windows

Approval Request Comment
[Feature/regressing bug #]: Bug 376679
[User impact if declined]: Mouse wheel scrolling are always consumed by *windowless* Flash Player until fixing bug 1234958. This adds a workaround pref for users who don't need to use mouse wheel on plugins (I guess that such users are majority, so, for such users, the new behavior is a regression).
[Describe test coverage new/current, TreeHerder]: Already landed on m-c.
[Risks and why]: Low because this just adds a pref for reverting the patch for bug 376679.
[String/UUID change made/needed]: Nothing.

# FYI: This request is for 46. If it's possible, I'll request this for 45 ESR.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8723402 [details] [diff] [review]
Add a pref to disable supporting mouse wheel of windowless plugins on Windows

Supports x64 users mouse scroll over flash, please uplift to beta 46

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6044cc7a7598

Comment 24

[Ada [:adalucinet]]

Reproduced with Nightly 46.0a1, under Windows 10 64-bit.
With latest 46.0b9 (Build ID: 20160407053945) this issue is no longer reproducible when scrolling starts outside of the Flash content. Although, wheel scroll doesn't work if the cursor is over Flash content; Tested under Windows 10 64-bit and Windows 7 64-bit with 'plugin.mousewheel.enabled' pref set to 'true' by default.
Note that with 'plugin.mousewheel.enabled' set to 'false', I'm able to scroll in both circumstances - cursor starts outside/on Flash content. 
Can you shade some light on this matter? Thanks in advance!

Comment 25

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

Watch bug bug 1234958 for checking the root cause fix, but unfortunately, I don't have much time to work on this now... (But I'll try to find a chance to work on that.)

Comment 26

[Petruta Horea [:phorea], Desktop Test Engineering]

Tested again with 47 beta 7 under Win 10 64-bit and had no issues while scrolling over flash content.
Marking as verified based on this and the above comments.