872636 - Private browsing indicator overlaps the Australis tabstrip on OS X

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot of tabstrip overlap

With tabs in the titlebar of OS X (bug 865374), the private browsing indicator overlaps the tabstrip.

Shall the indicator move to the tabstrip or should we do something like the fullscreen button spacer such that the tabstrip doesn't go underneath the indicator?

Comment 1

[Stephen Horlander [:shorlander]]

Attachment: Indicator Spacer

For now we can just leave space for it(In reply to Matthew N. [:MattN] from comment #0)
> Created attachment 749952 [details]
> Screenshot of tabstrip overlap
> 
> With tabs in the titlebar of OS X (bug 865374), the private browsing
> indicator overlaps the tabstrip.
> 
> Shall the indicator move to the tabstrip or should we do something like the
> fullscreen button spacer such that the tabstrip doesn't go underneath the
> indicator?

For now we can just leave space so they don't overlap. It will have to move down once the window controls are moved though; and we need a more general solution for all platforms.

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Change offsets

Comment 3

[Matthew N. [:MattN]]

Attachment: Screenshots of v.1 on 10.6

Comment 4

[:Gijs (he/him)]

Comment on attachment 759057 [details] [diff] [review]
v.1 Change offsets

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

Generally, this looks fine, although I wish UX didn't have a completely broken DOMI so that I could understand what your display: inherit bit is doing. However, the screenshots look good, and it looks good on my mountain lion machine across all 4 permutations, so r+. More documentation for how this CSS works would be good, though.

Comment 5

[(no longer active)]

Should Dao review this patch as well?  He caught a lot of problems with my changes here earlier...

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 759057 [details] [diff] [review]
v.1 Change offsets

>+#main-window[privatebrowsingmode=temporary][inFullscreen] .titlebar-placeholder[type="fullscreen-button"] {
>+  display: inherit;
>+}

I don't think I understand this. What does this have to do with private browsing?

Comment 7

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 759057 [details] [diff] [review]
> v.1 Change offsets
> 
> >+#main-window[privatebrowsingmode=temporary][inFullscreen] .titlebar-placeholder[type="fullscreen-button"] {
> >+  display: inherit;
> >+}
> 
> I don't think I understand this. What does this have to do with private
> browsing?

This makes a bit more sense with ":-moz-locale-dir(ltr)" so assume that it's appended as I will do that if you agree.

In private browsing mode in LTR on Lion+, the PB indicator is between the tabstrip and the fullscreen button in the titlebar in non-fullscreen Windows. When going fullscreen I don't think we want to move the indicator to the right to take up the space where the fullscreen button was because: 
a) it causes more shifting during the lion fullscreen transition 
b) it looks a bit unusual so close to the corner
c) if I don't use the existing placeholder to take up that space then I need to add another ruleset to adjust the offset by another 20px in fullscreen mode to take that space into account.

In non-PB windows since there is no indicator, it seemed like we should continue to let the tabstrip go to the edge of the window otherwise people will consider it a bug that there is a space for no reason.

Thoughts?

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 759057 [details] [diff] [review]
v.1 Change offsets

>+#main-window[privatebrowsingmode=temporary][inFullscreen] .titlebar-placeholder[type="fullscreen-button"] {
>+  display: inherit;
>+}

please document why you're doing this and use -moz-box instead of inherit

Comment 9

[Matthew N. [:MattN]]

OK. Changes made and comments added. Thanks.

https://hg.mozilla.org/projects/ux/rev/5c9ac2fcd9b2

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/5c9ac2fcd9b2