Bug 28203 – Mouse wheel scrolling on a page with expanded drop down list detaches drop down

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brian Weinstein]

Created attachment 34619 [details]
Fix

Mouse wheel scrolling on a page with expanded drop down list detaches drop down

<rdar://6728361>

[Adam Roben (:aroben)]

Comment on attachment 34619 [details]
Fix

While it looks like this fixes the bug when the mousewheel is used, what happens if a page triggers a scroll on a timer? e.g., the page sets a timer for 5 seconds in the future to scroll the page, the user opens the popup menu, then the timer fires?

[Brian Weinstein]

Other browser behavior:

Opera - Passes wheel events to pop up menu.
IE - Ignores Wheel Event.
Firefox - Closes popup window, doesn't scroll window.
Chrome - Closes popup window, scrolls window.

Safari for Mac - Ignores Wheel Event.

[Adam Roben (:aroben)]

Closing the window seems like fine behavior. It would probably be hard to get the popup window to move in sync with the scrolling of the WebView anyway.

[Brian Weinstein]

Created attachment 34691 [details]
Little Cleaner Way

[Adam Roben (:aroben)]

Comment on attachment 34691 [details]
Little Cleaner Way

> From 2326797b9b9af58a1e28949e516be86d21e20a3a Mon Sep 17 00:00:00 2001
> From: Brian Weinstein <[e-mail]>
> Date: Wed, 12 Aug 2009 14:33:44 -0700
> Subject: [PATCH] 2009-08-12  Brian Weinstein  <[e-mail]>
> 
>         Reviewed by NOBODY (OOPS!).
> 
>         Fix of <rdar://6728361> Mouse wheel scrolling on a page with expanded drop down
>         list detaches drop down.
> 
>         Added a check in mouseWheel to see if our focus is currently in a popup, if so, close
>         the popup (matches other browser behavior).
> 
>         * WebView.cpp:
>         (WebView::mouseWheel):
> 
> 2009-08-12  Brian Weinstein  <[e-mail]>
> 
>         Reviewed by NOBODY (OOPS!).
> 
>         Fix of <rdar://6728361> Mouse wheel scrolling on a page with expanded drop down
>         list detaches drop down.
> 
>         Added a function for Windows PopupMenu's to return their class name.
> 
>          * platform/PopupMenu.h:
>          * platform/win/PopupMenuWin.cpp:
>          (WebCore::PopupMenu::popupClassName):

Looks like your commit message got doubled.

> +    HWND focusHwnd;
> +    if ((focusHwnd = GetFocus()) && focusHwnd != m_viewWindow) {

I'd move the GetFocus() call up to the previous line.

HWND focusHwnd = GetFocus();
if (focusHwnd && focusHwnd != m_viewWindow) {

"focusedWindow" might be nicer than "focusHwnd".

> +        // Our focus is on a different hwnd, see if it's a PopupMenu and if so, set the focus back on us (which will hide the popup).
> +        TCHAR className[256];
> +        if (GetClassName(focusHwnd, className, ARRAYSIZE(className)) && !_tcscmp(className, PopupMenu::popupClassName())) {
> +            SetFocus(m_viewWindow);
> +            return true;
> +        }
> +    }

It would be good to add an assert like ASSERT(ARRAYSIZE(className) > _tcslen(PopupMenu::popupClassName()) so that we know the possible truncation of the class name won't affect the comparison.

You should add a FIXME mentioning the other scrolling-related popup menu bug and how we'd like to fix them both together.

You should add a comment about why we don't let the WebView scroll in response to this event.

r=me

[Brian Weinstein]

Committed in http://trac.webkit.org/changeset/47160.