When in private browsing mode, make the Firefox button purple

RESOLVED FIXED in Firefox 4.0b7

Status

()

Firefox
Private Browsing
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: faaborg, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {dev-doc-complete, ux-mode-error})

Trunk
Firefox 4.0b7
All
Windows 7
dev-doc-complete, ux-mode-error
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(3 attachments, 12 obsolete attachments)

210.61 KB, image/png
Details
5.05 KB, patch
Details | Diff | Splinter Review
2.82 KB, patch
dao
: review+
Details | Diff | Splinter Review
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)
(Reporter)

Comment 1

7 years ago
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.
blocking2.0: --- → ?
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?
blocking2.0: ? → final+
(In reply to comment #3)
> Ehsan: can you take this?

Sure.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Version: unspecified → Trunk
I need to know which color to use as the purple background.  The current orange color we use is rgb(228,120,14).
Keywords: uiwanted
Created attachment 472932 [details] [diff] [review]
Purple Firefox Button in PBM - 01

Make the Firefox Button purple when in Private Browsing mode.
Assignee: ehsan → shorlander
Attachment #472932 - Flags: review?(dao)
Created attachment 472933 [details]
Screenshot of Purple Firefox Button
Keywords: uiwanted
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?
Attachment #472932 - Flags: review?(dao)
(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! :)
Created attachment 474585 [details] [diff] [review]
Purple Firefox Button in PBM - 02

Updated to use #main-window[browsingmode=private]
Attachment #472932 - Attachment is obsolete: true
Attachment #474585 - Flags: review?(dao)
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.
Attachment #474585 - Flags: review?(dao) → review-
Is there a way to do that with CSS?
Assignee: shorlander → ehsan
Created attachment 474841 [details] [diff] [review]
Part 1: add a privatebrowsingmode=permanent attribute to browser.xul's doc root

This will enable us to theme based on whether the PB session is permanent or not.
Attachment #474841 - Flags: review?(dao)
Created attachment 474849 [details] [diff] [review]
Part 2: actually make the button purple
Attachment #474849 - Flags: review?(dao)
Flags: in-litmus?
Attachment #474585 - Attachment is obsolete: true
Created attachment 474879 [details] [diff] [review]
Part 2: actually make the button purple

With a small fix...
Attachment #474849 - Attachment is obsolete: true
Attachment #474879 - Flags: review?(dao)
Attachment #474849 - Flags: review?(dao)
Created attachment 475177 [details] [diff] [review]
Part 1: add a privatebrowsingmode=permanent attribute to browser.xul's doc root

Actually make sure that |docElement| is defined before using it...
Attachment #474841 - Attachment is obsolete: true
Attachment #475177 - Flags: review?(dao)
Attachment #474841 - Flags: review?(dao)
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
Attachment #475177 - Flags: review?(dao) → review-
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

Updated

7 years ago
Attachment #474879 - Flags: review?(dao)
(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.
Created attachment 476370 [details] [diff] [review]
Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root
Attachment #475177 - Attachment is obsolete: true
Attachment #476370 - Flags: review?(dao)
Keywords: dev-doc-needed
Created attachment 476373 [details] [diff] [review]
Part 2: actually make the button purple

Based on the new privatebrowsingmode attribute.
Attachment #474879 - Attachment is obsolete: true
Attachment #476373 - Flags: review?(dao)

Updated

7 years ago
Duplicate of this bug: 575289
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.
Created attachment 477229 [details] [diff] [review]
Part 2: actually make the button purple
Attachment #476373 - Attachment is obsolete: true
Attachment #477229 - Flags: review?(dao)
Attachment #476373 - Flags: review?(dao)
(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.
Created attachment 477325 [details] [diff] [review]
Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root
Attachment #476370 - Attachment is obsolete: true
Attachment #477325 - Flags: review?(dao)
Attachment #476370 - Flags: review?(dao)
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");.
Attachment #477325 - Flags: review?(dao) → review+
Created attachment 477544 [details] [diff] [review]
Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root

With nits addressed.

Please note that the part 2 patch is still awaiting review.
Attachment #477325 - Attachment is obsolete: true
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
Created attachment 477687 [details] [diff] [review]
Part 1: add a privatebrowsingmode=temporary/permanent attribute to browser.xul's doc root

Ah, right.  Fixed.
Attachment #477544 - Attachment is obsolete: true
Whiteboard: [needs review dao]
dao: ping?
Whiteboard: [needs review dao] → [has patch][needs review dao]

Updated

7 years ago
Duplicate of this bug: 605026
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.
Created attachment 484869 [details] [diff] [review]
Part 2: actually make the button purple

With comments addressed.
Attachment #477229 - Attachment is obsolete: true
Attachment #484869 - Flags: review?(dao)
Attachment #477229 - Flags: review?(dao)
(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.
Created attachment 485746 [details] [diff] [review]
Part 2: actually make the button purple

OK, this should address your comments.
Attachment #484869 - Attachment is obsolete: true
Attachment #485746 - Flags: review?(dao)
Attachment #484869 - Flags: review?(dao)

Updated

7 years ago
Attachment #485746 - Flags: review?(dao) → review+
Whiteboard: [has patch][needs review dao] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/a0baa33cd4ca
http://hg.mozilla.org/mozilla-central/rev/ae29792742d8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → Firefox 4.0b8

Updated

7 years ago
Depends on: 607482

Updated

7 years ago
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7
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
Keywords: dev-doc-needed → dev-doc-complete
https://litmus.mozilla.org/show_test.cgi?id=13805 to Litmus.
Flags: in-litmus? → in-litmus+

Comment 61

3 years ago
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?

Comment 62

3 years ago
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.

Comment 63

3 years ago
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.

Comment 65

3 years ago
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...

Comment 66

3 years ago
Confirmed no problem seen in Windows FF 7.0,1 or 12.0.
You need to log in before you can comment on or make changes to this bug.