1434621 - Window controls with CSD on are not vertically centered

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Pascal Chevrel:pascalc]

Attachment: wrong_position.png

Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 ID:20180131100706

Ubuntu 17.10, OSX-Arc White theme, layout.css.devPixelsPerPx set to 1.5

When I activate the CSD through the customization pane, the window controls are drawn to the top edge of the window instead of being vertically centered in the tabbar like in other gnome apps such as gedit or nautilus. 

I am attaching a screenshot

The buttons are also no longer spaced consistently with other apps (not enough space between the widgets) but maybe this is bug 1433092? If not I can file another bug

Comment 1

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

Bug 1433092 is not related here as it's about horizontal spacing.

Comment 2

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

Let's fix that at Bug 1419442.

Comment 3

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

Reopening per https://bugzilla.mozilla.org/show_bug.cgi?id=1419442#c23 - I'd fix that separately after all.

Comment 4

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

Attachment: Bug 1434621 - [Linux/Titlebar rendering] Center titlebar buttons,

Linux renders titlebar buttons vertically centered so emulate that for our titlebar rendering.
When menubar is visible center titlebar buttons according menubar, othervise center against tabbar.
Also consider titlebar button sizes which are be defined by actual Gtk+ theme.

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8952077 [details]
Bug 1434621 - [Linux/Titlebar rendering] Center titlebar buttons,

https://reviewboard.mozilla.org/r/221308/#review227136


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/browser-tabsintitlebar.js:238
(Diff revision 1)
>          let extraMargin = tabAndMenuHeight - titlebarContentHeight;
> -        if (AppConstants.platform != "macosx") {
> +        if (AppConstants.platform == "linux") {
> +          // Linux centers buttons at titlebar so let's emulate that.
> +          if (!fullMenuHeight) {
> +            // Menubar is not visible - center buttons according to tabs.
> +            titlebarContent.style.marginTop = extraMargin/2 + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/base/content/browser-tabsintitlebar.js:239
(Diff revision 1)
> -        if (AppConstants.platform != "macosx") {
> +        if (AppConstants.platform == "linux") {
> +          // Linux centers buttons at titlebar so let's emulate that.
> +          if (!fullMenuHeight) {
> +            // Menubar is not visible - center buttons according to tabs.
> +            titlebarContent.style.marginTop = extraMargin/2 + "px";
> +            titlebarContent.style.marginBottom = extraMargin/2 + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/base/content/browser-tabsintitlebar.js:241
(Diff revision 1)
> +          if (!fullMenuHeight) {
> +            // Menubar is not visible - center buttons according to tabs.
> +            titlebarContent.style.marginTop = extraMargin/2 + "px";
> +            titlebarContent.style.marginBottom = extraMargin/2 + "px";
> +          } else {
> +            if (fullMenuHeight > titlebarContentHeight) {

Error: Unexpected if as the only statement in an else block. [eslint: no-lonely-if]

::: browser/base/content/browser-tabsintitlebar.js:244
(Diff revision 1)
> +            titlebarContent.style.marginBottom = extraMargin/2 + "px";
> +          } else {
> +            if (fullMenuHeight > titlebarContentHeight) {
> +              // Buttons are smaller than menubar so center them according to menubar.
> +              let fullMenuHeightMargin = fullMenuHeight - titlebarContentHeight;
> +              titlebarContent.style.marginTop = fullMenuHeightMargin/2 + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/base/content/browser-tabsintitlebar.js:245
(Diff revision 1)
> +          } else {
> +            if (fullMenuHeight > titlebarContentHeight) {
> +              // Buttons are smaller than menubar so center them according to menubar.
> +              let fullMenuHeightMargin = fullMenuHeight - titlebarContentHeight;
> +              titlebarContent.style.marginTop = fullMenuHeightMargin/2 + "px";
> +              titlebarContent.style.marginBottom = fullMenuHeightMargin/2 + (extraMargin - fullMenuHeightMargin) + "px";

Error: Infix operators must be spaced. [eslint: space-infix-ops]

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

I'm trying to figure out a simpler fix. How is this?

Comment 8

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

Comment on attachment 8952673 [details] [diff] [review]
patch

That's very good!

Comment 9

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

Attachment: titlebar padding patch

I also suggest to remove titlebar padding loaded from Gtk+ theme and apply only the one you added at #titlebar-buttonbox. Otherwise the titlebar is too big on themes which defines titlebar padding (like Adwaita).

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8952739 [details] [diff] [review]
titlebar padding patch

Yeah, I figured we'd want to take this part of your patch but forgot about it.

Comment 11

[Dão Gottwald [:dao]]

Gonna land both patches in a bit.

Comment 12

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

Great, Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8ac6e2c5e0
part 1: vertically center title bar buttons on Linux. r=stransky
https://hg.mozilla.org/integration/mozilla-inbound/rev/134bfa14acf9
part 2: ignore vertical titlebar padding from the Gtk+ theme since we set it with CSS on #titlebar-buttonbox. r=dao

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cf8ac6e2c5e0
https://hg.mozilla.org/mozilla-central/rev/134bfa14acf9