1275655 - The window buttons are too high in Private browsing windows when the title bar is shown

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

[Markus Stange [:mstange]]

STR:
 1. Open a new private browsing window.
 2. Enable the title bar in customization.
 3. Compare the position of the window buttons ("traffic light buttons") to a regular OS X window.

The buttons are a few pixels too close to the top edge of the window.

---

Here's the titlebar XUL again:

vbox#titlebar
  hbox#titlebar-content
    spacer#titlebar-spacer
    hbox#titlebar-buttonbox-container
      hbox#titlebar-buttonbox
    hbox#titlebar-secondary-buttonbox

If you set the height of #titlebar-buttonbox-container to 22px (the default titlebar height on OS X since forever), and set its -moz-box-align to center, then you get the desired result in this scenario, but that might not be the best fix.
If the titlebar is hidden, then we shouldn't set those properties because the current appearance is already correct in that case.
And changing the heights of these boxes also affects the private browsing indicator. See also bug 1275653.

Comment 1

[Andrew Martin McDonough (:mmcdonough)]

This looks extremely similar to Bug 1168556

Comment 2

[:Gijs (he/him)]

Markus, you suggested I look at this in bug 1216245. So I tried to inspect these buttons in a normal window with the titlebar enabled - but it seems the builtin browser devtools have no idea about the positioning of these buttons at all. All of them are 0-sized, as is the button-box, and so I'm a bit stumped as to how to even attack this. Is it possible we're not showing these based on XUL in a normal window, when we're not putting tabs in the titlebar and a lwtheme is also not applied? I remember there's some awkwardness here where mac is very special because of both lwthemes and the private browsing indicator, and we end up drawing in the titlebar even when tabs aren't in the titlebar, or something. Is that what's going wrong here? If so, I don't think the positioning mentioned would affect non-private windows - but it might affect windows when tabsintitlebar is on. We could select for that, of course... Mike, what do you think? Do you remember how this is "supposed" to work?

Comment 3

[Markus Stange [:mstange]]

(In reply to :Gijs Kruitbosch from comment #2)
> Markus, you suggested I look at this in bug 1216245. So I tried to inspect
> these buttons in a normal window with the titlebar enabled - but it seems
> the builtin browser devtools have no idea about the positioning of these
> buttons at all. All of them are 0-sized, as is the button-box, and so I'm a
> bit stumped as to how to even attack this.

I haven't tried the regular devtools; I still use the DOM Inspector for inspecting chrome. (It still works in e10s windows if you open it from a non-e10s tab, like about:config.) And the DOM Inspector showed proper rectangles for all of these elements.

> Is it possible we're not showing
> these based on XUL in a normal window, when we're not putting tabs in the
> titlebar and a lwtheme is also not applied?

We're showing them based on XUL whenever the chromemargin attribute is set on the window, i.e. whenever we're drawing in the title bar. We do that if any of these is true: if we have tabsintitlebar, or if a lightweight theme is used, or if we're in private browsing mode.

Comment 4

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #3)
> We're showing them based on XUL whenever the chromemargin attribute is set
> on the window, i.e. whenever we're drawing in the title bar. We do that if
> any of these is true: if we have tabsintitlebar, or if a lightweight theme
> is used, or if we're in private browsing mode.

OK, but that makes sense then, it means we're not using the position of the buttons to draw in 'normal' mode, and that also explains why they show up as hidden in the builtin devtools, on a window like that. They show up normally on the private window.

AFAICT the titlebar is already set to 22px, so maybe the moz-box-align: center will be enough to fix this if we specialcase it for the non-tabsintitlebar (non-lwtheme, see below) case?

Also... from just trying this out, there's something funny going on here. Try this:

0. start with a clean profile
1. open customize mode
2. set a lwtheme
3. turn on the title bar

Now the titlebar is the default grey background.

Alternatively:
0. clean profile
1. open customize mode
2. turn on the title bar first
3. turn on a lwtheme

Now the titlebar participates in the lwtheme, and on dark themes the titlebar text is illegible. :-\

Is that a(n orthogonal) cocoa issue or something?

Comment 5

[Markus Stange [:mstange]]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Markus Stange [:mstange] from comment #3)
> > We're showing them based on XUL whenever the chromemargin attribute is set
> > on the window, i.e. whenever we're drawing in the title bar. We do that if
> > any of these is true: if we have tabsintitlebar, or if a lightweight theme
> > is used, or if we're in private browsing mode.
> 
> OK, but that makes sense then, it means we're not using the position of the
> buttons to draw in 'normal' mode, and that also explains why they show up as
> hidden in the builtin devtools, on a window like that. They show up normally
> on the private window.

OK, good.

> AFAICT the titlebar is already set to 22px, so maybe the moz-box-align:
> center will be enough to fix this if we specialcase it for the
> non-tabsintitlebar (non-lwtheme, see below) case?

That actually sounds good.

> Also... from just trying this out, there's something funny going on here.
> Try this:
> 
> 0. start with a clean profile
> 1. open customize mode
> 2. set a lwtheme
> 3. turn on the title bar
> 
> Now the titlebar is the default grey background.

Uh oh. Looks like frontend JS gets confused and doesn't set the chromemargin attribute when it should. Luckily we have
#main-window:not([chromemargin]) > #titlebar { display: none; }
so the content at least doesn't get pushed down by the 22px high #titlebar.

> Alternatively:
> 0. clean profile
> 1. open customize mode
> 2. turn on the title bar first
> 3. turn on a lwtheme
> 
> Now the titlebar participates in the lwtheme,

This is the expected behavior.

> and on dark themes the
> titlebar text is illegible. :-\

And this is bug 530103 :-(

I'd prefer somebody to put the title text into the XUL and display that. The window doesn't expose an API to change the color of the native title text.

Comment 6

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1275655 - Put the window buttons in the right place when we don't have tabsintitlebar. r?Gijs

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

Comment 7

[Markus Stange [:mstange]]

Ok, this was easier than expected. I didn't mess with -moz-box-align at all, I just reused the solution for lwthemes. The :not([tabsintitlebar]) part is really what determines where we need to place the buttons.

Comment 8

[:Gijs (he/him)]

Attachment: 1px out

When I apply that patch they seem to still be 1px too low.

Worse, when I try to change that rule to be 2px, nothing seems to change. However, altering them *does* seem to influence what happens when a lwtheme is applied. :-\

Ideas? Am I just missing something?

Comment 9

[:Gijs (he/him)]

If I add -moz-box-align: center to #titlebar-content and decrement the margin to 2px, the private browsing window looks perfect. But then, if I apply a lightweight theme, the normal window shifts up by 1px. :-\

Comment 10

[:Gijs (he/him)]

Comment on attachment 8757020 [details]
MozReview Request: Bug 1275655 - Put the window buttons in the right place when we don't have tabsintitlebar. r?Gijs

https://reviewboard.mozilla.org/r/55530/#review52424

Clearing review for now based on my last few comments.

Comment 11

[Markus Stange [:mstange]]

I don't see the misalignment that you're seeing. What are your toolbar settings? I don't recognize the rounded white box in your screenshots, and I don't see any tabs in the screenshot either.

Comment 12

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #11)
> I don't see the misalignment that you're seeing. What are your toolbar
> settings?

Nothing out of the ordinary that I'm aware of. I enabled the titlebar, I don't think anything else would matter on OS X? Am I missing something?

> I don't recognize the rounded white box in your screenshots, and I
> don't see any tabs in the screenshot either.

It's a selected tab, assuming we're talking about the same thing.

Comment 13

[Markus Stange [:mstange]]

Oops, yes, that is indeed a tab.

Hmm, I still can't reproduce this. The buttons definitely are aligned correctly in my local build. Maybe my tree is too old. I'll re-request review after my next pull from mozilla-central if I don't see the issue by then.

Comment 14

[Andrew Martin McDonough (:mmcdonough)]

Have you disabled tabs in titlebar?

Comment 15

[Markus Stange [:mstange]]

Both of us have, otherwise the buttons would be a lot lower.

Comment 16

[:Gijs (he/him)]

These buttons got moved about a bit in photon, is there anything left to do here or can we close this out?

Comment 17

[Markus Stange [:mstange]]

Let's close it out.