This is a merely placeholder for Bug 1283299 to have another patch available here as reviewboard does not allow to have more review request opened simultaneously and I don't want to overwrite the original patches at Bug 1283299.

Git repo with the patch is available here: https://github.com/stransky/gecko-dev/tree/titlebar-csd

Karl, I'm going to comment that patch heavily to speed up the feedback process. If you agree with this approach I'll file separated patches. Thanks.

Comment on attachment 8907768 [details] Bug 1399611 - Emulate CSD on toolkit level, https://reviewboard.mozilla.org/r/179444/#review184614 ::: browser/themes/linux/browser.css:796 (Diff revision 1) > .restore-tabs-button:hover:active:not([disabled="true"]) { > padding: 3px; > } > + > +/* Titlebar/CSD */ > +@media (-moz-gtk-csd-available) { This is a simplified version of original Andrew's patch. Just set the -moz-window-titlebar appearance which is translated to GtkHeaderBar on toolkit level.

Comment on attachment 8907768 [details] Bug 1399611 - Emulate CSD on toolkit level, https://reviewboard.mozilla.org/r/179444/#review184616 ::: widget/gtk/WidgetStyleCache.cpp:33 (Diff revision 1) > static GtkWidget* > -CreateWindowWidget() > +CreateWindowWidget(WidgetNodeType type) > { > GtkWidget *widget = gtk_window_new(GTK_WINDOW_POPUP); > gtk_widget_set_name(widget, "MozillaGtkWidget"); > + if (type == MOZ_GTK_WINDOW_CSD) { I don't support solid-csd - just the transparent/alpha ones. ::: widget/gtk/WidgetStyleCache.cpp:546 (Diff revision 1) > + static auto sGtkHeaderBarNewPtr = (GtkWidget* (*)()) > + dlsym(RTLD_DEFAULT, "gtk_header_bar_new"); > + static const char* MOZ_GTK_STYLE_CLASS_TITLEBAR = "titlebar"; > + > + GtkWidget* headerbar = sGtkHeaderBarNewPtr(); > + if (aMaximized) { To allow stateless style creation I added new root MOZ_GTK_WINDOW_MAXIMIZED widget container which is just a maximized version of MOZ_GTK_WINDOW container and holds widgets added to maximized GtkWindow. ::: widget/gtk/WidgetStyleCache.cpp:1294 (Diff revision 1) > mozilla::PodArrayZero(sStyleStorage); > > /* This will destroy all of our widgets */ > if (sWidgetStorage[MOZ_GTK_WINDOW]) > gtk_widget_destroy(sWidgetStorage[MOZ_GTK_WINDOW]); > + if (sWidgetStorage[MOZ_GTK_WINDOW_MAXIMIZED]) Deletes the new root container. ::: widget/gtk/gtk3drawing.cpp:254 (Diff revision 1) > gtk_style_context_get_style(style, "handle_size", size, NULL); > return MOZ_GTK_SUCCESS; > } > > +void > +moz_gtk_get_window_border(gint* top, gint* right, gint* bottom, gint* left) I didn't change this code - it's the original one. ::: widget/gtk/gtk3drawing.cpp:2760 (Diff revision 1) > > return metrics; > } > > +void > +moz_gtk_window_decoration_paint(cairo_t *cr, GdkRectangle* rect) This function draws decoration (shadows) to mShell. I used cairo to clear background under the decoration only and draw over it. moz_gtk_get_window_border() can be replaced by cached values from nsWindow. ::: widget/gtk/nsLookAndFeel.cpp:1066 (Diff revision 1) > &mFieldFontName, &mFieldFontStyle); > > gtk_widget_destroy(window); > g_object_unref(labelWidget); > + > + // Require GTK 3.20 for client-side decoration support. I added widget.allow-client-side-decoration which is off by default to enable testing for power users only when the patch is checked in. ::: widget/gtk/nsWindow.h:559 (Diff revision 1) > nsSizeMode mLastSizeMode; > > + // If true, draw our own window decorations (where supported). > + bool mDrawWindowDecoration; > + GtkBorder mDecorationSize; > + bool mDecorationSizeChanged; Please ignore it - it's just a lefover. ::: widget/gtk/nsWindow.cpp:1622 (Diff revision 1) > > if (!mContainer) > return; > > gdk_window_set_cursor(gtk_widget_get_window(GTK_WIDGET(mContainer)), newCursor); > + if (IsClientDecorated()) { mShell size extends mContainer size (holds the shadows) so we need to also set cursor for it. ::: widget/gtk/nsWindow.cpp:2613 (Diff revision 1) > } > #endif /* MOZ_X11 */ > + // Client is decorated and we're getting the motion event for mShell > + // window which draws the CSD decorations around mContainer. > + if (IsClientDecorated()) { > + if (aEvent->window == gtk_widget_get_window(mShell)) { When CSD enabled resize when cursor is on mShell (shadows) only because mContainer does not have any resizers and the cursor colides with content. ::: widget/gtk/nsWindow.cpp:2649 (Diff revision 1) > + } > + SetCursor(cursor); > + return; > + } > + } > + // We're not on resize handle - check if we need to reset cursor back. We need to also reset cursor when leaves mShell (shadows) only area. ::: widget/gtk/nsWindow.cpp:3731 (Diff revision 1) > GTK_WINDOW_TOPLEVEL : GTK_WINDOW_POPUP; > mShell = gtk_window_new(type); > > - bool useAlphaVisual = (mWindowType == eWindowType_popup && > - aInitData->mSupportTranslucency); > + bool useAlphaVisual = false; > +#if (MOZ_WIDGET_GTK == 3) > + // When CSD is available we can emulate it for toplevel windows. We create CSD ready window setup (mShell/mContainer roles) when CSD is generally available. But actually enable it if we're running on compositor only and we can draw the shadows. ::: widget/gtk/nsWindow.cpp:3863 (Diff revision 1) > GtkWidget *container = moz_container_new(); > mContainer = MOZ_CONTAINER(container); > > #if (MOZ_WIDGET_GTK == 3) > - // "csd" style is set when widget is realized so we need to call > + /* There are tree possible situations here: > + * I reworked this part as we need to solve more situations here. I hope the comment here explains that well. ::: widget/gtk/nsWindow.cpp:3994 (Diff revision 1) > break; > } > > // label the drawing window with this object so we can find our way home > g_object_set_data(G_OBJECT(mGdkWindow), "nsWindow", this); > + if (mIsCSDEnabled) { This is needed for mouse events (resize, button press) to work correctly because they use mGdkWindow (by mContainer) by default but we need to work with mShell GdkWindow. ::: widget/gtk/nsWindow.cpp:5670 (Diff revision 1) > }, > widget); > > return FALSE; > } > + This is a tricky part - rendering CSD in main thread. Can we do that? ::: widget/gtk/nsWindow.cpp:6747 (Diff revision 1) > window->ClearCachedResources(); > } > } > } > > +NS_IMETHODIMP I'm aware of your comments for nsWindow::SetNonClientMargins() an I'm going to address that. ::: widget/gtk/nsWindow.cpp:7040 (Diff revision 1) > return NS_OK; > } > #endif > > +bool > +nsWindow::IsClientDecorated() const Draw the decorations for normal size windows only. ::: widget/gtk/nsWindow.cpp:7087 (Diff revision 1) > + // mShell/mContainer is not allocated/resized yet and has default 1x1 size. > + // Just resize to some minimal value which will be changed > + // by Gecko soon. > + GtkAllocation allocation; > + gtk_widget_get_allocation(GTK_WIDGET(mContainer), &allocation); > + if (allocation.width < left + right || allocation.height < top + bottom) { This may look uglty but only reason for that is to make Gtk+ happy and doesn't throw warning messages. I use mContainer margin to make space between mShell and mContainer to make the decorations (shadows) actually visible. We're going to resize the mContainer/mShell later anyway but when we set the margin here early it does not produce rendering artifacts when main window is shown. ::: widget/gtk/nsWindow.cpp:7104 (Diff revision 1) > + gtk_widget_set_margin_top(GTK_WIDGET(mContainer), mDecorationSize.top); > + gtk_widget_set_margin_bottom(GTK_WIDGET(mContainer), mDecorationSize.bottom); > +} > + > +bool > +nsWindow::CheckResizerEdge(LayoutDeviceIntPoint aPoint, GdkWindowEdge& aOutEdge) This is the original code and needs to be updated for the 'L' corner resizers.

(In reply to Martin Stránský from comment #1) > Git repo with the patch is available here: > https://github.com/stransky/gecko-dev/tree/titlebar-csd Note: You need to set "widget.allow-client-side-decoration" pref value to true to enable the CSD rendering.

Comment on attachment 8907768 [details] Bug 1399611 - Emulate CSD on toolkit level, https://reviewboard.mozilla.org/r/179444/#review184614 > This is a simplified version of original Andrew's patch. Just set the -moz-window-titlebar appearance which is translated to GtkHeaderBar on toolkit level. Removal of #main-window[tabsintitlebar][sizemode="normal"] { -moz-appearance: -moz-gtk-window-decoration; } and moving drawing to the same part of code that is handling events is appealing.

Comment on attachment 8907768 [details] Bug 1399611 - Emulate CSD on toolkit level, https://reviewboard.mozilla.org/r/179444/#review184616 > I don't support solid-csd - just the transparent/alpha ones. Yes, I agree dropping solid-csd support is sensible. > To allow stateless style creation I added new root MOZ_GTK_WINDOW_MAXIMIZED widget container which is just a maximized version of MOZ_GTK_WINDOW container and holds widgets added to maximized GtkWindow. A separate maximized window and descendant header bar should work fine if only on the header bar depends on the difference in the windows. > This function draws decoration (shadows) to mShell. I used cairo to clear background under the decoration only and draw over it. > > moz_gtk_get_window_border() can be replaced by cached values from nsWindow. I'm surprised that the background needs to be cleared here. Clearing the background differs from Gecko's model for translucent windows where the background is initially cleared and drawing only draws the parts that are not clear, with operator_over. It seems to differ from GTK's model also as I don't see gtk_window_draw clearing the background. gdk_window_begin_draw_frame documentation says "if you’re drawing the contents of the widget yourself, you can assume that the widget has a cleared background". I suspect that this will not be necessary without the gtk_widget_set_double_buffered(, false) call on this widget. I haven't looked at the details of borders. I doubt there's a need to cache values, but if there already exist cached values, then perhaps it makes sense to use them. Getting from here to an nsWindow would be awkward though. > I added widget.allow-client-side-decoration which is off by default to enable testing for power users only when the patch is checked in. Sounds good. > This is a tricky part - rendering CSD in main thread. Can we do that? Yes, drawing with GTK on the main thread is fine, as Gecko's drawing is not involved, but gtk_widget_set_double_buffered(, false) should not be called on this widget as it can lead to flickering. > Draw the decorations for normal size windows only. OK > This may look uglty but only reason for that is to make Gtk+ happy and doesn't throw warning messages. > > I use mContainer margin to make space between mShell and mContainer to make the decorations (shadows) actually visible. > > We're going to resize the mContainer/mShell later anyway but when we set the margin here early it does not produce rendering artifacts when main window is shown. I haven't looked closely at this.

Comment on attachment 8907768 [details] Bug 1399611 - Emulate CSD on toolkit level, https://reviewboard.mozilla.org/r/179444/#review193384 If widget code will handle events on decorations, then I like that widget code also does the drawing, assuming that using separate windows for drawing decorations and content is acceptable. But, if separate windows for drawing decorations and content are acceptable, then why not let GTK draw the decorations and handle the events on the shell GtkWindow? That would be much simpler than re-implementing in Gecko. With an empty titlebar on the GtkWindow, Gecko can still draw a titlebar and handle titlebar events. I'm assuming that is the difference in approach that you are proposing for feedback. (I don't have before and after versions to easily compare because the patches in bug 1283299 and have very different base revisions.) If you can spell out precisely the parts on which you are seeking feedback, then it is easier for me to reply more promptly. But the topic of who draws the CSD seems the key decision to make, as that can make much of the rest irrelevant. Have you considered my suggestion?

(In reply to Karl Tomlinson (:karlt) from comment #9) > Comment on attachment 8907768 [details] > Bug 1399611 - Emulate CSD on toolkit level, > > https://reviewboard.mozilla.org/r/179444/#review193384 > > If widget code will handle events on decorations, then I like that widget > code also does the drawing, assuming that using separate windows for drawing > decorations and content is acceptable. > > But, if separate windows for drawing decorations and content are acceptable, > then why not let GTK draw the decorations and handle the events on the shell > GtkWindow? > > That would be much simpler than re-implementing in Gecko. With an empty > titlebar on the GtkWindow, Gecko can still draw a titlebar and handle > titlebar > events. > > I'm assuming that is the difference in approach that you are proposing for > feedback. (I don't have before and after versions to easily compare because > the patches in bug 1283299 and have very different base revisions.) > > If you can spell out precisely the parts on which you are seeking feedback, > then it is easier for me to reply more promptly. > > But the topic of who draws the CSD seems the key decision to make, as that > can > make much of the rest irrelevant. Have you considered my suggestion? Hi Karl, Thanks for your feedback. This is really a good idea to leave Gtk+ to draw the CSD - I'll try this way and reply here. I was seeking your feedback for this area - the separate GdkWindow rendering. Thanks again.

(In reply to Karl Tomlinson (:karlt) from comment #9) > Comment on attachment 8907768 [details] > If widget code will handle events on decorations, then I like that widget > code also does the drawing, assuming that using separate windows for drawing > decorations and content is acceptable. > > But, if separate windows for drawing decorations and content are acceptable, > then why not let GTK draw the decorations and handle the events on the shell > GtkWindow? > > That would be much simpler than re-implementing in Gecko. With an empty > titlebar on the GtkWindow, Gecko can still draw a titlebar and handle > titlebar > events. Hi, I implemented your suggestion and that works and greatly simplifies it, I created a new git branch for that [1]. So I'd go with this way, which mean we should close Bug 1365218 and review Bug 1364843 as the header bar still needs to be rendered by Gecko. [1] https://github.com/stransky/gecko-dev/tree/titlebar-gtk

(In reply to Karl Tomlinson (:karlt) from comment #8) I'll elaborate your remarks here. > > To allow stateless style creation I added new root MOZ_GTK_WINDOW_MAXIMIZED widget container which is just a maximized version of MOZ_GTK_WINDOW container and holds widgets added to maximized GtkWindow. > > A separate maximized window and descendant header bar should work fine if > only on the header bar depends on the difference in the windows. > > This function draws decoration (shadows) to mShell. I used cairo to clear background under the decoration only and draw over it. > > > > moz_gtk_get_window_border() can be replaced by cached values from nsWindow. > > I'm surprised that the background needs to be cleared here. > > Clearing the background differs from Gecko's model for translucent windows > where the background is initially cleared and drawing only draws the parts > that are not clear, with operator_over. > > It seems to differ from GTK's model also as I don't see gtk_window_draw > clearing the background. > > gdk_window_begin_draw_frame documentation says "if you’re drawing the > contents > of the widget yourself, you can assume that the widget has a cleared > background". > > I suspect that this will not be necessary without > the gtk_widget_set_double_buffered(, false) call on this widget. > > I haven't looked at the details of borders. I doubt there's a need to cache > values, but if there already exist cached values, then perhaps it makes sense > to use them. Getting from here to an nsWindow would be awkward though. Yes, it's weird. I re-checked again and the clearing is not required - I wonder why I got the rendering artifacts. Anyway, double buffering on mShell is not disabled [1] when rendering to mContainer. [1] https://github.com/stransky/gecko-dev/blob/titlebar-gtk/widget/gtk/nsWindow.cpp#L4028 > > This is a tricky part - rendering CSD in main thread. Can we do that? > > Yes, drawing with GTK on the main thread is fine, as Gecko's drawing is not > involved, but gtk_widget_set_double_buffered(, false) should not be called > on this widget as it can lead to flickering. Great, fortunately we won't need that. > > This may look uglty but only reason for that is to make Gtk+ happy and doesn't throw warning messages. > > > > I use mContainer margin to make space between mShell and mContainer to make the decorations (shadows) actually visible. > > > > We're going to resize the mContainer/mShell later anyway but when we set the margin here early it does not produce rendering artifacts when main window is shown. > > I haven't looked closely at this. That part is already removed.

(In reply to Karl Tomlinson (:karlt) from comment #8) > Comment on attachment 8907768 [details] > > To allow stateless style creation I added new root MOZ_GTK_WINDOW_MAXIMIZED widget container which is just a maximized version of MOZ_GTK_WINDOW container and holds widgets added to maximized GtkWindow. > > A separate maximized window and descendant header bar should work fine if > only on the header bar depends on the difference in the windows. I don't thinks it's a problem - I see that the "maximized" style is used for GtkWindow only (update_window_style_classes() at gtkwindow.c / gtk+-3.22.21).

(In reply to Martin Stránský from comment #12) > Anyway, double buffering on mShell > is not disabled [1] when rendering to mContainer. Ah, yes, thanks. I was misreading the code. (In reply to Martin Stránský from comment #11) > Hi, I implemented your suggestion and that works and greatly simplifies it, > I created a new git branch for that [1]. So I'd go with this way, which mean > we should close Bug 1365218 and review Bug 1364843 as the header bar still > needs to be rendered by Gecko. > > [1] https://github.com/stransky/gecko-dev/tree/titlebar-gtk Thank you for trying that. Pleased it worked.

Hi all, I am testing the CSD patch on Debian via flatpak. It works quite well except for the fact that superimposed to my icon theme buttons (close and maximize, minimize if present)there is another set of gray icons. Changing the theme, my icons change but the superimposed ones persist. I can't figure out if it is a Flatpak issue or it's due to the program itself.

(In reply to lord from comment #15) > Hi all, > I am testing the CSD patch on Debian via flatpak. It works quite well except > for the fact that superimposed to my icon theme buttons (close and maximize, > minimize if present)there is another set of gray icons. Changing the theme, > my icons change but the superimposed ones persist. I can't figure out if it > is a Flatpak issue or it's due to the program itself. I'm aware of that - it isn't caused by flatpak. Let's track it at Bug 1408335.

Already done.