Bug 100317 – Change PopupMenu positioning on Windows such that behaviour on multiple monitors matches Windows standards.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Roger Fong]

The existing code determines which screen the popup menu "belongs" to by determining which screen the owning application's hwnd belongs to,
where ownership is determined by how much of the hwnd is on which screen.
To match what most Windows applications do, the owning screen should be whichever screen the drop down button belongs to.
To determine which screen an element belongs to in Windows we need to pass in an hwnd for that element.
However, since the drop down button is something that WebKit renders there is no hwnd.
        
To remedy this issue, we can temporarily move the popup menu's hwnd to match the position and size of the button,
determine the correct screen, and then eventually move it back to the correct final position after the rest of 
the calculations have been completed. This is all done in the same function call so no rendering of the popup menu occurs
between the temporary and final positionings.

[Roger Fong]

Created attachment 170533 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/12570435>

[EFL EWS Bot]

Comment on attachment 170533 [details]
patch

Attachment 170533 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14593039

[Early Warning System Bot]

Comment on attachment 170533 [details]
patch

Attachment 170533 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14553454

[Early Warning System Bot]

Comment on attachment 170533 [details]
patch

Attachment 170533 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14554464

[kov's GTK+ EWS bot]

Comment on attachment 170533 [details]
patch

Attachment 170533 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14552436

[Peter Beverloo (cr-android ews)]

Comment on attachment 170533 [details]
patch

Attachment 170533 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14566416

[Sam Weinig]

Comment on attachment 170533 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170533&action=review

> Source/WebCore/platform/PlatformScreen.h:55
> +    FloatRect monitorFromHwnd(HWND);

This will certainly break all the builds except windows, since HWND is a windows type.

I'm also not clear on why this should be in platform code, if it is only called from one place.

[Roger Fong]

(In reply to comment #8)
> (From update of attachment 170533 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170533&action=review
> 
> > Source/WebCore/platform/PlatformScreen.h:55
> > +    FloatRect monitorFromHwnd(HWND);
> 
> This will certainly break all the builds except windows, since HWND is a windows type.

Right...

> I'm also not clear on why this should be in platform code, if it is only called from one place.

I stuck it there because it looked like many similar methods were coming from there (such as getting the screen from a Widget). Looking at PlatformScreenWin.cpp again, perhaps it makes more sense to just to make the method static and stick it in there. That seems to be what we did for other Windows specific screen info getting methods.

Does that seem reasonable or should I just stick it in PopupMenuWin?

[Roger Fong]

Err right, I can't do that of course...static in a source file means no access to anything but source file...right...
Anyways, I'll just stick it with PopupMenuWin.h and call it a day.

[Roger Fong]

Created attachment 170749 [details]
revised patch

[Sam Weinig]

Comment on attachment 170749 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170749&action=review

> Source/WebCore/platform/win/PopupMenuWin.cpp:694
> +FloatRect PopupMenuWin::monitorFromHwnd(HWND hwnd)
> +{
> +    HMONITOR monitor = MonitorFromWindow(hwnd, MONITOR_DEFAULTTOPRIMARY);
> +    MONITORINFOEX monitorInfo;
> +    monitorInfo.cbSize = sizeof(MONITORINFOEX);
> +    GetMonitorInfo(monitor, &monitorInfo);
> +    return monitorInfo.rcWork;
> +}

This can just be a static function, no need to be a member function.

[Roger Fong]

Created attachment 171501 [details]
made monitorFromHwnd static

[Roger Fong]

Committed here: http://trac.webkit.org/changeset/133092