1283299 - Implement titlebar rendering on GTK 3.20+

Status: ASSIGNED

Description

[Andrew Comminos [:acomminos]]

We'd like to draw window decorations in Gecko on GTK 3.20 and above. This will allow us to draw tabs in the titlebar, and permit drawing on Wayland using OMTC (as GTK restricts drawing to the main-thread "draw" handler on Wayland, as it must draw its own decorations).

3.20 is chosen as a target as it provides us with gtk_render_background_get_clip, which is required for us to fetch the metrics for the drop shadows that GTK uses for its window decorations. It also exposes CSD styling sanely via the "decoration" CSS node on GtkWindow.

Comment 1

[Andrew Comminos [:acomminos]]

Created attachment 8768174 [details]
Bug 1283299 - Part 1: Mark GTK3 as a supported toolkit for DRAW_IN_TITLEBAR.

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

Comment 2

[Andrew Comminos [:acomminos]]

Created attachment 8768175 [details]
Bug 1283299 - Part 2: Add support for drawing decorated windows on GTK 3.20.

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

Comment 3

[Andrew Comminos [:acomminos]]

Created attachment 8768176 [details]
Bug 1283299 - Part 3: Add pref for using ARGB visuals on GTK.

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

Comment 4

[Andrew Comminos [:acomminos]]

Created attachment 8768177 [details]
Bug 1283299 - Part 4: Add support for using ARGB windows with OMTC when an X11 compositor is active.

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

Comment 5

[Andrew Comminos [:acomminos]]

Created attachment 8768178 [details]
Bug 1283299 - Part 5: Undecorate GTK window when chrome margins are 0.

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

Comment 6

[Andrew Comminos [:acomminos]]

Created attachment 8768179 [details]
Bug 1283299 - Part 6: Implement titlebar and titlebar button drawing using GtkHeaderBar.

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

Comment 7

[Andrew Comminos [:acomminos]]

Created attachment 8768180 [details]
Bug 1283299 - Part 7: Report GTK CSD shadow extents to window manager.

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

Comment 8

[Andrew Comminos [:acomminos]]

Created attachment 8768181 [details]
Bug 1283299 - Part 8: Add client-size resizers for GTK windows.

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

Comment 9

[Andrew Comminos [:acomminos]]

Created attachment 8768182 [details]
Bug 1283299 - Part 9: Update browser styles to use CSD on GTK.

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

Comment 10

[Andrew Comminos [:acomminos]]

Created attachment 8768183 [details]
Bug 1283299 - Part 10: Disable tabs in titlebar on GTK versions before 3.20.

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

Comment 11

[Andrew Comminos [:acomminos]]

Here's an initial draft of a CSD implementation. A few things of note;

- CSD is restricted to GTK 3.20. This is because it allows us to get the shadow extents, as well as provides the "solid-csd" style class which we use on windows whose visuals lack an alpha channel.
- The "decoration" CSS node on GtkWindow is used to draw the decorated window.
- GtkHeaderBar is used for titlebar styling.
- RGBA windows are currently not requested by default, a pref has been added however.
  I'm thinking we might want to add a path for window re-realization with an ARGB visual when CSD is activated.
- Client side resizers have been added when client decorations are used. Hit detection uses the border margins, much like native GTK client resizers.
- When using non-solid CSD, shadow extents are reported to the window manager.

Feedback would be greatly appreciated- thanks Karl!

Comment 12

[[e-mail]]

There are some problems:
* Something wrong with borders around window
* It adds minimize/maximize buttons along with close button even I have those disabled

Comment 13

[[e-mail]]

Created attachment 8770233 [details]
FF screenshot

Comment 14

[[e-mail]]

Created attachment 8770234 [details]
Another GTK app on same system

Comment 15

[Jim Mathies [:jimm]]

Hey Karl, are you going to get to these? Maybe there's someone else who could take these on?

Comment 17

[Karl Tomlinson (:karlt)]

I'm keen to look at these, but I've been distracted by native theming problems needing attention, sorry.

The ABI stability promised to have arrived in GTK3 should mean that the distractions will fade, as well as providing a good foundation for the drawing here.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8768177 [details]
Bug 1283299 - Part 4: Add support for using ARGB windows with OMTC when an X11 compositor is active.

https://reviewboard.mozilla.org/r/62482/#review116624

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8768182 [details]
Bug 1283299 - Part 9: Update browser styles to use CSD on GTK.

https://reviewboard.mozilla.org/r/62492/#review130784

Looks good, but please add comments to explain, or remove the main-window background- properties.

::: browser/themes/linux/browser.css:1952
(Diff revision 1)
> +  background-color: transparent;
> +  background-clip: border-box;
> +  background-origin: border-box;

Are these actually necessary?

My assumption has been that -moz-appearance replaces drawing of background-color and -moz-appearance draws the background to the border box anyway.

Is the background always drawn, or is this a special case for the root element?

Or is the background-color used to detect translucency somewhere?

What effect does changing the background-clip and origin have if the background is transparent?
Is there a background-image involved?

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8768176 [details]
Bug 1283299 - Part 3: Add pref for using ARGB visuals on GTK.

https://reviewboard.mozilla.org/r/62480/#review130716

::: widget/gtk/nsWindow.cpp:3654
(Diff revision 1)
>          GtkWindowType type =
>              mWindowType != eWindowType_popup || aInitData->mNoAutoHide ?
>                GTK_WINDOW_TOPLEVEL : GTK_WINDOW_POPUP;
>          mShell = gtk_window_new(type);
>  
> +        if (Preferences::GetBool("mozilla.widget.use-argb-visuals", false)) {

Can you add a comment, please, along the lines of "use-argb-visuals is a hidden pref defaulting to false to allow experimentation.  UpdateOpaqueRegion() should be implemented using gdk_window_set_opaque_region() before ARGB windows are used more widely."

::: widget/gtk/nsWindow.cpp:3655
(Diff revision 1)
> +            GdkScreen *screen = gtk_widget_get_screen(mShell);
> +#if (MOZ_WIDGET_GTK == 2)
> +            GdkColormap *colormap = gdk_screen_get_rgba_colormap(screen);
> +            gtk_widget_set_colormap(mShell, colormap);
> +#else
> +            GdkVisual *visual = gdk_screen_get_rgba_visual(screen);
> +            gtk_widget_set_visual(mShell, visual);
> +#endif

I think it would also be nice to move the existing mWindowType == eWindowType_popup && aInitData->mSupportTranslucency to here so that this is all in one place and done only once.  That would bring with it the gdk_screen_is_composited() check, which I think would be appropriate.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] (back to classes, will be slow) from comment #11)
> - RGBA windows are currently not requested by default, a pref has been added
> however.
>   I'm thinking we might want to add a path for window re-realization with an
> ARGB visual when CSD is activated.

On compositing window managers supporting _NET_WM_OPAQUE_REGION, the cost of
ARGB windows should be small I expect, and so there's probably no need for
unrealize/realize.

I don't think there's a need to be too concerned about existing windows when composited is enabled or disabled, provided a restart resolves the situation.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8768178 [details]
Bug 1283299 - Part 5: Undecorate GTK window when chrome margins are 0.

https://reviewboard.mozilla.org/r/62484/#review116628

This is one option that could work.  I'm interested to hear what you think about getting GTK to handle the CSD borders.

::: widget/gtk/nsWindow.h:357
(Diff revision 1)
>                                                  double aPointerPressure,
>                                                  uint32_t aPointerOrientation,
>                                                  nsIObserver* aObserver) override;
>  #endif
>  
> +    NS_IMETHOD SetNonClientMargins(LayoutDeviceIntMargin& aMargins) override;

This is now just a plain virtual, without NS_IMETHOD, since changes for bug 1325234.

::: widget/gtk/nsWindow.h:401
(Diff revision 1)
>                                        GtkWidget* aOldContainer);
>  
>      virtual void RegisterTouchWindow() override;
>  
> +    // Decorations
> +    bool IsClientDecorated() const;

IsGeckoDecorated() would be clearer to distinguish from GTK client decorated.

::: widget/gtk/nsWindow.h:567
(Diff revision 1)
> +    // If true, draw our own window decorations (where supported).
> +    bool mDrawsInTitlebar;

Please insert this alongside some other bool members.

::: widget/gtk/nsWindow.cpp:6891
(Diff revision 1)
>          }
>      }
>  }
>  
> +NS_IMETHODIMP
> +nsWindow::SetNonClientMargins(LayoutDeviceIntMargin &aMargins)

updateTitlebarDisplay() in browser-tabsintitlebar.js requests that the
non-client top-margin only be removed, but this is interpreted as removing all
the margins, which is a little confusing.  Please document that this is what
happens.

More than that, the shadow extents and resizers assume that the client will
draw decorations.  That is not expected from the SetNonClientMargins() method,
and so please document the expectations of the widget's document to use
-moz-gtk-window-decoration when sizemode is normal and chromemargin top is not
default.

There is another option to consider:

If GTK draws the CSD borders, then it will handle shadows, resizing, and
gtk_widget_set_csd_input_shape().

GtkWindow can have a dummy titlebar set with zero height, and Gecko can draw
the titlebar and handle titlebar mouse events.

I think this would be simpler overall.  The disadvantage is that Gecko would
be using a different window, but I assume drawing/resizing synchronization is
already broken with OMTC.

I assume Gecko could use an opaque window and so there's no need to track the
opaque region.

nsIWidget window sizes should not include the shadow and GTK could handle
that, but the window sizes were always incorrect anyway in applying outer
dimensions as inner window sizes.

What do you think?

::: widget/gtk/nsWindow.cpp:6898
(Diff revision 1)
> +  SetDrawsInTitlebar(aMargins.top == 0);
> +  return NS_OK;
> +}
> +
> +void
> +nsWindow::SetDrawsInTitlebar(bool aState)

This method is not really about drawing in the titlebar but about doing
decorations in Gecko, so please name the method and mDrawsInTitlebar and
IsClientDecorated() to indicate this.

I'm assuming that usage of drawintitlebar was removed in
https://hg.mozilla.org/mozilla-central/rev/35d68446e000 for bug 576740
and it is not necessary to implement SetDrawsInTitlebar().

::: widget/gtk/nsWindow.cpp:6905
(Diff revision 1)
> +  if (aState == mDrawsInTitlebar)
> +    return;
> +
> +  if (mShell) {
> +    NS_WARNING("gtk_window_set_decorated may not have any effect when called on a window that is already visible.");
> +    gtk_window_set_decorated(GTK_WINDOW(mShell), !aState);

HideWindowChrome() and mBorderStyle should also be considered, I guess.  If
HideWindowChrome(false) is called while drawing in the titlebar, then I guess
decorations should not be shown.

I suspect these methods/parameters should have orthogonal state.

::: widget/gtk/nsWindow.cpp:7213
(Diff revision 1)
>  }
> +
> +bool
> +nsWindow::IsClientDecorated() const
> +{
> +    return mDrawsInTitlebar;

I think it would be simpler and clearer to consider the sizemode here instead in all the callers.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8768175 [details]
Bug 1283299 - Part 2: Add support for drawing decorated windows on GTK 3.20.

https://reviewboard.mozilla.org/r/62478/#review116256

::: widget/gtk/WidgetStyleCache.cpp:35
(Diff revision 1)
> +  case MOZ_GTK_WINDOW_CSD:
> +    gtk_style_context_add_class(style, "csd");
> +    break;
> +  case MOZ_GTK_WINDOW_SOLID_CSD:
> +    gtk_style_context_add_class(style, "solid-csd");

I haven't reviewed the style context creation, so this is an issue to record that this part still needs review, if this approach is pursued.

::: widget/gtk/gtk3drawing.cpp:739
(Diff revision 1)
> +  GtkStyleContext* style = ClaimStyleContext(MOZ_GTK_WINDOW_DECORATION);
> +
> +  // Available on GTK 3.20+.
> +  static auto sGtkRenderBackgroundGetClip =
> +    (void (*)(GtkStyleContext*, gdouble, gdouble, gdouble, gdouble, GdkRectangle*))
> +    dlsym(RTLD_DEFAULT, "gtk_render_background_get_clip");

The margin is also considered.  See get_shadow_width() in gtkwindow.c.

::: widget/gtk/gtk3drawing.cpp:2846
(Diff revision 1)
> +    case MOZ_GTK_WINDOW_DECORATION_SOLID:
> +        {
> +            style = ClaimStyleContext(MOZ_GTK_WINDOW_DECORATION_SOLID);
> +            moz_gtk_add_style_border(style, left, top, right, bottom);
> +            moz_gtk_add_style_padding(style, left, top, right, bottom);
> +            moz_gtk_add_style_margin(style, left, top, right, bottom);

moz_gtk_window_decoration_solid_paint() and gtk_window_draw() in gtkwindow.c don't apply the margin, and so it should not be here.

::: widget/gtk/nsWindow.h:356
(Diff revision 1)
> +    bool HasARGBVisual() const;
> +    bool IsComposited() const;

Can IsComposited() just check HasARGBVisual() and so avoid the need for two calls.

I'd prefer not to have these public, if they are not used elsewhere.

::: widget/gtk/nsWindow.cpp:6867
(Diff revision 1)
> +}
> +
> +bool
> +nsWindow::UseSolidCSD() const
> +{
> +  return !HasARGBVisual() || !IsComposited();

This would be the same as IsComposited(), if that includes the ARGB check.  And I think that would be a better name, as "UseSolidCSD" may be read as meaning that CSD is being used, but it may not be.

So I think a single public IsComposited() is what is needed here.

That could then also be used from OnExposeEvent().

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8768174 [details]
Bug 1283299 - Part 1: Mark GTK3 as a supported toolkit for DRAW_IN_TITLEBAR.

https://reviewboard.mozilla.org/r/62476/#review115812

It is the right approach to land what is ready and then add the
missing components incrementally.  However, we need something to prevent
exposing this before it is ready.

There is a pref involved here but it is one exposed in the UI, and people
expect features in the UI to work, and so the likelihood of a flood of bug
reports for things we already know about.

I wondered about making this Nightly-only, but that has the disadvantage of
not testing the shipped code on Nightly.

I think the best solution might be to hide
customization-titlebar-visibility-button from the UI until this is ready.

When ready to ship, hopefully that can still be hidden when CSD is not
available.

https://bugzilla.mozilla.org/show_bug.cgi?id=893682 reports a performance difference from setting chromemargin too late, so beware that a solution will be required if talos detects this.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8768183 [details]
Bug 1283299 - Part 10: Disable tabs in titlebar on GTK versions before 3.20.

https://reviewboard.mozilla.org/r/62494/#review130702

::: browser/themes/linux/browser.css:1950
(Diff revision 1)
> +@media not all and (-moz-gtk-csd-available) {
> +  #main-window > #titlebar {
> +    /* We need to hide the titlebar explicitly on versions of GTK without CSD. */
> +    display: none;

Why don't these rules hide the titlebar?
http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/browser/base/content/browser.css#218

::: browser/themes/linux/browser.css:1957
(Diff revision 1)
> +@media (-moz-gtk-csd-available) {
> -#main-window[tabsintitlebar][sizemode="normal"] {
> +  #main-window[tabsintitlebar][sizemode="normal"] {

Given TabsInTitlebar._update sets tabsintitlebar only for (-moz-gtk-csd-available) media, is the media query necessary here?

::: browser/themes/linux/browser.css:1998
(Diff revision 1)
>  
> -#titlebar-close {
> +  #titlebar-close {
> -  list-style-image: url("moz-icon://stock/window-close-symbolic?size=menu");
> +    list-style-image: url("moz-icon://stock/window-close-symbolic?size=menu");
> -  -moz-appearance: -moz-window-button-close;
> +    -moz-appearance: -moz-window-button-close;
> -}
> +  }
> -
> +}

Can customization-titlebar-visibility-button be hidden when CSD is not available?
Filing a followup bug is probably the best answer if this button is hidden until ready.

::: widget/gtk/nsLookAndFeel.cpp:1412
(Diff revision 1)
>      gtk_widget_destroy(window);
> +
> +    // Require GTK 3.20 for client-side decoration support.
> +    // 3.20 exposes gtk_render_background_get_clip, which is required for
> +    // calculating shadow metrics for decorated windows.
> +    sCSDAvailable = gtk_check_version(3, 20, 0) == nullptr;

I wonder whether there should be other factors here, but this is probably the best thing to do.

(In reply to Andrew Comminos [:acomminos] (back to classes, will be slow) from comment #11)
> Here's an initial draft of a CSD implementation. A few things of note;
> 
> - CSD is restricted to GTK 3.20. This is because it allows us to get the
> shadow extents, as well as provides the "solid-csd" style class which we use
> on windows whose visuals lack an alpha channel.

Restricting to 3.20 is good.  Backporting would be further work, and doing
that would be lower priority.

However, I'll point out that even GTK with GTK_CSD=1 doesn't use CSD if
gtk_window_supports_client_shadow returns false.

The solid-csd style seems to exist for the sake of applications that force a
header bar, but these applications are awful to use on some systems
https://bugzilla.gnome.org/show_bug.cgi?id=729721 tracks these kind of
problems.
The solid-csd doesn't use the shape extension either and so looks bad too on
systems that typically have rounded corners.
I understand why having a header bar with controls might be a good reason to
stop the window manager from adding a title bar.  I suspect it may have been
better to continue to let the window manager draw the decorations, but
https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-set-decorations
explains that very few window managers honor all possible combinations of
decoration hints.

I think it is reasonable to give users the choice to opt into tabs-in-titlebar
on systems without compositing and _GTK_FRAME_EXTENTS, but I think we should
always keep the default as wm-side decorations on systems that expect this.

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 8768180 [details]
Bug 1283299 - Part 7: Report GTK CSD shadow extents to window manager.

https://reviewboard.mozilla.org/r/62488/#review117710

::: widget/gtk/nsWindow.cpp:7228
(Diff revision 1)
> +  // Shadows are only used for normal, non-solid client windows with CSD.
> +  gint top = 0, right = 0, bottom = 0, left = 0;
> +  if (IsClientDecorated() && !UseSolidCSD() && mSizeState == nsSizeMode_Normal) {
> +    moz_gtk_get_window_decoration_extents(&top, &right, &bottom, &left);
> +  }
> +
> +  static auto sGdkWindowSetShadowWidth =
> +    (void (*)(GdkWindow*, gint, gint, gint, gint))
> +    dlsym(RTLD_DEFAULT, "gdk_window_set_shadow_width");
> +  sGdkWindowSetShadowWidth(mGdkWindow, left, right, top, bottom);

Looks like this call would conflict with GTK's calls when using GTK's CSD (which may happen when !mDrawsInTitlebar).

I suspect gdk_window_set_shadow_width() should only be called when using Gecko's CSD.

(Looks like the only way to undo the call when switching to server-side decorations would be to remove the property, but maybe that's unnecessary because it is ignored in that case.)

Comment 27

[u589863]

This should blocker this bug 1325171 & bug 1349423

Comment 28

[Karl Tomlinson (:karlt)]

Comment on attachment 8768181 [details]
Bug 1283299 - Part 8: Add client-size resizers for GTK windows.

https://reviewboard.mozilla.org/r/62490/#review117662

It feels a little odd having the widget code do hit detection for mouse events, but browser.xul/.css determine the appearance and size of borders.  However, getting browser.xul to handle events would trigger widget code anyway, and would only be more complicated, I assume.  And widget code would still need to handle the input shape and shadow extents.  So this seems reasonable if Gecko is drawing the decorations.  This would be unnecessary if GTK is drawing the decorations.

Please update the commit message to indicate that this part is just the mouse event handling.  i.e. the drawing of resizer decorations was in a different patch.

::: widget/gtk/nsWindow.h:409
(Diff revision 1)
>      // Informs the window manager about the size of the shadows surrounding
>      // a client-side decorated window.
>      void UpdateClientShadowWidth();
>  
> +    // Returns true if the given point (in device pixels) is within a resizer
> +    // region of the window. Only used when drawing decorations client side.

"Always returns false if not drawing decorations in Gecko." instead of the last sentence.

::: widget/gtk/nsWindow.h:410
(Diff revision 1)
>      // a client-side decorated window.
>      void UpdateClientShadowWidth();
>  
> +    // Returns true if the given point (in device pixels) is within a resizer
> +    // region of the window. Only used when drawing decorations client side.
> +    bool CheckResizerEdge(LayoutDeviceIntPoint aPoint, GdkWindowEdge& aOutEdge);

Name this IsOnResizerEdge() or similar, to more clearly indicate a function without side-effects.

Use a pointer for the out parameter to more clearly indicate the out nature at the call site, and to follow Gecko style guide.

::: widget/gtk/nsWindow.cpp:7324
(Diff revision 1)
> +  if (leftDist <= resizerSize && topDist <= resizerSize) {
> +    aOutEdge = GDK_WINDOW_EDGE_NORTH_WEST;

This needs to be a little more complex so as not to resize when inside the padding.  i.e. the corner resizers need to be L-shaped rather than square.  My reading of update_border_windows() in gtkwindow.c is that decoration-resize-handle indicates the length of the line segments on the inside of the L, not the length on the outside.
i.e. the length in addition to the corner square.

The decoration-resize-handle number should also be adjusted for small windows.

Once gtk_widget_set_csd_input_shape() is called appropriately, I guess that should prevent resizing from the shadow.

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 8768179 [details]
Bug 1283299 - Part 6: Implement titlebar and titlebar button drawing using GtkHeaderBar.

https://reviewboard.mozilla.org/r/62486/#review117336

::: widget/gtk/WidgetStyleCache.cpp:144
(Diff revision 1)
> +
> +  GtkWidget* widget = sGtkHeaderBarNewPtr();
> +  AddToWindowContainer(widget);
> +
> +  GtkStyleContext* style = gtk_widget_get_style_context(widget);
> +  gtk_style_context_add_class(style, MOZ_GTK_STYLE_CLASS_TITLEBAR);

create_titlebar() in gtkwindow.c also adds class "default-decoration" and
Adwaita has selectors looking for this.  However gtk_window_set_titlebar()
adds only "titlebar", without "default-decoration".  I don't know what is
intended by the distinction, but I guess Gecko is not drawing precisely the
"default decoration", and so omitting that class (as in this patch) would
be appropriate.  Something to watch out for though if there are visual differences.

::: widget/gtk/WidgetStyleCache.cpp:428
(Diff revision 1)
> +  GtkWidget* window = GetWidget(MOZ_GTK_WINDOW);
> +  GtkStyleContext* windowStyle = gtk_widget_get_style_context(window);
> +  if (aFlags & MOZ_WINDOW_MAXIMIZED)
> +    gtk_style_context_add_class(windowStyle, "maximized");

I suspect this is going to invalidate resolved styles on all descendant widgets,
which will slow down actions on other widgets, even when they are not expected
to depend on the window size state.  It would be better to set and remove
"maximized" only for widgets where the MOZ_WINDOW_MAXIMIZED flag is set
appropriately, and do this only when there is a change in what is required.

One option would be to pass MOZ_WINDOW_MAXIMIZED or MOZ_WINDOW_NOT_MAXIMIZED
flags to ClaimStyleContext() only when the state is expected to matter, and ClaimStyleContext() would make no changes to the "maximized" class if there is no such flag.

However, if this is only relevant for titlebars, then I suspect it may be
tidier for the caller to make changes in the "maximized" class

::: widget/gtk/WidgetStyleCache.cpp:480
(Diff revision 1)
>    sCurrentStyleContext = nullptr;
>  #endif
> +
> +  GtkWidget* window = GetWidget(MOZ_GTK_WINDOW);
> +  GtkStyleContext* style = gtk_widget_get_style_context(window);
> +  gtk_style_context_remove_class(style, "maximized");

I would like to remove the need for ReleaseStyleContext() for bug 1319650.

But, as described above, I think it is also better to leave this class on the window until it needs to be removed, instead of adding it and removing it soon after.

::: widget/gtk/gtk3drawing.cpp:2616
(Diff revision 1)
> +    style = ClaimStyleContext(MOZ_GTK_HEADER_BAR, GTK_TEXT_DIR_LTR,
> +                              state_flags, style_flags);
> +
> +    gtk_render_background(style, cr, rect->x, rect->y, rect->width,

Gadgets subtract margins before drawing IIRC.

::: widget/gtk/gtk3drawing.cpp:2874
(Diff revision 1)
> +            style = ClaimStyleContext(MOZ_GTK_HEADER_BAR);
> +            moz_gtk_add_style_border(style, left, top, right, bottom);

In Ambiance for 16.10, the titlebar border depends on the .maximized class, and so I assume MOZ_GTK_CSD_MAXIMIZED should be considered here also.

::: widget/gtk/gtkdrawing.h:75
(Diff revision 1)
>  } GtkTabFlags;
>  
> +// Flags used when drawing window decorations.
> +typedef enum {
> +  MOZ_GTK_CSD_MAXIMIZED = 1 << 1
> +} GtkCSDFlags;

MozGtkCSDFlags or WidgetCSDFlags or similar, please, so as not to add more names in GTK's Gtk namespace.

::: widget/gtk/nsNativeThemeGTK.cpp:717
(Diff revision 1)
> +  case NS_THEME_WINDOW_BUTTON_CLOSE:
> +  case NS_THEME_WINDOW_BUTTON_MINIMIZE:
> +  case NS_THEME_WINDOW_BUTTON_MAXIMIZE:
> +  case NS_THEME_WINDOW_BUTTON_RESTORE:
> +    aGtkWidgetType = MOZ_GTK_HEADER_BAR_BUTTON;

GTK adds "minimize", "maximize", and "close" classes for these buttons, and so three different style contexts may need to be cached.  (Maximize and Restore states both use the "maximize" class.)

Adwaita doesn't use these, but Ambiance for 16.10 has ".titlebar button.titlebutton.close" selectors.

Comment 30

[Martin Stránský [:stransky]]

I'm going to continue here unless Andrew wants to step in. I'd prefer to file dependent bugs to incrementally commit what's done to prevent bit rot of the patches and move forward smoothly.

Comment 31

[Martin Stránský [:stransky]]

Comment on attachment 8768176 [details]
Bug 1283299 - Part 3: Add pref for using ARGB visuals on GTK.

https://reviewboard.mozilla.org/r/62480/#review142012

::: widget/gtk/nsWindow.cpp:3654
(Diff revision 1)
>          GtkWindowType type =
>              mWindowType != eWindowType_popup || aInitData->mNoAutoHide ?
>                GTK_WINDOW_TOPLEVEL : GTK_WINDOW_POPUP;
>          mShell = gtk_window_new(type);
>  
> +        if (Preferences::GetBool("mozilla.widget.use-argb-visuals", false)) {

Fixed here:
https://bugzilla.mozilla.org/attachment.cgi?id=8867135

Comment 32

[Martin Stránský [:stransky]]

Comment on attachment 8768176 [details]
Bug 1283299 - Part 3: Add pref for using ARGB visuals on GTK.

https://reviewboard.mozilla.org/r/62480/#review130716

> Can you add a comment, please, along the lines of "use-argb-visuals is a hidden pref defaulting to false to allow experimentation.  UpdateOpaqueRegion() should be implemented using gdk_window_set_opaque_region() before ARGB windows are used more widely."

The UpdateOpaqueRegion is implemented here:
https://bugzilla.mozilla.org/attachment.cgi?id=8867137

Comment 33

[Martin Stránský [:stransky]]

Comment on attachment 8768176 [details]
Bug 1283299 - Part 3: Add pref for using ARGB visuals on GTK.

https://reviewboard.mozilla.org/r/62480/#review130716

> I think it would also be nice to move the existing mWindowType == eWindowType_popup && aInitData->mSupportTranslucency to here so that this is all in one place and done only once.  That would bring with it the gdk_screen_is_composited() check, which I think would be appropriate.

Fixed here:
https://bugzilla.mozilla.org/attachment.cgi?id=8867135

Comment 34

[Martin Stránský [:stransky]]

Comment on attachment 8768175 [details]
Bug 1283299 - Part 2: Add support for drawing decorated windows on GTK 3.20.

https://reviewboard.mozilla.org/r/62478/#review142018

::: widget/gtk/nsWindow.h:356
(Diff revision 1)
> +    bool HasARGBVisual() const;
> +    bool IsComposited() const;

Fixed here:
https://bugzilla.mozilla.org/attachment.cgi?id=8867136

Comment 35

[Martin Stránský [:stransky]]

Comment on attachment 8768179 [details]
Bug 1283299 - Part 6: Implement titlebar and titlebar button drawing using GtkHeaderBar.

https://reviewboard.mozilla.org/r/62486/#review142460

::: widget/gtk/WidgetStyleCache.cpp:144
(Diff revision 1)
> +
> +  GtkWidget* widget = sGtkHeaderBarNewPtr();
> +  AddToWindowContainer(widget);
> +
> +  GtkStyleContext* style = gtk_widget_get_style_context(widget);
> +  gtk_style_context_add_class(style, MOZ_GTK_STYLE_CLASS_TITLEBAR);

I think we need that as it works as a selector for widgets in titlebar.

::: widget/gtk/WidgetStyleCache.cpp:428
(Diff revision 1)
> +  GtkWidget* window = GetWidget(MOZ_GTK_WINDOW);
> +  GtkStyleContext* windowStyle = gtk_widget_get_style_context(window);
> +  if (aFlags & MOZ_WINDOW_MAXIMIZED)
> +    gtk_style_context_add_class(windowStyle, "maximized");

I used the MOZ_GTK_HEADER_BAR_MAXIMIZED class which holds the maximized widget style.

Thus we don't need the extra CSD flags and widget restore.

::: widget/gtk/WidgetStyleCache.cpp:480
(Diff revision 1)
>    sCurrentStyleContext = nullptr;
>  #endif
> +
> +  GtkWidget* window = GetWidget(MOZ_GTK_WINDOW);
> +  GtkStyleContext* style = gtk_widget_get_style_context(window);
> +  gtk_style_context_remove_class(style, "maximized");

No needed with extra _MAXIMIZED widget type.

::: widget/gtk/gtk3drawing.cpp:2616
(Diff revision 1)
> +    style = ClaimStyleContext(MOZ_GTK_HEADER_BAR, GTK_TEXT_DIR_LTR,
> +                              state_flags, style_flags);
> +
> +    gtk_render_background(style, cr, rect->x, rect->y, rect->width,

Fixed.

::: widget/gtk/gtk3drawing.cpp:2874
(Diff revision 1)
> +            style = ClaimStyleContext(MOZ_GTK_HEADER_BAR);
> +            moz_gtk_add_style_border(style, left, top, right, bottom);

Fixed.

::: widget/gtk/gtkdrawing.h:75
(Diff revision 1)
>  } GtkTabFlags;
>  
> +// Flags used when drawing window decorations.
> +typedef enum {
> +  MOZ_GTK_CSD_MAXIMIZED = 1 << 1
> +} GtkCSDFlags;

I used a differen approach so we don't need that.

::: widget/gtk/nsNativeThemeGTK.cpp:717
(Diff revision 1)
> +  case NS_THEME_WINDOW_BUTTON_CLOSE:
> +  case NS_THEME_WINDOW_BUTTON_MINIMIZE:
> +  case NS_THEME_WINDOW_BUTTON_MAXIMIZE:
> +  case NS_THEME_WINDOW_BUTTON_RESTORE:
> +    aGtkWidgetType = MOZ_GTK_HEADER_BAR_BUTTON;

Added as extra clases

Comment 36

[Martin Stránský [:stransky]]

Comment on attachment 8768175 [details]
Bug 1283299 - Part 2: Add support for drawing decorated windows on GTK 3.20.

https://reviewboard.mozilla.org/r/62478/#review143174

::: widget/gtk/gtk3drawing.cpp:739
(Diff revision 1)
> +  GtkStyleContext* style = ClaimStyleContext(MOZ_GTK_WINDOW_DECORATION);
> +
> +  // Available on GTK 3.20+.
> +  static auto sGtkRenderBackgroundGetClip =
> +    (void (*)(GtkStyleContext*, gdouble, gdouble, gdouble, gdouble, GdkRectangle*))
> +    dlsym(RTLD_DEFAULT, "gtk_render_background_get_clip");

Fixed.

::: widget/gtk/gtk3drawing.cpp:2846
(Diff revision 1)
> +    case MOZ_GTK_WINDOW_DECORATION_SOLID:
> +        {
> +            style = ClaimStyleContext(MOZ_GTK_WINDOW_DECORATION_SOLID);
> +            moz_gtk_add_style_border(style, left, top, right, bottom);
> +            moz_gtk_add_style_padding(style, left, top, right, bottom);
> +            moz_gtk_add_style_margin(style, left, top, right, bottom);

Fixed.

::: widget/gtk/nsWindow.cpp:6867
(Diff revision 1)
> +}
> +
> +bool
> +nsWindow::UseSolidCSD() const
> +{
> +  return !HasARGBVisual() || !IsComposited();

Fixed.

Comment 37

[Hugo Osvaldo Barrera]

I hope this isn't the wrong place to mentions this, but please do keep in mind that CSDs don't make the same amount of sense everywhere, and this probably needs some detection/configurability (this is somewhat of an issue on current firefox too, but only when fullscreen, AFAIK).

Some clear example scenarios are wayland/sway, or xorg/i3. Neither of these support inconifying applications (nowadays usually called by it's windows term "minimize", not sure what you guys call it). So the minimize button would just be a little "noop" button on firefox (this is currently true for fullscreen firefox windows).

Another example is again, sway, where titlebars are not meant to be drawn. KDE has a "server-side decoration protocol" where the server tells applications that it'll be responsibly for drawing decorations (sway is currently implementing this too). In these cases, again, the "minimize", maximize and close buttons shouldn't be there, because it's not a desktop where applications draw this (nor is it mouse-driven).
(the same can be said of i3, but I'm not entirely sure how firefox is meant to detect that applications have no titlebar there).


I'm in no way saying that this ticket shouldn't be pushed forward; what I'm clearly saying obviously applies to a great minority of the users, but I'd just like to point out these corner cases.

FWIW, chromium by default draws its own borders/titlebars/buttons everywhere (even in situations where it shouldn't), but has a checkbox to use the system ones, and doesn't draw any itself. A checkbox is probably a good-enough solution.

Comment 38

[Hugo Osvaldo Barrera]

Sorry for the rather fast self-followup: I think I should have been a bit broader and clarified that the above applies to almost any tiling WM on both X11 and wayland. I just used i3 and sway as examples because they're the ones I've used.

Also, the maximize button is also generally a no-op on tiling WMs, because the concept of maximization really only belongs to floating WMs.

Comment 39

[Jan Niklas Hasse]

Nothing Firefox needs to do there, as GtkHeaderBar already reads from a global setting, if minimize or maximize buttons should be shown.

Comment 40

[Hugo Osvaldo Barrera]

GTK currently has no setting to configure these (there's actually a request for that https://bugzilla.gnome.org/show_bug.cgi?id=779416); so I'm pretty sure GtkHeaderBar can't use a global setting (because there is none).

On the current stable release of firefox:
* The three buttons are shown on firefox when fullscreen on X11/i3.
* The three buttons are shown on firefox (both fullscreen and normal) on wayland/sway.

In none of these scenarios does the minimize button do anything. While they might warrant separate bugs, the implementation taking place here can probably cover these scenarios from day-0. I also wanted to point this out to avoid showing the three buttons on non-fullscreen mode on X11.

Do keep in mind that CSD also includes borders (which, for example, chromium draws unless manually configured to use the system ones).

Comment 41

[Jan Niklas Hasse]

I don't mean a setting for whether to use CSD at all or not, but for which buttons to show. See http://roman-yagodin.github.io/how-to/2015/05/24/gtk-decoration-layout for example.
You can configure GtkHeaderBar to show only a close button, which is also GNOME's default.

Comment 42

[Nika Layzell [:mystor]]

What's the status of this bug? I don't see any currently requested reviews or pending needinfos. 

It also looks like you're taking this bug over, perhaps you should assign it to yourself :).

Comment 43

[Martin Stránský [:stransky]]

(In reply to Michael Layzell [:mystor] from comment #42)
> What's the status of this bug? I don't see any currently requested reviews
> or pending needinfos. 
> 
> It also looks like you're taking this bug over, perhaps you should assign it
> to yourself :).

Yem I'm working on that - see "Depends on" bugs for progress.

Comment 44

[Justin Dolske [:Dolske]]

This came up during a UX review of Photon... We'd certainly like to get the appearance of Firefox on Linux updated to remove the titlebar, but it sounds like there's enough work remaining here that's it's not going to happen in the timeframe for the Photon UI refresh in Firefox 57. But I think we would be interested in helping with the design and front-end UI once the platform support for this is in place (subject to the usual resource constrains and priorities, natch).

Thanks for pushing this forward in the meantime!

Comment 45

[Jan Niklas Hasse]

Why not move the release of the Photon UI refresh to >57 in order to get this included?

Comment 46

[Hugo Osvaldo Barrera]

Justin: Does that mean that firefox 57 for linux will be pushing a titlebar? Isn't it better to postpone, rather than release a know regression (especially given the time until 57's release).

Comment 47

[ganlu]

(In reply to Justin Dolske [:Dolske] from comment #44)
> This came up during a UX review of Photon... We'd certainly like to get the
> appearance of Firefox on Linux updated to remove the titlebar, but it sounds
> like there's enough work remaining here that's it's not going to happen in
> the timeframe for the Photon UI refresh in Firefox 57. But I think we would
> be interested in helping with the design and front-end UI once the platform
> support for this is in place (subject to the usual resource constrains and
> priorities, natch).
> 
> Thanks for pushing this forward in the meantime!

Patches has been here for 2 monthes without further reviews, sorry, but I have to ask if there are any other reasons for this decision?

Comment 48

[ganlu]

Sorry for my bad tone, I just really don't understand and feel bad, and can't stop wondering if I and all other linuxsers were the second citizen in the mozilla community.

Comment 49

[Jan Niklas Hasse]

> and can't stop wondering if I and all other linuxsers were the second citizen in the mozilla community.

That's definitely the case. Have a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1280523 for example.

Comment 50

[Marco Castelluccio [:marco]]

(In reply to ganlu from comment #47)
> (In reply to Justin Dolske [:Dolske] from comment #44)
> > This came up during a UX review of Photon... We'd certainly like to get the
> > appearance of Firefox on Linux updated to remove the titlebar, but it sounds
> > like there's enough work remaining here that's it's not going to happen in
> > the timeframe for the Photon UI refresh in Firefox 57. But I think we would
> > be interested in helping with the design and front-end UI once the platform
> > support for this is in place (subject to the usual resource constrains and
> > priorities, natch).
> > 
> > Thanks for pushing this forward in the meantime!
> 
> Patches has been here for 2 monthes without further reviews, sorry, but I
> have to ask if there are any other reasons for this decision?

The patches have been moved to other bugs and the work is progressing.

Remember that Bugzilla is not the place for discussion about priorities and things like these, comments like yours will likely be ignored in this setting.
If you want to discuss, you can use one of the mailing lists.

Comment 51

[Michal Stanke (Mozilla.cz) [:MikkCZ][:mstanke] (use needinfo)]

Hi :karlt. What is the current status of this bug and drawing tabs in the titlebar on Linux, please? Seems the patches in bug 1364843 and bug 1365218 are waiting for your review.

Comment 52

[Karl Tomlinson (:karlt)]

That's correct.  This bug will track the various pieces required in its dependencies.

Comment 53

[Martin Stránský [:stransky]]

For those who want to test it all those patches are updated to master and applied to my github repo at https://github.com/stransky/gecko-dev/tree/headerbar (https://github.com/stransky/gecko-dev at headerbar branch).

Comment 54

[Jan Andre Ikenmeyer [:darkspirit]]

Created attachment 8901232 [details]
2017-08-25 17-11-20 Gnome Default Theme.png

(In reply to Martin Stránský from comment #53)
Thank you! :)

It's hard to click on X because I get a "resize this corner" cursor.
The Default Theme and Light Theme don't have rounded corners.

Comment 55

[Jan Andre Ikenmeyer [:darkspirit]]

Created attachment 8901236 [details]
2017-08-25 17-12-12 Gnome Dark Theme.png

(In reply to Martin Stránský from comment #53)

Dark Theme has rounded top corners, but you can see the black tabbar background with angular corners behind them.
Window buttons are only (really) visible if you hover them.

Comment 56

[Jan Andre Ikenmeyer [:darkspirit]]

Created attachment 8901239 [details]
2017-08-25 17-13-12 Gnome Space Fantasy.png

(In reply to Martin Stránský from comment #53)

Space Fantasy has rounded corners at first glance, but you can see some very dark blue in the corners. This theme is broken as long as tabs are drawed in the titlebar.

Comment 57

[Jan Andre Ikenmeyer [:darkspirit]]

Created attachment 8901247 [details]
2017-08-25_17-15-18 KDE Space Fantasy.png

(In reply to Martin Stránský from comment #53)

Window decorations are not modified on KDE (Debian Testing), but one can rightclick and hide borders+titlebar with KDE itself (bottom window).
The prettiest version is currently the broken Spacy Fantasy (top window). The new window buttons work.

Comment 58

[Jan Andre Ikenmeyer [:darkspirit]]

As this is not linked in this bug yet:
Ubuntu Specs: http://design.firefox.com/people/shorlander/photon/Mockups/linux.html
(doubleclick on "Extra Drag Space" and then optionally select another theme)

Comment 59

[Charles Milette]

After much pain, I got the headerbar branch on the GitHub linked earlier to build, but now I have a few minor graphical issues:

- No shadows
- No rounded corners
- There seems to be some buttons overlaid on my GTK theme's buttons.
- No border outlines

See following screenshot: http://i.imgur.com/TIWqukI.png

Comment 60

[Martin Stránský [:stransky]]

(In reply to Charles Milette from comment #59)
> After much pain, I got the headerbar branch on the GitHub linked earlier to
> build, but now I have a few minor graphical issues:
> 
> - No shadows
> - No rounded corners
> - There seems to be some buttons overlaid on my GTK theme's buttons.
> - No border outlines
> 
> See following screenshot: http://i.imgur.com/TIWqukI.png

Yes, I'm working on that now.

Comment 61

[Martin Stránský [:stransky]]

The shadow rendering seems to be affected by Bug 1344839

Comment 62

[Martin Stránský [:stransky]]

An updated patch with CSD emulation on toolkit level is available for testing at Bug 1399611.

Comment 63

[Martin Stránský [:stransky]]

Comment on attachment 8768183 [details]
Bug 1283299 - Part 10: Disable tabs in titlebar on GTK versions before 3.20.

https://reviewboard.mozilla.org/r/62494/#review185442

::: browser/themes/linux/browser.css:1950
(Diff revision 1)
> +@media not all and (-moz-gtk-csd-available) {
> +  #main-window > #titlebar {
> +    /* We need to hide the titlebar explicitly on versions of GTK without CSD. */
> +    display: none;

This works as run-time disable for the #titlebar box. When omitted it produces bugs like this one:
https://bugzilla.redhat.com/show_bug.cgi?id=1492044

Comment 64

[Martin Stránský [:stransky]]

There's available new version [1] based on latest trunk and Bug 1399611. I also provide Fedora test builds based on recent Firefox 55 [2] and the repo gets fixes from it.

[1] https://github.com/stransky/gecko-dev/tree/titlebar-csd
[2] https://lists.fedoraproject.org/archives/list/[e-mail]/message/XXNOQ6GVGGNNTFGSEUIR3IXURMMRMOJK/

Comment 65

[Michel Alexandre Salim]

Any idea if/when we can get Firefox 57-based Fedora builds with CSD enabled (at least for Fedora 27)? Thanks Martin!

Comment 66

[rugk]

CSD?

Comment 67

[mirh]

(In reply to rugk from comment #66)
> CSD?

Client Dide Decorations, the thing in the title..

Comment 68

[Jan Niklas Hasse]

I've just tried the latest commit of the titlebar-csd branch: Works very good, great work :)

Only thing that's not quite right: When maximizing the window there's still a border at the top. This should really be 0 px because of fitt's law. Similar to how it looks like on Windows.

Comment 69

[Anass Ahmed]

Just tested the latest version of the new branch `titlebar-csd` and it works very well.

The same note of @jhasse in the previous comment, There's a spacing/padding above the tabs that shouldn't be there and also there's a leading space before tabs which appears when the browser in a non-maximized state. The "new tab" button is not on the same level as the "close window" button too.

My screenshot: https://i.imgur.com/D04cJoF.png

I compare the style with this image you released on the official mozilla hacks blog: https://2r4s9p1yi1fa2jd7j43zph8r-wpengine.netdna-ssl.com/files/2017/09/Screen-Shot-2017-09-19-at-13.12.53.png

Also, I noticed that double clicking the address bar (awesomebar) maximizes/restores the window, I don't know if this is intentional to have another way to maximize/restore the window if the titlebar is stacked with tabs and there's no place to click, but it conflicts with the double click to select the whole URL in the addressbar!

Comment 70

[Anass Ahmed]

For whoever wants to easily and quickly test the new changes, I've a Flatpak that you can install which is compiled from this branch `titlbar-csd`: https://www.dropbox.com/s/uso9f501d8bgxkx/org.mozilla.FirefoxNightlyTitlebar.flatpak?dl=0

Comment 71

[Jan Niklas Hasse]

Is it the same as the one hosted here? https://firefox-flatpak.mojefedora.cz/

Comment 72

[Martin Stránský [:stransky]]

(In reply to Anass Ahmed from comment #69)
> The same note of @jhasse in the previous comment, There's a spacing/padding
> above the tabs that shouldn't be there and also there's a leading space
> before tabs which appears when the browser in a non-maximized state. The
> "new tab" button is not on the same level as the "close window" button too.
> 
> My screenshot: https://i.imgur.com/D04cJoF.png
> 
> I compare the style with this image you released on the official mozilla
> hacks blog:
> https://2r4s9p1yi1fa2jd7j43zph8r-wpengine.netdna-ssl.com/files/2017/09/
> Screen-Shot-2017-09-19-at-13.12.53.png

The upper space for dragging was introduced at Bug 1349552 but that's for Windows/MacOS only. I'd need to implement it for Linux too. What you see now is hot fix I added for it.
 
> Also, I noticed that double clicking the address bar (awesomebar)
> maximizes/restores the window, I don't know if this is intentional to have
> another way to maximize/restore the window if the titlebar is stacked with
> tabs and there's no place to click, but it conflicts with the double click
> to select the whole URL in the addressbar!

Yes, the double clicking was added intentionally. We can reconsider that later when Bug 1349552 is fixed for linux.

Comment 73

[Anass Ahmed]

(In reply to Jan Niklas Hasse from comment #71)
> Is it the same as the one hosted here? https://firefox-flatpak.mojefedora.cz/

No, it's not. It's based on it though, but after modifying the source files to compile the titlebar-csd branch, you can see the source code here: https://github.com/anassahmed/firefox-flatpak/tree/titlebar

Comment 74

[Anass Ahmed]

(In reply to Martin Stránský from comment #72)
> The upper space for dragging was introduced at Bug 1349552 but that's for
> Windows/MacOS only. I'd need to implement it for Linux too. What you see now
> is hot fix I added for it.

Note that the "close" button itself is "draggable" (see how Nautilus (GNOME Files) reacts when you drag the close button).

Also, In GNOME 3, the top bar acts as a draggable titlebar in the space between the app title and the clock in the middle, and between the clock and the system icons on the right, so you can easily drag the app from maximized space there.

> Yes, the double clicking was added intentionally. We can reconsider that
> later when Bug 1349552 is fixed for linux.

Maybe you can add some space between the "close" (and other window management) button(s) and between the tabs stack like Chromium does right now. This space will be draggable, and double-clickable.

Comment 75

[Jan Niklas Hasse]

(In reply to Anass Ahmed from comment #73)
> (In reply to Jan Niklas Hasse from comment #71)
> > Is it the same as the one hosted here? https://firefox-flatpak.mojefedora.cz/
> 
> No, it's not. It's based on it though, but after modifying the source files
> to compile the titlebar-csd branch, you can see the source code here:
> https://github.com/anassahmed/firefox-flatpak/tree/titlebar

I meant the version "org.mozilla.FirefoxNightlyCSD - Firefox Nighly with Client Side Decorations patches applied, hosted there: (currently hosted there: https://github.com/stransky/gecko-dev/tree/titlebar" on that page.

Comment 76

[Anass Ahmed]

(In reply to Jan Niklas Hasse from comment #75)
> I meant the version "org.mozilla.FirefoxNightlyCSD - Firefox Nighly with
> Client Side Decorations patches applied, hosted there: (currently hosted
> there: https://github.com/stransky/gecko-dev/tree/titlebar" on that page.

Ah, that's a new one. I've not noticed it before.
It's basically the same of what my code does.

BTW, the link leads to the old branch, but this is the directory of the flatpak configuration files:
https://github.com/xhorak/firefox-devedition-flatpak/tree/master/org.mozilla.FirefoxNightlyCSD

I'm still waiting for a merged branch between Wayland and CSD.

Comment 77

[Martin Stránský [:stransky]]

Guys, the https://github.com/stransky/gecko-dev/tree/titlebar branch is obsoleted and already deleted. The only valid one is https://github.com/stransky/gecko-dev/tree/titlebar-csd, see comment 64.

Comment 78

[Marios Titas]

@Martin Does your work apply to wayland only? Or will it also address the issue for X11 users as well?

Comment 79

[Jan Niklas Hasse]

Works on X11 too.

Comment 80

[Jan Niklas Hasse]

> The upper space for dragging was introduced at Bug 1349552 but that's for Windows/MacOS only. I'd need to implement it for Linux too. What you see now is hot fix I added for it.

I'm only talking about the maximized state though and as far as I can tell there's still no border at the top on Windows (to make tabs easily selectable; fitts' law). This is crucial on Linux as well for environments without a top bar and is also what Chrome does.

Comment 83

[Anass Ahmed]

I'm still confused by the "drag space" checkbox in the "customize" page. It doesn't do anything here in Firefox 57 on Fedora 27.

I tried also using Compact, Normal, and Touch density but I couldn't get rid of the thin space above tabs.

From the quantum[1] page, there's a video[2] that demos screenshots without this thin space atop the tabs and the space before the first tab .. Why is this not implemented? Do you plan to implement it that way?

[1] https://www.mozilla.org/en-US/firefox/quantum/
[2] https://assets.mozilla.net/video/quantum/screenshots.webm

Comment 84

[Pat Fedida]

Question: will also allow the address bar to replace the title bar, as opposed to the tab bar which is shown in the demos?

This is in particular significant for users using "Tree Style Tabs" while hiding the title bar, for example seeking to save vertical screen real estate. This may also be helpful for users trying to hide the title bar by making the large address inputs smaller, rather than the sometimes crowded tab bar.

Thanks!

Comment 85

[Martin Stránský [:stransky]]

(In reply to Anass Ahmed from comment #83)
> I'm still confused by the "drag space" checkbox in the "customize" page. It
> doesn't do anything here in Firefox 57 on Fedora 27.
> 
> I tried also using Compact, Normal, and Touch density but I couldn't get rid
> of the thin space above tabs.
> 
> From the quantum[1] page, there's a video[2] that demos screenshots without
> this thin space atop the tabs and the space before the first tab .. Why is
> this not implemented? Do you plan to implement it that way?
> 
> [1] https://www.mozilla.org/en-US/firefox/quantum/
> [2] https://assets.mozilla.net/video/quantum/screenshots.webm

You're probably talking about:

https://bugzilla.mozilla.org/show_bug.cgi?id=1387415
https://bugzilla.mozilla.org/show_bug.cgi?id=1349552

which are not implemented for Linux yet.

Comment 86

[Martin Stránský [:stransky]]

(In reply to Pat Fedida from comment #84)
> Question: will also allow the address bar to replace the title bar, as
> opposed to the tab bar which is shown in the demos?
> 
> This is in particular significant for users using "Tree Style Tabs" while
> hiding the title bar, for example seeking to save vertical screen real
> estate. This may also be helpful for users trying to hide the title bar by
> making the large address inputs smaller, rather than the sometimes crowded
> tab bar.

Please file a bug for "browser" component - this bug is about general functionality which can be extended further by browser themes.

Comment 87

[Jan Niklas Hasse]

(In reply to Martin Stránský [:stransky] from comment #85)
> You're probably talking about:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1387415
> https://bugzilla.mozilla.org/show_bug.cgi?id=1349552
> 
> which are not implemented for Linux yet.

No, he means the space above tabs (not the flexible spaces). This space should be removed when maximized (so that tabs can be selected on the top edge of the screen on desktop environments without a top bar, just as on Windows; see Fitts' Law).

Comment 88

[Vlad Zagorodniy]

Created attachment 8928580 [details]
"Old" window buttons in fullscreen mode

OS: Fedora 27
Firefox version: 57

In fullscreen mode Firefox uses "old" style window buttons(minimize, maximize, close). It is expected that window buttons in fullscreen mode are from gtk theme.

Comment 89

[Vlad Zagorodniy]

Created attachment 8928592 [details]
Default themes are not fully applied

OS: Fedora 27
Firefox version: 57

Default themes(light and dark) do not "fill" whole titlebar.

Comment 90

[Vlad Zagorodniy]

Also, what does "drag space" checkbox do?

Comment 91

[Richard Marti (:Paenglab)]

(In reply to vladzzag from comment #90)
> Also, what does "drag space" checkbox do?

See comment 85

Comment 92

[rugk]

Wait… I use Fedora 27 too, but my titlebar is still shown in final Firefox release. (no matter what option for "titlebar" or "drag space" I choose) b Same in FirefoxDevEdition (58). Or is this a custom build?

Wayland here

Comment 93

[Марко М. Костић (Marko M. Kostić) [:Kostic]]

@rugk, you need to enable the experimental flag which allows the drawing of the tabs into the titlebar. Open the about:config page, search for "widget.allow-client-side-decoration" and set it to true. Restart your Firefox and the tabs will be drawn in the titlebar.

Comment 94

[rugk]

Ah thanks FYI. That works. Just note that in the flatpak (FirefoxDevEdition, https://firefox-flatpak.mojefedora.cz/) neither this config flag is available nor does it work when I manually add it. I assume that's due to flatpaks, but it would anyway be nice if it would work there, too.

Comment 95

[rugk]

Just to share my experience: It's great! Besides the already mentioned issues there is another one: You cannot double click on the tab bar (anymore, as with the title bar) to maximize the window, and as there is also no maximize button in my GNOME configuration (default in GNOME, can be configured in gnome-tweak-tool), so I have literally no way to maximize the Firefox. (except of dragging it to the top)

Comment 96

[Jake Dane]

@rugk, you can use keyboard shortcuts on GNOME. Super+Up to maximize current window and Super+Down to unmaximize (Super is usually the key with the Windows logo on it).

Comment 97

[N. W.]

"widget.allow-client-side-decoration" is not available via about:config here.

Running KDE neon here with both Firefox 57 from the Ubuntu 16.04 repo as well as Nightly 59.0a1 (2017-11-15) (64-bit) from the firefox-trunk PPA: https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/ppa

"browser.tabs.drawInTitlebar" is available and I've set it to "true" but it doesn't do anything.

"widget.allow-client-side-decoration" is _not_ available via about:config.

Is this to be expected?

Regards

Comment 98

[Jan Niklas Hasse]

Yes, it's only available in Fedora's version of Firefox 57 for now.

Comment 99

[Daniel]

(In reply to Jan Niklas Hasse from comment #98)
> Yes, it's only available in Fedora's version of Firefox 57 for now.

What about other distros? Should we wait for FF58?

Comment 100

[Martin Stránský [:stransky]]

(In reply to Daniel from comment #99)
> (In reply to Jan Niklas Hasse from comment #98)
> > Yes, it's only available in Fedora's version of Firefox 57 for now.
> 
> What about other distros? Should we wait for FF58?

FF58 is missing patches from Bug 1415481 but it can be uplifted.

Comment 101

[N. W.]

(In reply to Martin Stránský [:stransky] from comment #100)
> (In reply to Daniel from comment #99)
> > (In reply to Jan Niklas Hasse from comment #98)
> > > Yes, it's only available in Fedora's version of Firefox 57 for now.
> > 
> > What about other distros? Should we wait for FF58?
> 
> FF58 is missing patches from Bug 1415481 but it can be uplifted.

As already mentioned, I am also running Nightly 59.0a1 (2017-11-15) (64-bit) from the firefox-trunk PPA: https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/ppa

It is firefox-trunk - 59.0~a1~hg20171115r391869-0ubuntu0.16.04.1~umd1.

It was built 22 hours ago.

It is missing "widget.allow-client-side-decoration".

But as far as I can tell from your comment on https://bugzilla.mozilla.org/show_bug.cgi?id=1415481#c27 , you pushed the patches to Nightly 21 hours ago.

So, should it now work with the firefox-trunk PPA mentioned above?

Or is it still not enabled for the firefox-trunk PPA builds?

Or do we need to wait for the firefox-trunk PPA to be updated from Nightly 59.0a1 (2017-11-15) to Nightly 59.0a1 (2017-11-16)?

Comment 102

[N. W.]

PS: As currently being discussed on #ubuntu-mozillateam on freenode, it will probably not work on Ubuntu 16.04 Xenial Xerus anyway, since 16.04 based distros unfortunately only have GTK+ 3.18.

So, Ubuntu 16.04 users would probably have to wait for https://bugzilla.mozilla.org/show_bug.cgi?id=1414212 .

Comment 103

[N. W.]

The new Nightly build in https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/ppa now has the "widget.allow-client-side-decoration" pref available in about:config.

It's set to "false" by default. At least on Ubuntu 16.04.

Setting it to "true" does not seem to do anything on 16.04. Probably because 16.04 uses GTK+ 3.18: https://packages.ubuntu.com/search?suite=all&section=all&arch=any&keywords=libgtk-3&searchon=names

Since Ubuntu 16.10 and onward ship with GTK+ 3.22, I would guess it should now work on 16.10 and onwards, haven't tested that though.

However, 16.04 users would probably have to wait on https://bugzilla.mozilla.org/show_bug.cgi?id=1414212 .

So, let's hope https://bugzilla.mozilla.org/show_bug.cgi?id=1414212 will be implemented soon.

Comment 104

[N. W.]

Correction: The devs on #ubuntu-mozillateam on freenode just said that it should work on Ubuntu 17.10 and upwards, whereas older Ubuntu releases would need to wait on https://bugzilla.mozilla.org/show_bug.cgi?id=1414212 .

Comment 105

[helios.solaris]

Debian Testing is using libgtk 3.22 and setting widget.allow-client-side-decoration to true does nothing.

Comment 106

[N. W.]

(In reply to helios.solaris from comment #105)
> Debian Testing is using libgtk 3.22 and setting
> widget.allow-client-side-decoration to true does nothing.

Maybe you forgot to set "browser.tabs.drawInTitlebar" to "true" ?

Comment 107

[Anass Ahmed]

(In reply to Martin Stránský [:stransky] from comment #85)
> 
> You're probably talking about:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1387415
> https://bugzilla.mozilla.org/show_bug.cgi?id=1349552
> 
> which are not implemented for Linux yet.

I'm not talking about the opposite, you've implemented a "drag space" above the tabs, and I want to remove it. the checkbox "drag space" in the "customize" page doesn't do anything!

Comment 108

[rugk]

(In reply to Jake Dane from comment #96)
> @rugk, you can use keyboard shortcuts on GNOME. Super+Up to maximize current
> window and Super+Down to unmaximize (Super is usually the key with the
> Windows logo on it).

Yeah, I know, but that is **** a touch device. ;)
Just wanna let you know about that…

Comment 109

[helios.solaris]

(In reply to N. W. from comment #106)
> (In reply to helios.solaris from comment #105)
> > Debian Testing is using libgtk 3.22 and setting
> > widget.allow-client-side-decoration to true does nothing.
> 
> Maybe you forgot to set "browser.tabs.drawInTitlebar" to "true" ?

Nope. widget.allow-client-side-decoration and browser.tabs.drawInTitlebar are both set to true.

Comment 110

[moriel5]

(In reply to helios.solaris from comment #109)
> (In reply to N. W. from comment #106)
> > (In reply to helios.solaris from comment #105)
> > > Debian Testing is using libgtk 3.22 and setting
> > > widget.allow-client-side-decoration to true does nothing.
> > 
> > Maybe you forgot to set "browser.tabs.drawInTitlebar" to "true" ?
> 
> Nope. widget.allow-client-side-decoration and browser.tabs.drawInTitlebar
> are both set to true.

I can confirm this issue on my end, wit an additional twist.
With the same identical settings (after reset), this works fine (aside from the window controls being slightly discolored) under root, however it fails to work under the standard user.

Comment 111

[moriel5]

(In reply to moriel5 from comment #110)

> I can confirm this issue on my end, wit an additional twist.
> With the same identical settings (after reset), this works fine (aside from
> the window controls being slightly discolored) under root, however it fails
> to work under the standard user.

Sorry, I had forgotten to post my system details:
Firefox 59a1 (2017-11-16)(64-bit), Solus Unstable
I had manually pushed/installed Firefox to /usr/lib64/ and icons to /usr/share/icons/.

Comment 112

[aleksandar]

Experiencing a weird "double-icons" issue, most notably seen in the middle photo of the already uploaded attachment in this thread: https://bug1283299.bmoattachments.org/attachment.cgi?id=8901247

Loosely explained, it seems to work as expected, however, the titlebar doesn't get removed. As a result, I have two sets of windows controls.

You can see the result here: https://i.imgur.com/JpLMpS0.png

Running Ubuntu 17.10 and Nightly from the PPA (so the version 59.0a1 (2017-11-16)). I can confirm that I'm experiencing the same issue with both Wayland and X11 sessions. Both "widget.allow-client-side-decorations" and "browser.tabs.drawInTitlebar" have been set to true.

In the unmaximized fields, I see a top line between the old titlebar and the new titlebar+tabs (https://i.imgur.com/46kRc7A.png). When the window is maximized, the line disappears.

In the Customize window, both "Title Bar" and "Drag Space" are unchecked. Using the default theme (slightly modified using userChrome.css, but I've recreated this using a temporary profile as well) and density set to Touch.

Not sure what else I could do to help you out with replicating this issue, but I'll be glad to provide further info if necessary.

Comment 113

[Marco Castelluccio [:marco]]

(In reply to aleksandar from comment #112)
> Experiencing a weird "double-icons" issue, most notably seen in the middle
> photo of the already uploaded attachment in this thread:
> https://bug1283299.bmoattachments.org/attachment.cgi?id=8901247

I think what you are describing is bug 1417933. CC yourself there so you know when
it is fixed or if somebody wants to ask you more information.

Comment 114

[rugk]

Another issue: With the black Firefox theme (default GNOME theme), you cannot even see the close/minimize buttons without hovering over them. They are just black…

Also Bug 1418611 could possibly be related to this, not sure however.

Comment 115

[Anass Ahmed]

So, after installing Adapta theme, and applying the default theme in Firefox (in compact density), the space atop tabs disappeared and the close button was aligned nicely with the rest of the headerbar as you can see here: https://i.imgur.com/J3p90I0.png

If I returned back to the dark or light themes, I get those drag spaces atop the tabs back!

Finally, I can watch YouTube in a big size player again. :D

Also, I think the remaining task of this bug is to smooth the animation responsible for adding the drag space when resizing the window to its original size.

Comment 116

[Charles Milette]

There's one last nitpick for me on the current Nightly build:
- Currently there is custom buttons overlaying the GTK system buttons. The plan seems to be removing GTK buttons and using only custom buttons. I don't like that, it makes visual and sometimes usage inconsistency with my other programs.

Comment 117

[paulcalvin]

OS=Ubuntu 17.10
Firefox Nightly

Applying persona removes close maximize,minimize,buttons

Comment 118

[Anass Ahmed]

(In reply to Charles Milette from comment #116)
> There's one last nitpick for me on the current Nightly build:
> - Currently there is custom buttons overlaying the GTK system buttons. The
> plan seems to be removing GTK buttons and using only custom buttons. I don't
> like that, it makes visual and sometimes usage inconsistency with my other
> programs.

It's really hard to combine both custom Pesonas and default GTK themes. I'm using Adapta theme and it had a Firefox theme (but it used the deprecated API which is now unusable with v57.0), I'm all for removing the Personas and leaving the original integration with GTK themes, but I don't think this a decision that can be flipped today.

Comment 119

[peter]

(In reply to paulcalvin from comment #117)
> OS=Ubuntu 17.10
> Firefox Nightly
> 
> Applying persona removes close maximize,minimize,buttons

OS=Arch Linux; fully up-to-date as of this writing
DE=Gnome 3.26

I have the same issue. I'm experiencing no unused space on the left, but empty space on the right that is appropriate for the missing buttons. When I turn on the menu bar permanently (i.e., tab bar context->check menu bar as opposed to tapping alt), the tab bar expands to fill the width, with space in the menu bar sufficient for drawing the buttons.

Exposing the menu bar by pressing alt does not evoke this change.

I have widget.allow-client-side-decoration, gfx.webrender, and browser.tabs.drawInTitlebar all enabled.

Comment 120

[priyanshujindal1995]

Working on Manjaro Linux
GTK Version: 3.22.26-1
Firefox nightly: 59.0a1.20171121220115-1 (from aur: firefox-always-nightly)
With Gnome version: 3.26.2-1
And theme adapta-gtk-theme: 3.92.1.90-2 (Adapta-Eta)

With widget.allow-client-side-decoration set to true and browser.tabs.drawInTitlebar set to true

Windows control in title bar working
Drag of title bar is working
Menu Bar with alt key is working

No major issue, just the styling of the minimize, maximize and close button is a little weird.
This is my screenshot: https://imgur.com/a/DP8m0

Comment 122

[rugk]

Anyone using MinVid with this option?

I am currently seeing issues there: https://github.com/meandavejustice/min-vid/issues/1137

Comment 123

[gagemorgan]

Currently testing Nightly on Antergos (latest release). The problem is that Firefox used to display GREAT without the titlebar, but now it's gone back to showing it, regardless of the Customizer and about:config. The issue can be seen here:

https://i.imgur.com/eLJdOA1.png

The X draws itself twice. I don't know if this is a known bug, but for a few days I was really happy with the CSD, now it's gone. Granted this is Nightly and I shouldn't be too miffed because of that, but just wanted to pop in and see if this might warrant a new bug, or if it's known and is being worked on.

Comment 124

[Michael Monreal [:micmon]]

It's not just you @gagemorgan,I am seeing the same on Fedora 27 for a few days.

Comment 125

[Robin]

(In reply to gagemorgan from comment #123)
> Currently testing Nightly on Antergos (latest release). The problem is that
> Firefox used to display GREAT without the titlebar, but now it's gone back
> to showing it, regardless of the Customizer and about:config. The issue can
> be seen here:
> 
> https://i.imgur.com/eLJdOA1.png
> 
> The X draws itself twice. I don't know if this is a known bug, but for a few
> days I was really happy with the CSD, now it's gone. Granted this is Nightly
> and I shouldn't be too miffed because of that, but just wanted to pop in and
> see if this might warrant a new bug, or if it's known and is being worked on.

You are probably referring to this bug 1423869. For now, try to set MOZ_GTK_TITLEBAR_DECORATION=client before launching firefox and it should work fine.

Comment 126

[gagemorgan]

(In reply to Robin from comment #125)
> (In reply to gagemorgan from comment #123)
> > Currently testing Nightly on Antergos (latest release). The problem is that
> > Firefox used to display GREAT without the titlebar, but now it's gone back
> > to showing it, regardless of the Customizer and about:config. The issue can
> > be seen here:
> > 
> > https://i.imgur.com/eLJdOA1.png
> > 
> > The X draws itself twice. I don't know if this is a known bug, but for a few
> > days I was really happy with the CSD, now it's gone. Granted this is Nightly
> > and I shouldn't be too miffed because of that, but just wanted to pop in and
> > see if this might warrant a new bug, or if it's known and is being worked on.
> 
> You are probably referring to this bug 1423869. For now, try to set
> MOZ_GTK_TITLEBAR_DECORATION=client before launching firefox and it should
> work fine.

It still doesn't work. That's a shell variable, right? I tried both "export MOZ_GTK_TITLEBAR_DECORATION=client" and "./firefox MOZ_GTK_TITLEBAR_DECORATION=client" but neither do anything. It also doesn't make the "Customize" option work either.

Comment 127

[gagemorgan]

(In reply to gagemorgan from comment #126)
> (In reply to Robin from comment #125)
> > (In reply to gagemorgan from comment #123)
> > > Currently testing Nightly on Antergos (latest release). The problem is that
> > > Firefox used to display GREAT without the titlebar, but now it's gone back
> > > to showing it, regardless of the Customizer and about:config. The issue can
> > > be seen here:
> > > 
> > > https://i.imgur.com/eLJdOA1.png
> > > 
> > > The X draws itself twice. I don't know if this is a known bug, but for a few
> > > days I was really happy with the CSD, now it's gone. Granted this is Nightly
> > > and I shouldn't be too miffed because of that, but just wanted to pop in and
> > > see if this might warrant a new bug, or if it's known and is being worked on.
> > 
> > You are probably referring to this bug 1423869. For now, try to set
> > MOZ_GTK_TITLEBAR_DECORATION=client before launching firefox and it should
> > work fine.
> 
> It still doesn't work. That's a shell variable, right? I tried both "export
> MOZ_GTK_TITLEBAR_DECORATION=client" and "./firefox
> MOZ_GTK_TITLEBAR_DECORATION=client" but neither do anything. It also doesn't
> make the "Customize" option work either.

EDIT: Running in BASH, "MOZ_GTK_TITLEBAR_DECORATION=client ./firefox" doesn't work either.

Comment 128

[Shubham Arora]

(In reply to gagemorgan from comment #127)
> (In reply to gagemorgan from comment #126)
> > (In reply to Robin from comment #125)
> > > (In reply to gagemorgan from comment #123)
> > > > Currently testing Nightly on Antergos (latest release). The problem is that
> > > > Firefox used to display GREAT without the titlebar, but now it's gone back
> > > > to showing it, regardless of the Customizer and about:config. The issue can
> > > > be seen here:
> > > > 
> > > > https://i.imgur.com/eLJdOA1.png
> > > > 
> > > > The X draws itself twice. I don't know if this is a known bug, but for a few
> > > > days I was really happy with the CSD, now it's gone. Granted this is Nightly
> > > > and I shouldn't be too miffed because of that, but just wanted to pop in and
> > > > see if this might warrant a new bug, or if it's known and is being worked on.
> > > 
> > > You are probably referring to this bug 1423869. For now, try to set
> > > MOZ_GTK_TITLEBAR_DECORATION=client before launching firefox and it should
> > > work fine.
> > 
> > It still doesn't work. That's a shell variable, right? I tried both "export
> > MOZ_GTK_TITLEBAR_DECORATION=client" and "./firefox
> > MOZ_GTK_TITLEBAR_DECORATION=client" but neither do anything. It also doesn't
> > make the "Customize" option work either.
> 
> EDIT: Running in BASH, "MOZ_GTK_TITLEBAR_DECORATION=client ./firefox"
> doesn't work either.

Try:

env MOZ_GTK_TITLEBAR_DECORATION=client ./firefox 

Working fine on elementary.

Comment 129

[rnd]

^ This works for me. THANKS!
(Ubuntu 17.10, gnome-shell 3.26.2, wayland)

Comment 130

[Martin Stránský [:stransky]]

Please note that due to regression Bug 1423810 the titlebar rendering was temporary disabled on systems without gnome-shell/Cinnamon. It's tracked as Bug 1424974.