Closed
Bug 1218552
Opened 5 years ago
Closed 5 years ago
Drag and drop broken on HiDPI displays
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: billm, Assigned: billm)
References
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
17.29 KB,
patch
|
karlt
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
16.29 KB,
patch
|
Details | Diff | Splinter Review | |
735 bytes,
patch
|
Details | Diff | Splinter Review |
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)
Updated•5 years ago
|
Blocks: 1209774
Keywords: regression
Comment 1•5 years ago
|
||
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+
Assignee | ||
Comment 3•5 years ago
|
||
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 4•5 years ago
|
||
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+
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
Flags: needinfo?(wmccloskey)
Comment 7•5 years ago
|
||
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•5 years ago
|
||
[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
Comment 10•5 years ago
|
||
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: ship-gtk3
Comment 11•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5977dbf312fe
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Comment 12•5 years ago
|
||
Should I request uplift then?
Assignee: nobody → wmccloskey
Flags: needinfo?(wmccloskey)
Comment 13•5 years ago
|
||
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?
Updated•5 years ago
|
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•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/6affa42e4a24
This doesn't seem to apply cleanly to beta. Can we get a rebased patch for uplift to beta?
Flags: needinfo?(wmccloskey)
Comment 17•5 years ago
|
||
(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.
Comment 20•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6affa42e4a24
status-b2g-v2.5:
--- → fixed
Comment 21•5 years ago
|
||
Flags: needinfo?(wmccloskey)
Updated•5 years ago
|
Keywords: checkin-needed
Whiteboard: [43 uplift needs bug 1209774 first] → [c-n beta/43]
Comment 22•5 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/581b3e8f954f
Keywords: checkin-needed
Comment 23•5 years ago
|
||
This was public on 44 since https://hg.mozilla.org/mozilla-central/rev/0ed4298f0cc6
Updated•5 years ago
|
Whiteboard: [c-n beta/43]
Belatedly tracking since this was a regression.
You need to log in
before you can comment on or make changes to this bug.
Description
•