832290 - Proper styling for full-screen private windows on Mac

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: screenshot

[Environment]
* http://hg.mozilla.org/mozilla-central/rev/b52c02f77cf5
* OS X 10.8

[Step To Reproduce]
1. Go to OS X fullscreen with private browsing window.
2. Open many tabs.

[Result]
Please see screenshot.

Comment 1

[(no longer active)]

Dao, do we have a selector for the full-screen mode?  Would it be acceptable to add a right-padding to the tabstrip for example?

Comment 2

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/search?string=%3A-moz-full&find=.css&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 3

[(no longer active)]

Attachment: Patch (v1)

I'll attach two screenshots as well.

Comment 4

[(no longer active)]

Attachment: Lion and above, tabs on top, LTR screenshot

Comment 5

[(no longer active)]

Attachment: Lion and above, tabs on top, RTL screenshot

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 704747 [details] [diff] [review]
Patch (v1)

> @media (-moz-mac-lion-theme) {
>-  #main-window[privatebrowsingmode=temporary] {
>+  #main-window[privatebrowsingmode=temporary]:not([inFullscreen]) {
>     background-position: top right 40px;
>   }
> 
>-  #main-window[privatebrowsingmode=temporary]:-moz-locale-dir(rtl) {
>+  #main-window[privatebrowsingmode=temporary][inFullscreen] {
>+    background-position: top right 10px;
>+  }

:not([inFullscreen]) is redundant.

>+  #main-window[privatebrowsingmode=temporary]:not([inFullscreen]):-moz-locale-dir(rtl) {
>     background-position: top left 70px;
>   }
>+
>+  #main-window[privatebrowsingmode=temporary]:-moz-locale-dir(rtl) {
>+    background-position: top left 10px;
>+  }

Use [inFullscreen] in the second selector, get rid of :not([inFullscreen]).

>+  #main-window[privatebrowsingmode=temporary][inFullscreen] #TabsToolbar::after {
>+    display: -moz-box;
>+    content: "";
>+    width: 50px;
>+    background: transparent;
>+  }

background: transparent; is redundant.

How about tabs on bottom?

This patch only touches the @media (-moz-mac-lion-theme) block. Is this bug limited to OS X Lion?

Comment 7

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #6)
> background: transparent; is redundant.

I didn't have that initially, but without that there is a rendering glitch where the box is.  Adding background: transparent will fix that.  I was planning to file a follow-up bug on that once this gets landed, since I think that is a regression from Matt's recent work, but I'd like to keep background in this patch so that it would be low-risk enough for Aurora.

> How about tabs on bottom?

I need to test that, good point.

> This patch only touches the @media (-moz-mac-lion-theme) block. Is this bug
> limited to OS X Lion?

The full-screen mode was first introduced in OS X Lion.

Comment 8

[Dão Gottwald [:dao]]

There's a pre-Lion fullscreen mode, it just works differently.

Comment 9

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #8)
> There's a pre-Lion fullscreen mode, it just works differently.

Hmm, yeah I just tested, and the bug just occurs differently there.  There is a full-screen button there which overlays the privacy icon.

Comment 10

[(no longer active)]

Attachment: Lion and above, tabs on bottom, LTR screenshot

Comment 11

[(no longer active)]

Attachment: Lion and above, tabs on bottom, RTL screenshot

Comment 12

[(no longer active)]

Attachment: Pre-Lion, tabs on top, LTR screenshot

Comment 13

[(no longer active)]

Attachment: Pre-Lion, tabs on top, RTL screenshot

Comment 14

[(no longer active)]

Attachment: Pre-Lion, tabs on bottom, LTR screenshot

Comment 15

[(no longer active)]

Attachment: Pre-Lion, tabs on bottom, RTL screenshot

Comment 16

[(no longer active)]

Attachment: Patch (v2)

I think I've covered all of the possible permutations of the theme in play here.  This should be ready for review.

I took out most of the rules previously inside the lion query selector, since they're not really Lion specific and it was my mistake before to put them in that selector.  There is a non-Lion override for background-position in order to adjust for the fact that the full-screen window glyph doesn't exist on pre-Lion.  The non-Lion selector involving #window-controls is needed because of the special positioning of the #window-controls element which is only relevant in the tabs on top configuration.

I also filed bug 834742 on the rendering glitch that I mentioned in comment 7.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking since this is part of the new feature expected in 20.

Comment 18

[(no longer active)]

dao: ping?

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 706410 [details] [diff] [review]
Patch (v2)

>+#main-window[privatebrowsingmode=temporary] {
>+background-position: top right 40px;
>+}
>+
>+#main-window[privatebrowsingmode=temporary][inFullscreen] {
>+background-position: top right 10px;
>+}
>+
>+#main-window[privatebrowsingmode=temporary]:-moz-locale-dir(rtl) {
>+background-position: top left 70px;
>+}
>+
>+#main-window[privatebrowsingmode=temporary][inFullscreen]:-moz-locale-dir(rtl) {
>+background-position: top left 10px;
>+}

Indentation is messed up.

>+#main-window[privatebrowsingmode=temporary][inFullscreen] #nav-bar[tabsontop=false]::after {
>+  display: -moz-box;
>+  content: "";
>+  width: 50px;
>+}

Why are you using ::after here rather than -moz-padding-end?

> @media (-moz-mac-lion-theme) {
>+  #main-window[privatebrowsingmode=temporary][inFullscreen] #TabsToolbar::after {
>+    display: -moz-box;
>+    content: "";
>+    width: 50px;
>+    background: transparent;
>+  }

ditto

Also, isn't this supposed to be restricted to tabs on top?

Comment 20

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #19)
> Comment on attachment 706410 [details] [diff] [review]
> Patch (v2)
> 
> >+#main-window[privatebrowsingmode=temporary] {
> >+background-position: top right 40px;
> >+}
> >+
> >+#main-window[privatebrowsingmode=temporary][inFullscreen] {
> >+background-position: top right 10px;
> >+}
> >+
> >+#main-window[privatebrowsingmode=temporary]:-moz-locale-dir(rtl) {
> >+background-position: top left 70px;
> >+}
> >+
> >+#main-window[privatebrowsingmode=temporary][inFullscreen]:-moz-locale-dir(rtl) {
> >+background-position: top left 10px;
> >+}
> 
> Indentation is messed up.

I'll fix it.

> >+#main-window[privatebrowsingmode=temporary][inFullscreen] #nav-bar[tabsontop=false]::after {
> >+  display: -moz-box;
> >+  content: "";
> >+  width: 50px;
> >+}
> 
> Why are you using ::after here rather than -moz-padding-end?

Hmm, I suppose I could do that...

> Also, isn't this supposed to be restricted to tabs on top?

Well, you asked me to look into tabs on bottom in comment 6.  ;-)  That cause me to realize that tabs on bottom is also broken, so I morphed the bug and fixed them both in the same patch.

Comment 21

[Dão Gottwald [:dao]]

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #20)
> > Also, isn't this supposed to be restricted to tabs on top?
> 
> Well, you asked me to look into tabs on bottom in comment 6.  ;-)  That
> cause me to realize that tabs on bottom is also broken, so I morphed the bug
> and fixed them both in the same patch.

Take another look at the code I quoted. It adds padding to the tabs toolbar regardless of whether or not tabs at the top.

Comment 22

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #21)
> (In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #20)
> > > Also, isn't this supposed to be restricted to tabs on top?
> > 
> > Well, you asked me to look into tabs on bottom in comment 6.  ;-)  That
> > cause me to realize that tabs on bottom is also broken, so I morphed the bug
> > and fixed them both in the same patch.
> 
> Take another look at the code I quoted. It adds padding to the tabs toolbar
> regardless of whether or not tabs at the top.

Oh, I see what you mean.  You're right.

Comment 23

[(no longer active)]

It seems like the padding rule only works for the tabs toolbar.

Comment 24

[(no longer active)]

Attachment: Patch (v3)

Comment 25

[Dão Gottwald [:dao]]

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #23)
> It seems like the padding rule only works for the tabs toolbar.

Why would it only work for the tabs toolbar?

Comment 26

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #23)
> > It seems like the padding rule only works for the tabs toolbar.
> 
> Why would it only work for the tabs toolbar?

I'm not sure.  I'm not very familiar with how the XUL layout works.

Comment 27

[Dão Gottwald [:dao]]

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #26)
> (In reply to Dão Gottwald [:dao] from comment #25)
> > (In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #23)
> > > It seems like the padding rule only works for the tabs toolbar.
> > 
> > Why would it only work for the tabs toolbar?
> 
> I'm not sure.  I'm not very familiar with how the XUL layout works.

Is the padding overridden by some other code or applied correctly but not visible? Please check this out with DOM inspector or something.

Comment 28

[(no longer active)]

Attachment: Patch (v4)

Comment 29

[(no longer active)]

Attachment: Patch (v1)

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 710921 [details] [diff] [review]
Patch (v1)

> @media (-moz-mac-lion-theme) {
>+  #main-window[privatebrowsingmode=temporary][inFullscreen][tabsontop=true] #TabsToolbar {

#main-window[privatebrowsingmode=temporary][inFullscreen] #TabsToolbar[tabsontop=true] {

Comment 31

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd30e04a0ddc

Comment 32

[(no longer active)]

Comment on attachment 710921 [details] [diff] [review]
Patch (v1)

This is required for per-window PB.  It's only a theme change, and has minimal risk.

Comment 33

[(no longer active)]

Josh, I'd appreciate if you can land this on Aurora when it gets approved.

Thanks!

Comment 34

[Josh Matthews [:jdm]]

You got it.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd30e04a0ddc

Comment 36

[bhavana bajaj [:bajaj]]

Comment on attachment 710921 [details] [diff] [review]
Patch (v1)

Approving on aurora, per comment# 32.

Comment 37

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2b7ea57afb90

Comment 38

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0

Verified as fixed on Firefox 20 beta 6 (buildID: 20130320062118).