As an ambient and peripheral cue that the user is in private browsing mode, we should adjust the color of the Firefox button on Windows 7 and Vista from orange to purple. (purple is used on the masquerade mask in the content area message, and doesn't have any other semantic meaning in the product other than privacy). Additionally we should alter the text of the button to "Firefox (Private)" Any other modifications to the theme (like darkening the toolbars) will be done in addition to this change, but this is the minimum change for indicating the state of the browser. A specific non-goal of this bug is over the shoulder screen privacy. We are assuming that if you are using private browsing mode, you are probably not in a situation where other people can view your computer monitor (since the contents of content area are being shown just as much as the firefox button). We are however, very interested in giving users a reminder that Firefox is not storing any information about their activities on the Web, and we are interested in significantly reducing the number of mode errors that users are currently experiencing (staying in Firefox for long periods of time because they forget to exit). (listing this under content area notifications, which isn't entirely accurate since it persists after we show about:privatebrowsing, but I didn't really have any other place to put it)

Nominating blocking since there is no other indicator that you are in private browsing mode on Windows Vista/7. The change should be pretty minimal since we have a css selector for private browsing.

If i may request: On old PB mockup there was PB icon as indicator. However I suggest using it not only as a indicator but as off switch as well, that could be placed inside FXB. I tried this "feature" in StrataBuddy and I can say it is very comfortable.

Ehsan: can you take this?

(In reply to comment #3) > Ehsan: can you take this? Sure.

I need to know which color to use as the purple background. The current orange color we use is rgb(228,120,14).

Make the Firefox Button purple when in Private Browsing mode.

Comment on attachment 472932 [details] [diff] [review] Purple Firefox Button in PBM - 01 >diff --git a/browser/themes/winstripe/browser/browser-aero.css b/browser/themes/winstripe/browser/browser-aero.css >--- a/browser/themes/winstripe/browser/browser-aero.css >+++ b/browser/themes/winstripe/browser/browser-aero.css >@@ -25,16 +25,22 @@ > border-top: none; > -moz-border-left-colors: rgba(255,255,255,.5) rgba(83,42,6,.9); > -moz-border-bottom-colors: rgba(255,255,255,.5) rgba(83,42,6,.9); > -moz-border-right-colors: rgba(255,255,255,.5) rgba(83,42,6,.9); > -moz-box-shadow: 0 1px 0 rgba(255,255,255,.25) inset, > 0 0 2px 1px rgba(255,255,255,.25) inset; > } > >+ [browsingmode=private] #appmenu-button { Use #main-window[browsingmode=private] Do we still need some other, more accessible feedback?

(In reply to comment #8) > Do we still need some other, more accessible feedback? It would be nice to tag the button with text in some way. Maybe "(Private) Firefox". Or maybe "<maskicon> Firefox". Not sure if we have separate bugs for those and any kind of text would have to beat string freeze.

(In reply to comment #9) > (In reply to comment #8) > > Do we still need some other, more accessible feedback? > > It would be nice to tag the button with text in some way. Maybe "(Private) > Firefox". Or maybe "<maskicon> Firefox". Not sure if we have separate bugs for > those and any kind of text would have to beat string freeze. I think adding a mask icon is better, since adding the text would make the button too wide.

it's really hard to do icons well on these buttons. I think we should go with "Firefox - Private", it's wider, but that also helps the user realize that they are in a mode (and there is certainly no lack of space on the title bar).

The question is how to handle this with bug 571785. The only possible solution would be replacing FX icon with PB icon.

(In reply to comment #12) > The question is how to handle this with bug 571785. The only possible solution > would be replacing FX icon with PB icon. We could put a mask ON the Firefox ;)

Nice idea. It would look unique and it is creative.

(In reply to comment #13) > (In reply to comment #12) > > The question is how to handle this with bug 571785. The only possible solution > > would be replacing FX icon with PB icon. > > We could put a mask ON the Firefox ;) Even better! :)

Updated to use #main-window[browsingmode=private]

Comment on attachment 474585 [details] [diff] [review] Purple Firefox Button in PBM - 02 You should also take into account the permanent private browsing mode as well. We don't show (Private Browsing) in the title bar for permanent private browsing mode sessions, by the same logic we shouldn't change the color of the Firefox button to purple when that mode is active.

Is there a way to do that with CSS?

This will enable us to theme based on whether the PB session is permanent or not.

With a small fix...

Actually make sure that |docElement| is defined before using it...

Comment on attachment 475177 [details] [diff] [review] Part 1: add a privatebrowsingmode=permanent attribute to browser.xul's doc root I don't see the point of "browsingmode". Seems like we want three states: [privatebrowsingmode absent] privatebrowsingmode=temporary privatebrowsingmode=permanent However, if browsingmode=normal isn't actually useful for styling, maybe privatebrowsingmode=temporary wouldn't be either. Which leaves us with: [privatebrowsingmode absent] privatebrowsingmode=permanent

Oh, so you do want the color change for temporary private browsing but not otherwise. So this would leave us with this: [privatebrowsingmode absent] privatebrowsingmode=temporary

(In reply to comment #24) > Oh, so you do want the color change for temporary private browsing but not > otherwise. So this would leave us with this: > > [privatebrowsingmode absent] > privatebrowsingmode=temporary But I guess we would also need privatebrowsingmode=permanent as well, for themes which might need that for styling permanent PB sessions differently, right? Please note that this was originally added in order to support theme styling.

Based on the new privatebrowsingmode attribute.

Is there a similar bug for XP when the firefox button is shown? This bug seems very win7/Vista specific.

Comment on attachment 476370 [details] [diff] [review] Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root Ok, do we want to support special styling for privatebrowsingmode=permanent? If not, why set the attribute?

Comment on attachment 476373 [details] [diff] [review] Part 2: actually make the button purple >+#appmenu-button:-moz-window-inactive, >+#main-window[privatebrowsingmode=temporary] #appmenu-button:-moz-window-inactive { >+appmenu-button:-moz-window-inactive { > background-image: none; > border-color: rgba(0,0,0,.4); > } Broken selector.

(In reply to comment #30) > Ok, do we want to support special styling for privatebrowsingmode=permanent? If > not, why set the attribute? We don't currently, but themes might.

(In reply to comment #32) > (In reply to comment #30) > > Ok, do we want to support special styling for privatebrowsingmode=permanent? If > > not, why set the attribute? > > We don't currently, but themes might. They might want to, but this doesn't mean it's a good idea which should be supported. I don't know why we don't have an indicator for permanent private browsing, but I assumed there would be a good reason. Is it more of an arbitrary choice?

(In reply to comment #34) > (In reply to comment #32) > > (In reply to comment #30) > > > Ok, do we want to support special styling for privatebrowsingmode=permanent? If > > > not, why set the attribute? > > > > We don't currently, but themes might. > > They might want to, but this doesn't mean it's a good idea which should be > supported. I don't know why we don't have an indicator for permanent private > browsing, but I assumed there would be a good reason. Is it more of an > arbitrary choice? The reason is that setting Firefox to always be in private browsing mode is an explicit choice which you usually do inside the preferences window, and we assume that if you choose that setting, you're aware of the implications and do not need to be reminded about it every time you look at the Firefox window. I'd still opt for making it possible for themes to style the permanent mode differently, but if you don't like that, I can take that out of the patch, as I don't like to keep this blocker bug blocking on that issue any more.

Comment on attachment 476370 [details] [diff] [review] Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root >+ let docElement = document.documentElement; > if (this._privateBrowsingService.autoStarted) { > // Disable the menu item in auto-start mode > document.getElementById("privateBrowsingItem") > .setAttribute("disabled", "true"); > #ifdef MENUBAR_CAN_AUTOHIDE > document.getElementById("appmenu_privateBrowsing") > .setAttribute("disabled", "true"); > #endif > document.getElementById("Tools:PrivateBrowsing") > .setAttribute("disabled", "true"); >+ docElement.setAttribute("privatebrowsingmode", "permanent"); > } > else if (window.location.href == getBrowserURL()) { > // Adjust the window's title >- let docElement = document.documentElement; > docElement.setAttribute("title", > docElement.getAttribute("title_privatebrowsing")); > docElement.setAttribute("titlemodifier", > docElement.getAttribute("titlemodifier_privatebrowsing")); >- docElement.setAttribute("browsingmode", "private"); >+ docElement.setAttribute("privatebrowsingmode", "temporary"); > gBrowser.updateTitlebar(); > } Looks like docElement.setAttribute("privatebrowsingmode", "permanent") should depend on (window.location.href == getBrowserURL()) as well. > this._setPBMenuTitle("start"); > >+ let docElement = document.documentElement; > if (window.location.href == getBrowserURL()) { > // Adjust the window's title >- let docElement = document.documentElement; > docElement.setAttribute("title", > docElement.getAttribute("title_normal")); > docElement.setAttribute("titlemodifier", > docElement.getAttribute("titlemodifier_normal")); >- docElement.setAttribute("browsingmode", "normal"); > } > > // Enable the menu item in after exiting the auto-start mode > document.getElementById("privateBrowsingItem") > .removeAttribute("disabled"); > #ifdef MENUBAR_CAN_AUTOHIDE > document.getElementById("appmenu_privateBrowsing") > .removeAttribute("disabled"); > #endif > document.getElementById("Tools:PrivateBrowsing") > .removeAttribute("disabled"); >+ docElement.removeAttribute("privatebrowsingmode"); Here you're removing the dependency on (window.location.href == getBrowserURL()), which looks wrong.

Comment on attachment 477325 [details] [diff] [review] Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root >+ let docElement = document.documentElement; > if (this._privateBrowsingService.autoStarted) { > // Disable the menu item in auto-start mode > document.getElementById("privateBrowsingItem") > .setAttribute("disabled", "true"); > #ifdef MENUBAR_CAN_AUTOHIDE > document.getElementById("appmenu_privateBrowsing") > .setAttribute("disabled", "true"); > #endif > document.getElementById("Tools:PrivateBrowsing") > .setAttribute("disabled", "true"); >+ if (window.location.href == getBrowserURL()) { >+ docElement.setAttribute("privatebrowsingmode", "permanent"); >+ } nit: drop the braces >+ let docElement = document.documentElement; > if (window.location.href == getBrowserURL()) { > // Adjust the window's title >- let docElement = document.documentElement; > docElement.setAttribute("title", > docElement.getAttribute("title_normal")); > docElement.setAttribute("titlemodifier", > docElement.getAttribute("titlemodifier_normal")); >- docElement.setAttribute("browsingmode", "normal"); > } > > // Enable the menu item in after exiting the auto-start mode > document.getElementById("privateBrowsingItem") > .removeAttribute("disabled"); > #ifdef MENUBAR_CAN_AUTOHIDE > document.getElementById("appmenu_privateBrowsing") > .removeAttribute("disabled"); > #endif > document.getElementById("Tools:PrivateBrowsing") > .removeAttribute("disabled"); >+ if (window.location.href == getBrowserURL()) { >+ docElement.removeAttribute("privatebrowsingmode"); >+ } It looks like there's no reason to shuffle this, i.e. you should just replace docElement.setAttribute("browsingmode", "normal"); with docElement.removeAttribute("privatebrowsingmode");.

With nits addressed. Please note that the part 2 patch is still awaiting review.

Comment on attachment 477544 [details] [diff] [review] Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root >+ let docElement = document.documentElement; > if (window.location.href == getBrowserURL()) { > // Adjust the window's title >- let docElement = document.documentElement; > docElement.setAttribute("title", > docElement.getAttribute("title_normal")); > docElement.setAttribute("titlemodifier", > docElement.getAttribute("titlemodifier_normal")); >- docElement.setAttribute("browsingmode", "normal"); >+ docElement.removeAttribute("privatebrowsingmode"); > } bogus change around let docElement

Ah, right. Fixed.

dao: ping?

Comment on attachment 477229 [details] [diff] [review] Part 2: actually make the button purple >--- a/browser/themes/winstripe/browser/browser-aero.css >+++ b/browser/themes/winstripe/browser/browser-aero.css >+ #main-window[privatebrowsingmode=temporary] #appmenu-button { >+ -moz-border-left-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); >+ -moz-border-bottom-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); >+ -moz-border-right-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); >+ } >+ > #appmenu-button:-moz-window-inactive { > -moz-border-left-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > -moz-border-bottom-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > -moz-border-right-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > box-shadow: 0 0 0 1px rgba(255,255,255,.25) inset; > } Does #appmenu-button:-moz-window-inactive take precedence over #main-window[privatebrowsingmode=temporary] #appmenu-button? >--- a/browser/themes/winstripe/browser/browser.css >+++ b/browser/themes/winstripe/browser/browser.css >-#appmenu-button:-moz-window-inactive { >+#main-window[privatebrowsingmode=temporary] #appmenu-button { >+ background-image: -moz-linear-gradient(rgb(153,38,211), rgb(105,19,163) 95%); >+ border-color: rgba(43,8,65,.9); >+} Can you add :not(:-moz-window-inactive) here? (Though if the answer to the above question is "yes", this may not be needed.) >+#appmenu-button:-moz-window-inactive, >+#main-window[privatebrowsingmode=temporary] #appmenu-button:-moz-window-inactive { > background-image: none; > border-color: rgba(0,0,0,.4); > } And drop #main-window[privatebrowsingmode=temporary] #appmenu-button:-moz-window-inactive here?

(In reply to comment #44) > Comment on attachment 477229 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=477229 > Part 2: actually make the button purple > > >--- a/browser/themes/winstripe/browser/browser-aero.css > >+++ b/browser/themes/winstripe/browser/browser-aero.css > > >+ #main-window[privatebrowsingmode=temporary] #appmenu-button { > >+ -moz-border-left-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > >+ -moz-border-bottom-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > >+ -moz-border-right-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > >+ } > >+ > > #appmenu-button:-moz-window-inactive { > > -moz-border-left-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > -moz-border-bottom-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > -moz-border-right-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > box-shadow: 0 0 0 1px rgba(255,255,255,.25) inset; > > } > > Does #appmenu-button:-moz-window-inactive take precedence over > #main-window[privatebrowsingmode=temporary] #appmenu-button? The specificity of the former is (a=0, b=1, c=1, d=0), and the specificity of the latter is (a=0, b=2, c=1, d=0), therefore the latter takes precedence on the former. > >--- a/browser/themes/winstripe/browser/browser.css > >+++ b/browser/themes/winstripe/browser/browser.css > > >-#appmenu-button:-moz-window-inactive { > >+#main-window[privatebrowsingmode=temporary] #appmenu-button { > >+ background-image: -moz-linear-gradient(rgb(153,38,211), rgb(105,19,163) 95%); > >+ border-color: rgba(43,8,65,.9); > >+} > > Can you add :not(:-moz-window-inactive) here? (Though if the answer to the > above question is "yes", this may not be needed.) Well, the answer to the above question was no, but I don't see why the :not rule would be needed here. This selector takes precedence over #appmenu-button:-moz-window-inactive, which is what we want, right? > >+#appmenu-button:-moz-window-inactive, > >+#main-window[privatebrowsingmode=temporary] #appmenu-button:-moz-window-inactive { > > background-image: none; > > border-color: rgba(0,0,0,.4); > > } > > And drop #main-window[privatebrowsingmode=temporary] > #appmenu-button:-moz-window-inactive here? This will cause the border-color rule from the previous selector to be applied to that case. Is that what we really want?

(In reply to comment #45) > > And drop #main-window[privatebrowsingmode=temporary] > > #appmenu-button:-moz-window-inactive here? > > This will cause the border-color rule from the previous selector to be applied > to that case. Is that what we really want? No, which is why I suggested adding :not(:-moz-window-inactive) there.

With comments addressed.

(In reply to comment #45) > (In reply to comment #44) > > Comment on attachment 477229 [details] [diff] [review] > > --> https://bugzilla.mozilla.org/attachment.cgi?id=477229 > > Part 2: actually make the button purple > > > > >--- a/browser/themes/winstripe/browser/browser-aero.css > > >+++ b/browser/themes/winstripe/browser/browser-aero.css > > > > >+ #main-window[privatebrowsingmode=temporary] #appmenu-button { > > >+ -moz-border-left-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > > >+ -moz-border-bottom-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > > >+ -moz-border-right-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > > >+ } > > >+ > > > #appmenu-button:-moz-window-inactive { > > > -moz-border-left-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > > -moz-border-bottom-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > > -moz-border-right-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > > box-shadow: 0 0 0 1px rgba(255,255,255,.25) inset; > > > } > > > > Does #appmenu-button:-moz-window-inactive take precedence over > > #main-window[privatebrowsingmode=temporary] #appmenu-button? > > The specificity of the former is (a=0, b=1, c=1, d=0), and the specificity of > the latter is (a=0, b=2, c=1, d=0), therefore the latter takes precedence on > the former. Wait... does former/latter refer to the CSS source or to what I wrote?

(In reply to comment #48) > (In reply to comment #45) > > (In reply to comment #44) > > > Comment on attachment 477229 [details] [diff] [review] > > > --> https://bugzilla.mozilla.org/attachment.cgi?id=477229 > > > Part 2: actually make the button purple > > > > > > >--- a/browser/themes/winstripe/browser/browser-aero.css > > > >+++ b/browser/themes/winstripe/browser/browser-aero.css > > > > > > >+ #main-window[privatebrowsingmode=temporary] #appmenu-button { > > > >+ -moz-border-left-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > > > >+ -moz-border-bottom-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > > > >+ -moz-border-right-colors: rgba(255,255,255,.5) rgba(43,8,65,.9); > > > >+ } > > > >+ > > > > #appmenu-button:-moz-window-inactive { > > > > -moz-border-left-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > > > -moz-border-bottom-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > > > -moz-border-right-colors: rgba(255,255,255,.4) rgba(0,0,0,.5); > > > > box-shadow: 0 0 0 1px rgba(255,255,255,.25) inset; > > > > } > > > > > > Does #appmenu-button:-moz-window-inactive take precedence over > > > #main-window[privatebrowsingmode=temporary] #appmenu-button? > > > > The specificity of the former is (a=0, b=1, c=1, d=0), and the specificity of > > the latter is (a=0, b=2, c=1, d=0), therefore the latter takes precedence on > > the former. > > Wait... does former/latter refer to the CSS source or to what I wrote? What you wrote. (I resisted the temptation of saying "the latter" again! ;-) )

It seems to me that the :-moz-window-inactive rule should take precedence, i.e. you should add :not(:-moz-window-inactive) to the other selector just like in browser.css. By the way, please add that to #appmenu-button rather than #main-window.

(In reply to comment #50) > It seems to me that the :-moz-window-inactive rule should take precedence, i.e. > you should add :not(:-moz-window-inactive) to the other selector just like in > browser.css. By the way, please add that to #appmenu-button rather than > #main-window. I'm kind of lost what you mean here. I used the specificity of the selectors as defined in the CSS spec. And I'm not sure what we're trying to fix here... :/

(In reply to comment #51) > (In reply to comment #50) > > It seems to me that the :-moz-window-inactive rule should take precedence, i.e. > > you should add :not(:-moz-window-inactive) to the other selector just like in > > browser.css. By the way, please add that to #appmenu-button rather than > > #main-window. > > I'm kind of lost what you mean here. I used the specificity of the selectors > as defined in the CSS spec. You're saying the privatebrowsingmode=temporary styling overrides the window-inactive styling. (Aren't you?) I'm saying we probably want it the other way around.

How about modifying #appmenu-button:-moz-window-inactive to #main-window #appmenu-button:-moz-window-inactive so that the two rules become similarly specific, and the second one would override the first one?

You think that's better than adding :not(:-moz-window-inactive) to the first selector?

(In reply to comment #54) > You think that's better than adding :not(:-moz-window-inactive) to the first > selector? Adding :not(:-moz-window-inactive) to the first selector doesn't help anything, it only makes that selector more specific than it already is. Or am I missing something?

It makes the selector not match in the window-inactive case, specificity doesn't even matter then.

OK, this should address your comments.

http://hg.mozilla.org/mozilla-central/rev/a0baa33cd4ca http://hg.mozilla.org/mozilla-central/rev/ae29792742d8

Documented the API change here: https://developer.mozilla.org/En/Supporting_private_browsing_mode#Detecting_whether_private_browsing_mode_is_permanent Added an example of skinning the firefox button using the privatebrowsingmode attribute here: https://developer.mozilla.org/En/Supporting_private_browsing_mode#Private_browsing_for_theme_designers

https://litmus.mozilla.org/show_test.cgi?id=13805 to Litmus.

All I know is I went to the public library where they use Windows, and hit CTRL-N which I recall had the same golden color as the non-private window. I recall there was a message inside the window saying that it was a private window. I double checked that I was using indeed the latest Firefox 27. Isn't it a security issue, making private windows look like non-private windows?

I think I mean to say that choosing private browsing from the menus makes the same colored windows as the usual CTRL-N non-private windows.

P.S. and why have things look so different on Linux?

(In reply to Dan Jacobson from comment #61) > All I know is I went to the public library where they use Windows, and hit > CTRL-N which I recall had the same golden color as the non-private window. I > recall there was a message inside the window saying that it was a private > window. I double checked that I was using indeed the latest Firefox 27. It is possible to set browser.privatebrowsing.autostart to true in about:config. This makes private browsing mode the default state. Could be what you were seeing. > Isn't it a security issue, making private windows look like non-private > windows? Potentially a mode error, but I don't see how it is a security issue.

Well OK but I recall they were all orange color, not purple. OK next time I go to town I will try to get more details. If a danger signal is always on even though there is no danger, one day when there is danger, users will ignore it. If there is no security issue in both modes being the same color, then one might ask why bother to distinguish them in the first place...

Confirmed no problem seen in Windows FF 7.0,1 or 12.0.