Closed Bug 1250050 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Plug-ins, defect)

All
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- verified
firefox-esr38 --- wontfix
firefox-esr45 --- affected

People

(Reporter: emk, Assigned: masayuki)

References

Details

Attachments

(1 file, 1 obsolete file)

This is intended a temporary workaround until bug 1234958 gets fixed in the proper manner.
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.
Flags: needinfo?(masayuki)
Wait a moment, I'm now trying to fix bug 1234958.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
OS: Unspecified → Windows
Hardware: Unspecified → All
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.
Attachment #8722343 - Flags: review?(bugs)
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.
Attachment #8722343 - Flags: review?(bugs) → review+
(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?
Flags: needinfo?(bugs)
> +  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.
(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.
Flags: needinfo?(bugs)
(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.
(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.
Flags: needinfo?(bugs)
Improving around ComputeScrollTargetOptions.
Attachment #8722343 - Attachment is obsolete: true
Attachment #8723402 - Flags: review?(bugs)
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.
Flags: needinfo?(bugs)
So either we don't take the patch, or modify the patch so that pref is false by default.
during the lunch I changed my mind :) Maybe we could take the patch and think about the default value in some other bug.
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 <masayuki@d-toybox.com>
># 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
Attachment #8723402 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e7e8a495ded4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Could you consider uplift this?
Flags: needinfo?(masayuki)
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.
Attachment #8723402 - Flags: approval-mozilla-aurora?
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
Attachment #8723402 - Flags: approval-mozilla-beta+
Attachment #8723402 - Flags: approval-mozilla-aurora?
Attachment #8723402 - Flags: approval-mozilla-aurora-
Flags: qe-verify+
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!
Flags: needinfo?(masayuki)
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.)
Flags: needinfo?(masayuki)
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.