1434646 - [Ambiance] Symbols in window controls are no longer centered

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P2)

Description

[Marco Castelluccio [:marco]]

Attachment: shot.png

One of the patches that landed recently caused the symbols in the window controls not to be centered.

Comment 1

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

That comes from Bug 1427999 and should be fixed by Bug 1408335.

Comment 2

[Marco Castelluccio [:marco]]

I will test again tomorrow with the Nightly containing the patch from bug 1427999.

Comment 3

[Marco Castelluccio [:marco]]

No difference.

Comment 4

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

Is that on HiDPI display?

Comment 5

[Marco Castelluccio [:marco]]

Yes.

Comment 6

[Marco Castelluccio [:marco]]

There's also a related problem, the controls are very close between each other, they are supposed to be separated by some space.

Comment 7

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

(In reply to Marco Castelluccio [:marco] from comment #6)
> There's also a related problem, the controls are very close between each
> other, they are supposed to be separated by some space.

That's Bug 1433092.

Comment 8

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

I can reproduce that without HiDPI so does not look like HiDPI related.

Comment 9

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

Attachment: Bug 1434646 - Titlebar rendering - Place titlebar buttons in GtkBox,

Some themes (Ambiance for instance) uses first-child/last-child css selectors
to style titlebar buttons. Ubuntu Ambiance theme places titlebar buttons closer
by negative margin applied to them.

We put titlebar buttons to GtkBox as well as Gtk+ does and also keep
the button order here to match first-child/last-child selectors. It also means
we must have maximize/restore as one button to keep the correct order.

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

Comment 10

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

Comment on attachment 8954032 [details]
Bug 1434646 - Titlebar rendering - Place titlebar buttons in GtkBox,

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

Comment 11

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

Comment on attachment 8954032 [details]
Bug 1434646 - Titlebar rendering - Place titlebar buttons in GtkBox,

https://reviewboard.mozilla.org/r/223184/#review229544

::: widget/gtk/WidgetStyleCache.cpp:712
(Diff revision 2)
> +  gint buttonSpacing = 6;
> +  g_object_get(headerBar, "spacing", &buttonSpacing, nullptr);
> +
> +  GtkWidget *box = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, buttonSpacing);
> +
> +  if (IsToolbarButtonEnabled(MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE)) {

We need to check if theme-change signal is also emitted when buttons are changed and act accordingly here.

Comment 12

[Jan Horak [:jhorak]]

Comment on attachment 8954032 [details]
Bug 1434646 - Titlebar rendering - Place titlebar buttons in GtkBox,

https://reviewboard.mozilla.org/r/223184/#review229534

::: commit-message-abada:1
(Diff revision 2)
> +Bug 1434646 - Titlebar rendering - Place titlebar buttons at GtkBox, r?jhorak

nit: at/in

::: widget/gtk/WidgetStyleCache.cpp:700
(Diff revision 2)
> -  LoadWidgetIconPixbuf(image);
> +   LoadWidgetIconPixbuf(image);
> +}
>  
> -  return widget;
> +// TODO - Also return style for buttons located at Maximized toolbar.
> +static void
> +CreateHeaderBarButton(WidgetNodeType aWidgetType)

Argument is not used. Consider plural form, like CreateHeaderBarButtons to emphatize you're creating all button there.

::: widget/gtk/WidgetStyleCache.cpp:712
(Diff revision 2)
> +  gint buttonSpacing = 6;
> +  g_object_get(headerBar, "spacing", &buttonSpacing, nullptr);
> +
> +  GtkWidget *box = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, buttonSpacing);
> +
> +  if (IsToolbarButtonEnabled(MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE)) {

Can't we use GetGtkHeaderBarButtonLayout there and loop over returned WidgetNodeType and call AddHeaderBarButton(WidgetNodeType[i]). Basically reuse content of IsToolbarButtonEnabled.

Comment 13

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

Comment on attachment 8954032 [details]
Bug 1434646 - Titlebar rendering - Place titlebar buttons in GtkBox,

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

Comment 14

[Jan Horak [:jhorak]]

Comment on attachment 8954032 [details]
Bug 1434646 - Titlebar rendering - Place titlebar buttons in GtkBox,

https://reviewboard.mozilla.org/r/223184/#review229932

::: widget/gtk/gtkdrawing.h:588
(Diff revisions 2 - 3)
>   * Get ToolbarButtonGTKMetrics for recent theme.
>   */
>  const ToolbarButtonGTKMetrics*
>  GetToolbarButtonMetrics(WidgetNodeType aWidgetType);
>  
> -/* Get toolbar button state.
> +/* Get toolbar button layout.

Thanks for the comment. To have it exported to doxygen you need to start comment by /** (note double stars) like previous functions are descripted.

Comment 15

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

Comment on attachment 8954032 [details]
Bug 1434646 - Titlebar rendering - Place titlebar buttons in GtkBox,

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24fc18ac7eeb
Titlebar rendering - Place titlebar buttons in GtkBox, r=jhorak

Comment 17

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/24fc18ac7eeb