1168556 - Titlebar buttons drawn too high for lightweight themes in OS X without tabs in titlebar

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Andrew Martin McDonough (:mmcdonough)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150526004004

Steps to reproduce:

Start Firefox Web Developer edition with the web dev theme (default for Web Developer edition).


Actual results:

The maximize, minimize, and close buttons are drawn several pixels to high up in the title bar.


Expected results:

The maximize, minimize, and close buttons should be drawn at the normal height.

Comment 1

[Andrew Martin McDonough (:mmcdonough)]

I had noticed this in Private Browsing windows for a long time, but it seems to have ended up in normal windows now, as well.

Comment 2

[Andrew Martin McDonough (:mmcdonough)]

It's also important to note that browser.tabs.drawInTitlebar must be set to false.

Comment 3

[Andrew Martin McDonough (:mmcdonough)]

Attachment: Fixes titlebar button spacing

This patch seems to fix the issue.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: titlebar-devedition.png

screenshot of the issue

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: titlebar-normal.png

Without the devedition theme applied

Comment 6

[Andrew Martin McDonough (:mmcdonough)]

Attachment: with_patch.png

With the patch applied.

Comment 7

[Andrew Martin McDonough (:mmcdonough)]

Attachment: native_toolbar.png

An example of the native toolbar, to show that the positioning with the patch is correct.

Comment 8

[Brian Grinstead [:bgrins]]

I believe this affects all lightweight themes.  Andrew, can you confirm this?

Comment 9

[Brian Grinstead [:bgrins]]

There is also an issue with the titlebar in the devedition theme where the titlebar doesn't match the native styling - I think due to the following rules no longer applying after Bug 1148996:  https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#94-102:

#main-window:not(:-moz-lwtheme) > #titlebar {
  -moz-appearance: -moz-window-titlebar;
}

@media (-moz-mac-yosemite-theme) {
  #main-window:not(:-moz-lwtheme) > #titlebar {
    -moz-appearance: -moz-mac-vibrancy-light;
  }
}

Comment 10

[Andrew Martin McDonough (:mmcdonough)]

Testing in 40.0a2 (2015-06-02), it does affect all lightweight themes.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8614246 [details] [diff] [review]
Fixes titlebar button spacing

Gijs, who is a good person to review a patch for OSX lightweight themes?  This is to fix a button/text offset issue when the tabs are not in the titlebar.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8614246 [details] [diff] [review]
Fixes titlebar button spacing

mconley would be the best person to review this, but if this only happens with lwts and the patch also affects non-lwt, then that makes it sound like the patch isn't the right patch...

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Dev Edition on Mountain Lion

I can't reproduce this on Mountain Lion (I'm a luddite and haven't yet upgraded to Yosemite)...

See screenshot.

So does this only affect newer versions of OS X?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8614246 [details] [diff] [review]
Fixes titlebar button spacing

Dropping feedback request until I get more information from the reporter about this bug.

Comment 15

[Andrew Martin McDonough (:mmcdonough)]

I do not have anything except Yosemite to test with. But it's extremely consistent with for me on Yosemite (in Aurora/WebDev, in Nightly, in Nightly I build myself). I don't have any way to test with any other version of OS X.

It also affected private browsing for a long time, but I had assumed this was a (fairly strange) conscious UI choice. Maybe it affects private browsing in other OS X versions? Maybe Firefox is not drawing its own title bar in Mountain Lion (and private browsing could possibly force it to)?

Comment 16

[Mike Conley (:mconley) (:⚙️)]

I guess I'll need a machine running Yosemite to test this on.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Ah, ok, I can reproduce this now. I had the STR wrong.

STR:

1) Start up Firefox (developer edition or otherwise)
2) Click on the menu button, and choose "Customize"
3) In the bottom left, click on "Title bar" so that the tabs no longer draw into the titlebar.
4) Next, still in customize mode, click Themes, and choose a lightweight theme to apply.

ER:

The window buttons should be properly aligned within the titlebar space.

AR:

The window buttons are a tad too high. See screenshot.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

mmcdonough:

Do my STR in comment 17 sound right? Looking at your screenshots, I see a white background where the native OS X window theme should be in the titlebar... do you have some kind of lightweight theme applied?

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> mmcdonough:
> 
> Do my STR in comment 17 sound right? Looking at your screenshots, I see a
> white background where the native OS X window theme should be in the
> titlebar... do you have some kind of lightweight theme applied?


I believe this is the issue in Comment 9.  Which looks like a separate issue just with the dev edition theme, yosemite, and tabs not in titlebar.  We need to make sure that -moz-appearance: -moz-mac-vibrancy-light is properly set in devedition.css.  I've just filed Bug 1172553 for this.

Comment 20

[Andrew Martin McDonough (:mmcdonough)]

No, the STR in comment 17 do not work for me. Following those steps I get a correctly drawing titlebar. If follow those steps and I then restart Firefox, I do get the white titlebar with the misaligned buttons. I can reproduce it without a restart if, instead of steps 1-3, I toggle browser.tabs.drawInTitlebar in about:config.

I guess I'm also not 100% sure what a lightweight theme is, I thought it was a theme that doesn't provide new images. I'm applying the WebDev theme (in comparison to the default), and the other themes I've been testing only do recolors.

I do think they look like separate issues, especially since the button issue appeared in the private browsing titlebar long before the normal titlebar lost its gradient.

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Andrew Martin McDonough (:mmcdonough) from comment #20)
> No, the STR in comment 17 do not work for me. Following those steps I get a
> correctly drawing titlebar. If follow those steps and I then restart
> Firefox, I do get the white titlebar with the misaligned buttons. I can
> reproduce it without a restart if, instead of steps 1-3, I toggle
> browser.tabs.drawInTitlebar in about:config.

FWIW, the STR in comment 17 do work for me in a clean profile.  I believe that clicking the "Title bar" button in customize mode is just a shortcut for setting browser.tabs.drawInTitlebar.

> I guess I'm also not 100% sure what a lightweight theme is, I thought it was
> a theme that doesn't provide new images. I'm applying the WebDev theme (in
> comparison to the default), and the other themes I've been testing only do
> recolors.

If you are in Customize mode and then click one of the "recommended" themes in the theme dropdown menu, those are lightweight themes.  They just do some simple modifications on top of the default theme (like changing the background image), and can be applied without a restart.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8614246 [details] [diff] [review]
Fixes titlebar button spacing

Review of attachment 8614246 [details] [diff] [review]:
-----------------------------------------------------------------

Hey, sorry for the delay.

::: browser/themes/osx/browser.css
@@ +121,4 @@
>    margin-top: @windowButtonMarginTop@;
>  }
>  
> +#main-window:not([tabsintitlebar]) > #titlebar > #titlebar-content > #titlebar-buttonbox-container,

We probably only want this to apply if the -moz-lwtheme pseudoselector matches on the last element, so please add:

:-moz-lwtheme

to #titlebar-buttonbox-container and #titlebar-fullscreen-button.

Comment 23

[Andrew Martin McDonough (:mmcdonough)]

Attachment: Fixes titlebar button spacing in lightweight themes

Fixes titlebar button spacing, applies only when using lightweight themes

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8621266 [details] [diff] [review]
Fixes titlebar button spacing in lightweight themes

Review of attachment 8621266 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c7eec18f6d2b

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c7eec18f6d2b

Comment 27

[Andrew Martin McDonough (:mmcdonough)]

The issue is fixed on normal windows, but it seems to still occur for me on private browsing windows even after the patch.

Perhaps this affects more than just lightweight themes?

Comment 28

[:Gijs (he/him)]

(In reply to Andrew Martin McDonough (:mmcdonough) from comment #27)
> The issue is fixed on normal windows, but it seems to still occur for me on
> private browsing windows even after the patch.
> 
> Perhaps this affects more than just lightweight themes?

Please file a new bug with more detailed STR and a screenshot of what the current state is like. Thank you!

Comment 29

[Carmen]

I was able to reproduce the issue on the affected build Aurora 40.0a2 (2015-05-26) using steps to reproduce offered in the description and 2nd comment. 

The fix was verified using the latest Aurora 50.0a2 (2016-09-19) on Mac OS X 10.12 Sierra Beta.