982121 - Scrolling while menupopup is open separates it from menulist-button

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Elbart]

STR:
- Open the browser and hit Ctrl+Shift+I (Developer Tools, other locales than 
  en-US may have different shortcuts)
- Go into the options (gear-icon)
- Open the "Default color unit"-menulist and scroll with the mousewheel.

ER:
The menupopup either sticks to the menulist-button which invoked it, or it 
closes, similar to the menupopups in HTML-documents.

AR:
The opened menupopup stays where it is and the background is being scrolled, 
thus detaching the menupopup from the menulist-button.
Additionally, there are now two areas where the menulist-hover-highlight is 
being registered:
- One area is where the menupopup is located
- The other area is where the menupopup is supposed to be if it still was 
  attached to the menulist-button.

Screencap:
http://i.imgur.com/wMmO2Qo.gif

Another area where this is happening is in "Options" under "Applications" 
with the "Action"-menulists once the richlist has enough items to be scrollable.

Addons are also affected, mostly in their options, i.e.
- AdBlock Plus (Filter subscriptions -> Actions)
- BetterStop
- Configuration Mania (Browser -> Page control -> Color Management)
- Download Panel Tweaker
- Download Panel Tweaks
- EnhancedSteam

Comment 2

[Neil Deakin]

This is just implementing the Windows version of bug 631473.

Comment 3

[Elbart]

Attachment: menupopup_back.gif

Another way to cause the detachment:

- Go into any "More"-section of a plugin (Flash etc.).
- Open the state-menupopup.
- Hit the backwards-button of the mouse.

Comment 5

[Neil Deakin]

This bug involves calling ShouldConsumeOnMouseWheelEvent within nsWindow::DealWithPopups and returning true if so.

Comment 7

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 982121 - consume wheel events when popups are open, r?enndeakin

Review commit: https://reviewboard.mozilla.org/r/41803/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41803/

Comment 8

[Neil Deakin]

https://reviewboard.mozilla.org/r/41803/#review38255

::: widget/windows/nsWindow.cpp:7509
(Diff revision 1)
>  
>      case WM_MOUSEWHEEL:
>      case WM_MOUSEHWHEEL:
> +      // Check if we should consume this event even if we don't roll-up:
> +      shouldConsumeMouseWheel =
> +        rollupListener->ShouldConsumeOnMouseWheelEvent();

This should happen inside the EventIsInsideWindow part as it only applies to events outside the popup window.

::: widget/windows/nsWindow.cpp:7662
(Diff revision 1)
>    sRollupMsgWnd = nullptr;
>  
>    // If we are NOT supposed to be consuming events, let it go through
> -  if (consumeRollupEvent && nativeMessage != WM_RBUTTONDOWN) {
> +  if (shouldConsumeMouseWheel ||
> +      (consumeRollupEvent && nativeMessage != WM_RBUTTONDOWN)) {
>      *aResult = MA_ACTIVATE;

This actually shouldn't be setting aResult (MA_ACTIVATE only applies to the WM_MOUSEACTIVATE message) but I guess the existing code does this so this is ok for now.

::: widget/windows/nsWindow.cpp:7663
(Diff revision 1)
>  
>    // If we are NOT supposed to be consuming events, let it go through
> -  if (consumeRollupEvent && nativeMessage != WM_RBUTTONDOWN) {
> +  if (shouldConsumeMouseWheel ||
> +      (consumeRollupEvent && nativeMessage != WM_RBUTTONDOWN)) {
>      *aResult = MA_ACTIVATE;
>      return true;

For consistency with other platforms, we should just use the same 'consumeRollupEvent' variable. So move the declaration of it earlier.

See gtk/nsWindow.cpp CheckForRollup() for the logic here.

Comment 9

[:Gijs (he/him)]

This dropped off my radar for some reason. :-\

Comment 10

[:Gijs (he/him)]

Comment on attachment 8733534 [details]
MozReview Request: Bug 982121 - consume wheel events when popups are open, r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41803/diff/1-2/

Comment 11

[Neil Deakin]

Comment on attachment 8733534 [details]
MozReview Request: Bug 982121 - consume wheel events when popups are open, r?enndeakin

https://reviewboard.mozilla.org/r/41803/#review42311

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4cab5471b2fa

Comment 13

[:Gijs (he/him)]

Neil, should we back this out considering the regressions Alice filed?

Comment 14

[Alice0775 White]

Why behavior is different between in-content-preferences/dev-tools and ordinary html?

[1] Menulist(Dev-tools/in-content prefs) popup is sticking when I turn mouse wheel.
[2] html select-option is rolling up when I turn mouse wheel.

I think rolling-up is better than sticking the popup.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4cab5471b2fa

Comment 16

[Alice0775 White]

This fix causes huge side effects of UX.
Please reconsider from the point of view of UX.

Comment 17

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #16)
> This fix causes huge side effects of UX.
> Please reconsider from the point of view of UX.

I think Neil is better able to make a decision here than me.

Comment 18

[Neil Deakin]

All of the regression bugs except 1263982 are just variations of the same thing, about the mousewheel not scrolling while a context menu is open.

This bug is just the Windows version of a bug (631473 and 849544), that was fixed on Mac and Linux a while ago, but never fixed on Windows. The mousewheel should allow scrolling when an arrowpanel or autocomplete panel is open, but not when a menu or context menu is open. This is the behaviour seen on other applications on all three OS's.

Even if we decided that it would be useful to break this OS convention, this bug only changed Windows behaviour, and I would think we would want to break the convention consistently on all platforms.

For bug 1263982, we can remove rolluponmousewheel="true" to make this consistent with <select> elements as well. That was likely added to be consistent with non-e10s popups. There has always been a bit of a fight over the <select> element regarding whether to match the <menulist> element which more accurately matches OS behaviour versus the non-e10s <select> implementation which does not.

Comment 19

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 30.0a1 (Build ID:20140311030201) on 
Windows 8.1, 64-bit.

Verified as fixed with Firefox beta 48.0b6 (Build ID: 20160706215822)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

Comment 20

[Mihai Boldan, Desktop QA [:mboldan]]

Thanks Mohammad Maruf Islam for helping us.
I also verified this issue on Firefox 48.0b6 and on Windows 7 x64 and it's no longer reproducible.
I am marking this issue Verified Fixed.

Comment 21

[Alice0775 White]

bug 982121 was only fix visual issue.
However, bug 982121 destroyed the function itself of the browsere.

I think that the importance is the function of the browser, not visual issue.
backing out of bug 982121 is needed.