749394 - Desktop theme changes for per-window Private Browsing (Windows and OS X)

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[(no longer active)]

We need to come up with a set of theme changes that we'd like to have for per-window Private Browsing.  shorlander has designs with a theme which is darkened but that might be too much work (because it looks like we're going to need one additional theme per platform).  Boriss suggested maybe we can just get away by using a persona.  (But I hear personas are extremely expensive to load).

We need to come up with designs here, and plans to implement them.

Comment 1

[aardmaat]

isn't the current private browsing ui (with the purple button) enough?

Comment 2

[Josh Matthews [:jdm]]

No, since the button isn't cross-platform.

Comment 3

[aardmaat]

ah
than maybe we could make the awesome bar light purple or display that little mask we use for private browsing in it between the url and the place where now the favicon still is?
some people might not like the changing of persona's, and adding a new complete theme seems to be a little bit overkill

Comment 4

[u428464]

The venetian mask could provide a clear indication (Chrome does it with the guy with the hat in the left corner).

Comment 5

[(no longer active)]

Hmm, yeah, the idea of using the purple mask is good, I think, but I don't know where we would use it.  I don't personally like the way that Chrome incognito icon is positioned, but I will leave this to the UX folks to figure out.  :-)

Comment 6

[aardmaat]

well, maybe the mask could just go left of the url in the urlbar, or we could make the about:privatebrowsing an awesome bar-less apptab (like about:addons) so the favicon of it (the mask) indicates the private browsing session and leads to more information about private browsing

Comment 7

[u428464]

As info in the latest Chromium builds the color of the private browsing indicator (the incognito guy) has been changed to be less noticeable (grey instead of bright white).

Comment 8

[(no longer active)]

Dao, one question here.  If we want to implement some of a theme darkening along the lines of http://people.mozilla.com/~shorlander/private-browsing-mode/mockups/australis-pbm.png (pre-Australis), can you please give me an idea how much work that's going to be?  Thanks!

Comment 9

[Dão Gottwald [:dao]]

You could probably apply a built-in dark persona, which wouldn't be a lot of work.

Comment 10

[(no longer active)]

(In reply to comment #9)
> You could probably apply a built-in dark persona, which wouldn't be a lot of
> work.

True, but that would mean bad performance.  Would it be possible for us to avoid using a persona?

Comment 11

[Dão Gottwald [:dao]]

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> (In reply to comment #9)
> > You could probably apply a built-in dark persona, which wouldn't be a lot of
> > work.
> 
> True, but that would mean bad performance.

What makes you think that?

> Would it be possible for us to avoid using a persona?

Anything is possible. It's a lot more work.

Comment 12

[(no longer active)]

(In reply to comment #11)
> (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > (In reply to comment #9)
> > > You could probably apply a built-in dark persona, which wouldn't be a lot of
> > > work.
> > 
> > True, but that would mean bad performance.
> 
> What makes you think that?

Bug 650968, but I just saw that has been fixed...  So maybe this is actually an option now.

> > Would it be possible for us to avoid using a persona?
> 
> Anything is possible. It's a lot more work.

Understood!  Thanks.

Comment 13

[Josh Matthews [:jdm]]

Hmm, will we need to build some machinery to apply per-window personas?

Comment 14

[(no longer active)]

(In reply to comment #13)
> Hmm, will we need to build some machinery to apply per-window personas?

We already have that! <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7088>  We can change the theme based on the :root[privatebrowsingmode=temporary] CSS selector.

Comment 15

[Dão Gottwald [:dao]]

The best way to implement this is the LightweightThemeConsumer constructor. The theme data could be serialized in a pref.

Comment 16

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #15)
> The best way to implement this is the LightweightThemeConsumer constructor.
> The theme data could be serialized in a pref.

That is a great idea!  Thanks.

Comment 17

[Caspy7]

Just a thought that we might want to keep in mind how we will communicate to Windows 8 users when Firefox is or is not in PB mode. Ultimately we'd like some consistency of message visually between the desktop & Metro interface.

From what I gather glancing at bug 771188, Firefox for Metro is basically one window so it would be all or nothing when it comes to PB.

Comment 18

[(no longer active)]

Dao, given the mock-ups that I received from shorlander in bug 729865, how much work would you estimate implementing the Windows 7 theme would be?  I'd like to know if it's going to be feasible in the short term in your opinion, and if it's not, figure out how to convince the shorlander to go with a Persona.  :-)

Comment 19

[Josh Matthews [:jdm]]

Attachment: Add the ability to theme private browsing windows on a per-product basis. Add a terrible, terrible theme to Firefox as a demonstration.

Comment 20

[(no longer active)]

Comment on attachment 684777 [details] [diff] [review]
Add the ability to theme private browsing windows on a per-product basis. Add a terrible, terrible theme to Firefox as a demonstration.

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

The code change looks good to me.  The image, not so much.  ;-)

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 684777 [details] [diff] [review]
Add the ability to theme private browsing windows on a per-product basis. Add a terrible, terrible theme to Firefox as a demonstration.

>+// The serialized lightweight theme settings for private browsing windows
>+pref("browser.privatebrowsing.theme", "{\"headerURL\":\"chrome://browser/content/pbheader.png\",\"footerURL\":\"chrome://browser/content/pbfooter.png\"}");

You also need to specify the textcolor (e.g. #fff) and accentcolor (e.g. #000) properties.

>+    content/browser/pbheader.png                          (content/pbheader.png)
>+    content/browser/pbfooter.png                          (content/pbfooter.png) 

nit: rename to privateBrowsingHeader, privateBrowsingFooter. remove trailing space.

>+  if (PrivateBrowsingUtils.isWindowPrivate(this._win)) {
>+    let serialized = Services.prefs.getCharPref('browser.privatebrowsing.theme');

nit: use double quotes

You need to try/catch here, otherwise you're breaking applications that don't have the pref set.

>+    if (serialized != '') {

if (serialized)

>+  if (!theme) {
>+    var temp = {};

nit: use let

Comment 22

[(no longer active)]

Note to self: colors used on mobile:

ibarlow
ehsan: are you looking for the title bar background, or the url field background?
ibarlow
title bar is top: #222629 bottom #25292d
ibarlow
er, url bar i mean
ibarlow
and title bar is top: #53575c and bottom #3b4045

Comment 23

[(no longer active)]

Attachment: Patch (v2)

I'll create try server builds for ui-review.

Comment 24

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=50f393456b98

Comment 25

[Mozilla RelEng Bot]

Try run for 50f393456b98 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=50f393456b98
Results (out of 3 total builds):
    success: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-50f393456b98

Comment 26

[(no longer active)]

Windows, Mac and Linux builds available here for ui-review: <http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-50f393456b98/>

Comment 27

[Caspy7]

Attachment: Screenshot comparing private & non-private windows.

Dunno if I should be posting this here or in bug 729865 ...

Using the above build I installed the dark persona I normally use (Gloss. Black). I then opened a new private browsing window. 
See accompanying attachment for comparison screenshot.
Can you tell the difference between the two? I can't.

I think we should revisit the discussion from earlier in this bug about using a consistent graphical indicator (I encourage you to read comments 0-7).
Whether you're aesthetically fond of the indicator in the upper left of Chrome's incognito window, you always know exactly which type of window you're using.
When you open a PB window, you sacrifice your theme customization for the PB functionality. I think people intuitively get this. I don't think we should hesitate to add an I-know-I'm-private-browsing graphic.

Tab-less windows (such as a "popped out" gmail compose window) suffer the the same problem I've illustrated even more acutely as it looks like it doesn't matter what platform you're on or what theme you're using, Private & non-private windows are indiscernible.

Placing a graphic in the location bar to the left of the URL icon would seem to solve the issue in all these situations.

I also feel compelled to point out that we allow users to remove *every* UI component (as well as the entire navigation toolbar itself) leaving just tabs.  The solution to this issue would seem to be doing what Chrome does which is to insert a global graphic in the upper left of everything.

Comment 28

[u428464]

Some comments about current private browsing UI : 
According to the Windows non-Australis mockup the URL bar and the search bar should be blackened. Do we still want this ?

The Firefox button should also show a mask next to "Firefox". It's a good indicator.

Australis tabstrip is also almost ready to land. Coordination is needed with Matthew N.

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 687567 [details] [diff] [review]
Patch (v2)

The footer image is just one pixel wide and not repeated, so it's basically invisible. I think you can just drop it.

The header image seems unreasonably large. At the very least, you should convert it to a grayscale PNG. Is the dithering intended?

Comment 30

[(no longer active)]

Attachment: New mock-up on OS X

Looks like we're not going to do the persona thing...  New mock-ups from Stephen.

Comment 31

[(no longer active)]

Attachment: New mock-up on Linux

Comment 32

[u428464]

The Google Chrome approach is nice, but on these mockups the indication is way too subtle.

Comment 33

[u428464]

Any new decisions made on this ? (I assume per-window private browsing is very close to land)

Comment 34

[Josh Matthews [:jdm]]

Like a sucker I have agreed to take this on.

Comment 35

[(no longer active)]

(In reply to Guillaume C. [:ge3k0s] from comment #33)
> Any new decisions made on this ? (I assume per-window private browsing is
> very close to land)

See the mock-ups that I attached here?

Comment 36

[Caspy7]

(In reply to Josh Matthews [:jdm] from comment #34)
> Like a sucker I have agreed to take this on.

Ha. :)

(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> Created attachment 687971 [details]
> New mock-up on OS X
> 
> Looks like we're not going to do the persona thing...  New mock-ups from
> Stephen.

Does this mean we're not changing the overall look of the UI?
I think the dark persona plus a global graphic makes good sense.  The persona serves to differentiate the window types to users generally.  And the graphic takes care of the edge case of a default dark persona and chromeless windows.

Comment 37

[u428464]

(In reply to Ehsan Akhgari [:ehsan] from comment #35)
> (In reply to Guillaume C. [:ge3k0s] from comment #33)
> > Any new decisions made on this ? (I assume per-window private browsing is
> > very close to land)
> 
> See the mock-ups that I attached here?

I was asking, because there is no Windows mockup attached and in the last planning meeting notes (https://wiki.mozilla.org/Firefox/Planning/2012-12-05) a link to mockups was given (http://shorlander.dropmark.com/94052) and it still include the stealth version of private browsing windows.

What seem to be the most recent mockups show an indicator that isn't very noticeable.

Comment 38

[Stephen Horlander [:shorlander]]

(In reply to Mark S. from comment #36)
> Does this mean we're not changing the overall look of the UI?
> I think the dark persona plus a global graphic makes good sense.  The
> persona serves to differentiate the window types to users generally.  And
> the graphic takes care of the edge case of a default dark persona and
> chromeless windows.

Long term I think we definitely need something comprehensive like the full UI stealth mode. However using a persona is not the right approach in its current state. Short-term we can use a per window indicator.

Comment 39

[Josh Matthews [:jdm]]

Attachment: Add private browsing icon to private windows.

WIP. This adds an icon in the tab bar on all platforms; I haven't looked into separate behaviour for windows yet.

Comment 40

[Josh Matthews [:jdm]]

Attachment: Add private browsing icon to private windows.

Comment 41

[Josh Matthews [:jdm]]

Builds for all platforms demonstrating this theme change should be available at https://tbpl.mozilla.org/?tree=Try&rev=67ada274dab5 in the next couple hours.

Comment 42

[Josh Matthews [:jdm]]

Sorry, https://tbpl.mozilla.org/?tree=Try&rev=40b948c5228d.

Comment 43

[(no longer active)]

Hmm, I don't see any theme changes on Mac.

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 691608 [details] [diff] [review]
Add private browsing icon to private windows.

The styling belongs entirely in browser/themes/.

Comment 45

[Josh Matthews [:jdm]]

Attachment: Add private browsing icon to private windows.

This works for Linux and OS X 10.6, and might work for 10.7 as well. I need to boot up Windows next.

Comment 46

[Josh Matthews [:jdm]]

Attachment: Add private browsing icon to private windows

Comment 47

[Josh Matthews [:jdm]]

Attachment: Add a private browsing indicator to private windows.

Comment 48

[Josh Matthews [:jdm]]

Attachment: 10.6 theme

Comment 49

[Josh Matthews [:jdm]]

Attachment: Linux theme with menu bar

Comment 50

[Josh Matthews [:jdm]]

Attachment: Linux theme without menu bar

Comment 51

[Josh Matthews [:jdm]]

Attachment: Add a private browsing indicator to private windows.

Comment 52

[Josh Matthews [:jdm]]

Attachment: Add a private browsing indicator to private windows.

Comment 53

[Josh Matthews [:jdm]]

Attachment: Windows 7 theme with appmenu

Comment 54

[Josh Matthews [:jdm]]

Attachment: Windows 7 theme with menu bar

Comment 55

[Josh Matthews [:jdm]]

Attachment: Windows XP

Comment 56

[Josh Matthews [:jdm]]

Attachment: Add a private browsing indicator to private windows.

Comment 57

[Josh Matthews [:jdm]]

Comment on attachment 694079 [details] [diff] [review]
Add a private browsing indicator to private windows.

Stephen says these look fine, so have at it Dão.

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 694079 [details] [diff] [review]
Add a private browsing indicator to private windows.

>+    document.documentElement.setAttribute("privatewindow", true);

This isn't needed. We already set the privatebrowsingmode attribute.

> <vbox id="titlebar">
>   <hbox id="titlebar-content">
>     <hbox id="appmenu-button-container">
>       <button id="appmenu-button"
>               type="menu"
>+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+#ifdef XP_WIN
>+              image="chrome://browser/content/privatebrowsing-light.png"
>+#endif
>+#endif

This belongs in browser/themes/. Use the list-style-image or background-image CSS properties.

> #ifdef APPMENU_ON_TABBAR
>       <toolbarbutton id="appmenu-toolbar-button"
>                      class="chromeclass-toolbar-additional"
>                      type="menu"
>                      label="&brandShortName;"
>+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+#ifdef XP_UNIX
>+                     image="chrome://browser/content/privatebrowsing-mask.png"
>+#endif
>+#endif
>                      tooltiptext="&appMenuButton.tooltip;">

ditto

>--- a/browser/components/privatebrowsing/jar.mn
>+++ b/browser/components/privatebrowsing/jar.mn
>@@ -1,6 +1,16 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> browser.jar:
> *   content/browser/aboutPrivateBrowsing.xhtml            (content/aboutPrivateBrowsing.xhtml) 
>+#ifdef XP_LINUX
>+    content/browser/privatebrowsing-mask.png              (content/pbw-mask-linux-01.png)
>+#endif
>+#ifdef XP_MACOSX
>+    content/browser/privatebrowsing-mask.png              (content/pbw-mask-osx-01.png)
>+#endif
>+#ifdef XP_WIN
>+    content/browser/privatebrowsing-light.png             (content/pbw-mask-windows-01.png)
>+    content/browser/privatebrowsing-dark.png              (content/pbw-mask-linux-01.png)
>+#endif

These files belong in browser/themes/.

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+%ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+#main-window[privatewindow] {
>+  background-image: url("chrome://browser/content/privatebrowsing-mask.png") !important;
>+  background-position: top right !important;
>+  background-repeat: no-repeat !important;
>+  -moz-appearance: none !important;
>+}
>+
>+#main-window[privatewindow] #toolbar-menubar {
>+  -moz-appearance: none !important;
>+}
>+%endif

Get rid of redundant !important flags.

Comment 59

[Josh Matthews [:jdm]]

Attachment: Add a private browsing indicator to private windows.

Comment 60

[(no longer active)]

Comment on attachment 694425 [details] [diff] [review]
Add a private browsing indicator to private windows.

This is ready for review now, and it's ui-r+'ed on IRC.

Comment 61

[Josh Matthews [:jdm]]

Dao, review ping?

Comment 62

[Dão Gottwald [:dao]]

Comment on attachment 694425 [details] [diff] [review]
Add a private browsing indicator to private windows.

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+#main-window[privatebrowsingmode] {

Shouldn't this be #main-window[privatebrowsingmode=temporary]?

>+  background-image: url("chrome://browser/skin/privatebrowsing-mask.png");
>+  background-position: top right;
>+  background-repeat: no-repeat;
>+  -moz-appearance: none;
>+}

Is this correct for RTL?

>+#main-window[privatebrowsingmode] #toolbar-menubar {
>+  -moz-appearance: none;
>+}

I see there's a stock Gnome screenshot. Does this also look reasonable with the default Ubuntu theme?

>+#main-window [privatebrowsingmode] #appmenu-button {

This selector is broken.

>\ No newline at end of file

Add newline.

>--- a/browser/themes/pinstripe/browser.css
>+++ b/browser/themes/pinstripe/browser.css

>+#main-window[privatebrowsingmode] {
>+  background-image: url("chrome://browser/skin/privatebrowsing-mask.png");
>+  background-position: top right;
>+  background-repeat: no-repeat;
>+  background-color: -moz-mac-chrome-active;
>+}
>+
>+@media (-moz-mac-lion-theme) {
>+  #main-window[privatebrowsingmode] {
>+    background-position: top right 40px;
>+  }
>+}

RTL?

>+#main-window[privatebrowsingmode] #navigator-toolbox[tabsontop="true"]::before {
>+    -moz-appearance: none;
>+}

Can we just add #main-window:not([privatebrowsingmode=temporary]) to the selector originally setting this style?

>\ No newline at end of file

Add newline.

>--- a/browser/themes/winstripe/browser-aero.css
>+++ b/browser/themes/winstripe/browser-aero.css
>@@ -453,8 +453,20 @@
>     background-image: -moz-linear-gradient(#f8f9f9, #eaeaea);
>     border-color: #d8d7d7;
>   }
> 
>   .splitmenu-menu[_moz-menuactive]:not(:hover):not([open]) {
>     background-image: none;
>   }
> }
>+
>+%ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+#main-window [privatebrowsingmode] {
>+  background-image: url("chrome://browser/skin/privatebrowsing-dark.png");
>+  background-position: top right;
>+  background-repeat: no-repeat;
>+}
>+
>+#main-window [privatebrowsingmode] #appmenu-button {
>+  list-style-image: url("chrome://browser/skin/privatebrowsing-light.png");
>+}
>+%endif
>\ No newline at end of file

This seems to duplicate the code from browser/themes/winstripe/browser.css, which is unnecessary.

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css

>+#main-window [privatebrowsingmode] {

Broken selector.

>+  background-image: url("chrome://browser/skin/privatebrowsing-dark.png");
>+  background-position: top right;
>+  background-repeat: no-repeat;
>+}

RTL?

>+#main-window [privatebrowsingmode] #appmenu-button {

Broken selector.

>\ No newline at end of file

Add newline.

Comment 63

[(no longer active)]

Regarding RTL, we correctly mirror the titlebar on Windows, so we should handle that here.  On Mac and Linux we don't do that, but on Mac the title is centered so we can put the icon on the left-hand side.  On Linux though, the title of the window remains on the left side so we can't really change anything.

Comment 64

[(no longer active)]

Attachment: Patch (v2)

Comment 65

[(no longer active)]

Attachment: Patch (v2)

Forgot to add the binary files to the patch.

Also, I'll attach a screenshot with Ubuntu's default theme in a few mins.

Comment 66

[(no longer active)]

Attachment: Ubuntu default theme screenshot

This looks terrible in Ubuntu default theme, I suspect because of the -moz-appearance: none rules.  Dao, do you know how this can be fixed?

Comment 67

[Dão Gottwald [:dao]]

Comment on attachment 698104 [details]
Ubuntu default theme screenshot

I don't understand this screenshot. Does it show a private window?

Comment 68

[u428464]

(In reply to Dão Gottwald [:dao] from comment #67)
> Comment on attachment 698104 [details]
> Ubuntu default theme screenshot
> 
> I don't understand this screenshot. Does it show a private window?

I think the wrong one has been posted. This is Firefox 12 on this pic.

Comment 69

[Dão Gottwald [:dao]]

Comment on attachment 698088 [details] [diff] [review]
Patch (v2)

> #navigator-toolbox[tabsontop="true"]:not(:-moz-lwtheme)::before {
>+  content: '';
>+  display: block;
>+  height: 25px;
>+  margin-bottom: -25px;
>+}
>+
>+#main-window:not([privatebrowsingmode=temporary]) #navigator-toolbox[tabsontop="true"]:not(:-moz-lwtheme)::before {
>   /* We want the titlebar to be unified, but we still want to be able
>    * to give #TabsToolbar a background. So we can't set -moz-appearance:
>    * toolbar on #TabsToolbar itself. Instead, we set it on a box of the
>    * right size which is put underneath #TabsToolbar.
>    */
>-  content: '';
>-  display: block;
>   -moz-appearance: toolbar;
>-  height: 25px;
>-  margin-bottom: -25px;
> }

Put all properties in the second rule, get rid of the first one.

Comment 70

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #67)
> Comment on attachment 698104 [details]
> Ubuntu default theme screenshot
> 
> I don't understand this screenshot. Does it show a private window?

Ah, looks like I attached the wrong screenshot.  The menu bar for private windows has a white background and the icon doesn't show up there...

Comment 71

[(no longer active)]

Attachment: Ubuntu default theme screenshot

Here is the correct screenshot.

Comment 72

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #71)
> Created attachment 698360 [details]
> Ubuntu default theme screenshot
> 
> Here is the correct screenshot.

So I got the icon to show up there properly but I still can't figure out what color I must use for the background.  I've tried -moz-dialog, Menu, Window, transparent, etc. and none of them seem to give out the correct color for the default Ubuntu theme.  Dao, Karl, do you know what the appropriate color to use would be?

Comment 73

[(no longer active)]

Attachment: Ubuntu default theme screenshot

I ended up implementing the -moz-menubar color which returns the correct menubar color on gtk.  Using that, here's what private windows look like in Ubuntu's default theme.

I'll attach the patches shortly.

Comment 74

[(no longer active)]

Attachment: Implement -moz-menubar

Comment 75

[:Gavin Sharp [email: [e-mail]]]

Can you put the moz-menubar patch in another bug?

Comment 76

[(no longer active)]

Attachment: Patch (v3)

Comment 77

[(no longer active)]

Comment on attachment 698379 [details] [diff] [review]
Implement -moz-menubar

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #75)
> Can you put the moz-menubar patch in another bug?

Sure.

Comment 78

[Dão Gottwald [:dao]]

Comment on attachment 698378 [details]
Ubuntu default theme screenshot

Unfortunately the colors don't quite match here. This isn't catastrophic, but eventually we should fix this, and if this means we need to put the icon differently there (e.g. as an <image>), then it's questionable whether we want bug 827075.

Comment 79

[(no longer active)]

Attachment: Ubuntu default theme screenshot

(In reply to Dão Gottwald [:dao] from comment #78)
> Comment on attachment 698378 [details]
> Ubuntu default theme screenshot
> 
> Unfortunately the colors don't quite match here. This isn't catastrophic,
> but eventually we should fix this, and if this means we need to put the icon
> differently there (e.g. as an <image>), then it's questionable whether we
> want bug 827075.

There's no color difference, that's just the result of taking the screenshot off of a VNC window, apparently.  Here's a screenshot taken directly from the Ubuntu desktop.

Comment 80

[(no longer active)]

Attachment: Patch (v4)

Fixed a bug in the appmenu button for the Linux theme.

Comment 81

[Dão Gottwald [:dao]]

Comment on attachment 698495 [details] [diff] [review]
Patch (v4)

- With the Firefox button displayed, the added mask icon increases the tab strip's height on Linux.
- With the menu bar hidden in favor of a global menu bar outside of the window (e.g. on stock Ubuntu), there's no visible indicator.

Comment 82

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #81)
> Comment on attachment 698495 [details] [diff] [review]
> Patch (v4)
> 
> - With the Firefox button displayed, the added mask icon increases the tab
> strip's height on Linux.

OK, I'll see what I can do to fix that.

> - With the menu bar hidden in favor of a global menu bar outside of the
> window (e.g. on stock Ubuntu), there's no visible indicator.

Can you please explain more how I can reproduce this?  I have no idea what that even means.  On Ubuntu, I get the menu bar similar to other linux distros.

Comment 83

[(no longer active)]

Attachment: Ubuntu default theme screenshot

This shows the vertical height of the tab-bar being fixed.

Comment 84

[(no longer active)]

CCing Chris to let him know about the changes here.

Comment 85

[Marco Castelluccio [:marco]]

(In reply to :Ehsan Akhgari from comment #82)
> Can you please explain more how I can reproduce this?  I have no idea what
> that even means.  On Ubuntu, I get the menu bar similar to other linux
> distros.

http://cloudfront.omgubuntu.co.uk/wp-content/uploads/2011/01/Screenshot-21.png

Comment 86

[(no longer active)]

Attachment: Patch (v5)

This fixes the tab-bar height problem, and also should fix the global menu issue, since we no longer stick the image under the menubar.  I tried to test it with the global menu bar extension (with mconley's help who explained to me what that thing is) but that repo seems to be a full-clone of the mozilla-central repo, and I don't know how to just build that extension for testing.

Dao, can we please take this patch and file a follow-up bug if it proves to break on the global bar Ubuntu extension?

Comment 87

[(no longer active)]

(In reply to Marco Castelluccio [:marco] from comment #85)
> (In reply to :Ehsan Akhgari from comment #82)
> > Can you please explain more how I can reproduce this?  I have no idea what
> > that even means.  On Ubuntu, I get the menu bar similar to other linux
> > distros.
> 
> http://cloudfront.omgubuntu.co.uk/wp-content/uploads/2011/01/Screenshot-21.
> png

Hmm, can you please tell me how to build this extension?

Comment 88

[Marco Castelluccio [:marco]]

(In reply to :Ehsan Akhgari from comment #87)
> Hmm, can you please tell me how to build this extension?

You could try to install the extension through the package manager and you'll find the code in /usr/lib/firefox-addons/extensions/[e-mail]

Comment 89

[Marco Castelluccio [:marco]]

Or you can download it from http://packages.ubuntu.com/search?keywords=firefox-globalmenu

Comment 90

[Dão Gottwald [:dao]]

Comment on attachment 698780 [details] [diff] [review]
Patch (v5)

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+#main-window[privatebrowsingmode=temporary] {
>+  background-image: url("chrome://browser/skin/privatebrowsing-mask.png");
>+  background-position: top right;
>+  background-repeat: no-repeat;
>+  background-color: -moz-menubar;
>+  -moz-appearance: none;
>+}

This background color on <window> has more impact than we want here. For instance, the separator between the history side bar and the content are picks up that color.

Also, I don't really see how this solves the problem with both the menu bar and the Firefox button hidden on Ubuntu. The background image would be hidden behind the tab bar, right?

Comment 91

[(no longer active)]

(In reply to Marco Castelluccio [:marco] from comment #89)
> Or you can download it from
> http://packages.ubuntu.com/search?keywords=firefox-globalmenu

FWIW I tried them both, and it seems like the extension is broken on trunk (even when I modify install.rdf to manually allow 20.0.* as the max version).  At least, it doesn't manage to put the menu bar in the global Ubuntu menu bar.

Comment 92

[(no longer active)]

Attachment: Windows and Mac theme

Got rid of the Linux theme so that we can land this sooner.  I'll move the Linux theme to another bug.

Comment 93

[(no longer active)]

Filed bug 827454 for the Linux theme part.

Comment 94

[Dão Gottwald [:dao]]

Comment on attachment 698802 [details] [diff] [review]
Windows and Mac theme

>+#main-window[privatebrowsingmode=temporary] {
>+  background-image: url("chrome://browser/skin/privatebrowsing-dark.png");
>+  background-position: top right;
>+  background-repeat: no-repeat;
>+}
>+
>+#main-window[privatebrowsingmode=temporary]:-moz-locale-dir(rtl) {
>+  background-position: top left;
>+}
>+
>+#main-window[privatebrowsingmode=temporary] #appmenu-button {
>+  list-style-image: url("chrome://browser/skin/privatebrowsing-light.png");
>+}

I don't really understand how this works on Windows. What hides privatebrowsing-dark.png when the Firefox button is visible?

Comment 95

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #94)
> Comment on attachment 698802 [details] [diff] [review]
> Windows and Linux theme
> 
> >+#main-window[privatebrowsingmode=temporary] {
> >+  background-image: url("chrome://browser/skin/privatebrowsing-dark.png");
> >+  background-position: top right;
> >+  background-repeat: no-repeat;
> >+}
> >+
> >+#main-window[privatebrowsingmode=temporary]:-moz-locale-dir(rtl) {
> >+  background-position: top left;
> >+}
> >+
> >+#main-window[privatebrowsingmode=temporary] #appmenu-button {
> >+  list-style-image: url("chrome://browser/skin/privatebrowsing-light.png");
> >+}
> 
> I don't really understand how this works on Windows. What hides
> privatebrowsing-dark.png when the Firefox button is visible?

That part of the patch was written by Josh.  Let me build on Windows and test it...

Comment 96

[(no longer active)]

Attachment: Patch (v7)

Comment 97

[(no longer active)]

The uplift already happened, so landed on inbound.  :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d44408e050e

Comment 98

[(no longer active)]

Comment on attachment 698827 [details] [diff] [review]
Patch (v7)

This is a front-end change for the theme for per-window PB on Windows and OS X.  We need it to ship per-window PB.  Should not be very risky.

Comment 99

[Alex Keybl [:akeybl]]

Comment on attachment 698827 [details] [diff] [review]
Patch (v7)

Approving for uplift, given how close we are after the merge.

Comment 100

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/222fa2543353

Comment 101

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0d44408e050e

Comment 102

[Stanley Chan]

Attachment: Titlebar and private theme changes look corrupted on private windows

I don't know if I should file another bug or put up my findings here; for maybe the past week, I noticed that the private browsing icon wasn't at the end like it was supposed to be, but near the center.

More recently, I noticed that the icon looks corrupted, and the titlebar would occasionally "mess up."

If it's important, I am running a retina MacBook Pro with a resolution of 1920 x 1200. What else can I do about this?

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130119 Firefox/21.0 ID:20130119030922

Comment 103

[Josh Matthews [:jdm]]

Stanley, thanks for reporting the problem. You're most likely seeing bug 831829 and bug 740923, so you probably don't need to file any new bugs right now.

Comment 104

[(no longer active)]

Lawrence, what do we want to relnote here?

Comment 105

[Lawrence Mandel [:lmandel] (use needinfo)]

No action needed. I was just flagging the bug for the relnote about per-window private browsing for the relman team.

Comment 106

[(no longer active)]

(In reply to comment #105)
> No action needed. I was just flagging the bug for the relnote about per-window
> private browsing for the relman team.

Yeah, that's my question.  Usually we relnote outstanding problems.  It's not clear to me what needs to be relnoted here.

Comment 107

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to :Ehsan Akhgari from comment #106)
> Yeah, that's my question.  Usually we relnote outstanding problems.  It's
> not clear to me what needs to be relnoted here.

We relnote notable new features, fixes, and outstanding problems. Here's an example
https://www.mozilla.org/en-US/firefox/19.0/releasenotes/

Comment 108

[(no longer active)]

(In reply to comment #107)
> (In reply to :Ehsan Akhgari from comment #106)
> > Yeah, that's my question.  Usually we relnote outstanding problems.  It's
> > not clear to me what needs to be relnoted here.
> 
> We relnote notable new features, fixes, and outstanding problems. Here's an
> example
> https://www.mozilla.org/en-US/firefox/19.0/releasenotes/

Oh I see, thanks!  In that case, I'd expect the relnote entry to mention "per-window private browsing", in which case perhaps the relnote bugzilla flag should be set on bug 463027 which is the main bug behind this feature, but as long as we're all on the same page, that doesn't matter a lot, I guess!

Comment 109

[Lawrence Mandel [:lmandel] (use needinfo)]

Yes, we are on the same page. :) IIRC, akeybl already had that bug flagged. I'll leave it to relman to set the flag on the bug that they prefer.

Comment 110

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0