1421974 - [CSD] Use gtk_window_set_titlebar() to hide titlebar

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Description

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

Follow up from Bug 1419456 comments 17 - 21:

Let's use gtk_window_set_titlebar() and set empty titlebar on systems where GDK_DECOR_BORDER does not work.

Comment 1

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

Attachment: Bug 1421974 - refactor nsWindow::HideWindowChrome() to nsWindow::SetWindowDecoration() and share it with nsWindow::SetDrawsInTitlebar,

Review commit: https://reviewboard.mozilla.org/r/204594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204594/

Comment 2

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

Attachment: Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

This patch is based on Karl Tomlinson's (:karlt) demo from Bug 1419456. We use gtk_window_set_titlebar()
to set invisible widget. The widget takes place of GtkHeaderBar which leads Gtk+ to render CSD shadows
and handle window resizing ut does not render any titlebar.

gtk_window_set_titlebar() works on unrealized windows only and mShell is aleady realized at time
of nsWindow::SetDrawsInTitlebar() call. So we create a temporary GtkWindow, reparent mContainer
(which owns mGdkWindow) to it, unrealize mShell and set up the titlebar for mShell toplevel window.

As a workaround for Gtk+ Bug 791081 we also allocate some valid size for mShell before it's newly realized
with the updated titlebar.

Review commit: https://reviewboard.mozilla.org/r/204596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204596/

Comment 3

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

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

Karl, I'd like to have your feedback on the mShell reparent code which looks a bit tricky to me.

Comment 4

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '10896' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc'
remote: abort: working directory of /repo/hg/mozilla/try: timed out waiting for lock held by 'hgssh4.dmz.scl3.mozilla.com/effffffc:15934'
abort: stream ended unexpectedly (got 0 bytes, expected 4)

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

https://reviewboard.mozilla.org/r/204596/#review210804

Yes, I agree that reparenting is the best way to preserve the hierarchy during
the unrealize.  This may still have some undesirable consequences such as
moving windows to the current desktop.  The only other option I can imagine
would be to apply the setting only to new windows.  I don't think there is an
option with all the features you'll want.

If Gecko is putting any settings directly on the shell GdkWindow, instead of
the GtkWindow, then those will be lost, and so watch out for that.

::: widget/gtk/nsWindow.cpp:383
(Diff revision 1)
>  static GtkWidget *gInvisibleContainer = nullptr;
> +static GtkWidget *gInvisibleGtkWindow = nullptr;

gInvisibleContainer exists for situations where the container needs to live
after the function returns.

SetDrawsInTitlebar() is different because the GtkWindow is destroyed before
the function returns.  Create and destroy the window in SetDrawsInTitlebar(),
and the global is unnecessary.

::: widget/gtk/nsWindow.cpp:657
(Diff revision 1)
> +    if (!gInvisibleGtkWindow) {
> +        // Using GTK_WINDOW_POPUP rather than
> +        // GTK_WINDOW_TOPLEVEL in the hope that POPUP results in less
> +        // initialization and window manager interaction.
> +        gInvisibleGtkWindow = gtk_window_new(GTK_WINDOW_POPUP);
> +        gtk_widget_set_has_window(gInvisibleGtkWindow, false);

gtk_widget_set_has_window() "should only be called by widget implementations".

I can't imagine how a top level widget could be realized with no window, and
so I suspect this is a no-op for GtkWindow.  It shouldn't be necessary anyway.

::: widget/gtk/nsWindow.cpp:3815
(Diff revision 1)
>           * 1) We're running on Gtk+ without client side decorations.
>           *    Content is rendered to mShell window and we listen
>           *    to the Gtk+ events on mShell
> -         * 2) We're running on Gtk+ > 3.20 and client side decorations
> +         * 2) We're running on Gtk+ and client side decorations
>           *    are drawn by Gtk+ to mShell. Content is rendered to mContainer
>           *    and we listen to the Gtk+ events on mContainer.

These descriptions are no longer accurate as Gecko renders to the container window if CSD is available even if GTK is not drawing to mShell.

Adding a child window even when not using CSD is a significant change and one
which would not be expected just from reading the checkin comment.  Please add
something to checkin comment to describe this change.

There was a time where avoiding the child window on the MozContainer provided
smoother initial presentation and resize of windows, but I don't know whether
it still makes any difference.

Please check the appearance of opening new windows and resizing, with and
without this child window, on window managers with compositing and without.

If you are happy, then this approach is good because it is the simplest.

It would be possible to instead reparent children of the MozContainer and so
unrealize and realize the MozContainer also.  That would allow having no
window on the MozContainer, but would be much more complicated due to the need
to move the signal handlers from one widget to another.

But, if it is OK to add the child window for GTK versions that support CSD,
then it is OK to add the child window for another GTK versions, and the
no-window path can be removed altogether.

::: widget/gtk/nsWindow.cpp:3823
(Diff revision 1)
> -         * 2) We're running on Gtk+ > 3.20 and client side decorations
> +         * 2) We're running on Gtk+ and client side decorations
>           *    are drawn by Gtk+ to mShell. Content is rendered to mContainer
>           *    and we listen to the Gtk+ events on mContainer.
>           */
>          GtkStyleContext* style = gtk_widget_get_style_context(mShell);
> -        drawToContainer = gtk_style_context_has_class(style, "csd");
> +        drawToContainer = mIsCSDAvailable ||

UpdateOpaqueRegion() will become a no-op.  That's OK if the container window
is (usually) opaque.  Is it opaque?

::: widget/gtk/nsWindow.cpp:6706
(Diff revision 1)
> +              // titlebar.  GtkFixed is a somewhat random choice for a simple unused
> +              // widget. gtk_window_set_titlebar() takes ownership of the titlebar
> +              // widget.
> +              sGtkWindowSetTitlebar(GTK_WINDOW(mShell), gtk_fixed_new());
> +          } else {
> +              sGtkWindowSetTitlebar(GTK_WINDOW(mShell), nullptr);

Check this behaves as expected with GTK_CSD=1 in the environment.

Comment 6

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

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

https://reviewboard.mozilla.org/r/204596/#review210960

::: widget/gtk/nsWindow.cpp:3815
(Diff revision 1)
>           * 1) We're running on Gtk+ without client side decorations.
>           *    Content is rendered to mShell window and we listen
>           *    to the Gtk+ events on mShell
> -         * 2) We're running on Gtk+ > 3.20 and client side decorations
> +         * 2) We're running on Gtk+ and client side decorations
>           *    are drawn by Gtk+ to mShell. Content is rendered to mContainer
>           *    and we listen to the Gtk+ events on mContainer.

I suspect all systems with CSD enabled (which is our case) suffers from:

https://bugzilla.gnome.org/show_bug.cgi?id=748498

there's no dirrenece if the main GtkWindow has any child object (as the testcase I provided use just plain GtkWindow).

I filed Bug 1423151 to make sure we enable titlebar CSD rendering mode and mContainer rendering only on system with compositor enabled - in that case there's no penality for us when we render to mContainer.

::: widget/gtk/nsWindow.cpp:3823
(Diff revision 1)
> -         * 2) We're running on Gtk+ > 3.20 and client side decorations
> +         * 2) We're running on Gtk+ and client side decorations
>           *    are drawn by Gtk+ to mShell. Content is rendered to mContainer
>           *    and we listen to the Gtk+ events on mContainer.
>           */
>          GtkStyleContext* style = gtk_widget_get_style_context(mShell);
> -        drawToContainer = gtk_style_context_has_class(style, "csd");
> +        drawToContainer = mIsCSDAvailable ||

Yes, the container window is opaque - it also depends on Bug 1423151.

::: widget/gtk/nsWindow.cpp:6706
(Diff revision 1)
> +              // titlebar.  GtkFixed is a somewhat random choice for a simple unused
> +              // widget. gtk_window_set_titlebar() takes ownership of the titlebar
> +              // widget.
> +              sGtkWindowSetTitlebar(GTK_WINDOW(mShell), gtk_fixed_new());
> +          } else {
> +              sGtkWindowSetTitlebar(GTK_WINDOW(mShell), nullptr);

Checked, I see no difference. GTK_CSD is no-op for us as gtk_window_set_titlebar() always enables it:

https://developer.gnome.org/gtk3/stable/gtk-running.html
"CSD is always used for windows with a custom titlebar widget set, as the WM should not draw another titlebar or other decorations around the custom one."

Comment 7

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

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204596/diff/1-2/

Comment 8

[Jan Horak [:jhorak]]

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

https://reviewboard.mozilla.org/r/204596/#review211036

::: commit-message-36da4:5
(Diff revision 2)
> +Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work, r?jhorak
> +
> +This patch is based on Karl Tomlinson's (:karlt) demo from Bug 1419456. We use gtk_window_set_titlebar()
> +to set invisible widget. The widget takes place of GtkHeaderBar which leads Gtk+ to render CSD shadows
> +and handle window resizing ut does not render any titlebar.

please fix a typo "ut" and add comma if it should be there after "resizing"

::: commit-message-36da4:7
(Diff revision 2)
> +
> +This patch is based on Karl Tomlinson's (:karlt) demo from Bug 1419456. We use gtk_window_set_titlebar()
> +to set invisible widget. The widget takes place of GtkHeaderBar which leads Gtk+ to render CSD shadows
> +and handle window resizing ut does not render any titlebar.
> +
> +gtk_window_set_titlebar() works on unrealized windows only and mShell is aleady realized at time

"aleady"

::: widget/gtk/nsWindow.cpp:6679
(Diff revision 2)
> +          gtk_widget_unrealize(GTK_WIDGET(mShell));
> +
> +          // Available as of GTK 3.10+
> +          static auto sGtkWindowSetTitlebar = (void (*)(GtkWindow*, GtkWidget*))
> +              dlsym(RTLD_DEFAULT, "gtk_window_set_titlebar");
> +

Please assert why do you expect sGtkWindowSetTitlebar to be no null there.

Comment 9

[Jan Horak [:jhorak]]

Comment on attachment 8933641 [details]
Bug 1421974 - refactor nsWindow::HideWindowChrome() to nsWindow::SetWindowDecoration() and share it with nsWindow::SetDrawsInTitlebar,

https://reviewboard.mozilla.org/r/204594/#review211044

Looks more clear to me, thanks.

Comment 10

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

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204596/diff/2-3/

Comment 11

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

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

https://reviewboard.mozilla.org/r/204596/#review210804

> These descriptions are no longer accurate as Gecko renders to the container window if CSD is available even if GTK is not drawing to mShell.
> 
> Adding a child window even when not using CSD is a significant change and one
> which would not be expected just from reading the checkin comment.  Please add
> something to checkin comment to describe this change.
> 
> There was a time where avoiding the child window on the MozContainer provided
> smoother initial presentation and resize of windows, but I don't know whether
> it still makes any difference.
> 
> Please check the appearance of opening new windows and resizing, with and
> without this child window, on window managers with compositing and without.
> 
> If you are happy, then this approach is good because it is the simplest.
> 
> It would be possible to instead reparent children of the MozContainer and so
> unrealize and realize the MozContainer also.  That would allow having no
> window on the MozContainer, but would be much more complicated due to the need
> to move the signal handlers from one widget to another.
> 
> But, if it is OK to add the child window for GTK versions that support CSD,
> then it is OK to add the child window for another GTK versions, and the
> no-window path can be removed altogether.

I suspect all systems with CSD enabled (which is our case) suffers from:

https://bugzilla.gnome.org/show_bug.cgi?id=748498

there's no dirrenece if the main GtkWindow has any child object (as the testcase I provided use just plain GtkWindow).

I filed Bug 1423151 to make sure we enable titlebar CSD rendering mode and mContainer rendering only on system with compositor enabled - in that case there's no penality for us when we render to mContainer.

Comment 12

[Jan Horak [:jhorak]]

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

https://reviewboard.mozilla.org/r/204596/#review211330

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/663151801680
refactor nsWindow::HideWindowChrome() to nsWindow::SetWindowDecoration() and share it with nsWindow::SetDrawsInTitlebar, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/ed817bc6cc3b
use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work, r=jhorak

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/663151801680
https://hg.mozilla.org/mozilla-central/rev/ed817bc6cc3b

Comment 15

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

Comment on attachment 8933642 [details]
Bug 1421974 - use gtk_window_set_titlebar() to hide titlebar on WM where GDK_DECOR_BORDER does not work,

https://reviewboard.mozilla.org/r/204596/#review210804

> Check this behaves as expected with GTK_CSD=1 in the environment.

Filed as Bug 1423985 to make sure we don't have double shadows (one from CSD and one from WM).