After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771841 - [Wayland] Drop-down menus are broken in position and size on HiDPI screens
[Wayland] Drop-down menus are broken in position and size on HiDPI screens
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Wayland
3.21.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: WaylandRelated
 
 
Reported: 2016-09-22 15:17 UTC by Matteo F. Vescovi
Modified: 2016-10-14 16:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screencast showing the strange behaviour (586.53 KB, video/webm)
2016-09-22 15:17 UTC, Matteo F. Vescovi
  Details
wayland/xdg-shell: Scale positioner coordinates (3.54 KB, patch)
2016-09-23 06:06 UTC, Jonas Ådahl
none Details | Review
With patch (931.90 KB, image/png)
2016-09-23 06:58 UTC, Bjørn Lie
  Details
wayland/xdg-popup: Always use monitor of toplevel (2.85 KB, patch)
2016-09-23 09:20 UTC, Jonas Ådahl
committed Details | Review
wayland/xdg-shell: Scale configure relative popup coordinate (1.69 KB, patch)
2016-09-23 09:20 UTC, Jonas Ådahl
committed Details | Review
wayland/xdg-popup: Force monitor of the top-level (2.03 KB, patch)
2016-09-23 21:19 UTC, Sjoerd Simons
committed Details | Review
wayland/xdg-shell: update popup window monitor early (1.28 KB, patch)
2016-09-23 21:19 UTC, Sjoerd Simons
committed Details | Review
Daniel setup info (3.96 KB, text/plain)
2016-10-11 17:57 UTC, Daniel Buch
  Details
GtkMenu-Click-1 (379.52 KB, image/png)
2016-10-11 18:00 UTC, Daniel Buch
  Details
GtkMenu-Click-2 (436.53 KB, image/png)
2016-10-11 18:03 UTC, Daniel Buch
  Details
Popup positioning (315.40 KB, image/png)
2016-10-11 18:06 UTC, Daniel Buch
  Details
wayland/xdg-shell: Scale positioner coordinates (3.05 KB, patch)
2016-10-13 06:00 UTC, Jonas Ådahl
committed Details | Review

Description Matteo F. Vescovi 2016-09-22 15:17:51 UTC
Created attachment 336090 [details]
Screencast showing the strange behaviour

[Please, reassign to the right component in charge for this, I'm probably mistaking it]

Attached, you'll find a screencast I made to explain visually the issue encountered.
The same steps followed under Xorg don't show any kind of that weirdness.

Seems like this happens only on HiDPI screens.

Cheers.
Comment 1 Jonas Ådahl 2016-09-23 06:06:25 UTC
Created attachment 336140 [details] [review]
wayland/xdg-shell: Scale positioner coordinates

When the monitor is scaled (i.e. HiDPI scaling) the placement
coordinates are still in unscaled xdg_surface window geometry
coordinate space. Fix this by simply scaling the coordinates by the
monitor scale of the parent toplevel window.

This is inherently racy, but since we won't move the toplevel window
before the popup is placed, and we won't move the window without
unmapping the popup, there is little point in introducing more complex
adaptive scaling, especially when the end goal is to get rid of all
these types of scaling hacks.
Comment 2 Bjørn Lie 2016-09-23 06:58:23 UTC
Created attachment 336142 [details]
With patch

Works for me, but if possible, look at the oversize in the attached screenshot.

This is in any case much better than what we have :-) so stil +1 from me
Comment 3 Jonas Ådahl 2016-09-23 07:14:33 UTC
(In reply to Bjørn Lie from comment #2)
> Created attachment 336142 [details]
> With patch
> 
> Works for me, but if possible, look at the oversize in the attached
> screenshot.
> 
> This is in any case much better than what we have :-) so stil +1 from me

Ah, some more coordinates that need the scale hack probably.
Comment 4 Sjoerd Simons 2016-09-23 08:15:59 UTC
This fixes the issue for me on my hidpi screen, however when using mixed setup (hidpi laptop + normal dpi monitor) the issue still occurs on the non-hidpi screen.
Comment 5 Jonas Ådahl 2016-09-23 09:20:33 UTC
Created attachment 336145 [details] [review]
wayland/xdg-popup: Always use monitor of toplevel

Always use the monitor of the toplevel surface's window, so that the
popup menu and the parent will always have the same scale. This fixes
the dimensions sent in the xdg_popup configure event.
Comment 6 Jonas Ådahl 2016-09-23 09:20:38 UTC
Created attachment 336146 [details] [review]
wayland/xdg-shell: Scale configure relative popup coordinate

The parent local popup coordinate needs to be scaled according to the
monitor scale it is assigned.
Comment 7 Sjoerd Simons 2016-09-23 12:11:15 UTC
Seems that even with these two, the configure on the non-hdpi screen still gets a width/height half of the requested one
Comment 8 Jonas Ådahl 2016-09-23 13:41:00 UTC
(In reply to Sjoerd Simons from comment #7)
> Seems that even with these two, the configure on the non-hdpi screen still
> gets a width/height half of the requested one

The configure event of the pop-up? I can't reproduce that. Could you send WAYLAND_DEBUG=1 log?
Comment 9 Sjoerd Simons 2016-09-23 21:19:25 UTC
Created attachment 336172 [details] [review]
wayland/xdg-popup: Force monitor of the top-level

Directly set the monitor of the toplevel window for the popup to avoid
the change not being applied due to later constraints calculation.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Comment 10 Sjoerd Simons 2016-09-23 21:19:32 UTC
Created attachment 336173 [details] [review]
wayland/xdg-shell: update popup window monitor early

As meta_window_place_with_placement_rule will trigger a configure event
being sent ensure that the popup is placed on the correct monitor first
to ensure the right scale factor is applied.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Comment 11 Sjoerd Simons 2016-09-23 21:27:13 UTC
With the last 2 patches on top of those from Jonas the issue is fixed for me. 

The reason why i was apparently still hitting it is that the configure event is triggered by meta_window_place_with_placement_rule in finish_popup_setup which was before the popup monitor was synced with that of the toplevel, hence the configure event using the wrong monitors scale.


Would be nice if in the long-term mutter kept the buffer coordinates (pixels) and surface coordinates seperate so all this fragile scaling can go away :)
Comment 12 Jonas Ådahl 2016-09-23 23:47:54 UTC
(In reply to Sjoerd Simons from comment #11)
> With the last 2 patches on top of those from Jonas the issue is fixed for
> me. 
> 
> The reason why i was apparently still hitting it is that the configure event
> is triggered by meta_window_place_with_placement_rule in finish_popup_setup
> which was before the popup monitor was synced with that of the toplevel,
> hence the configure event using the wrong monitors scale.
> 
> 
> Would be nice if in the long-term mutter kept the buffer coordinates
> (pixels) and surface coordinates seperate so all this fragile scaling can go
> away :)

The long term plan is to have the stage coordinate space to be in logical pixels, meaning that indeed all these hacks will go away.
Comment 13 Sjoerd Simons 2016-09-26 06:52:45 UTC
Great to hear.

FWIW i reproduce the issue by running mutter with:
  MUTTER_DEBUG_NUM_DUMMY_MONITORS=2 MUTTER_DEBUG_DUMMY_MONITOR_SCALES=2,1 

iotw dual monitor setup, left (primary) monitor hidpi. The issue can be seen by starting e.g. gtk3-demo and moving it to the right monitor before opening a popup.
Comment 14 Jonas Ådahl 2016-09-28 03:49:43 UTC
Review of attachment 336172 [details] [review]:

A popup can potentially change scale: if its not a grabbing popup, and the parent window is moved, the popup should move with the parent surface. With that said, we don't handle this situation yet (since we don't have a non-grabbing popup used by a client yet) so it probably wouldn't work to begin with. With that said, why is this patch needed? from == to should always hold (except for the initial set, which is handled) in the use cases we currently have.
Comment 15 Jonas Ådahl 2016-09-28 03:49:54 UTC
Review of attachment 336173 [details] [review]:

Looks reasonable to me.
Comment 16 Sjoerd Simons 2016-09-28 08:56:41 UTC
Review of attachment 336172 [details] [review]:

The initial case block doesn't actually happen as _meta_window_shared_new will set the initial monitor via meta_screen_calculate_monitor_for_window, which means from is never NULL. So the first round through this function actually has from != to. The problem this patch avoids is not syncing the popup monitor to the top-level monitor as the scaled_new monitor doesn't match the top-level monitor.
Comment 17 Sjoerd Simons 2016-09-28 08:57:26 UTC
Review of attachment 336173 [details] [review]:

fwiw i don't have commit access on git.gnome anymore, so please commit for me ;)
Comment 18 Carlos Garnacho 2016-09-28 10:57:08 UTC
(In reply to Sjoerd Simons from comment #17)
> Review of attachment 336173 [details] [review] [review]:
> 
> fwiw i don't have commit access on git.gnome anymore, so please commit for
> me ;)

OT, probably https://www.dragonsreach.it/2014/10/07/the-gnome-infrastructure-is-now-powered-by-freeipa/ has instructions that may help you.
Comment 19 Jonas Ådahl 2016-09-30 02:02:09 UTC
(In reply to Sjoerd Simons from comment #16)
> Review of attachment 336172 [details] [review] [review]:
> 
> The initial case block doesn't actually happen as _meta_window_shared_new
> will set the initial monitor via meta_screen_calculate_monitor_for_window,
> which means from is never NULL. So the first round through this function
> actually has from != to. The problem this patch avoids is not syncing the
> popup monitor to the top-level monitor as the scaled_new monitor doesn't
> match the top-level monitor.

Ah, I see. I guess we can go with that approach then, and fix it properly after I've reworked how MetaWindow's are constructed.
Comment 20 Sjoerd Simons 2016-10-04 12:03:25 UTC
(In reply to Jonas Ådahl from comment #19)
> (In reply to Sjoerd Simons from comment #16)
> > Review of attachment 336172 [details] [review] [review] [review]:
> > 
> > The initial case block doesn't actually happen as _meta_window_shared_new
> > will set the initial monitor via meta_screen_calculate_monitor_for_window,
> > which means from is never NULL. So the first round through this function
> > actually has from != to. The problem this patch avoids is not syncing the
> > popup monitor to the top-level monitor as the scaled_new monitor doesn't
> > match the top-level monitor.
> 
> Ah, I see. I guess we can go with that approach then, and fix it properly
> after I've reworked how MetaWindow's are constructed.

Could you commit these in that case ;). Would be great to have that sorted in git
Comment 21 Daniel Buch 2016-10-08 12:55:41 UTC
I installed a built with attachment 336172 [details] [review], and for me it sadly doesn't work for me. Any news i missed?
Comment 22 Daniel Boles 2016-10-09 11:40:39 UTC
(In reply to Daniel Buch from comment #21)
> I installed a built with attachment 336172 [details] [review] [review], and for me it
> sadly doesn't work for me. Any news i missed?

Did you build with all the other related patches? They seem to be required:

(In reply to Sjoerd Simons from comment #11)
> With the last 2 patches on top of those from Jonas the issue is fixed for
> me.
Comment 23 Daniel Buch 2016-10-09 17:11:12 UTC
(In reply to djb from comment #22)
> (In reply to Daniel Buch from comment #21)
> > I installed a built with attachment 336172 [details] [review] [review] [review], and for me it
> > sadly doesn't work for me. Any news i missed?
> 
> Did you build with all the other related patches? They seem to be required:
> 
> (In reply to Sjoerd Simons from comment #11)
> > With the last 2 patches on top of those from Jonas the issue is fixed for
> > me.

I managed to apply all attached patches. But problem still persists.
This thread is now moved to Gtk so i guess the problem lies there and not in mutter.
Comment 24 Sjoerd Simons 2016-10-11 07:04:50 UTC
Daniel can you indicate how exactly you rpreoduce the issue and what your setup is, there might be some subtilities that these patches haven't covere ofcourse.
Comment 25 Daniel Buch 2016-10-11 17:57:37 UTC
Created attachment 337452 [details]
Daniel setup info
Comment 26 Daniel Buch 2016-10-11 18:00:05 UTC
Created attachment 337453 [details]
GtkMenu-Click-1

This is when i click once, next will be shrink further by factor two. Next attachment will show that
Comment 27 Daniel Buch 2016-10-11 18:03:07 UTC
Created attachment 337454 [details]
GtkMenu-Click-2
Comment 28 Daniel Buch 2016-10-11 18:06:41 UTC
Created attachment 337455 [details]
Popup positioning

This shows that popup position (x,y) is _NOT_ multiplied by scaling factor. Look at cursor and popup
Comment 29 Daniel Buch 2016-10-11 18:12:08 UTC
This behavior persist even though i patch these mutter patches.
As a last comment, i discovered that the shrinking problem, is widget instance-wise. So clicking one button and the popup shrink to half the size exponentially for each click.

From my understanding, i believe this logic is now in gdk, not gtk and not mutter - is that correct?
Comment 30 Daniel Buch 2016-10-11 18:15:06 UTC
Eventually the program will crash with this:

(gnome-calculator:9872): Gtk-WARNING **: Negative content width -7 (allocation 1, extents 4x4) while allocating gadget (node arrow, owner GtkTreeMenu)

(gnome-calculator:9872): Gdk-WARNING **: Error 71 (Protocol error) dispatching to Wayland display.
Comment 31 Sjoerd Simons 2016-10-12 19:50:42 UTC
Daniel i meant what gtk+ version, what mutter version did you apply the patches on what's your screen setup (one hidpi, other normal dpi? or?)

The patches from Jonas are enough to solve the issue for a single hidpi screen, mine are needed on top to fix the issue for mixed dpi setups.
Comment 32 Daniel Buch 2016-10-13 05:24:40 UTC
Perfect, i managed to make it work with mutter 3.22.1 and gtk 3.22.1 including these patches.
Comment 33 Jonas Ådahl 2016-10-13 06:00:32 UTC
Created attachment 337556 [details] [review]
wayland/xdg-shell: Scale positioner coordinates

When the monitor is scaled (i.e. HiDPI scaling) the placement coordinates
ere still in unscaled xdg_surface window geometry coordinate space when
used to place the window. Fix this by scaling the coordinates by the
monitor scale of the parent toplevel window before using them.

-----

Rewrote the first patch to scale the coordinates as a more sane place. Also added a comment documenting the coordinate space of the stored placement rule fields.
Comment 34 Jonas Ådahl 2016-10-13 06:02:58 UTC
Review of attachment 336172 [details] [review]:

Marking as a-c-n. Ihis patch doesn't stop the re-scaling would the parent window move, since the scaling that is left out is the not-to-be-run "avoid jumping-back-and-forth-between-scales" scaling, so looks good to me.
Comment 35 Sjoerd Simons 2016-10-13 20:25:52 UTC
Review of attachment 337556 [details] [review]:

Looks good to me, tested the set with this patch as opposed to its previous iteration and everything seems happy as expected
Comment 36 François Guerraz 2016-10-14 05:39:30 UTC
Review of attachment 337556 [details] [review]:

Tested on mutter-3.22.1 on arch 64, works well.
Comment 37 Jonas Ådahl 2016-10-14 16:10:41 UTC
Attachment 336145 [details] pushed as c0c132a - wayland/xdg-popup: Always use monitor of toplevel
Attachment 336146 [details] pushed as a3d7ae6 - wayland/xdg-shell: Scale configure relative popup coordinate
Attachment 336172 [details] pushed as 8a6fa72 - wayland/xdg-popup: Force monitor of the top-level
Attachment 336173 [details] pushed as d2f79af - wayland/xdg-shell: update popup window monitor early
Attachment 337556 [details] pushed as 68645df - wayland/xdg-shell: Scale positioner coordinates