Proper styling for full-screen private windows on Mac

VERIFIED FIXED in Firefox 20

Status

()

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: tetsuharu, Assigned: Ehsan)

Tracking

Trunk
Firefox 21
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20+ verified)

Details

Attachments

(10 attachments, 4 obsolete attachments)

Posted image 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.
Blocks: 749394
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?
Posted patch Patch (v1) (obsolete) — Splinter Review
I'll attach two screenshots as well.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #704747 - Flags: review?(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?
Attachment #704747 - Flags: review?(dao) → review-
(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.
There's a pre-Lion fullscreen mode, it just works differently.
(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.
Summary: Tab bar item lap over on private browsing icon when OS X fullscreen → Proper styling for full-screen private windows on Mac
Attachment #706207 - Attachment description: Tabs on bottom, RTL screenshot → Lion and above, tabs on bottom, RTL screenshot
Attachment #706205 - Attachment description: Tabs on bottom, LTR screenshot → Lion and above, tabs on bottom, LTR screenshot
Attachment #704749 - Attachment description: Screenshot (RTL) → Lion and above, tabs on top, RTL screenshot
Attachment #704748 - Attachment description: Screenshot (LTR) → Lion and above, tabs on top, LTR screenshot
Posted patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #704747 - Attachment is obsolete: true
Attachment #706410 - Flags: review?(dao)
Tracking since this is part of the new feature expected in 20.
dao: ping?
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?
(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.
(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.
(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.
It seems like the padding rule only works for the tabs toolbar.
Posted patch Patch (v3) (obsolete) — Splinter Review
Attachment #706410 - Attachment is obsolete: true
Attachment #706410 - Flags: review?(dao)
Attachment #710881 - Flags: review?(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?
(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.
(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.
Posted patch Patch (v4) (obsolete) — Splinter Review
Attachment #710881 - Attachment is obsolete: true
Attachment #710881 - Flags: review?(dao)
Attachment #710918 - Flags: review?(dao)
Posted patch Patch (v1)Splinter Review
Attachment #710918 - Attachment is obsolete: true
Attachment #710918 - Flags: review?(dao)
Attachment #710921 - Flags: review?(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] {
Attachment #710921 - Flags: review?(dao) → review+
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.
Attachment #710921 - Flags: approval-mozilla-aurora?
Josh, I'd appreciate if you can land this on Aurora when it gets approved.

Thanks!
You got it.
https://hg.mozilla.org/mozilla-central/rev/fd30e04a0ddc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment on attachment 710921 [details] [diff] [review]
Patch (v1)

Approving on aurora, per comment# 32.
Attachment #710921 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.