Closed Bug 1168556 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Theme, defect)

40 Branch
x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox41 --- verified

People

(Reporter: mmcdonough, Assigned: mmcdonough)

References

Details

Attachments

(6 files, 1 obsolete file)

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.
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.
It's also important to note that browser.tabs.drawInTitlebar must be set to false.
Attached patch Fixes titlebar button spacing (obsolete) — Splinter Review
This patch seems to fix the issue.
Summary: Titlebar buttons are drawn with incorrect vertical offset on OS X in private browsing and web developer themes → Titlebar buttons drawn too high in OS X private browsing and web developer themes without tabs in titlebar
Component: Untriaged → Theme
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Flags: needinfo?(bgrinstead)
Attached image titlebar-devedition.png
screenshot of the issue
Attached image titlebar-normal.png
Without the devedition theme applied
Attached image with_patch.png
With the patch applied.
Attached image native_toolbar.png
An example of the native toolbar, to show that the positioning with the patch is correct.
I believe this affects all lightweight themes.  Andrew, can you confirm this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bgrinstead) → needinfo?(foolkingcrown)
Summary: Titlebar buttons drawn too high in OS X private browsing and web developer themes without tabs in titlebar → Titlebar buttons drawn too high for lightweight themes in OS X without tabs in titlebar
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;
  }
}
Testing in 40.0a2 (2015-06-02), it does affect all lightweight themes.
Flags: needinfo?(foolkingcrown)
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.
Flags: needinfo?(gijskruitbosch+bugs)
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...
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8614246 - Flags: feedback?(mconley)
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?
Flags: needinfo?(foolkingcrown)
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.
Attachment #8614246 - Flags: feedback?(mconley)
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)?
Flags: needinfo?(foolkingcrown)
I guess I'll need a machine running Yosemite to test this on.
Flags: needinfo?(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.
Flags: needinfo?(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?
Flags: needinfo?(foolkingcrown)
(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.
Flags: needinfo?(foolkingcrown)
See Also: → 1172553
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.
(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 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.
Fixes titlebar button spacing, applies only when using lightweight themes
Attachment #8614246 - Attachment is obsolete: true
Comment on attachment 8621266 [details] [diff] [review]
Fixes titlebar button spacing in lightweight themes

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

Thanks!
Attachment #8621266 - Flags: review?(mconley) → review+
Assignee: nobody → foolkingcrown
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/c7eec18f6d2b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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?
(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!
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.