[CSD] Wrong titlebar button sizes

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
We need to correctly calculate titlebar button sizes from GTK+ CSS properties min-width / min-height and border/padding for the titlebar buttons.
(Assignee)

Updated

a year ago
Assignee: nobody → stransky
Blocks: 1422276
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Priority: P3 → P2
(Assignee)

Comment 2

a year ago
Comment on attachment 8939774 [details]
Bug 1427999 - Implement GetToolbarButtonMetrics() to get titlebar button size and border,

Seems to be broken on some themes (Arc-Dark), I'll investigate it.
Attachment #8939774 - Flags: review?(jhorak)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1408335
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
Comment on attachment 8939774 [details]
Bug 1427999 - Implement GetToolbarButtonMetrics() to get titlebar button size and border,

https://reviewboard.mozilla.org/r/210090/#review220210

::: widget/gtk/gtk3drawing.cpp:28
(Diff revision 3)
>  static gboolean notebook_has_tab_gap;
>  
>  static ScrollbarGTKMetrics sScrollbarMetrics[2];
>  static ToggleGTKMetrics sCheckboxMetrics;
>  static ToggleGTKMetrics sRadioMetrics;
> +static ToolbarButtonGTKMetrics sToolbarButtonMetrics;

You can make it sToolbarButtonMetrics[4] to cover maximize, minimize, restore, close but we can do that in another bug.

::: widget/gtk/gtk3drawing.cpp:308
(Diff revision 3)
>      gtk_style_context_get_style(style, "handle_size", size, NULL);
>      return MOZ_GTK_SUCCESS;
>  }
>  
> +gint
> +moz_gtk_titlebar_button_get_metrics(GtkBorder* aButtonBorder,

This is somehow different implementation approach from how we done ScrollbarGTKMetrics and ToggleGTKMetrics. Please return pointer to ToolbarButtonGTKMetrics instead of providing arguments to be returned. 

I'm aware that you used other moz_gtk_..._get_metric as a template, but these calls are not caching data.

::: widget/gtk/gtk3drawing.cpp:313
(Diff revision 3)
> +moz_gtk_titlebar_button_get_metrics(GtkBorder* aButtonBorder,
> +                                    gint* aButtonWidth,
> +                                    gint* aButtonHeight)
> +{
> +    if (!sToolbarButtonMetrics.initialized) {
> +        GtkStyleContext* style = GetStyleContext(MOZ_GTK_HEADER_BAR_BUTTON_CLOSE);

Hm, I'm afraid we can use 'close' button style for every other buttons, at least if we want to mimic the behaviour of GTK CSS styles as much as possible.

::: widget/gtk/gtk3drawing.cpp:313
(Diff revision 3)
> +        GtkStyleContext* style = GetStyleContext(MOZ_GTK_HEADER_BAR_BUTTON_CLOSE);
> +
> +        gint left = 0, top = 0, right = 0, bottom = 0;
> +        moz_gtk_add_margin_border_padding(style, &left, &top, &right, &bottom);

Please move this closer to the usage, ie to:
        width += left + right
        height += top + bottom;

::: widget/gtk/gtk3drawing.cpp:341
(Diff revision 3)
> +        if (height < iconHeight)
> +            height = iconHeight;
> +
> +        width += left + right;
> +        height += top + bottom;
> +        sToolbarButtonMetrics.minSizeWithBorder.width = width;

Rename to minSizeWithBorderMargin to be precise

::: widget/gtk/gtk3drawing.cpp:2355
(Diff revision 3)
>          }
>      case MOZ_GTK_HEADER_BAR_BUTTON_CLOSE:
>      case MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE:
>      case MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE:
>          {
> -            style = GetStyleContext(widget);
> +            GtkBorder border;

This is no longer needed when the bug 1408335 lands, because buttons won't be used as containers anymore. 

Please don't touch this there and remove it in the 1408335 and move the MOZ_GTK_HEADER_BAR_BUTTON_\* below to the list of elements which are not containers.

::: widget/gtk/gtkdrawing.h:87
(Diff revision 3)
>    bool initialized;
>    MozGtkSize minSizeWithBorder;
>    GtkBorder borderAndPadding;
>  } ToggleGTKMetrics;
>  
> +typedef struct {

What should be sufficient to store is:
- icon relative x/y position - for moz_gtk_header_bar_button_paint
- min widget size - for GetMinimumWidgetSize
Attachment #8939774 - Flags: review?(jhorak) → review-

Comment 7

a year ago
mozreview-review
Comment on attachment 8944376 [details]
Bug 1427999 - Use GetToolbarButtonMetrics() to get correct titlebar button size at nsNativeThemeGTK::GetMinimumWidgetSize,

https://reviewboard.mozilla.org/r/214580/#review220478

::: widget/gtk/nsNativeThemeGTK.cpp:1556
(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:
> +    {
> +      moz_gtk_titlebar_button_get_metrics(nullptr,

Please use the cached entries directly, like  this do: 
https://searchfox.org/mozilla-central/source/widget/gtk/nsNativeThemeGTK.cpp#1398
Attachment #8944376 - Flags: review?(jhorak) → review-
(Assignee)

Comment 8

a year ago
mozreview-review
Comment on attachment 8939774 [details]
Bug 1427999 - Implement GetToolbarButtonMetrics() to get titlebar button size and border,

https://reviewboard.mozilla.org/r/210090/#review220826

::: widget/gtk/gtk3drawing.cpp:28
(Diff revision 3)
>  static gboolean notebook_has_tab_gap;
>  
>  static ScrollbarGTKMetrics sScrollbarMetrics[2];
>  static ToggleGTKMetrics sCheckboxMetrics;
>  static ToggleGTKMetrics sRadioMetrics;
> +static ToolbarButtonGTKMetrics sToolbarButtonMetrics;

Let's do that as another bug as well as box spacing.

::: widget/gtk/gtk3drawing.cpp:313
(Diff revision 3)
> +moz_gtk_titlebar_button_get_metrics(GtkBorder* aButtonBorder,
> +                                    gint* aButtonWidth,
> +                                    gint* aButtonHeight)
> +{
> +    if (!sToolbarButtonMetrics.initialized) {
> +        GtkStyleContext* style = GetStyleContext(MOZ_GTK_HEADER_BAR_BUTTON_CLOSE);

We're going to fix that in another bug as well as the spacing.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8939774 [details]
Bug 1427999 - Implement GetToolbarButtonMetrics() to get titlebar button size and border,

https://reviewboard.mozilla.org/r/210090/#review220898
Attachment #8939774 - Flags: review?(jhorak) → review+

Comment 14

a year ago
mozreview-review
Comment on attachment 8944376 [details]
Bug 1427999 - Use GetToolbarButtonMetrics() to get correct titlebar button size at nsNativeThemeGTK::GetMinimumWidgetSize,

https://reviewboard.mozilla.org/r/214580/#review220900
Attachment #8944376 - Flags: review?(jhorak) → review+

Comment 15

a year ago
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/d1b018ec55a8
Implement GetToolbarButtonMetrics() to get titlebar button size and border, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/670be06477e0
Use GetToolbarButtonMetrics() to get correct titlebar button size at nsNativeThemeGTK::GetMinimumWidgetSize, r=jhorak

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1b018ec55a8
https://hg.mozilla.org/mozilla-central/rev/670be06477e0
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1434646
You need to log in before you can comment on or make changes to this bug.