1209659 - set GTK_CSD=1 - GTK 3: Dialog size not set correctly

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Martin Giger [:freaktechnik]]

Attachment: Confirm close Dialog

Whenever any kind of dialog is shown, only the titlebar is displayed by default. The content can be revealed by resizing the dialog window. See the attachment for an example of the "do you really want to close all open tabs" warning.

This is on elementary OS freya with GTK 3.14.15 and the OS default theme.

The "about" dialog shows some content, but not all, a screenshot I'll attach next.

Comment 1

[Martin Giger [:freaktechnik]]

Attachment: About Dialog

Comment 2

[Martin Giger [:freaktechnik]]

I forgot to mention that I get these warnings when running Nightly from the console:

*** BUG ***
In pixman_region32_init_rect: Invalid rectangle passed
Set a breakpoint on '_pixman_log_error' to debug

Comment 3

[Martin Stránský [:stransky] (ni? me)]

I can see that there too.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Also caused by GTK_CSD=1.

Comment 5

[Martin Giger [:freaktechnik]]

I've noticed when working with the instantbird nightly, that windows with saved sizes get smaller and smaller (in Instantbird that affects the Add-on Manager, the account manager, the buddy list and the chat window...). It feels as if The size is set with the border that doesn't get repainted included and then the size of the child window gets safed, but that's a completely untested assumption, from how much the windows shrink. This could also explain the errors for small dialogs, since there is not enough space for a child window within the set size.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

I suspect the GtkHeaderBar causes that, it's automatically added as a child widget of every window in CSD mode and we're not aware of it.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

It's caused by broken gtk_window_resize(), it counts CSD extents to the window size:
https://bugzilla.redhat.com/show_bug.cgi?id=1269437

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

This one utilizes the gtk container allocation mechanism, see:
https://bugzilla.redhat.com/show_bug.cgi?id=1269437#c11
for details. I'm not sure about the NativeResize() - is it okay to move it after the allocation check? Thanks!

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this since it's a recent regression, and setting affected branch flags based on bug 1195002.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

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

https://bugzilla.redhat.com/show_bug.cgi?id=1269437#c13

Removing review request as Matthias is questioning this approach.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Gtk bug: https://bugzilla.gnome.org/show_bug.cgi?id=756618

Karl, may we use the allocation workaround for older Gtk releases?

Comment 12

[Karl Tomlinson (:karlt)]

I don't really know what the consequences of this workaround might be, but 
gtk_widget_size_allocate sounds scary.

I think a better solution is to temporarily unset GTK_CSD on affected versions at the appropriate time (realize, perhaps?).

Comment 13

[Karl Tomlinson (:karlt)]

I'd take a fix for this along the lines of comment 12, but I don't think this is a must-fix for shipping the GTK3 port.

I think it is reasonable to expect distros that set GTK3_CSD to use a version of gtk that works with that setting.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> I think a better solution is to temporarily unset GTK_CSD on affected
> versions at the appropriate time (realize, perhaps?).

GTK_CSD has to be disabled all the time. It's checked by get_shadow_width() which computes window border for window positioning/scaling.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Attachment: csd disable patch

There's the patch if we need it.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8683640 [details] [diff] [review]
csd disable patch

(In reply to Martin Stránský from comment #14)
> GTK_CSD has to be disabled all the time. It's checked by get_shadow_width()
> which computes window border for window positioning/scaling.

Ah, OK thanks.
I'm happy to save people with GTK versions < 3.20 from the perils of CSD.

>+++ b/toolkit/xre/nsAppRunner.cpp

>+#if (MOZ_WIDGET_GTK == 3)
>+  // Workaround for bug 1209659 which is fixed by Gtk3.20
>+  if(gtk_check_version(3, 20, 0))
>+    PR_SetEnv("GTK_CSD=0");
>+#endif

Because libxul is a dynamic library it will be unloaded before the program
exists, which means that we can't leave pointers to data in the library in the
environment.  Also, the environment is mutable (not const), ruling out literal strings.

Although leaking a string is often the typical solution for adding an environment variable, in this case unsetenv() is a nicer solution.

Can you move this to nsAppShell::Init(), please?
We'd like to keep platform-specific code out of nsAppRunner where not
absolutely necessary.  Here this just needs to be set before the first visible
window is created.  nsAppShell::Init() sets up the event loop so should be
early enough.

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.2

Thanks, so something like that?

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8687208 [details] [diff] [review]
patch v.2

>@@ -129,16 +134,20 @@ nsAppShell::Init()
>+    // Workaround for bug 1209659 which is fixed by Gtk3.20
>+    if(gtk_check_version(3, 20, 0) != nullptr)
>+        PR_SetEnv("GTK_CSD=0");

Use unsetenv("GTK_CSD") here instead of PR_SetEnv, and the code in the destructor is not required.

That is simpler and avoids the problem with a dangling pointer should exit() be called without destroying the nsAppShell and puts no const data in the environment.

Comment 19

[Mike Hommey [:glandium]]

This also doesn't restore whatever was set when executing subprocesses (like when opening a downloaded file with a helper application)

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to Mike Hommey [:glandium] from comment #19)
> This also doesn't restore whatever was set when executing subprocesses (like
> when opening a downloaded file with a helper application)

Yes, but that's not really a practical option.  A temporary setenv is not possible because GTK will check the env again when resizing (at least).  Keeping a record to restore env before fork is not worth the effort IMO.

https://bugzilla.gnome.org/show_bug.cgi?id=756618 will also affect other applications and we would be doing people a favour by saving them from that bug and CSD in general (c.f. bug 1195002 comment 28.).

Comment 21

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #20)
> https://bugzilla.gnome.org/show_bug.cgi?id=756618 will also affect other
> applications and we would be doing people a favour by saving them from that
> bug and CSD in general (c.f. bug 1195002 comment 28.).

Also that's a temporary workaround until fixed gtk 3.20 hits the users.

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.3

Fixed one.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8687835 [details] [diff] [review]
patch v.3

>+    if(gtk_check_version(3, 20, 0) != nullptr)

Space between "if" and "(" for Mozilla style.  Thanks!

Comment 24

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch for check-in

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae94dcf741c0

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7896f468df7e

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7896f468df7e

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Karl, Martin: Does this need to be uplifted to Aurora44?

Comment 28

[Karl Tomlinson (:karlt)]

I don't think it is important to uplift to 44 (for the reasons in comment 13), thanks.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #28)
> I don't think it is important to uplift to 44 (for the reasons in comment
> 13), thanks.

Thanks Karl. In that case we will mark this as wontfix for FF44.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 43 as well.