Window controls with CSD on are not vertically centered

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: pascalc, Assigned: dao)

Tracking

(Blocks 1 bug)

unspecified
Firefox 60
Unspecified
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted image 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
See Also: → 1422276
Bug 1433092 is not related here as it's about horizontal spacing.
Assignee: nobody → stransky
Let's fix that at Bug 1419442.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1419442
Reopening per https://bugzilla.mozilla.org/show_bug.cgi?id=1419442#c23 - I'd fix that separately after all.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment hidden (mozreview-request)
Status: REOPENED → ASSIGNED

Comment 5

a year ago
mozreview-review
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]
Duplicate of this bug: 1439274
Assignee

Comment 7

a year ago
Posted patch patchSplinter Review
I'm trying to figure out a simpler fix. How is this?
Attachment #8952673 - Flags: review?(stransky)
Comment on attachment 8952673 [details] [diff] [review]
patch

That's very good!
Attachment #8952673 - Flags: review?(stransky) → review+
Attachment #8952077 - Attachment is obsolete: true
Attachment #8952077 - Flags: review?(dao+bmo)
Attachment #8952077 - Attachment is obsolete: false
Attachment #8952077 - Attachment is obsolete: true
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).
Attachment #8952739 - Flags: review?(dao+bmo)
Assignee

Comment 10

a year ago
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.
Attachment #8952739 - Flags: review?(dao+bmo) → review+
Assignee

Comment 11

a year ago
Gonna land both patches in a bit.
Assignee: stransky → dao+bmo
Component: Widget: Gtk → Theme
OS: Unspecified → Linux
Product: Core → Firefox
Great, Thanks!

Comment 13

a year ago
Pushed by dgottwald@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf8ac6e2c5e0
https://hg.mozilla.org/mozilla-central/rev/134bfa14acf9
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee

Updated

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