Closed Bug 749394 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- verified
relnote-firefox --- 20+

People

(Reporter: ehsan, Assigned: ehsan)

References

(Depends on 1 open bug)

Details

Attachments

(11 files, 23 obsolete files)

3.66 MB, image/png
Details
3.66 MB, image/png
Details
81.09 KB, image/png
Details
68.58 KB, image/png
Details
68.86 KB, image/png
Details
87.08 KB, image/png
Details
165.24 KB, image/png
Details
29.11 KB, image/png
Details
22.49 KB, image/png
Details
14.09 KB, patch
dao
: review+
Details | Diff | Splinter Review
110.26 KB, image/png
Details
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.
isn't the current private browsing ui (with the purple button) enough?
No, since the button isn't cross-platform.
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
The venetian mask could provide a clear indication (Chrome does it with the guy with the hat in the left corner).
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.  :-)
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
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).
Blocks: 729865
Summary: Determine the required theme changes for per-window Private Browsing → Determine the required desktop theme changes for per-window Private Browsing
Blocks: fxPBnGen
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!
You could probably apply a built-in dark persona, which wouldn't be a lot of work.
(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?
(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.
(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.
Hmm, will we need to build some machinery to apply per-window personas?
(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.
The best way to implement this is the LightweightThemeConsumer constructor. The theme data could be serialized in a pref.
(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.
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.
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.  :-)
Attachment #684777 - Flags: review?(dao)
Attachment #684777 - Flags: review?(ehsan)
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.  ;-)
Attachment #684777 - Flags: review?(ehsan) → review+
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
Attachment #684777 - Flags: review?(dao) → review-
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
Assignee: nobody → ehsan
Attached patch Patch (v2) (obsolete) — Splinter Review
I'll create try server builds for ui-review.
Attachment #684777 - Attachment is obsolete: true
Attachment #687567 - Flags: ui-review?(madhava)
Attachment #687567 - Flags: review?(dao)
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/eakhgari@mozilla.com-50f393456b98
Windows, Mac and Linux builds available here for ui-review: <http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-50f393456b98/>
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.
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 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?
Attachment #687567 - Flags: review?(dao) → review-
Attached image New mock-up on OS X
Looks like we're not going to do the persona thing...  New mock-ups from Stephen.
Attachment #687567 - Attachment is obsolete: true
Attachment #687631 - Attachment is obsolete: true
Attachment #687567 - Flags: ui-review?(madhava)
Attached image New mock-up on Linux
The Google Chrome approach is nice, but on these mockups the indication is way too subtle.
Any new decisions made on this ? (I assume per-window private browsing is very close to land)
Like a sucker I have agreed to take this on.
Assignee: ehsan → josh
(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?
(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.
(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.
(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.
WIP. This adds an icon in the tab bar on all platforms; I haven't looked into separate behaviour for windows yet.
Attachment #689373 - Attachment is obsolete: true
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.
Hmm, I don't see any theme changes on Mac.
Comment on attachment 691608 [details] [diff] [review]
Add private browsing icon to private windows.

The styling belongs entirely in browser/themes/.
Attachment #691608 - Flags: review-
This works for Linux and OS X 10.6, and might work for 10.7 as well. I need to boot up Windows next.
Attachment #691608 - Attachment is obsolete: true
Attachment #692026 - Attachment is obsolete: true
Attachment #692482 - Attachment is obsolete: true
Attached image 10.6 theme
Attachment #692493 - Attachment is obsolete: true
Attachment #692505 - Attachment is obsolete: true
Attached image Windows XP
Attachment #693603 - Attachment is obsolete: true
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.
Attachment #694079 - Flags: review?(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.
Attachment #694079 - Flags: review?(dao) → review-
Attachment #694079 - Attachment is obsolete: true
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.
Attachment #694425 - Flags: review?(dao)
Dao, review ping?
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.
Attachment #694425 - Flags: review?(dao) → review-
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.
Attached patch Patch (v2) (obsolete) — Splinter Review
Assignee: josh → ehsan
Attachment #694425 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #698084 - Flags: review?(dao)
Attached patch Patch (v2) (obsolete) — Splinter Review
Forgot to add the binary files to the patch.

Also, I'll attach a screenshot with Ubuntu's default theme in a few mins.
Attachment #698084 - Attachment is obsolete: true
Attachment #698084 - Flags: review?(dao)
Attachment #698088 - Flags: review?(dao)
Attached image Ubuntu default theme screenshot (obsolete) —
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 on attachment 698104 [details]
Ubuntu default theme screenshot

I don't understand this screenshot. Does it show a private window?
(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 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.
(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...
Attached image Ubuntu default theme screenshot (obsolete) —
Here is the correct screenshot.
Attachment #698104 - Attachment is obsolete: true
(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?
Attached image Ubuntu default theme screenshot (obsolete) —
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.
Attachment #698360 - Attachment is obsolete: true
Attached patch Implement -moz-menubar (obsolete) — Splinter Review
Attachment #698379 - Flags: review?(roc)
Can you put the moz-menubar patch in another bug?
Attached patch Patch (v3) (obsolete) — Splinter Review
Attachment #698088 - Attachment is obsolete: true
Attachment #698088 - Flags: review?(dao)
Attachment #698380 - Flags: review?(dao)
Comment on attachment 698379 [details] [diff] [review]
Implement -moz-menubar

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #75)
> Can you put the moz-menubar patch in another bug?

Sure.
Attachment #698379 - Attachment is obsolete: true
Attachment #698379 - Flags: review?(roc)
Depends on: 827075
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.
Attached image Ubuntu default theme screenshot (obsolete) —
(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.
Attachment #698378 - Attachment is obsolete: true
Attached patch Patch (v4) (obsolete) — Splinter Review
Fixed a bug in the appmenu button for the Linux theme.
Attachment #698380 - Attachment is obsolete: true
Attachment #698380 - Flags: review?(dao)
Attachment #698495 - Flags: review?(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.
Attachment #698495 - Flags: review?(dao) → review-
(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.
This shows the vertical height of the tab-bar being fixed.
Attachment #698433 - Attachment is obsolete: true
CCing Chris to let him know about the changes here.
(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
Attached patch Patch (v5) (obsolete) — Splinter Review
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?
Attachment #698495 - Attachment is obsolete: true
Attachment #698780 - Flags: review?(dao)
(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?
(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/globalmenu@ubuntu.com
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?
Attachment #698780 - Flags: review?(dao) → review-
(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.
Attached patch Windows and Mac theme (obsolete) — Splinter Review
Got rid of the Linux theme so that we can land this sooner.  I'll move the Linux theme to another bug.
Attachment #698780 - Attachment is obsolete: true
Attachment #698802 - Flags: review?(dao)
Depends on: 827454
Filed bug 827454 for the Linux theme part.
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?
(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...
Attachment #698802 - Attachment description: Windows and Linux theme → Windows and Mac theme
Attached patch Patch (v7)Splinter Review
Attachment #698802 - Attachment is obsolete: true
Attachment #698802 - Flags: review?(dao)
Attachment #698827 - Flags: review?(dao)
Attachment #698827 - Flags: review?(dao) → review+
Summary: Determine the required desktop theme changes for per-window Private Browsing → Desktop theme changes for per-window Private Browsing (Windows and OS X)
The uplift already happened, so landed on inbound.  :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d44408e050e
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.
Attachment #698827 - Flags: approval-mozilla-aurora?
Comment on attachment 698827 [details] [diff] [review]
Patch (v7)

Approving for uplift, given how close we are after the merge.
Attachment #698827 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/0d44408e050e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Depends on: 831829
No longer depends on: 831829
Blocks: 832015
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
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.
Lawrence, what do we want to relnote here?
Flags: needinfo?(lmandel)
No action needed. I was just flagging the bug for the relnote about per-window private browsing for the relman team.
Flags: needinfo?(lmandel)
(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.
(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/
(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!
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.
Depends on: 854126
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
Depends on: 852295
Depends on: 839888
Duplicate of this bug: 763376
You need to log in before you can comment on or make changes to this bug.