Closed Bug 940509 Opened 7 years ago Closed 7 years ago

The private browsing indicator on Windows seems out of place

Categories

(Firefox :: Theme, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified

People

(Reporter: ehsan, Assigned: mconley)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3+])

Attachments

(9 files, 8 obsolete files)

80.49 KB, image/png
Details
33.72 KB, image/png
Details
860 bytes, image/png
Details
951 bytes, image/png
Details
445 bytes, image/png
Details
370 bytes, image/png
Details
942 bytes, image/png
Details
17.17 KB, patch
mconley
: review+
Details | Diff | Splinter Review
40.60 KB, image/jpeg
Details
It seems like the icon is put somewhere random in the titlebar.  Vladan can provide a screenshot!
Agreed. It could be placed as identity block and made address and search fields a bit purple-ish.
Just my 2 cents.
The other problem with this new indicator is that it's much more subtle than what we had before (the change in the color of the Firefox button.)  I was under the impression that we're going to implement something like the darker theme on Mac as part of Australis across the board.  Has that plan changed?
Flags: needinfo?(gavin.sharp)
(In reply to comment #4)
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=889681#c2

I'm not suggesting that we add the Firefox button back!
Flags: needinfo?(gavin.sharp) → needinfo?(shorlander)
Whiteboard: [Australis:P4]
It seems like something's changed recently in Australis's private windows. My lightweight theme isn't applied to the private window, the toolbar turns dark, and the hamburger button turns purple. So I guess this is bug is a dupe of whatever made those changes?
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #3)
> The other problem with this new indicator is that it's much more subtle than
> what we had before (the change in the color of the Firefox button.)  I was
> under the impression that we're going to implement something like the darker
> theme on Mac as part of Australis across the board.  Has that plan changed?

The plan is still for a more complete dark appearance for PBM windows. We can't get that before release though so this is what we have until we can fix that.
Flags: needinfo?(shorlander)
(In reply to Stephen Horlander [:shorlander] from comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #3)
> > The other problem with this new indicator is that it's much more subtle than
> > what we had before (the change in the color of the Firefox button.)  I was
> > under the impression that we're going to implement something like the darker
> > theme on Mac as part of Australis across the board.  Has that plan changed?
> 
> The plan is still for a more complete dark appearance for PBM windows. We
> can't get that before release though so this is what we have until we can
> fix that.

Hmm.  That makes me very sad.  But OK.
Why not position it as in chrome?
This... does seem odd, and doesn't really match up with the original design ideas in attachment 762117 [details]. Let's reconsider what we want to ship here at the least.
Flags: needinfo?(shorlander)
Whiteboard: [Australis:P4] → [Australis:P3+]
shorlander thinks we should try the approach from dolske's screenshot - 4th and 7th from the top.
Assignee: nobody → mconley
Flags: needinfo?(shorlander)
(In reply to Mike Conley (:mconley) from comment #11)
> shorlander thinks we should try the approach from dolske's screenshot - 4th
> and 7th from the top.

Sorry, which screenshot is that?
Flags: needinfo?(mconley)
Ah whoops, I should be more precise and less incorrect - not a screenshot, but attachment 762117 [details].
Flags: needinfo?(mconley)
Thanks, that sounds good.
I'm pretty sure we've previously avoided that design because it assumes a certain layout of the window control buttons that we do not have across Windows versions and themes. It also doesn't work when the native title bar is enabled.
Guys, why can't we use purple identity box with  PB icon? I believe it makes more sense there than in title bar.
OS: Mac OS X → Windows 7
(In reply to Peter Henkel [:Terepin] from comment #16)
> Guys, why can't we use purple identity box with  PB icon? I believe it makes
> more sense there than in title bar.

One reason is that the identity box needs to serve the identity function even if in private browsing mode. If it's always purple, we don't get our nice green "cert verified" appearance. And, if we switch to green when the cert is verified, we lose all indication that we're in private browsing mode. Introducing _new_ states for cert verified when in private browsing mode is also not too appealing. I think we need to leave the identity box alone. :/
Fair points. But we still can use adress bar, no? Something like making it purple-ish and/or placing indicator there. Dunno, it just feel right to have it there than anywhere else.
I'm back to dealing with talos regressions. I'll come back to this as soon as I can, unless someone else picks it up.
Assignee: mconley → nobody
Picking this up again now that the CART stuff has been dealt with.
Assignee: nobody → mconley
Ok, so I believe shorlander is working on the assets I'll need, but in the meantime, I'll whip up the patch.
Oops, somehow added black bars to the sides of this.
Attachment #8390020 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
This uses shorlander's latest assets.
Attached patch Patch v1 (obsolete) — Splinter Review
Now without lame Windows line endings.
Attachment #8390236 - Attachment is obsolete: true
Comment on attachment 8390240 [details] [diff] [review]
Patch v1

Thoughts?
Attachment #8390240 - Flags: ui-review?(shorlander)
So I've batted this back and forth with shorlander, and he's still looking at it, but some notes:

1) I've shrunk the mask down to 18px from 20px. Let's not do that.
2) On Windows 8 and Windows Classic, I should try to margin-top the mask away from the window border.
Attached patch WIP Patch 1 (obsolete) — Splinter Review
Checkpointing some work here...
Attachment #8390240 - Attachment is obsolete: true
Attachment #8390240 - Flags: ui-review?(shorlander)
Attached patch WIP Patch 2 (obsolete) — Splinter Review
Checkpointing - this looks good on Windows 7 Aero Glass, switching over to Windows 8 next, followed by XP and then the classic modes.
Attachment #8390734 - Attachment is obsolete: true
Attached patch WIP Patch 3 (obsolete) — Splinter Review
Adds in some taller tabstrip indicators from shorlander.
Attachment #8391272 - Attachment is obsolete: true
I'm kinda blocked here until I get new assets for Windows XP (since the caption buttons are larger, the current titlebar mask won't work - I need a taller one if we want to line up with the bottom of the window caption buttons).

I'm also concerned that this won't look right if those buttons are scaled up for larger font sizes...

and there's also classic mode to think about.

How should I proceed on those fronts, Stephen?
Flags: needinfo?(shorlander)
I spoke to shorlander in IRC, and he said he's less concerned with the alignment of the privacy mask when the system font is large, so we won't worry about that. We'll stick with images for now.

I'll still need a mask for the Luna themes as well as classic.
- The titlebar mask icon needs to be made taller by about 6px for both Windows 7 Aero Basic and XP Luna
- The titlebar mask icon needs to be 16px tall for Win 7 and XP classic
Attached patch Patch v1 (obsolete) — Splinter Review
So I'm still missing some assets here, so some of the dimensions are off, but I'm curious to know what others think of my approach here.

In particular, I had to do some hackery with the titlebar indicator. I had to put it in a container with display: block and position: absolute. I needed to do that in order to give myself the ability to offset the mask by a few pixels. position: relative, which is what I'd normally use, didn't seem to work. In fact, while I was messing with it in DOM inspector, I got some really strange behaviour if I just went the position: relative route.

Example - I'd set top: 1px, and the mask wouldn't move. And then I'd set it to 2px, and it'd jump down 2px. And then I'd set it to 1px again...and it'd jump down a third px... and every time I changed the value it'd keep just adding that value to the amount of space from the top of the window. It'd reset if I set it to 0.

So this is the route I went. Open to suggestions.
Attachment #8391304 - Attachment is obsolete: true
Attachment #8392214 - Flags: feedback?(jaws)
Comment on attachment 8392214 [details] [diff] [review]
Patch v1

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

You should include HiDPI versions of these graphics, especially for Windows8.

Also, you'll want to update your patch summary to reduce redundancy.

::: browser/themes/windows/browser.css
@@ +2701,5 @@
> +}
> +
> +#main-window[sizemode="normal"] > #titlebar > #titlebar-content > #titlebar-buttonbox-container > .private-browsing-indicator-titlebar > .private-browsing-indicator {
> +  position: relative;
> +  top: 1px;

The other titlebar buttons don't have this 1px vertical offset on Windows 8 when the windows is in restored/"normal" mode.
Attachment #8392214 - Flags: feedback?(jaws) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
So I believe this takes care of all of our cases / Windows variations.

Tested tabs in titlebar and tabs not in titlebar in:

Windows XP (Luna, Classic)
Windows 7 (Classic, Aero Basic, Aero Glass)
Windows 8

jaws - what are your thoughts on us landing this, and doing the hipdi work for this in a follow-up?
Attachment #8392214 - Attachment is obsolete: true
Comment on attachment 8393192 [details] [diff] [review]
Patch v2

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

::: browser/themes/windows/browser.css
@@ +2750,5 @@
>  }
>  
>  /* End customization mode */
>  
> +/* Private browsing indicators */

When we have new self-contained CSS like this we should consider putting it in a new CSS file and #include'ing it. This is similar to what we do with browser-*.js. Not sure if others agree and in this case it probably doesn't matter much since this code will likely all be gone when the new PB theme is introduced.
Comment on attachment 8393192 [details] [diff] [review]
Patch v2

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

Yeah, we can wait on the HiDPI graphics. I'm not so keen on separating the CSS out to a separate file, especially since it is Windows only and not shared between multiple files.
Attachment #8393192 - Flags: review+
Status: NEW → ASSIGNED
Comment on attachment 8393192 [details] [diff] [review]
Patch v2

>+@media not all and (-moz-windows-compositor) {
>+  @media not all and (-moz-windows-classic) {
>+    #main-window[sizemode="normal"] > #titlebar > #titlebar-content > #titlebar-buttonbox-container > .private-browsing-indicator-titlebar > .private-browsing-indicator {
>+      background-image: url("chrome://browser/skin/privatebrowsing-mask-titlebar-XPVista7-tall.png");
>+      height: 28px;
>+    }
>+  }
>+}

Not sure what configuration you're targeting here. Aero Basic? Can you please document this? If it's Aero Basic, combine -moz-windows-default-theme with !-moz-windows-compositor.

>+#main-window[privatebrowsingmode=temporary][tabsintitlebar] .private-browsing-indicator-titlebar > .private-browsing-indicator,
>+#main-window[privatebrowsingmode=temporary]:not([tabsintitlebar]) #TabsToolbar > .private-browsing-indicator {
>+  display: block;
>+}
>+
>+#TabsToolbar > .private-browsing-indicator {
>+  background: url("chrome://browser/skin/privatebrowsing-mask-tabstrip.png") no-repeat center -3px;
>+  -moz-margin-start: 4px;
>+  width: 48px;
>+  height: @tabHeight@;
>+}

Not sure why you're using display:block. The indicator in the tab bar should definitely be -moz-box and you shouldn't specify a height.
Attachment #8393192 - Flags: review-
Comment on attachment 8393192 [details] [diff] [review]
Patch v2

>+#ifdef XP_WIN
>+      <hbox class="private-browsing-indicator-titlebar">
>+        <hbox class="private-browsing-indicator" />
>+      </hbox>
>+#endif

private-browsing-indicator-titlebar should be an id not a class. I'm not sure why you need an extra container, though. If it's just to differentiate the two private-browsing-indicator elements, you could instead set an attribute on them.

nit: remove the space before />
(In reply to Dão Gottwald [:dao] from comment #43)
> Comment on attachment 8393192 [details] [diff] [review]
> Patch v2
> 
> >+@media not all and (-moz-windows-compositor) {
> >+  @media not all and (-moz-windows-classic) {
> >+    #main-window[sizemode="normal"] > #titlebar > #titlebar-content > #titlebar-buttonbox-container > .private-browsing-indicator-titlebar > .private-browsing-indicator {
> >+      background-image: url("chrome://browser/skin/privatebrowsing-mask-titlebar-XPVista7-tall.png");
> >+      height: 28px;
> >+    }
> >+  }
> >+}
> 
> Not sure what configuration you're targeting here. Aero Basic? Can you
> please document this? If it's Aero Basic, combine -moz-windows-default-theme
> with !-moz-windows-compositor.
> 

Done - yes, that seems to work nicely.

> >+#main-window[privatebrowsingmode=temporary][tabsintitlebar] .private-browsing-indicator-titlebar > .private-browsing-indicator,
> >+#main-window[privatebrowsingmode=temporary]:not([tabsintitlebar]) #TabsToolbar > .private-browsing-indicator {
> >+  display: block;
> >+}
> >+
> >+#TabsToolbar > .private-browsing-indicator {
> >+  background: url("chrome://browser/skin/privatebrowsing-mask-tabstrip.png") no-repeat center -3px;
> >+  -moz-margin-start: 4px;
> >+  width: 48px;
> >+  height: @tabHeight@;
> >+}
> 
> Not sure why you're using display:block. The indicator in the tab bar should
> definitely be -moz-box and you shouldn't specify a height.

Done.

(In reply to Dão Gottwald [:dao] from comment #44)
> Comment on attachment 8393192 [details] [diff] [review]
> Patch v2
> 
> >+#ifdef XP_WIN
> >+      <hbox class="private-browsing-indicator-titlebar">
> >+        <hbox class="private-browsing-indicator" />
> >+      </hbox>
> >+#endif
> 
> private-browsing-indicator-titlebar should be an id not a class. I'm not
> sure why you need an extra container, though. If it's just to differentiate
> the two private-browsing-indicator elements, you could instead set an
> attribute on them.

The extra container is necessary so that I can set position: absolute on it to take it out of the flow of the document, so that I can modify the position and height of the titlebar indicator without messing up the positioning of other things in the toolbars. Without it, for the taller graphic assets, I end up either pushing down the tabstrip, or clipping the indicator graphic.

> 
> nit: remove the space before />

Done.
Attachment #8393192 - Attachment is obsolete: true
Attachment #8393544 - Flags: review?(dao)
(In reply to Mike Conley (:mconley) from comment #45)
> > private-browsing-indicator-titlebar should be an id not a class. I'm not
> > sure why you need an extra container, though. If it's just to differentiate
> > the two private-browsing-indicator elements, you could instead set an
> > attribute on them.
> 
> The extra container is necessary so that I can set position: absolute on it
> to take it out of the flow of the document, so that I can modify the
> position and height of the titlebar indicator without messing up the
> positioning of other things in the toolbars. Without it, for the taller
> graphic assets, I end up either pushing down the tabstrip, or clipping the
> indicator graphic.

Why do you set position:absolute on #private-browsing-indicator-titlebar rather than directly on .private-browsing-indicator?
(In reply to Dão Gottwald [:dao] from comment #46)
> (In reply to Mike Conley (:mconley) from comment #45)
> > > private-browsing-indicator-titlebar should be an id not a class. I'm not
> > > sure why you need an extra container, though. If it's just to differentiate
> > > the two private-browsing-indicator elements, you could instead set an
> > > attribute on them.
> > 
> > The extra container is necessary so that I can set position: absolute on it
> > to take it out of the flow of the document, so that I can modify the
> > position and height of the titlebar indicator without messing up the
> > positioning of other things in the toolbars. Without it, for the taller
> > graphic assets, I end up either pushing down the tabstrip, or clipping the
> > indicator graphic.
> 
> Why do you set position:absolute on #private-browsing-indicator-titlebar
> rather than directly on .private-browsing-indicator?

Because that's the only way I could reliably control the offset of the indicator mask without running into very strange layout bugs.

For example, if I set position: absolute on just the indicator (and remove the #private-browsing-indicator-titlebar element), and then try to move it down by 1px via top, it doesn't move. When I set it to 2px, it _does_ move. When I set it to 1px again...it moves down (again) instead of up... it just starts to accumulate the offset. Very, very strange.

http://screencast.com/t/UUHuzbWrWbW

^-- It's easier to see than to explain. This is what I'm working around with the #private-browsing-indicator-titlebar element.

Keep in mind that this whole approach is temporary - private browsing mode is likely going to be restyled after Australis ships to more of a "stealth mode" look (pending UX specs and backlog triage). I think we should take this workaround for the time being, since it'll just get excised in the near future.
Comment on attachment 8393544 [details] [diff] [review]
Patch v2.1 (r+'d by jaws)

I think I'm just gonna go for this. If there are follow-up concerns, let's deal with it in some follow-up bugs.
Attachment #8393544 - Flags: review?(dao)
remote:   https://hg.mozilla.org/integration/fx-team/rev/94cc9fc1e356
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/94cc9fc1e356
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 31
Comment on attachment 8393544 [details] [diff] [review]
Patch v2.1 (r+'d by jaws)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis.


User impact if declined: 

A strange looking private browsing indicator on Windows.


Testing completed (on m-c, etc.): 

Lots of local testing on Windows XP, Windows 7 and 8, in classic, Luna, Aero Basic, and Aero Glass.


Risk to taking this patch (and alternatives if risky): 

Pretty low risk. The changes are almost 100% style, and pretty self-contained.


String or IDL/UUID changes made by this patch:

None.
Attachment #8393544 - Flags: review+
Attachment #8393544 - Flags: approval-mozilla-beta?
Attachment #8393544 - Flags: approval-mozilla-aurora?
Is it just me, or PB indicator is 1px bigger in maximized window and 1px smaller in restored window on Windows 8?
Flags: needinfo?(mconley)
Attachment #8393544 - Flags: approval-mozilla-beta?
Attachment #8393544 - Flags: approval-mozilla-beta+
Attachment #8393544 - Flags: approval-mozilla-aurora?
Attachment #8393544 - Flags: approval-mozilla-aurora+
Verified as fixed with Firefox 29 beta 2 on Win 7 x64.
(In reply to Peter Henkel [:Terepin] from comment #52)
> Is it just me, or PB indicator is 1px bigger in maximized window and 1px
> smaller in restored window on Windows 8?

Gah, good call, Terepin. You're right. I'll file a bug.
Flags: needinfo?(mconley)
Depends on: 987884
Attached image overlap_issue.jpg
With this fix on 29.0b2, it looks like if you have any icon up in the tab strip (such as the tab-groups icon.. which I believe used to default to that location), there is some icon overlap with this new location. Attaching a screenshot.

Windows 7.
(In reply to Tim Meader from comment #56)
> Created attachment 8396635 [details]
> overlap_issue.jpg
> 
> With this fix on 29.0b2, it looks like if you have any icon up in the tab
> strip (such as the tab-groups icon.. which I believe used to default to that
> location), there is some icon overlap with this new location. Attaching a
> screenshot.
> 
> Windows 7.

Shoot! That used to work - but it got broken during some of the review iterations. Thanks for catching that. I'll file a follow-up.
Depends on: 987929
Verified Fixed on Win 8 Firefox 31  [bugday-20140416]
Status: RESOLVED → VERIFIED
Verified on Windows 7 and Windows 8 32bit using Beta 30.0b3 (buildID: 20140508121358). 

Additional issue was found and logged separately: bug 1008183 .
You need to log in before you can comment on or make changes to this bug.