868640 - Implement private browsing indicator

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Bug 863753 removed the Firefox button which is the primary indicator of private browsing mode for operating systems that used the button. We will need to show an indicator somewhere else when the menubar is hidden.

IIRC, shorlander was fine with putting the mask icon in the top right of the browser window like we do with the menubar. I'm not sure if it should be in the tabs toolbar or the titlebar. We should consider the fact that the tabs are sometimes in the titlebar depending on OS and the sizemode of the window.

Comment 1

[u428464]

For now putting it in the tab trip on the left (à la Chrome) seems the best solution. It's the best place to spot easily the indicator.

Comment 2

[(no longer active)]

I thought that we're going to implement a dark theme for Australis?

Comment 3

[Matthew N. [:MattN]]

The long term plan is http://people.mozilla.com/~shorlander/private-browsing-mode/mockups/australis-pbm.png but we're not going to block on that for the Australis release.

Comment 4

[Matthew N. [:MattN]]

Stephen, see comment 0: Shall we put the indicator to the left of the window controls and use a spacer to prevent tabs from appearing below? Or shall we put an indicator on one of the sides of the tab strip? These options may require new indicator images for Windows and Linux(?).

See also bug 872636 for the overlap of the tabstrip on OS X.  An approach the works for all platforms would be ideal.

Comment 5

[Matthew N. [:MattN]]

Option 3: Perhaps we can just change the menu button colour to purple or overlay a mask on it?

Comment 6

[Stephen Horlander [:shorlander]]

Attachment: Private Browsing Indicator Ideas - i01

Here are some ways we could handle this. I think I prefer just going with the consistent approach of the indicator attached to titlebar. We can revisit post Australis for the stealth theme.

Comment 7

[Peter Henkel [:Terepin]]

(In reply to Stephen Horlander [:shorlander] from comment #6)
> Created attachment 762117 [details]
> Private Browsing Indicator Ideas - i01
> 
> Here are some ways we could handle this. I think I prefer just going with
> the consistent approach of the indicator attached to titlebar. We can
> revisit post Australis for the stealth theme.

Why not making it as a caption button next to the system's ones? And by pressing you could end PB.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Peter Henkel [:Terepin] from comment #7)
> (In reply to Stephen Horlander [:shorlander] from comment #6)
> > Created attachment 762117 [details]
> > Private Browsing Indicator Ideas - i01
> > 
> > Here are some ways we could handle this. I think I prefer just going with
> > the consistent approach of the indicator attached to titlebar. We can
> > revisit post Australis for the stealth theme.
> 
> Why not making it as a caption button next to the system's ones? And by
> pressing you could end PB.

Ending PB would be analogous to closing the window. The window controls that already exist should be enough for that. Adding another way to close the window doesn't seem helpful to me.

Comment 9

[Peter Henkel [:Terepin]]

Oops, you're right. I forgot that PB is in new window instead of new tab.
I don't know, it's just it feels... dunno, detached? Alone? Out of place? It doesn't feel right, at least not to me.

Comment 10

[u428464]

(In reply to Stephen Horlander [:shorlander] from comment #6)
> Created attachment 762117 [details]
> Private Browsing Indicator Ideas - i01
> 
> Here are some ways we could handle this. I think I prefer just going with
> the consistent approach of the indicator attached to titlebar. We can
> revisit post Australis for the stealth theme.

Yeah the titlebar seems the best place to put it since it also works in maximized mode.

Comment 11

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

Removing the items from M7 that do not block landing on m-c.

Comment 12

[Justin Dolske [:Dolske]]

Dao's going to take this.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Dao, what's the status here?

Comment 14

[Dão Gottwald [:dao]]

I just realized that this bug should be only about Windows. OS X was handled in bug 872636. If this is broken in some specific ways, that should be handled in separate bugs. There's already bug 898875. Linux has "Private Browsing" in the title bar.

Comment 15

[Dão Gottwald [:dao]]

Also, the indicator on the menu bar is broken...

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch

I couldn't put the indicator under the window controls in restored windows, as the window controls can be large enough to overlap that area. Also, the idea of styling the indicator like the window controls as shown in the fourth mockup assumes that window controls have the same shape across different Windows versions & themes, which they don't.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: bug-868640-privateWindow-indicator.png

Looks of the current patch in Win8

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: bug-868640-privateWindow-indicator-fullscreen.png

Looks of the current patch in Win8, fullscreen window.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 805453 [details] [diff] [review]
patch

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

I hate having to say this: visually, this does not look like something we should ship with (see screenshots). The indicator looks out of place and stuffed into a corner, without any context. I know Stephen is busy as it is already, but I think this does need a glance from him.

My suggestion is to unify this indicator across platforms and adopt the current OSX look (with minor platform-specific modifications), as a sticker slapped to the window border. That position provides the context it needs, as it's a private browsing _window_.

::: browser/base/content/browser.xul
@@ +874,5 @@
>                         oncommand="PanelUI.toggle(event);"/>
>        </toolbaritem>
>  
> +      <hbox id="window-controls" hidden="true" pack="end" skipintoolbarset="true"
> +            ordinal="1000">

Isn't the ordinal index here a styling detail? If so, putting `-moz-box-ordinal: 1000` in browser.js might be a better place.

Comment 20

[Mike de Boer [:mikedeboer]]

I meant `browser.css` of course :)

Comment 21

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> I hate having to say this: visually, this does not look like something we
> should ship with (see screenshots). The indicator looks out of place and
> stuffed into a corner, without any context.

Note that I'm just taking the icon we already have and moving it from the menu bar to the tab bar. Can you be more specific as to how it's more stuffed into a corner than it was before, what kind of context it lacks, etc.?

> My suggestion is to unify this indicator across platforms and adopt the
> current OSX look (with minor platform-specific modifications), as a sticker
> slapped to the window border.

As mentioned, the situation with the window controls is more complicated on Windows than it is on OS X. There's no single consistent style for those controls. I know there were plans to use custom window controls as shown in the mockup, but that would be massive scope creep for this bug. We could move the icon we have next to the window controls without the "sticker" shape, but it sounds like this wouldn't address your concern (that I don't fully understand yet).

> ::: browser/base/content/browser.xul
> @@ +874,5 @@
> >                         oncommand="PanelUI.toggle(event);"/>
> >        </toolbaritem>
> >  
> > +      <hbox id="window-controls" hidden="true" pack="end" skipintoolbarset="true"
> > +            ordinal="1000">
> 
> Isn't the ordinal index here a styling detail? If so, putting
> `-moz-box-ordinal: 1000` in browser.js might be a better place.

There's no situation where the controls shouldn't be at the very end of the toolbar, so I see no problem with using the ordinal attribute.

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #21)
> Note that I'm just taking the icon we already have and moving it from the
> menu bar to the tab bar. Can you be more specific as to how it's more
> stuffed into a corner than it was before, what kind of context it lacks,
> etc.?

I didn't say it looked _more_ stuffed in a corner, etc, but merely observed the current state without a context of prior state.

Regardless, I think this patch does solve the issue with minimal change. I also think, however, that we need a follow-up that is about improving the look and feel. Or do you think that can/ should be done within the scope of this bug?

Comment 23

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #22)
> Regardless, I think this patch does solve the issue with minimal change. I
> also think, however, that we need a follow-up that is about improving the
> look and feel. Or do you think that can/ should be done within the scope of
> this bug?

It depends on how exactly we'd improve it. There's a long-term goal of making private windows all black, which I'm sure is outside this bug's scope.

Stephen, if you're not ok with the current patch, could you please propose something that works with the layout variations we're facing (restored/maximized/fullscreen windows, different Windows versions and themes) and that doesn't depend on larger design changes unrelated to the indicator itself?

Comment 24

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #23)
> (In reply to Mike de Boer [:mikedeboer] from comment #22)
> > Regardless, I think this patch does solve the issue with minimal change. I
> > also think, however, that we need a follow-up that is about improving the
> > look and feel. Or do you think that can/ should be done within the scope of
> > this bug?
> 
> It depends on how exactly we'd improve it. There's a long-term goal of
> making private windows all black, which I'm sure is outside this bug's scope.

I wouldn't bet on that happening in the short (middle) term.  We should assume that whatever we come up with here will be shipped for an extended period of time, since I bet the rewrite of the PB theme would end up in a low priority bucket for quite a while.

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 805453 [details] [diff] [review]
patch

This patch takes care of the P1 a-ok. Another bug will make it look nize!

Comment 26

[Stephen Horlander [:shorlander]]

Comment on attachment 805937 [details]
bug-868640-privateWindow-indicator-fullscreen.png

(In reply to Dão Gottwald [:dao] from comment #23)
> Stephen, if you're not ok with the current patch, could you please propose
> something that works with the layout variations we're facing
> (restored/maximized/fullscreen windows, different Windows versions and
> themes) and that doesn't depend on larger design changes unrelated to the
> indicator itself?

I am ok with this approach and looking into some improvements, with or without the dark theme, in the future.

Comment 27

[Mike de Boer [:mikedeboer]]

(In reply to Stephen Horlander [:shorlander] from comment #26)
> I am ok with this approach and looking into some improvements, with or
> without the dark theme, in the future.

Thanks! Looking forward to it ;)

Comment 28

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/ux/rev/729ae62cd13b

Comment 33

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/729ae62cd13b