1218552 - Drag and drop broken on HiDPI displays

Status: RESOLVED FIXED

Description

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

Attachment: patch

Unfortunately the patch in bug 1209774 broke drag and drop. The STR is basically to drag around tabs in the tab strip on a HiDPI display. The drag will seem to happen very slowly since it moves at half speed.

I wrote a possible fix, but I don't know if it's the place to change. Maybe it's better to change drag_drop_event_cb and the like? Or nsDragService.cpp? Hopefully it makes clear what the problem is though.

Comment 1

[Karl Tomlinson (:karlt)]

Comment on attachment 8679073 [details] [diff] [review]
patch

Probably the clearest solution would be change nsDragService methods to use
LayoutDeviceIntPoint or GdkPoint instead of nsIntPoint.  If the former, then
conversion would be done in drag_motion_event_cb and drag_drop_event_cb; if
the latter, then this is one of places that needs to change.  With either
approach, at least some of the SetDragEndPoint() callers need changing to do
the conversion.

I probably prefer the former as being consistent in interpreting GDK coords
as soon as we get them and working with device pixels with Gecko, but perhaps
I could be persuaded otherwise.

Please use GdkPointToDevicePixels() though when GDK coords are integers, as
GdkEventCoordsToDevicePixels() expects doubles.  (Despite the names of the
callbacks, they are not handling GdkEvents, but OnSourceGrabEventAfter() is
handling events.)

Comment 3

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

Attachment: patch v2

I think this is basically what you wanted. I tried to avoid changing non-gtk code. I also wasn't sure what to do about get_window_for_gtk_widget not being available in nsDragService. Maybe that's not what I want to be doing anyway though.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8680809 [details] [diff] [review]
patch v2

(In reply to Bill McCloskey (:billm) from comment #3)
> I also wasn't sure what to do about get_window_for_gtk_widget not
> being available in nsDragService.

Ah, right.  I hadn't considered that.  sGrabWidget in nsDragService doesn't
have a nsWindow, and so this get_window_for_gtk_widget won't work.

>+static nsWindow *
>+get_window_for_gtk_widget(GtkWidget *widget)
>+{
>+    gpointer user_data = g_object_get_data(G_OBJECT(widget), "nsWindow");
>+
>+    return static_cast<nsWindow *>(user_data);
>+}
>+

>-        dragService->SetDragEndPoint(nsIntPoint(event->motion.x_root,
>-                                                event->motion.y_root));
>+        nsWindow *window = get_window_for_gtk_widget(widget);
>+        LayoutDeviceIntPoint p =
>+            window->GdkEventCoordsToDevicePixels(event->motion.x_root,
>+                                                 event->motion.y_root);
>+        dragService->SetDragEndPoint(p);

So drop these hunks for now and this can be a separate issue.
Or use GetGtkMonitorScaleFactor() with x_root * scale + 0.5.
(x_root and y_root should always be positive.
gtk_widget_get_scale_factor requires loading symbols.
GetGtkMonitorScaleFactor() will do the right thing until we have native Wayland support.)

>+            SetDragEndPoint(LayoutDeviceIntPoint(x / scale, y / scale));

(x * scale, y * scale)

The rest looks great, thanks.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47d076aeb8c4

Comment 6

[Wes Kocher (:KWierso)]

Backed out for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=16574807&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/662df9054efa

Comment 7

[Karl Tomlinson (:karlt)]

Initializer lists are special.

LayoutDeviceIntPoint p(event->motion.x_root * scale + 0.5,
                       event->motion.y_root * scale + 0.5);

would work fine I expect, but an explicit static_cast would be clearer.
The floor() is not necessary when the values are always non-negative, but doesn't cause much harm either.

Comment 8

[Karl Tomlinson (:karlt)]

[Tracking Requested - why for this release]:
Regression in usability on high-dpi displays.

Better high-dpi display support is a feature new to the GTK3 port and may be part of 43 marketing.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5977dbf312fe

Comment 10

[Karl Tomlinson (:karlt)]

This fixes the drag end point for the eDragEnd event, which was not correct even before changes in bug 1209774, so we should probably uplift this before shipping the GTK3 build, even if the GTK3 port is first shipped in 43.
Changes in bug 1209774 would need to be uplifted before these.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5977dbf312fe

Comment 12

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

Should I request uplift then?

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8680809 [details] [diff] [review]
patch v2

Approval Request Comment

The change in bug 1209774 is required before these changes can be applied on 43.

[Feature/regressing bug #]:
[User impact if declined]:

There is an obvious regression on 44 from changes in bug 1209774 in every drag
on high dpi displays.  The coordinates are wrong by a factor of 2.

On 43 the regression from bug 1186229 is more subtle.
It affects only the drag end point, which is used in much fewer drags.
I recall it being used to test whether drags out of the tab bar are far enough
from the tab bar to be considered as a drag into a new window rather than a
drag to the beginning or end of the tab bar.  Again these coordinates are wrong by a factor of 2, which will destroy the logic here.

[Describe test coverage new/current, TreeHerder]:
None.

[Risks and why]: 

The changes here only change the values on GTK systems with high dpi displays.  The change in types gives us confidence in the appropriate values.

There are changes to XP code, but they are type changes only, and these type
changes do not change underlying storage or values.

[String/UUID change made/needed]:
None.

Comment 14

[Liz Henry (:lizzard) (use needinfo)]

Comment on attachment 8680809 [details] [diff] [review]
patch v2

Support for gtk3, should only affect hi dpi displays. 
Ok to uplift to aurora and beta.

Comment 15

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6affa42e4a24

Comment 16

[Wes Kocher (:KWierso)]

This doesn't seem to apply cleanly to beta. Can we get a rebased patch for uplift to beta?

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to Wes Kocher (:KWierso) from comment #16)
> This doesn't seem to apply cleanly to beta. Can we get a rebased patch for
> uplift to beta?

Was that even after https://hg.mozilla.org/releases/mozilla-beta/rev/26f64ca8f630 landed?

Comment 18

[Wes Kocher (:KWierso)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #17)
> (In reply to Wes Kocher (:KWierso) from comment #16)
> > This doesn't seem to apply cleanly to beta. Can we get a rebased patch for
> > uplift to beta?
> 
> Was that even after
> https://hg.mozilla.org/releases/mozilla-beta/rev/26f64ca8f630 landed?

Part of the conflict is just the s/RefPtr/nsRefPtr/ patch, which is easy enough to work around. The other part is that we're missing bug 1216916 on beta.

Comment 19

[Ritu Kothari (:ritu)]

[Tracking Requested - why for this release]: GTK3 is new in 43, changing tracking nom from 44 to 43.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6affa42e4a24

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: patch for 43 branch

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/581b3e8f954f

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: fix 43 build: make GdkPointToDevicePixels() public

This was public on 44 since
https://hg.mozilla.org/mozilla-central/rev/0ed4298f0cc6

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/844ff2b4f267

Comment 25

[Liz Henry (:lizzard) (use needinfo)]

Belatedly tracking since this was a regression.