1561522 - MouseEvent.screenX/screenY sometimes returns physical coordinates instead of CSS coordinates on dragend

Status: RESOLVED DUPLICATE of bug 1756241

(Core :: Widget, defect, P2)

Description

[YUKI "Piro" Hiroshi]

Attachment: treestyletab-we.xpi: testing build with debug log

MouseEvent.screenX and MouseEvent.screenY should be coordinates based on CSS pixels, but sometimes they become coordinates based on physical pixels on the device unexpectedly on an event listener for dragend. I still couldn't narrow down minimum conditions to reproduce, but it happens on an actual addon Tree Style Tab (TST), so I file this as a bug of WebExtensions for now.

Steps to reproduce

1. Prepare Hi-dpi screen and set device pixel ratio larger than 1. (I tested with 1.25 on Windows 10.)
2. Start Nightly with a clean profile.
3. Go to about:debugging.
4. Load attached xpi (development build of TST with debug log) as a temporary addon. Then the sidebar appears and TST's sidebar panel is loaded.
5. Open a debugger tab for the loaded temporary addon TST.
6. Choose the console tab in the debugger.
7. Click the pull down menu in the sidebar header, and choose "Move Sidebar to RIght".
8. Put the browser window at bottom-right edge of the screen (to maximize the erratic coordinates.)
9. Drag an item in the sidebar and drop it on the dragged item itself (of course inside the sidebar area) again and again.

Actual result

Sometimes you get console output like below

expected range of screenX:  Object { from: 1355.199951171875, to: 1581.199951171875 }
screenX on dragstart:       1440
screenX on dragend:         1813 <= unnaturally too large!
expected range of screenY:  Object { from: 483.20001220703125, to: 929.2000122070312 }
screenY on dragstart:       771
screenY on dragend:         965 <=unnaturally too large!

Expected result:

Always you get console output like below

expected range of screenX:  Object { from: 1355.199951171875, to: 1581.199951171875 }
screenX on dragstart:       1440
screenX on dragend:         1450 <= between the expected range
expected range of screenY:  Object { from: 483.20001220703125, to: 929.2000122070312 }
screenY on dragstart:       771
screenY on dragend:         760 <= between the expected range

Environment

• Windows 10 Pro 1809 (64bit)
• Firefox 67.0.4 (64bit)
• Nightly 69.0a1 (64bit) build id: 20190625215814

Comment 1

[YUKI "Piro" Hiroshi]

I've tried logging at MouseEvent::ScreenX, with a line MOZ_LOG(sMouseEventLog, LogLevel::Info, ("MouseEvent::ScreenX: SCREEN %d", Event::GetScreenCoords(mPresContext, mEvent, mEvent->mRefPoint).x)); inserted before https://searchfox.org/mozilla-central/rev/7e158713cf5a8514fa8161dd4a239737b05da64d/dom/events/MouseEvent.cpp#226
Then I've realized that logged calculated results are correct but only the value on the JS layer is wrong. For example, when event.screenX of dragend is reported as 1720 via console.log(), the value logged via MOZ_LOG is 1376.

Comment 2

[YUKI "Piro" Hiroshi]

I misunderstood the target module to be researched, I should researched nsBaseDragService.
https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/widget/nsBaseDragService.cpp#515
At here, the value of mEndDragPoint is exact same to the one exposed as event.screenX/Y of the dragend event. So someone sets a wrong value for mEndDragPoint before dragend event is finally dispatched.

mEndDragPoint is updated in two cases:
https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/widget/nsBaseDragService.h#61
and after logging I've realized that SetDragEndPoint(mozilla::LayoutDeviceIntPoint aEndDragPoint) is called on success cases and SetDragEndPoint(nsIntPoint aEndDragPoint) is called on failure cases.

Search result of the function:
https://searchfox.org/mozilla-central/search?q=symbol:_ZN17nsBaseDragService15SetDragEndPointEN7mozilla3gfx13IntPointTypedINS1_12UnknownUnitsEEE&redirect=false

The function is called at these codes:
https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/widget/windows/nsNativeDragTarget.cpp#448
https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/widget/cocoa/nsChildView.mm#5385
https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/widget/cocoa/nsChildView.mm#5389

Comment 3

[YUKI "Piro" Hiroshi]

On my environment, this change looks to solve this problem. Changes are copied from https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/widget/windows/nsDragService.cpp#338

diff --git a/widget/windows/nsNativeDragTarget.cpp b/widget/windows/nsNativeDragTarget.cpp
--- a/widget/windows/nsNativeDragTarget.cpp
+++ b/widget/windows/nsNativeDragTarget.cpp
@@ -436,21 +436,34 @@ nsNativeDragTarget::Drop(LPDATAOBJECT pD
   // Let the win drag service know whether this session experienced
   // a drop event within the application. Drop will not oocur if the
   // drop landed outside the app. (used in tab tear off, bug 455884)
   winDragService->SetDroppedLocal();

   // tell the drag service we're done with the session
   // Use GetMessagePos to get the position of the mouse at the last message
   // seen by the event loop. (Bug 489729)
+  // Note that we must convert this from device pixels back to Windows logical
+  // pixels (bug 818927).
   DWORD pos = ::GetMessagePos();
-  POINT cpos;
-  cpos.x = GET_X_LPARAM(pos);
-  cpos.y = GET_Y_LPARAM(pos);
-  winDragService->SetDragEndPoint(nsIntPoint(cpos.x, cpos.y));
+  CSSPoint cssPos;
+  if (!LayoutDevicePointToCSSPoint(
+          LayoutDevicePoint(GET_X_LPARAM(pos), GET_Y_LPARAM(pos)), cssPos)) {
+    // fallback to the simple scaling
+    POINT pt = {GET_X_LPARAM(pos), GET_Y_LPARAM(pos)};
+    HMONITOR monitor = ::MonitorFromPoint(pt, MONITOR_DEFAULTTOPRIMARY);
+    double dpiScale = widget::WinUtils::LogToPhysFactor(monitor);
+    cssPos.x = GET_X_LPARAM(pos) / dpiScale;
+    cssPos.y = GET_Y_LPARAM(pos) / dpiScale;
+  }
+  // We have to abuse SetDragEndPoint to pass CSS pixels because
+  // Event::GetScreenCoords will not convert pixels for dragend events
+  // until bug 1224754 is fixed.
+  winDragService->SetDragEndPoint(
+      LayoutDeviceIntPoint(NSToIntRound(cssPos.x), NSToIntRound(cssPos.y)));
   ModifierKeyState modifierKeyState;
   serv->EndDragSession(true, modifierKeyState.GetModifiers());

   // release the ref that was taken in DragEnter
   NS_ASSERTION(mTookOwnRef, "want to release own ref, but not taken!");
   if (mTookOwnRef) {
     this->Release();
     mTookOwnRef = false;

Comment 4

[YUKI "Piro" Hiroshi]

Attachment: Bug 1561522 - Set coordinates based on CSS pixels for dragend event

Comment 5

[YUKI "Piro" Hiroshi]

I've created a patch but I think I still need to do more research on macOS.

Comment 6

[YUKI "Piro" Hiroshi]

I thought that I need to research on macOS because I got a similar problem report about Tree Style Tab on the platform, but it is still unconfirmed. After I successfully reproduce it I'll file another bug.

Comment 7

[YUKI "Piro" Hiroshi]

I've confirmed that similar problem happens on macOS, filed as the bug 1561879.

Comment 8

[YUKI "Piro" Hiroshi]

The patch I posted looks not reviewed yet. Should I ask to another reviewer?

Comment 9

[Jonathan Kew [:jfkthame]]

Sorry - I'll try to look at this soon. Leaving the needinfo flag as a reminder for now. My main concern here is whether this will work as expected across multi-screen and mixed-dpi configurations on the various platforms; coordinate systems get really confusing in those situations. :(

Comment 10

[YUKI "Piro" Hiroshi]

Any progress?

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:spohl, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I believe this was fixed by bug 1756241, but let me know if that's somehow not true. Piro, mind confirming? There's still bug 1773886 which I need to look at but I believe that this should be working otherwise.

Comment 13

[YUKI "Piro" Hiroshi]

I've tried monitoring of coordinates of dragend event, and I couldn't see the problem I saw around difference of CSS pixels and device pixels. Thanks!
(BTW I still see another coordinates issue of dragend: the bug 1773886.)