Edge: Use display-relative coordinates in ContextMenuRequested #1462
Conversation
|
@akoch-yatta Thanks for catching this in #1450 (comment) |
There was a problem hiding this comment.
Looks reasonable to me, but @akoch-yatta should definitely have a look at this.
| ptWidgetRelative.x = DPIUtil.autoScaleDown(ptWidgetRelative.x); // To Points | ||
| ptWidgetRelative.y = DPIUtil.autoScaleDown(ptWidgetRelative.y); // To Points |
There was a problem hiding this comment.
We may take the change to correct the usage of autoScaleDown to use the proper context zoom. It should be something like this:
| ptWidgetRelative.x = DPIUtil.autoScaleDown(ptWidgetRelative.x); // To Points | |
| ptWidgetRelative.y = DPIUtil.autoScaleDown(ptWidgetRelative.y); // To Points | |
| ptWidgetRelative.x = DPIUtil.scaleDown(ptWidgetRelative.x, browser.getShell().nativeZoom); // To Points | |
| ptWidgetRelative.y = DPIUtil.scaleDown(ptWidgetRelative.y, browser.getShell().nativeZoom); // To Points |
There was a problem hiding this comment.
Hmm, since the pt information comes from the WebView2 C library, isn't the actual device zoom irrelevant here? We always have to map to 100% pixel coordinates because that's what the SWT API works with?
There was a problem hiding this comment.
Hmm, I can't get this to work correctly in all combinations of
-Dswt.autoScale=[false|exact]-Dswt.autoScale.updateOnRuntime=[false|true]- DPI in windows =
[100|150|200]
Menu.setLocation(), calls org.eclipse.swt.widgets.Widget.getZoom(), which uses org.eclipse.swt.internal.DPIUtil.getZoomForAutoscaleProperty(int), which depends on swt.autoScale.
Menu.setLocationInPixels() is not visible from the .browser package.
The coordinates returned from Edge for the pt are relative to the WebView2, but are not zoomed.
I can't really put my head around all this scaling / DPI craziness involved here :O.
Maybe there even is a problem in how these methods currently work?
@akoch-yatta Would be great, if you could take a look here!
There was a problem hiding this comment.
I had a look into it. The conversion between points and pixels is always a bit messy. I think there is something wrong in Control::toDisplay and Control::toControl, to cover all cases, but I need to have a deeper look into that topic
There was a problem hiding this comment.
The problem is that Edge knows nothing about the swt.autoScale property, so we have to deal with 3 different coordinate systems here:
- Win32 POINTs received from Edge
- SWT Display coordinates (these are affected by the
swt.autoScale) - Pixels
So this has to be done in a 2-step manner:
- Up from Win32 POINTs to Pixel (enforcing the native zoom value, ignoring
swt.autoScale) - Down to SWT Display (respecting
swt.autoScale)
With the latest force-push things work fine for me in all possible combinations I tried. I guess #1536 also helped.
sratz
force-pushed
the
edge-menu-displaycoordinates
branch
from
9a5032f to
fb406f4
Compare
|
I'd also like to get this into M2 as the current behavior is completely broken with anything other than 100% OS zoom level. |
I agree that we should just merge this for M2. Maybe @amartya4256 can have another look into this? @akoch-yatta may also review the latest change after the merge (when he's back from vacation). |
sratz
force-pushed
the
edge-menu-displaycoordinates
branch
from
fb406f4 to
a39f3ec
Compare
sratz
force-pushed
the
edge-menu-displaycoordinates
branch
from
a39f3ec to
17e6a4c
Compare
|
Test failures are unrelated. |
No description provided.