[Ambiance] Symbols in window controls are no longer centered

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: marco, Assigned: stransky)

Tracking

(Blocks 1 bug, {polish, regression})

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 fixed)

Details

Attachments

(2 attachments)

Posted image shot.png
One of the patches that landed recently caused the symbols in the window controls not to be centered.
Flags: needinfo?(stransky)
That comes from Bug 1427999 and should be fixed by Bug 1408335.
Flags: needinfo?(stransky)
Reporter

Comment 2

Last year
I will test again tomorrow with the Nightly containing the patch from bug 1427999.
Blocks: 1427999
Flags: needinfo?(mcastelluccio)
Reporter

Comment 3

Last year
No difference.
Flags: needinfo?(mcastelluccio) → needinfo?(stransky)
Is that on HiDPI display?
Flags: needinfo?(stransky) → needinfo?(mcastelluccio)
Reporter

Comment 5

Last year
Yes.
Flags: needinfo?(mcastelluccio)
Assignee

Updated

Last year
Blocks: gtktitlebar
Summary: Symbols in window controls are no longer centered → [HiDPI] Symbols in window controls are no longer centered
Assignee

Updated

Last year
Assignee: nobody → stransky
Keywords: polish
Priority: -- → P2
Reporter

Updated

Last year
See Also: → 1422276
Reporter

Comment 6

Last year
There's also a related problem, the controls are very close between each other, they are supposed to be separated by some space.
(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.
I can reproduce that without HiDPI so does not look like HiDPI related.
Summary: [HiDPI] Symbols in window controls are no longer centered → [Ambiance] Symbols in window controls are no longer centered
Assignee

Updated

Last year
Depends on: 1433092
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

Last year
mozreview-review
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

Last year
mozreview-review
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.
Attachment #8954032 - Flags: review?(jhorak)
Comment hidden (mozreview-request)

Comment 14

Last year
mozreview-review
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.
Attachment #8954032 - Flags: review?(jhorak) → review+
Comment hidden (mozreview-request)

Comment 16

Last year
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/24fc18ac7eeb
Titlebar rendering - Place titlebar buttons in GtkBox, r=jhorak
https://hg.mozilla.org/mozilla-central/rev/24fc18ac7eeb
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1445253
You need to log in before you can comment on or make changes to this bug.