Closed Bug 868640 Opened 7 years ago Closed 6 years ago

Implement private browsing indicator

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P1])

Attachments

(4 files)

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.
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.
I thought that we're going to implement a dark theme for Australis?
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.
No longer blocks: 770135
Whiteboard: [Australis:M4] → [Australis:M7]
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.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
Keywords: uiwanted
Whiteboard: [Australis:M7] → [Australis:M6]
Option 3: Perhaps we can just change the menu button colour to purple or overlay a mask on it?
Whiteboard: [Australis:M6] → [Australis:M7]
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.
Flags: needinfo?(shorlander)
(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.
(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.
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.
(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.
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P1]
Dao's going to take this.
Assignee: mnoorenberghe+bmo → dao
Dao, what's the status here?
Flags: needinfo?(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.
Flags: needinfo?(dao)
OS: All → Windows 7
Also, the indicator on the menu bar is broken...
Summary: Implement private browsing indicator for when the menubar is hidden → Implement private browsing indicator
Component: Private Browsing → Theme
Keywords: uiwanted
Attached patch patchSplinter Review
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.
Attachment #805453 - Flags: review?(mdeboer)
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M9][Australis:P1]
Looks of the current patch in Win8
Attachment #805936 - Flags: feedback?(shorlander)
Looks of the current patch in Win8, fullscreen window.
Attachment #805937 - Flags: feedback?(shorlander)
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.
Attachment #805453 - Flags: review?(mdeboer) → feedback+
I meant `browser.css` of course :)
(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.
(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?
(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?
(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 on attachment 805453 [details] [diff] [review]
patch

This patch takes care of the P1 a-ok. Another bug will make it look nize!
Attachment #805453 - Flags: feedback+ → review+
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.
Attachment #805937 - Flags: feedback?(shorlander) → feedback+
(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 ;)
http://hg.mozilla.org/projects/ux/rev/729ae62cd13b
Whiteboard: [Australis:M9][Australis:P1] → [Australis:M9][Australis:P1][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/729ae62cd13b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P1][fixed-in-ux] → [Australis:M9][Australis:P1]
Target Milestone: --- → Firefox 28
Depends on: 940509
Depends on: 963512
You need to log in before you can comment on or make changes to this bug.