gtk+ passes an invalid rect with a width of 0 to set_anchor_rect() which triggers a Wayland protocol error.

That rect comes from pango, from:

->  popup_targets_received() in gtk+/gtk/gtktextview.c
-> gtk_text_view_get_iter_location()
-> gtk_text_layout_get_iter_location()
...
3022   pango_layout_index_to_pos (display->layout, byte_index, &pango_rect);
3023   
3024   rect->x = PANGO_PIXELS (x_offset + pango_rect.x);
3025   rect->y += PANGO_PIXELS (pango_rect.y) + display->top_margin;
3026   rect->width = PANGO_PIXELS (pango_rect.width);
3027   rect->height = PANGO_PIXELS (pango_rect.height);

Created attachment 343383 [details] [review]
[PATCH] wayland: avoid 0 width/height anchor rectangle

Passing a rectable with zero width or height to xdg_shell-v6
set_anchor_rect() will cause a protocol error and terminate the client,
as with gedit when pressing the Win key.

Reason for this is because the rectangle used to set the anchor comes
from gtk_text_layout_get_iter_location() which uses the pango layout
width/height, which can be empty if there is not character at the given
location.

Make sure we don't use 0 as width or height as an anchor rectangle to
avoid the protocol error.

Review of attachment 343383 [details] [review]:

Typo in the commit message: "rectable". In principle, the patch looks fine, but I would suggest to reconsider if it is a wise choice to build error conditions like this into the protocol.

Review of attachment 343383 [details] [review]:

Wouldn't this mean we offset the popup by 1 pixel in this case? I.e. if GTK+ wants to position something against 0x0+50+50, we'd now position it at (51,51) if the default move-to-rect rules apply. The problem though is that I can't think of a valid rule that positions the menu against a point|line, as the min size of the anchor rect is as mentioned 1.

So, this particular error condition might be a mistake, and should rather be if its negative size instead, allowing empty anchor rects.

Yes, as you say, xdg-shell v6 positionner definition makes it clear that the rectangle should not be 0:

  https://cgit.freedesktop.org/wayland/wayland-protocols/tree/unstable/xdg-shell/xdg-shell-unstable-v6.xml#n115

      For an xdg_positioner object to be considered complete, it must have a
      non-zero size set by set_size, and a non-zero anchor rectangle set by
      set_anchor_rect. Passing an incomplete xdg_positioner object when
      positioning a surface raises an error.

So the same error occurs with weston as well. 

There is a possibility that we can do, though, if the given size is zero, we use 1 as with attachment 343383 [details] [review]  but then compensate by moving the position by 1 pixel in the opposite direction.

Definitely a hack, but that would give the expected result without breaking the existing rules.

(In reply to Olivier Fourdan from comment #5)
> Yes, as you say, xdg-shell v6 positionner definition makes it clear that the
> rectangle should not be 0:
> 
>  
> https://cgit.freedesktop.org/wayland/wayland-protocols/tree/unstable/xdg-
> shell/xdg-shell-unstable-v6.xml#n115
> 
>       For an xdg_positioner object to be considered complete, it must have a
>       non-zero size set by set_size, and a non-zero anchor rectangle set by
>       set_anchor_rect. Passing an incomplete xdg_positioner object when
>       positioning a surface raises an error.
> 
> So the same error occurs with weston as well. 
> 
> There is a possibility that we can do, though, if the given size is zero, we
> use 1 as with attachment 343383 [details] [review] [review]  but then compensate by
> moving the position by 1 pixel in the opposite direction.
> 
> Definitely a hack, but that would give the expected result without breaking
> the existing rules.

The problem with that is that it'd be offset by 2 pixels if it flipped. Then again, if this is supposed to open at the "|" cursor (maybe it should still be a non-empty anchor that it should flip around, i.e. the width of the "|" cursor thing. So, I think the current patch is the best we can do for now, but we should still probably allow empty-size anchor in the next xdg-positioner version.

Created attachment 343414 [details] [review]
[PATCH v2] wayland: avoid 0 width/height anchor rectangle

Passing a rectangle with zero width or height to xdg_shell-v6
set_anchor_rect() will cause a protocol error and terminate the client,
as with gedit when pressing the Win key.

Reason for this is because the rectangle used to set the anchor comes
from gtk_text_layout_get_iter_location() which uses the pango layout
width/height, which can be empty if there is not character at the given
location.

Make sure we don't use 0 as width or height as an anchor rectangle to
avoid the protocol error, and compensate the logical position of the
given rectangle if the size is changed, so that the actual position
remains as expected by the client.
--
v2: Move the trickery to a separate sanitize_anchor_rect() function
    Compensate the position if the size is changed
    Fix typo in commit message

(In reply to Jonas Ådahl from comment #6)
> The problem with that is that it'd be offset by 2 pixels if it flipped. Then
> again, if this is supposed to open at the "|" cursor (maybe it should still
> be a non-empty anchor that it should flip around, i.e. the width of the "|"
> cursor thing.

The flip case is not so obvious to spot to be honest, and much less likely to occur (this is a problem that occurs only when the pango layout is empty, which is not so common, and when the menu needs flipping, which is even less common).

I reckon that attachment 343414 [details] [review] is better for the most general, common case. And tbh, I can't spot a wrong offset even when flipped using the zoom :)

So which one do we push, attachment 343383 [details] [review] (with a fix in the commit message) or attachment 343414 [details] [review] ?

(In reply to Olivier Fourdan from comment #9)
> So which one do we push, attachment 343383 [details] [review] [review] (with a fix in
> the commit message) or attachment 343414 [details] [review] [review] ?

I'd say we should avoid shifting the position. What if the position was at (0,0). xdg-shell doesn't allow positioning against outside of the window. We could do as in the latest patch, but also clamp so that its within the window geometry.

Created attachment 343542 [details] [review]
[PATCH v3] wayland: avoid 0 width/height anchor rectangle

v3: Make sure the anchor rectangle does not extend outside the window
    geometry of the parent surface after adjusting the size and position of
    the anchor rectangle to avoid a protocol error because of 0 width/height.

Review of attachment 343542 [details] [review]:

Looks good to me (with one style nit)

::: gdk/wayland/gdkwindow-wayland.c
@@ +2679,3 @@
+
+  rect->width  = MAX(1, rect->width);
+  rect->height = MAX(1, rect->height);

nit: space between MAX and (

Comment on attachment 343542 [details] [review]
[PATCH v3] wayland: avoid 0 width/height anchor rectangle

attachment 343542 [details] [review] pushed to git branch gtk-3-22 as commit 9a5ffcd - wayland: avoid 0 width/height anchor rectangle

attachment 343542 [details] [review] pushed to git master as commit 4259aba - wayland: avoid 0 width/height anchor rectangle