1209774 - EventUtils.synthesizeMouse broken with HiDPI display

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

For a while I've noticed that a lot of tests don't run for me locally. I suspected it was an issue with my Linux HiDPI display and I tried forcing layout.css.devPixelsPerPx to 1.0 and running the test. However, that did not fix the problem.

Today, though, I noticed that the attached patch *does* fix the problem (it just divides the coordinates by 2.0 before passing them to nsDOMWindowUtils::SendMouseEvent.

There's some sort of coordinate translation that happens here, so maybe that's where the bug is?
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#7860
The coordinates come from calling getBoundingClientRect() on a XUL node.

Kats, do you see anything wrong here? It's a bit strange since we presumably test HiDPI on MacOS in automation. So I don't understand why this would be broken for me but not on MacOS.

The STR is to get a HiDPI Linux machine and run a test like browser_onbeforeunload_navigation.js.

Comment 1

[Boris Zbarsky [:bzbarsky]]

SendMouseEvent is supposed to take coordinates in CSS pixels, but getBoundingClientRect should also be returning coordinates in CSS pixels...  Does it return the right numbers in this case?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

How would I know if they're right? Is there something I can do with a screenshot maybe?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I might be wrong but don't think any of our OSX automation runs on HiDPI. I don't have a HiDPI Linux or OS X machine so I can't reproduce this locally.

I agree with what bz said in comment 1 wrt everything being in CSS pixels. However looking at the test in question it looks like the |button| element that the click is synthesized on might be in a different window than the |window| object. That has thrown things off for me in the past, and it might be the problem here. Try adding |button.ownerDocument.defaultView| as a third parameter to the synthesizeMouseAtCenter call [1] and see if that helps.

If that doesn't work then it would probably help to dump the coordinates that are being passed to the DOMWindowUtils function and after the conversion through ToWidgetPoint to see if the values make sense.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/test/browser/browser_onbeforeunload_navigation.js?rev=ef432f51a591#99

Comment 4

[Boris Zbarsky [:bzbarsky]]

> How would I know if they're right?

Log them and see how they compare to the actual appearance of the thing on the screen?  If they're off by a factor of 2 it should be pretty obvious....

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, I tracked it down further. The window is the correct one, so that's not the issue.

Assume the button is at pixel position (10, 10) on my screen. When we enter nsContentUtils::SendMouseEvent, aX and aY are (5, 5). I think this is what's expected for CSS coordinates. Then, after the ToWidgetPoint conversion, event.refPoint is (10, 10). Then we call DispatchEvent:
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsWindow.cpp#527
Notice that the first thing it does is to call GdkCoordToDevicePixels on event.refPoint, which becomes (20, 20). That's bad. So there's clearly a problem here. I'm just not sure where.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Hm, I'm not sure what GdkCoords are. Andrew probably knows.

Comment 7

[Andrew Comminos [:acomminos]]

GDK coordinates are just density independent pixels where each unit is similar in size to a pixel on a ~96 DPI display.

Since we're already transforming into device pixels (https://dxr.mozilla.org/mozilla-central/rev/acdb22976ff86539dc10413c5f366e1fb429a680/dom/base/nsContentUtils.cpp#7860) and WidgetEvent::refPoint is in units of LayoutDevicePixel, I don't think we should be treating WidgetEvents as if they're in GDK's coordinate system (https://dxr.mozilla.org/mozilla-central/rev/acdb22976ff86539dc10413c5f366e1fb429a680/widget/gtk/nsWindow.cpp#534). It's clear that the only reason we're converting it there is because DispatchEvent gets called frequently from other parts of the widget code in GDK coordinates.

Ideally, nsWindow::DispatchEvent should keep the event in device pixels and the direct and indirect callers of DispatchEvent from within nsWindow should be responsible for converting from GDK coordinates (e.g. https://dxr.mozilla.org/mozilla-central/rev/acdb22976ff86539dc10413c5f366e1fb429a680/widget/gtk/nsWindow.cpp#2500). That should help ensure consistency.

I'll see if I can whip up a fix for this.

Comment 8

[Andrew Comminos [:acomminos]]

Attachment: Transform from GDK coords to layout device pixels before calling DispatchEvent.

This patch ensures that all points in WidgetEvents are in layout device pixels, converted from GDK coords before calling DispatchEvent. This seems to make docshell/test/browser/browser_onbeforeunload_navigation.js pass, and there don't seem to be any regressions testing with a scale factor of 2.

Thanks!

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8670501 [details] [diff] [review]
Transform from GDK coords to layout device pixels before calling DispatchEvent.

Looks great, thanks.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc13f5b7ada

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a67bbdf269fd

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Andrew, why was this backed out?

Comment 13

[Andrew Comminos [:acomminos]]

Backed out due to a warning regarding loss of precision in a cast from double to int that wasn't caught by my mozconfig. I'll post a revised version that correctly handles the window coordinates provided by GdkEvents.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK. This fixes the problem in bug 1211280, so it would be nice to get it landed soon.

Comment 16

[Andrew Comminos [:acomminos]]

Attachment: Transform from GDK coords to layout device pixels before calling DispatchEvent. Carry r=karlt

Updated to properly handle GDK window coordinates, which differ slightly from standard GDK coordinates (represented as double-precision floats). The helper function always takes in two arguments for convenience as we only see these type of coordinates in GdkEvents.

Comment 17

[Andrew Comminos [:acomminos]]

Comment on attachment 8673367 [details] [diff] [review]
Transform from GDK coords to layout device pixels before calling DispatchEvent. Carry r=karlt

Reflagging for review to make sure rounding behaviour is as expected and that the helper is reasonable. The gdouble parameters provided by events are computed as (X11 display pixels)/(GDK scale factor), so I don't think we need to worry about loss of precision.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8673367 [details] [diff] [review]
Transform from GDK coords to layout device pixels before calling DispatchEvent. Carry r=karlt

>+    gint scale = GdkScaleFactor();
>+    return LayoutDeviceIntPoint(x * scale, y * scale);

(In reply to Andrew Comminos [:acomminos] from comment #17)
> The gdouble parameters provided by events are
> computed as (X11 display pixels)/(GDK scale factor), so I don't think we
> need to worry about loss of precision.

While window_scale is a power of 2 there is no loss of precision, but it can
be any integer, which means that rounding errors and truncation to int could
end up rounding towards zero instead of to nearest.

With pointer grabs, I expect some of these may be negative, so to round to
nearest use floor(x * scale + 0.5).

Comment 19

[Andrew Comminos [:acomminos]]

Attachment: Transform from GDK coords to layout device pixels before calling DispatchEvent. Carry r=karlt

Updated to round to nearest pixel, thanks.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/811827428992

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/811827428992

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8673438 [details] [diff] [review]
Transform from GDK coords to layout device pixels before calling DispatchEvent. Carry r=karlt

Approval Request Comment
[Feature/regressing bug #]: bug 1186229
[User impact if declined]:
Problems with tab dragging on GTK systems with high dpi displays detection are
fixed by the change in bug 1218552.
See approval request bug 1218552 comment 13 there for more info.
That change requires this change.

[Describe test coverage new/current, TreeHerder]:
There is test coverage of synthesizeMouse, but that doesn't extend as far as
testing that toolkit event coordinates are interpreted correctly.
Treeherder only runs tests on regular displays, but some developers run tests
on high dpi displays.

[Risks and why]: 
This change affects only GTK systems with high dpi displays.
This change triggered the regression detected in bug 1218552.
Resolving that problem properly resolved the tab dragging issue mentioned
above.
[String/UUID change made/needed]:
None.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8673438 [details] [diff] [review]
Transform from GDK coords to layout device pixels before calling DispatchEvent. Carry r=karlt

Support for gtk3 hi dpi display. This has been on m-c and m-a for a while. 
OK to uplift to beta.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/26f64ca8f630