Drag and drop broken on HiDPI displays

RESOLVED FIXED in Firefox 43, Firefox OS v2.5

Status

()

Core
Widget: Gtk
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

({regression})

44 Branch
mozilla45
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43+ fixed, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 8679073 [details] [diff] [review]
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.
Attachment #8679073 - Flags: feedback?(karlt)
Blocks: 1209774
Keywords: regression
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.)
Attachment #8679073 - Flags: feedback?(karlt) → feedback+
Duplicate of this bug: 1218933
Created attachment 8680809 [details] [diff] [review]
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.
Attachment #8679073 - Attachment is obsolete: true
Attachment #8680809 - Flags: review?(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.
Attachment #8680809 - Flags: review?(karlt) → review+
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.
[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.
tracking-firefox44: --- → ?
Version: Trunk → 44 Branch
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.
Blocks: 1193807

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5977dbf312fe
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Should I request uplift then?
Assignee: nobody → wmccloskey
Flags: needinfo?(wmccloskey)
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.
Attachment #8680809 - Flags: approval-mozilla-beta?
Attachment #8680809 - Flags: approval-mozilla-aurora?
Whiteboard: [43 uplift needs bug 1209774 first]
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.
Attachment #8680809 - Flags: approval-mozilla-beta?
Attachment #8680809 - Flags: approval-mozilla-beta+
Attachment #8680809 - Flags: approval-mozilla-aurora?
Attachment #8680809 - Flags: approval-mozilla-aurora+

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6affa42e4a24
status-firefox44: affected → fixed
This doesn't seem to apply cleanly to beta. Can we get a rebased patch for uplift to beta?
Flags: needinfo?(wmccloskey)
(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?
Flags: needinfo?(wkocher)
(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.
Flags: needinfo?(wkocher)
[Tracking Requested - why for this release]: GTK3 is new in 43, changing tracking nom from 44 to 43.
status-firefox43: --- → affected
tracking-firefox43: --- → ?
tracking-firefox44: ? → ---
Created attachment 8687763 [details] [diff] [review]
patch for 43 branch
Flags: needinfo?(wmccloskey)
Keywords: checkin-needed
Whiteboard: [43 uplift needs bug 1209774 first] → [c-n beta/43]
https://hg.mozilla.org/releases/mozilla-beta/rev/581b3e8f954f
status-firefox43: affected → fixed
Keywords: checkin-needed
Created attachment 8687805 [details] [diff] [review]
fix 43 build: make GdkPointToDevicePixels() public

This was public on 44 since
https://hg.mozilla.org/mozilla-central/rev/0ed4298f0cc6
Whiteboard: [c-n beta/43]
Belatedly tracking since this was a regression.
tracking-firefox43: ? → +
You need to log in before you can comment on or make changes to this bug.