[CSD] Titlebar button spacing is missing

RESOLVED FIXED in Firefox 60

Status

()

defect
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
Gtk+ uses spacing between titlebar buttons - the buttons are places at vbox with default spacing 6pix.
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

https://reviewboard.mozilla.org/r/221362/#review227466

::: commit-message-ad133:1
(Diff revision 1)
> +Bug 1433092 - Add spacing among titlebar buttons, r?jhorak

s/among/around

::: widget/gtk/gtk3drawing.cpp:313
(Diff revision 1)
>      return MOZ_GTK_SUCCESS;
>  }
>  
> +// We support LTR layout only here for now.
> +static void
> +InitAddToolbarButtonSpacing(WidgetNodeType aWidgetType,

The AddTollbarButtonSpacing as a name of the function should be better, because I don't see any initialization there.

::: widget/gtk/gtk3drawing.cpp:346
(Diff revision 1)
> +  buttonSpacing /= 2;
> +
> +  static_assert(TOOLBAR_BUTTONS >= 4,
> +                "We need at least 4 titlebar buttons available.");
> +  // It's a first button - apply spacing to right side only.
> +  if (buttonPosition == 0) {

Please use switch there.

::: widget/gtk/gtk3drawing.cpp:363
(Diff revision 1)
> +      *spacingLeft += buttonSpacing;
> +  }
> +}
> +
>  static void
>  InitToolbarButtonMetrics(ToolbarButtonGTKMetrics *aButtonMetrics,

There's no initialization of static vars in this function, it just returns ToolbarButtonGTKMetrics, please rename to GetToolbarButtonMetric or similar.

::: widget/gtk/gtk3drawing.cpp:400
(Diff revision 1)
> +                                &aButtonMetrics->buttonMargin.right);
> +
>      gint left = 0, top = 0, right = 0, bottom = 0;
> -    moz_gtk_add_margin_border_padding(style, &left, &top, &right, &bottom);
> +    moz_gtk_add_border_padding(style, &left, &top, &right, &bottom);
>  
> +    // Buton size is calculated as min-width/height + border/padding.

typo Buton/Button

::: widget/gtk/gtk3drawing.cpp:418
(Diff revision 1)
> +    aButtonMetrics->minSizeWithBorderMargin.height = height +
> +        aButtonMetrics->buttonMargin.top + aButtonMetrics->buttonMargin.bottom;
>  }
>  
> -const ToolbarButtonGTKMetrics*
> -GetToolbarButtonMetrics(WidgetNodeType aWidgetType)
> +static void
> +InitAllToolbarButtonMetrics(void)

Hm, there seems to be a lot of Init functions which starts to be confusing.

InitAllToolbarButtonMetrics -> EnsureToolbarMetrics (or keep the InitToolbarButtonMetrics)
InitToolbarButtonMetrics -> CreateToolbarButtonMetrics
InitAddToolbarButtonSpacing -> AddToolbarButtonSpacing
or consider better names.
Attachment #8952128 - Flags: review?(jhorak)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1434646

Comment 10

a year ago
mozreview-review
Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

https://reviewboard.mozilla.org/r/221362/#review228574

::: widget/gtk/gtk3drawing.cpp:364
(Diff revision 8)
> -const ToolbarButtonGTKMetrics*
> -GetToolbarButtonMetrics(WidgetNodeType aWidgetType)
> +// We support LTR layout only here for now.
> +static void
> +CalculateToolbarButtonSpacing(WidgetNodeType aWidgetType)
> +{
> +    int buttonIndex = (aWidgetType - MOZ_GTK_HEADER_BAR_BUTTON_CLOSE);
> +    ToolbarButtonGTKMetrics* metrics = sToolbarMetrics.button + buttonIndex;

Could you please rather pass ToolbarButtonGTKMetrics as a function argument to avoid direct access to the static/global variable? You won't need to compute the buttonIndex after that.

Same for CalculateToolbarButtonMetrics.

Ideally that we use sToolbarMetrics only in GetToolbarButtonMetrics and EnsureToolbarMetrics.

::: widget/gtk/gtk3drawing.cpp:395
(Diff revision 8)
> +    metrics->minSizeWithBorderMargin.height +=
> +        metrics->buttonMargin.top + metrics->buttonMargin.bottom;
> +}
> +
> +static int
> +GetGtkHeaderBarLayout(WidgetNodeType* aButtonLayout, int aMaxButtonNums)

nit: please use GetGtkHeaderBarButtonLayout

::: widget/gtk/gtk3drawing.cpp:398
(Diff revision 8)
> +
> +static int
> +GetGtkHeaderBarLayout(WidgetNodeType* aButtonLayout, int aMaxButtonNums)
> +{
> +  NS_ASSERTION(aMaxButtonNums >= TOOLBAR_BUTTONS,
> +               "We're missing titlebar buttons!");

This is a bit misleading, please use something like:
Requested number of buttons is higher than storage capacity.

::: widget/gtk/gtk3drawing.cpp:439
(Diff revision 8)
> +
> +  return activeButtonNums;
> +}
> +
> +static void
> +EnableAndPlaceTitlebarButtons()

I think you can put this into EnsureToolbarMetrics or possibly join with the code in EnsureToolbarMetrics.

::: widget/gtk/gtk3drawing.cpp:456
(Diff revision 8)
> +        }
> +        // Mark last button.
> +        if (i == (activeButtonNums-1)) {
> +            metrics->lastButton = true;
> +        }
> +    }

...and call CalculateToolbar\* stuff there.
Attachment #8952128 - Flags: review?(jhorak)
Comment hidden (mozreview-request)

Comment 12

a year ago
mozreview-review
Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

https://reviewboard.mozilla.org/r/221362/#review229532
Attachment #8952128 - Flags: review?(jhorak) → review+

Comment 13

a year ago
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/360486fecfe8
Add spacing around titlebar buttons, r=jhorak

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/360486fecfe8
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
The spacing isn't exactly the same as other GNOME applications like Gedit (there's more spacing in Firefox compared to Gedit). I'll wait for bug 1434646 to be fixed before filing a bug.
Depends on: 1441899
See Also: → 1442364
See Also: → 1442366

Comment 16

a year ago
Posted image button alignment
I noticed that the spacing between the window control buttons and the vertical border is slightly off. See attachment (Arc GTK theme - top: Firefox; bottom: Nautilus)

On a different note: In Arc GTK theme the window buttons of *inactive* windows are faded to grayscale, while in Firefox with drawInTitlebar enabled they are not. Should I make a seperate bug report for this or is it on purpose?
(In reply to 3ship from comment #16)
> On a different note: In Arc GTK theme the window buttons of *inactive*
> windows are faded to grayscale, while in Firefox with drawInTitlebar enabled
> they are not. Should I make a seperate bug report for this or is it on
> purpose?

It's not on purpose, please file a bug. Thanks!

Updated

a year ago
Depends on: 1451792
You need to log in before you can comment on or make changes to this bug.