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

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: freaktechnik, Unassigned)

Tracking

({regression})

44 Branch
mozilla45
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ wontfix, firefox44+ wontfix, firefox45 fixed)

Details

()

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

4 years ago
Posted image 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.
Reporter

Comment 1

4 years ago
Posted image About Dialog
Reporter

Comment 2

4 years ago
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
Reporter

Updated

4 years ago
Blocks: ship-gtk3
I can see that there too.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Also caused by GTK_CSD=1.
Summary: GTK 3: Dialog size not set correctly → set GTK_CSD=1 - GTK 3: Dialog size not set correctly
Reporter

Comment 5

4 years ago
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.
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.
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
Posted patch patch (obsolete) — Splinter Review
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!
Attachment #8673059 - Flags: review?(karlt)
Tracking this since it's a recent regression, and setting affected branch flags based on bug 1195002.
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.
Attachment #8673059 - Flags: review?(karlt)
Flags: needinfo?(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?).
Flags: needinfo?(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.
No longer blocks: ship-gtk3
(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.
Posted patch csd disable patch (obsolete) — Splinter Review
There's the patch if we need it.
Attachment #8683640 - Flags: review?(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.
Attachment #8683640 - Flags: review?(karlt) → review-
Posted patch patch v.2 (obsolete) — Splinter Review
Thanks, so something like that?
Attachment #8683640 - Attachment is obsolete: true
Attachment #8687208 - Flags: review?(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.
Attachment #8687208 - Flags: review?(karlt) → review-
This also doesn't restore whatever was set when executing subprocesses (like when opening a downloaded file with a helper application)
(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.).
(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.
Posted patch patch v.3Splinter Review
Fixed one.
Attachment #8673059 - Attachment is obsolete: true
Attachment #8687208 - Attachment is obsolete: true
Attachment #8687835 - Flags: review?(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!
Attachment #8687835 - Flags: review?(karlt) → review+

Comment 26

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7896f468df7e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Karl, Martin: Does this need to be uplifted to Aurora44?
Flags: needinfo?(stransky)
Flags: needinfo?(karlt)
I don't think it is important to uplift to 44 (for the reasons in comment 13), thanks.
Flags: needinfo?(karlt)
(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.
Flags: needinfo?(stransky)
You need to log in before you can comment on or make changes to this bug.