973694 - [OSX] Private browsing windows without tabsintitlebar show the indicator in the wrong place and don't have the right theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[(no longer active)]

Attachment: Screenshot

Since bug 940093 made it so that we ship this UI, we should find a way to fix this I guess.

Comment 1

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

I might regret this, but assigning to self...

Comment 2

[Stephen Horlander [:shorlander]]

Attachment: privatebrowsing-mask.png

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: [e-mail]

Comment 4

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

Attachment: WIP Patch 1

I only got little part of the way before I was suck(er)ed into doing other P3's.

Comment 5

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

Attachment: WIP Patch 2

I think I might be onto something here with this patch. Seems to work for the matrix of modes (tabsintitlebar, customize mode, private browsing, lightweight theme, popup window). Doesn't yet integrate shorlander's new assets.

Comment 6

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

Comment on attachment 8396798 [details] [diff] [review]
WIP Patch 2

Crazy?

Comment 7

[:Gijs (out for now; he/him)]

Comment on attachment 8396798 [details] [diff] [review]
WIP Patch 2

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

This looks OK to me. Small note below. We should really fix our positioning of that mask, though. Can we at least just shove it 5px to the left or something? :-)

::: browser/themes/osx/browser.css
@@ +4242,5 @@
>    margin-bottom: 11px;
>  }
>  
>  #main-window[tabsintitlebar][customize-entered] > #titlebar > #titlebar-content,
> +#main-window:not([tabsintitlebar]):not(:-moz-lwtheme):not([privatebrowsingmode=temporary]) > #titlebar > #titlebar-content {

I suspect that because you now display: none this, this half of the selector can go away (the [tabsintitlebar][customize-entered] one will need to stay, however)

Comment 8

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

Attachment: WIP Patch 3

I went crazy and lobotomized a lot of our special-case titlebar code for OS X.

I _believe_ this simplifies things a great deal, since it boils the titlebar rules into two camps: tabsintitlebar, or no tabsintitlebar.

Let me give this a cursory self-review and then I'll try to explain what's going on.

Comment 9

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

Attachment: WIP Patch 4

I'm pretty certain this makes things more sane. Instead of a myriad of special-cases where we're mucking about with margins and padding, we have 2 cases only - tabs in titlebar, or no tabs in titlebar.

It really should have been like this from the start.

I've added commentary in the code. Please ignore the changes to browser.xul and the private browsing stuff - that's not what I'm asking for feedback on - just the things I've added / removed that have to do with the #titlebar and #titlebar-content elements.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8398756 [details] [diff] [review]
WIP Patch 4

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

I ignored the PB stuff like you asked and I just have some notes about the calculations and defines. It does seem to be reducing the magic numbers. I guess you're planning on splitting the PB stuff into a separate patch?
Have you tested with popup windows? Any reason to believe it won't work? I know they caused some trouble for a while and was hard to reproduce.
I haven't tested the patch and I'd like to run the screenshot diff tool on it when it's ready for review.

::: browser/themes/osx/browser.css
@@ +8,5 @@
>  %filter substitution
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 30
> +%define windowButtonMarginTop 11px

Shouldn't this really be calculated in TabsInTitlebar code to actually be centered? The size (maybe only width) varies by OS X version. Having the expression here to understand how this is "vertical center" would be good.

@@ -8,5 @@
>  %filter substitution
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 30
> -%define spaceAboveTabbar 9px

We're using this for fullscreen in bug 878436. IIRC this number came from shorlander and/or his spec so we will need this in that bug.

@@ +9,5 @@
>  %define forwardTransitionLength 150ms
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 30
> +%define windowButtonMarginTop 11px
> +%define tabsInTitlebarTitlebarHeight 39px

Isn't this @tabHeight@ + the former @spaceAboveTabbar@ minus the 1px overlap onto the nav-bar? A define is being added in bug 946987 for the overlap.

@@ +10,5 @@
>  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
>  %define conditionalForwardWithUrlbarWidth 30
> +%define windowButtonMarginTop 11px
> +%define tabsInTitlebarTitlebarHeight 39px
> +%define noTabsInTitlebarTitlebarHeight 22px

Isn't this the default OS X titlebar height? Names that describe the meaning of the value, not where it's currently used, are more stable.

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8398756 [details] [diff] [review]
WIP Patch 4

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

Nice!

Comment 13

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

(In reply to Matthew N. [:MattN] from comment #10)
> Comment on attachment 8398756 [details] [diff] [review]
> WIP Patch 4
> 
> Review of attachment 8398756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I ignored the PB stuff like you asked and I just have some notes about the
> calculations and defines. It does seem to be reducing the magic numbers. I
> guess you're planning on splitting the PB stuff into a separate patch?

Yeah, if this is the route we want to follow, I'd want to land this clean-up stuff separately and then do the PB stuff.

> Have you tested with popup windows? Any reason to believe it won't work? I
> know they caused some trouble for a while and was hard to reproduce.

Yep, tested that case. It was hard to reproduce because it required an external monitor and a Retina display, but I'm convinced that this problem has not resurfaced with this patch.

> I haven't tested the patch and I'd like to run the screenshot diff tool on
> it when it's ready for review.

Works for me.

> 
> ::: browser/themes/osx/browser.css
> @@ +8,5 @@
> >  %filter substitution
> >  %define forwardTransitionLength 150ms
> >  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> >  %define conditionalForwardWithUrlbarWidth 30
> > +%define windowButtonMarginTop 11px
> 
> Shouldn't this really be calculated in TabsInTitlebar code to actually be
> centered? The size (maybe only width) varies by OS X version. Having the
> expression here to understand how this is "vertical center" would be good.

It would, yes. We originally had code to do this, but it was removed at some point during the fixing of bug 930094. A bug to address this was filed at bug 967917.

So hard-coding in this notion of "vertical center" is no worse than what we're currently doing.

> 
> @@ -8,5 @@
> >  %filter substitution
> >  %define forwardTransitionLength 150ms
> >  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> >  %define conditionalForwardWithUrlbarWidth 30
> > -%define spaceAboveTabbar 9px
> 
> We're using this for fullscreen in bug 878436. IIRC this number came from
> shorlander and/or his spec so we will need this in that bug.
> 

Ok, I'll leave it be.

> @@ +9,5 @@
> >  %define forwardTransitionLength 150ms
> >  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> >  %define conditionalForwardWithUrlbarWidth 30
> > +%define windowButtonMarginTop 11px
> > +%define tabsInTitlebarTitlebarHeight 39px
> 
> Isn't this @tabHeight@ + the former @spaceAboveTabbar@ minus the 1px overlap
> onto the nav-bar? A define is being added in bug 946987 for the overlap.
> 

Yes! That sounds right.

> @@ +10,5 @@
> >  %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> >  %define conditionalForwardWithUrlbarWidth 30
> > +%define windowButtonMarginTop 11px
> > +%define tabsInTitlebarTitlebarHeight 39px
> > +%define noTabsInTitlebarTitlebarHeight 22px
> 
> Isn't this the default OS X titlebar height? Names that describe the meaning
> of the value, not where it's currently used, are more stable.

Good idea.

Comment 14

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

Attachment: WIP Patch 5

Extracted the titlebar simplification work to bug 990218.

Comment 15

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

Attachment: Patch v1

Ok, I _think_ this covers it.

We've got support for:

1) Tabs in titlebar
2) Tabs not in titlebar
3) Fullscreen mode

What we're missing, however, is the shorter mask asset for when tabs are not in the titlebar. Still waiting on shorlander for that - so in the meantime, I'm just using the old mask.

Comment 16

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

Might have to make some adjustments for Lion - will do that next.

Comment 17

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

Just have to make some adjustments for RTL, and then I'll be ready for review.

Comment 18

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

Attachment: Patch v1.1

Ok, RTL is working. Tested on Snow Leopard and Mountain Lion.

Comment 19

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

Comment on attachment 8402378 [details] [diff] [review]
Patch v1.1

Over to MattN for review!

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8402378 [details] [diff] [review]
Patch v1.1

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

Point form:
* You probably want to combine the two commit messages.
* Thanks for the CSS comments :) 
* I'll need to do another pass once bug 990218 is finalized.
* I noticed this is fixing bug 986797 :)
* The main visual issue I noticed is when the system titlebar is showing and you enter customization mode, the border is no longer shown at the bottom of the titlebar (which is kinda weird so close to the tabs anyways) and the customization mode grid doesn't extend into the titlebar. Since we're drawing there for the PB indicator, we should be able to extend the grid.

::: browser/base/content/browser.css
@@ +231,5 @@
>    -moz-box-align: start;
>  }
>  %endif
>  
> +%ifndef XP_LINUX

Don't use XP_LINUX for theming, use the following to match the change to browser.xul:
#if !defined(MOZ_WIDGET_GTK) && !defined(MOZ_WIDGET_QT)

See bug 948946.

@@ +233,5 @@
>  %endif
>  
> +%ifndef XP_LINUX
> +#TabsToolbar > .private-browsing-indicator {
> +  -moz-box-ordinal-group: 1000;

Why didn't we already need this for Windows? I didn't try this out there.

::: browser/base/content/browser.xul
@@ +604,5 @@
>                       label="&closeTab.label;"
>                       cui-areatype="toolbar"
>                       tooltiptext="&closeTab.label;"/>
>  
> +#ifndef XP_LINUX

Ditto: #if !defined(MOZ_WIDGET_GTK) && !defined(MOZ_WIDGET_QT)

::: browser/themes/osx/browser.css
@@ +3168,5 @@
>  }
>  
>  #main-window:not([tabsintitlebar]) > #titlebar {
>    min-height: @nativeTitlebarHeight@;
> +  max-height: @nativeTitlebarHeight@;

I'm not sure about this but would setting "height" be enough instead of setting both min and max? I didn't check what is cacading.

Comment 21

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

Attachment: Patch v1.2

(In reply to Matthew N. [:MattN] from comment #20)
> Comment on attachment 8402378 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8402378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Point form:
> * You probably want to combine the two commit messages.

Done. :)

> * I noticed this is fixing bug 986797 :)

\o/

> * The main visual issue I noticed is when the system titlebar is showing and
> you enter customization mode, the border is no longer shown at the bottom of
> the titlebar (which is kinda weird so close to the tabs anyways) and the
> customization mode grid doesn't extend into the titlebar. Since we're
> drawing there for the PB indicator, we should be able to extend the grid.

Alright, well, I've changed the rules to apply the texture to the titlebar when in private browsing mode. Look better?

> 
> ::: browser/base/content/browser.css
> @@ +231,5 @@
> >    -moz-box-align: start;
> >  }
> >  %endif
> >  
> > +%ifndef XP_LINUX
> 
> Don't use XP_LINUX for theming, use the following to match the change to
> browser.xul:
> #if !defined(MOZ_WIDGET_GTK) && !defined(MOZ_WIDGET_QT)

Fixed.

> @@ +233,5 @@
> >  %endif
> >  
> > +%ifndef XP_LINUX
> > +#TabsToolbar > .private-browsing-indicator {
> > +  -moz-box-ordinal-group: 1000;
> 
> Why didn't we already need this for Windows? I didn't try this out there.

Because the ordinal attribute was set on the XUL node directly.

> 
> ::: browser/base/content/browser.xul
> @@ +604,5 @@
> >                       label="&closeTab.label;"
> >                       cui-areatype="toolbar"
> >                       tooltiptext="&closeTab.label;"/>
> >  
> > +#ifndef XP_LINUX
> 
> Ditto: #if !defined(MOZ_WIDGET_GTK) && !defined(MOZ_WIDGET_QT)
>

Fixed.
 
> ::: browser/themes/osx/browser.css
> @@ +3168,5 @@
> >  }
> >  
> >  #main-window:not([tabsintitlebar]) > #titlebar {
> >    min-height: @nativeTitlebarHeight@;
> > +  max-height: @nativeTitlebarHeight@;
> 
> I'm not sure about this but would setting "height" be enough instead of
> setting both min and max? I didn't check what is cacading.

That appears to work just as well! Thanks.

Comment 22

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

Attachment: Patch v1.3

Ok, tested on both Snow Leopard and Mountain Lion. Ready for you to take another crack at it.

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8404212 [details] [diff] [review]
Patch v1.3

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

One thing that looked a little weird is that the PB indicator doesn't move with the padding in customize mode. This looked especially out of place on 10.6 where it's right in the corner. I thought we used to move the window controls in with customize mode but I guess not anymore. I think this is actually okay given that the other buttons don't move.

I guess there is an inconsistency that we move the PB and fullscreen button left in customize mode only while fullscreen (at least on 10.6 but probably others since they are in the tabstrip).

Please rebase on top of the further changes from bug 990218 as there is now some bitrot.

r- for now since there are enough questions to answer.

::: browser/base/content/browser.xul
@@ +497,5 @@
>          <toolbarbutton class="titlebar-button" id="titlebar-close" command="cmd_closeWindow"/>
>        </hbox>
>      </hbox>
>  #ifdef XP_MACOSX
> +    <hbox id="titlebar-fullscreen-button-container">

This "fullscreen" bit of this id is a bit misleading since it's not referring to fullscreen mode and it doesn't just contain the fullscreen button.

Was this to avoid the caccading name change to the fullscreenButtonWidth variable and the placeholder class? I think something should be improved here but I'm not sure if that should be comments or renaming things. I'm not sure titlebar-button-container is a better name since it's confusingly similar to titlebar-buttonbox which IMO could have had a name relating to "caption buttons" or "window controls".

Any ideas?

::: browser/themes/osx/browser.css
@@ +57,5 @@
>    margin-left: 7px;
>  }
>  
>  @media (-moz-mac-lion-theme) {
> +  #main-window:not([privatebrowsingmode=temporary]) .titlebar-placeholder[type="fullscreen-button"],

Sorry, it's late but can you explain why this needs to change as I'm not seeing it?

@@ +75,5 @@
>  /**
>   * When we hide #titlebar-content, we display the native window buttons in
>   * their default locations.
>   */
> +#main-window:not([tabsintitlebar]):not([privatebrowsingmode=temporary]) > #titlebar > #titlebar-content {

If I'm not mistaken, this ruleset can be deleted since we only center the buttons when [tabsintitlebar].
Note to self: we didn't want display:none in PB mode since the indicator would then be hidden. (I was going to ask for this is a comment if we don't delete it).

@@ -3966,5 @@
>    }
> -  #main-window[inFullscreen][privatebrowsingmode=temporary] {
> -    /* Adjust by the full element height of #titlebar, since that element is
> -     * not displayed in native full-screen. */
> -    padding-top: @spaceAboveTabbar@;

I'm pretty sure this is what's fixing bug 986797. You could move it to that bug and have r=me if you want (assuming you test it).

@@ +4384,5 @@
> + * we want between the mask and items on either side is 7px, so we use 4px,
> + * since the other 3px is accounted for by the empty space on either side.
> + */
> +#titlebar-fullscreen-button-container > .private-browsing-indicator,
> +#TabsToolbar > .private-browsing-indicator {

If I'm counting correctly, isn't there only two elements that match .private-browsing-indicator on OS X? If so, can't these two properties be set above in the ".private-browsing-indicator" rule?

@@ +4402,5 @@
>  }
>  
>  @media (min-resolution: 2dppx) {
> +  #TabsToolbar > .private-browsing-indicator,
> +  #titlebar-fullscreen-button-container > .private-browsing-indicator {

Why are the IDs of the parents needed here but not for 1x? I think they're not needed.

@@ +4426,5 @@
> +    display: -moz-box;
> +  }
> +
> +  #titlebar-fullscreen-button:-moz-locale-dir(rtl) {
> +    -moz-box-ordinal-group: 0;

OK, so this is to keep the "native" fullscreen button in the corner of the window. Perhaps a comment would be good?

@@ +4430,5 @@
> +    -moz-box-ordinal-group: 0;
> +  }
> +}
> +
> +#TabsToolbar > .private-browsing-indicator:-moz-locale-dir(rtl) {

So, did we decide not to put the PB indicator on the opposite side of LTR anymore? We did before your patch but this rule is specifically preventing it. I think this is fine since, as the deleted comment says, OS X doesn't flip their window controls and this is positioned beside them. Since Ehsan, worked on this in the past and he made the old indicator flip, I think you should consult with him about this if you haven't already.

If we want to continue to flip on RTL, it's going to be a problem having the fullscreen button and the PB indicator in one container. I'd like the implementation that is simplest and I suspect this is that implementation.

Comment 24

[Matthew N. [:MattN]]

Attachment: MattN's additional cleanup and making the titlebar color the same in PB

Since I wanted to test some further cleanup, I wrote a patch. It seems now that we're not doing the background-image hack for PB, further cleanup is possible to make PB not look foreign with the flat grey background. My understanding is that it was only like that due to the background-image implementation so now we can have prettier PB windows. Note that this reverts some rule changes from your patch and also includes some of the fixes from comment 23. Feel free to incorporate these changes into your patch if you agree with them.

Comment 25

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

Comment on attachment 8405374 [details] [diff] [review]
MattN's additional cleanup and making the titlebar color the same in PB

Oh yes, I like this better. I had forgotten that the background of the window was having its colour set _because_ of the old background image hack.

I'm going to fold these changes into my patch, which I've just de-bitrotted. I'll see if I can address your other concerns today.

Comment 26

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

Attachment: Patch v1.4

De-bitrotted, with MattN's work folded in.

Comment 27

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

Spoke to ehsan - he said that the RTL behaviour is totally fine. He was a little concerned about losing the flat background colour, as that helps to distinguish the two modes from one another.

But consulted with shorlander, and he thinks we should go with this approach (a.k.a, use the normal window style gradient).

Looking at the rest of your concerns now...

Comment 28

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

Attachment: Patch v1.5

(In reply to Matthew N. [:MattN] from comment #23)
> 
> ::: browser/base/content/browser.xul
> @@ +497,5 @@
> >          <toolbarbutton class="titlebar-button" id="titlebar-close" command="cmd_closeWindow"/>
> >        </hbox>
> >      </hbox>
> >  #ifdef XP_MACOSX
> > +    <hbox id="titlebar-fullscreen-button-container">
> 
> This "fullscreen" bit of this id is a bit misleading since it's not
> referring to fullscreen mode and it doesn't just contain the fullscreen
> button.
> 
> Was this to avoid the caccading name change to the fullscreenButtonWidth
> variable and the placeholder class? I think something should be improved
> here but I'm not sure if that should be comments or renaming things. I'm not
> sure titlebar-button-container is a better name since it's confusingly
> similar to titlebar-buttonbox which IMO could have had a name relating to
> "caption buttons" or "window controls".
> 
> Any ideas?

Hm, ok. Ideas include:

* titlebar-right-side-buttons-container
* titlebar-other-buttons-container
* titlebar-extra-buttons-container

I can't say I care too too much. :)

> 
> ::: browser/themes/osx/browser.css
> @@ +57,5 @@
> >    margin-left: 7px;
> >  }
> >  
> >  @media (-moz-mac-lion-theme) {
> > +  #main-window:not([privatebrowsingmode=temporary]) .titlebar-placeholder[type="fullscreen-button"],
> 
> Sorry, it's late but can you explain why this needs to change as I'm not
> seeing it?

Yeah, this is a gross hack to make the 7px spacing work right between items in the tabs toolbar, the private browsing indicator, and the fullscreen button. On reflection, this is just gross.

I found a nicer solution, which I've included.


> 
> @@ +75,5 @@
> >  /**
> >   * When we hide #titlebar-content, we display the native window buttons in
> >   * their default locations.
> >   */
> > +#main-window:not([tabsintitlebar]):not([privatebrowsingmode=temporary]) > #titlebar > #titlebar-content {
> 
> If I'm not mistaken, this ruleset can be deleted since we only center the
> buttons when [tabsintitlebar].
> Note to self: we didn't want display:none in PB mode since the indicator
> would then be hidden. (I was going to ask for this is a comment if we don't
> delete it).
> 

Good call - I think we can delete this safely.

> @@ -3966,5 @@
> >    }
> > -  #main-window[inFullscreen][privatebrowsingmode=temporary] {
> > -    /* Adjust by the full element height of #titlebar, since that element is
> > -     * not displayed in native full-screen. */
> > -    padding-top: @spaceAboveTabbar@;
> 
> I'm pretty sure this is what's fixing bug 986797. You could move it to that
> bug and have r=me if you want (assuming you test it).
> 

Naw, I'd rather keep the momentum in this bug rather than splitting it out.

> @@ +4384,5 @@
> > + * we want between the mask and items on either side is 7px, so we use 4px,
> > + * since the other 3px is accounted for by the empty space on either side.
> > + */
> > +#titlebar-fullscreen-button-container > .private-browsing-indicator,
> > +#TabsToolbar > .private-browsing-indicator {
> 
> If I'm counting correctly, isn't there only two elements that match
> .private-browsing-indicator on OS X? If so, can't these two properties be
> set above in the ".private-browsing-indicator" rule?
> 

Absolutely! Good catch.

> @@ +4402,5 @@
> >  }
> >  
> >  @media (min-resolution: 2dppx) {
> > +  #TabsToolbar > .private-browsing-indicator,
> > +  #titlebar-fullscreen-button-container > .private-browsing-indicator {
> 
> Why are the IDs of the parents needed here but not for 1x? I think they're
> not needed.
> 

Yep, another good catch.

> @@ +4426,5 @@
> > +    display: -moz-box;
> > +  }
> > +
> > +  #titlebar-fullscreen-button:-moz-locale-dir(rtl) {
> > +    -moz-box-ordinal-group: 0;
> 
> OK, so this is to keep the "native" fullscreen button in the corner of the
> window. Perhaps a comment would be good?
> 

Sure, I'll add one.

> @@ +4430,5 @@
> > +    -moz-box-ordinal-group: 0;
> > +  }
> > +}
> > +
> > +#TabsToolbar > .private-browsing-indicator:-moz-locale-dir(rtl) {
> 
> So, did we decide not to put the PB indicator on the opposite side of LTR
> anymore? We did before your patch but this rule is specifically preventing
> it. I think this is fine since, as the deleted comment says, OS X doesn't
> flip their window controls and this is positioned beside them. Since Ehsan,
> worked on this in the past and he made the old indicator flip, I think you
> should consult with him about this if you haven't already.
> 
> If we want to continue to flip on RTL, it's going to be a problem having the
> fullscreen button and the PB indicator in one container. I'd like the
> implementation that is simplest and I suspect this is that implementation.

Spoke with ehsan, and he's cool with this approach. shorlander digs the standard window gradient instead of the flat window colour.

Comment 29

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

As for that ID, jaws suggested "titlebar-secondary-buttonbox", which I dig. I'll update the patch.

Comment 30

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

Attachment: Patch v1.6

Ok, updated us to that new ID, and renamed the references to it - including the one in browser.js (updated the size variable name while I was at it).

Comment 31

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

Comment on attachment 8405504 [details] [diff] [review]
Patch v1.6

Ok, let's kick them tires again.

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8405504 [details] [diff] [review]
Patch v1.6

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

Thank you very much for doing this. I'm liking the nicer PB look! Thanks for your patience, I just wanted to be thorough to reduce risk for something this visible at this stage of 29.

(In reply to Mike Conley (:mconley) from comment #28)
> (In reply to Matthew N. [:MattN] from comment #23)
> > This "fullscreen" bit of this id is a bit misleading since it's not
> > referring to fullscreen mode and it doesn't just contain the fullscreen
> > button.
> > 
> > Any ideas?

What this patch has for the ID is fine.

==

I confirmed that this caused no differences in non-PB mode on 10.6 with the following configurations:
  Tabs WindowSize Toolbars TabsInTitlebar LightweightThemes CustomizeMode
In 10.9 for the same configurations I noticed it fixed 989604 but otherwise is the same.
I didn't see any issue in the 10.6 or 10.9 PB screenshots.

A reminder to file the bug for the asset swap from shorlander (if you're still expecting one).

::: browser/themes/osx/browser.css
@@ +63,2 @@
>      margin-right: 7px;
> +    margin-left: 7px;

I believe the change from this version is now fixing bug 989604 so this is (at least) a triple-whammy! (That's fixed for non-PB too btw.)

@@ +69,5 @@
>    -moz-appearance: -moz-window-titlebar;
>  }
>  
>  #main-window:not([tabsintitlebar]) > #titlebar {
> +  height: @nativeTitlebarHeight@;

FYI: This is the only use of the define so it could be inlined with a comment indicating it is the native titlebar height. I'll leave it up to you probably depending on whether you think anything else will need this in the future.

@@ +4425,5 @@
> +    display: -moz-box;
> +  }
> +
> +  #titlebar-fullscreen-button:-moz-locale-dir(rtl) {
> +    -moz-box-ordinal-group: 0;

> Sure, I'll add one.

I think you forget this and I think it still stands out as non-obvious.

I actually think we should instead just set @dir="ltr" on #titlebar-secondary-buttonbox (optionally with a comment reminding us that the window controls don't flip on OS X but I think most of us know that now and it's easy to notice if one tests RTL).

Comment 33

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

Attachment: Patch v1.7 (r+'d by MattN)

(In reply to Matthew N. [:MattN] from comment #32)
> Comment on attachment 8405504 [details] [diff] [review]
> Patch v1.6
> 
> Review of attachment 8405504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you very much for doing this. I'm liking the nicer PB look! Thanks for
> your patience, I just wanted to be thorough to reduce risk for something
> this visible at this stage of 29.

I totally get it, and agree whole-heartedly - and thank you for the thorough reviews!

> 
> (In reply to Mike Conley (:mconley) from comment #28)
> > (In reply to Matthew N. [:MattN] from comment #23)
> > > This "fullscreen" bit of this id is a bit misleading since it's not
> > > referring to fullscreen mode and it doesn't just contain the fullscreen
> > > button.
> > > 
> > > Any ideas?
> 
> What this patch has for the ID is fine.
> 

\o/

> ==
> 
> I confirmed that this caused no differences in non-PB mode on 10.6 with the
> following configurations:
>   Tabs WindowSize Toolbars TabsInTitlebar LightweightThemes CustomizeMode
> In 10.9 for the same configurations I noticed it fixed 989604 but otherwise
> is the same.
> I didn't see any issue in the 10.6 or 10.9 PB screenshots.

\o/

> 
> A reminder to file the bug for the asset swap from shorlander (if you're
> still expecting one).

Will file after I land this.

> 
> ::: browser/themes/osx/browser.css
> @@ +63,2 @@
> >      margin-right: 7px;
> > +    margin-left: 7px;
> 
> I believe the change from this version is now fixing bug 989604 so this is
> (at least) a triple-whammy! (That's fixed for non-PB too btw.)

\o/

> 
> @@ +69,5 @@
> >    -moz-appearance: -moz-window-titlebar;
> >  }
> >  
> >  #main-window:not([tabsintitlebar]) > #titlebar {
> > +  height: @nativeTitlebarHeight@;
> 
> FYI: This is the only use of the define so it could be inlined with a
> comment indicating it is the native titlebar height. I'll leave it up to you
> probably depending on whether you think anything else will need this in the
> future.
> 

Ok, removed the define, just used an inline comment to explain it.

> @@ +4425,5 @@
> > +    display: -moz-box;
> > +  }
> > +
> > +  #titlebar-fullscreen-button:-moz-locale-dir(rtl) {
> > +    -moz-box-ordinal-group: 0;
> 
> > Sure, I'll add one.
> 
> I think you forget this and I think it still stands out as non-obvious.
> 
> I actually think we should instead just set @dir="ltr" on
> #titlebar-secondary-buttonbox (optionally with a comment reminding us that
> the window controls don't flip on OS X but I think most of us know that now
> and it's easy to notice if one tests RTL).

Used dir=ltr and included a comment, and tested locally to confirm that it still works as expected.

Woo!

Comment 34

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

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

Comment 35

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

Filed bug 995617 for the new asset.

Comment 36

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

Comment on attachment 8405767 [details] [diff] [review]
Patch v1.7 (r+'d by MattN)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis.


User impact if declined: 

An uglier UI when in private browsing mode with tabs not in titlebar.


Testing completed (on m-c, etc.): 

Plenty of local testing in a variety of configurations on both OS X Snow Leopard and Mountain Lion, plus a very thorough review by MattN which used his screenshot tool to test a bunch of different window states.


Risk to taking this patch (and alternatives if risky): 

Low, given the thorough review, and lengthy local testing.


String or IDL/UUID changes made by this patch:

None.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d20804c31f61
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1a71019512d

Comment 39

[Mehmet]

Attachment: expected_vs_actual.png

Thanks for fixing it.

Just only one question: Is the the white line on top intentional? Please see the attached screencast. Thanks.

Comment 41

[Ioana (away)]

Attachment: PB icons in a buggy build + current builds.png

The icons are now larger, the mask is whiter and they have the lighter line mentioned in the above comment. Can someone please confirm that this is ok, given the line doesn't appear in the icon attachment?

Comment 42

[Ioana (away)]

Tested on Mac OS X 10.8.5 using Firefox 29.0b8 and the 04/15 Firefox 30.0a2 and 31.0a1.

Comment 43

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

(In reply to Ioana Budnar, QA [:ioana] from comment #41)
> Created attachment 8407414 [details]
> PB icons in a buggy build + current builds.png
> 
> The icons are now larger, the mask is whiter and they have the lighter line
> mentioned in the above comment. Can someone please confirm that this is ok,
> given the line doesn't appear in the icon attachment?

Yes, I'm going to say this is fine. The white line is indeed a bug, but not one that I'd back this out for.

Comment 44

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as Verified based on comment 41, comment 42 and comment 43.