Bug 148018 – [Win] The context menu on PluginView is displayed in wrong positions

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Plug-ins

[Sungmann Cho]

On Windows, the context menu on PluginView is displayed in wrong positions.
It is due to not taking the device scale factor into account when assembling WM_RBUTTONDOWN events.

[Sungmann Cho]

Created attachment 258987 [details]
Patch

[Sungmann Cho]

Created attachment 258988 [details]
simple flash content for testing

[Sungmann Cho]

This problem seems to occur when the value of "Make it easier to read what's on your screen" item in the display setting page is not equal to "Smaller - 100% (default)".

[Alex Christensen]

Comment on attachment 258987 [details]
Patch

Why don't the other events (WM_LBUTTONDOWN, WM_MBUTTONDOWN) have the same problem?  Doing this here specifically for the right button only seems like the wrong thing to do.

[Brent Fulgham]

Comment on attachment 258987 [details]
Patch

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

> Source/WebKit/win/Plugins/PluginViewWin.cpp:657
> +                npEvent.lParam = MAKELPARAM(point.x(), point.y());

This is correct if this event is being passed into WebCore, which wants to work in logical pixels. But it looks like this event is handed to the plugin, which may or may not be working in logical pixels. It could be that Flash is not high-DPI aware, and therefore needs the scaled pixel values -- but what happens if Flash is updated to recognize High-DPI?

Also, It does seem like if the adjusted point is needed for the context menu, it's probably need for mouse down and other actions, as Alex pointed out.

[Alex Christensen]

Is this fixed by https://bugs.webkit.org/show_bug.cgi?id=148020 ?

[Sungmann Cho]

Created attachment 259619 [details]
Patch

[Sungmann Cho]

(In reply to comment #6)
> Is this fixed by https://bugs.webkit.org/show_bug.cgi?id=148020 ?

No. The problem still occurs after landing of that patch.

[Alex Christensen]

Comment on attachment 259619 [details]
Patch

I'm not sure where the problem really is, but I really think that scaling only the right button event positions is not the right solution.

[Sungmann Cho]

(In reply to comment #5)
> It could be that Flash is not high-DPI aware, and therefore needs the scaled pixel values --
> but what happens if Flash is updated to recognize High-DPI?

This problem still occurs even after updating to the latest version of Flash (18.0.0.232).
 
> Also, It does seem like if the adjusted point is needed for the context menu,
> it's probably need for mouse down and other actions, as Alex pointed out.

Things get even worse when we pass the adjusted point to other down events.
When I tried to do so with the attached test content, I couldn't click any buttons or links on it.

According to the codes in WebView::handleContextMenuEvent() (https://goo.gl/RI8Mvz),
it seems that we are scaling the given logical point with the device scale factor
only when invoking TrackPopupMenu(). Any thoughts on this?

[Brent Fulgham]

(In reply to comment #10)
> > Also, It does seem like if the adjusted point is needed for the context menu,
> > it's probably need for mouse down and other actions, as Alex pointed out.
> 
> Things get even worse when we pass the adjusted point to other down events.
> When I tried to do so with the attached test content, I couldn't click any
> buttons or links on it.

In general, the code in the WebKit layer (i.e., the stuff in Source/WebKit/win) work in terms of device (screen) pixels. So, position/size information received from Windows messages are device pixels, as are anything we send back to Windows via API or messages since Windows only understands device pixels.

When we hand coordinates/sizes down to WebCore, we need to covert to "logical" pixels, so we have to adjust by the scaling factor. E.g., in a High DPI device one logical pixel maps to a group of 2x2 device pixels.

So when we get a message and hand it to the plugin, we have to decide if the plugin wants to get device pixels or logical pixels -- and I'm not sure what Flash wants.

Since the coordinates coming through HTMLPluginElement on their way to PluginViewWin are from WebCore, they are in logical units.

If you have to scale the coordinates to get the context menu (which I assume is internal to Flash), then it doesn't make sense that you would not need to do this for all the other action as well!

> According to the codes in WebView::handleContextMenuEvent()
> (https://goo.gl/RI8Mvz),
> it seems that we are scaling the given logical point with the device scale
> factor
> only when invoking TrackPopupMenu(). Any thoughts on this?

Nearly all mouse events get scaled in the constructors for PlatformMouseEventWin. However, the context menu doesn't use that event object, so it was done manually.

Is it possible that the plugin wants screen coordinates (i.e., referenced against the edge of the monitor), but we are handling it content coordinates (based on the plugin)? Maybe something was broken for Plugin handling when I made some of these other changes?

[Alexey Proskuryakov]

Mass closing plug-in bugs, as plug-in support has been removed from WebKit.

Please comment and/or reopen if this still affects WebKit in some way.