939010 - OS X native fullscreen window has inactive appearance

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[jamie_larsson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131114040203

Steps to reproduce:

Click the fullscreen button in the upper-right corner


Actual results:

It went fullscreen, but the window appears to be inactive, even though it is active.


Expected results:

The window should appear active because it is active.

Comment 1

[Drew Willcoxon :adw]

Works for me, but I'm on 10.7.  Could you please post a screenshot showing the problem?

Comment 2

[Steven Michaud [:smichaud] (Retired)]

I see this, even on OS X 10.7 (with the 2013-11-18-09-41-34-mozilla-central nightly, which is the second of today's two m-c nightlies).  It happens clicking the fullscreen button or using Cmd+Shift+f.

I don't see it with an m-c nightly from about a month ago.  I'll look for a regression range.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Jamie, you apparently were testing with a ux-branch nightly.  You should have told us this.

I've found two regression ranges, one on the trunk and one on the ux branch:

firefox-2013-11-18-03-02-03-mozilla-central
firefox-2013-11-18-09-41-34-mozilla-central
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=beddd6d4bcdf&tochange=f2adb62d07eb

firefox-2013-10-07-04-02-02-ux
firefox-2013-10-08-04-02-04-ux
https://hg.mozilla.org/projects/ux/pushloghtml?fromchange=18fd395752cd&tochange=aa03fbc1149f

With this, it shouldn't be hard to find exactly which patch is at fault.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Mike, the trigger for this bug is almost certainly one of your patches for bug 924181, bug 924182 or bug 924201.

Comment 6

[Nils Maier [:nmaier]]

Attachment: bug-939010.diff

Before Australis, the styling came from:
https://hg.mozilla.org/releases/mozilla-release/file/66755f89d981/browser/themes/osx/browser.css#l2481
After Australis, the bits and pieces are still there, but the selector now disables them via :not([tabsintitlebar]), the styling is lost.
Re-enabling the stuff in [inFullScreen] makes it work again.

I also killed the -moz-mac-lion-theme bits, which are apparently supposed to add some padding to the top. However the code was defunct before, but would have become functional again, so I removed it...

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8336319 [details] [diff] [review]
bug-939010.diff

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

::: browser/themes/osx/browser.css
@@ -3574,5 @@
>  
>  /* Lion Fullscreen window styling */
>  @media (-moz-mac-lion-theme) {
> -  #navigator-toolbox[inFullscreen]:not(:-moz-lwtheme)::before {
> -    height: calc(@tabHeight@ + 11px) !important;

If you don't delete this block then you would be fixing the lack of padding mentioned in bug 932597. The padding should always be there in LWT and non-LWT modes. Bug 738335 will get rid of the top padding. Can you also fix the padding on PB mode in fullscreen? If you don't want to deal with the padding issues, we can reopen bug 932597 and do it there.

Comment 9

[Nils Maier [:nmaier]]

Attachment: Fix window colors (active/inactive) in fullscreen mode.

This patch fixes the active/inactive color stuff in native full screen, and also keeps the padding for all, native-themed, private browsing and lw-theme.
(Well, at least it does all that for me ;)

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8336461 [details] [diff] [review]
Fix window colors (active/inactive) in fullscreen mode.

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

::: browser/themes/osx/browser.css
@@ +2537,5 @@
>    min-height: @tabHeight@;
>  }
>  
> +#main-window:not(:-moz-any([privatebrowsingmode=temporary],[customizing],[tabsintitlebar])) #navigator-toolbox:not(:-moz-lwtheme)::before,
> +#main-window:not(:-moz-any([privatebrowsingmode=temporary],[customizing])) #navigator-toolbox[inFullscreen]:not(:-moz-lwtheme)::before {

Nit: add the new rule above the previous one to avoid changing blame on the existing rule.

Isn't this the only change required to fix this bug? i.e. what breaks without the rest of the changes? Aren't the others for bug 878436? If so, can we split the patch to make reviews and regression-finding easier.

@@ +3578,5 @@
> +  #navigator-toolbox[inFullscreen]::before {
> +    /* Add some top-padding in native full screen.
> +     * This corresponds to the padding of #titlebar, which is 9px, plus 1px to
> +     * account for a -1px margin-bottom from the non-lion rule. */
> +    height: calc(@tabHeight@ + 10px) !important;

expand the "10px" to "@spaceAboveTabbar@ + 1px" and then you can remove the hard-coded reference to "9px" in the comment.

@@ +3584,5 @@
> +  :-moz-any(#main-window[inFullscreen][privatebrowsingmode=temporary], #main-window[inFullscreen]:-moz-lwtheme) #navigator-toolbox {
> +    /* Add some top-padding in native full screen, for windows not themed by
> +     * the platform.
> +     * This corresponds to the padding of #titlebar, which is 9px. */
> +    padding-top: 9px !important;

I think you want @spaceAboveTabbar@ here and then you can remove the last line of the comment. 
I don't see why the !important is necessary? Can you show me an example where it is needed otherwise please remove it.

I would feel much better about the vertical tab position changes if there was a test for them but it seems like those changes may move ot bug 878436.

Comment 11

[Nils Maier [:nmaier]]

(In reply to Matthew N. [:MattN] from comment #10)
> Comment on attachment 8336461 [details] [diff] [review]
> Fix window colors (active/inactive) in fullscreen mode.
> 
> Review of attachment 8336461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/osx/browser.css
> @@ +2537,5 @@
> >    min-height: @tabHeight@;
> >  }
> >  
> > +#main-window:not(:-moz-any([privatebrowsingmode=temporary],[customizing],[tabsintitlebar])) #navigator-toolbox:not(:-moz-lwtheme)::before,
> > +#main-window:not(:-moz-any([privatebrowsingmode=temporary],[customizing])) #navigator-toolbox[inFullscreen]:not(:-moz-lwtheme)::before {
...
> Isn't this the only change required to fix this bug? i.e. what breaks
> without the rest of the changes? Aren't the others for bug 878436? If so,
> can we split the patch to make reviews and regression-finding easier.

With only this change, there will be a top padding, but only for non-lwtheme, non-PB windows. That's why the first patch removed the top padding, to have no padding for all for all windows, like it it right now (and also pre-Australis incl. Firefox Stable).
Then you asked me to actually keep the padding and fix it for the other windows, which I did, hence the other changes.

So either remove the padding here, and fix that in bug 878436, or fix it here, and resolve bug 878436 as a duplicate of this one?

> I would feel much better about the vertical tab position changes if there
> was a test for them but it seems like those changes may move ot bug 878436.

How would I test this?

Comment 12

[Matthew N. [:MattN]]

(In reply to Nils Maier [:nmaier] from comment #11)
> (In reply to Matthew N. [:MattN] from comment #10)
> > Comment on attachment 8336461 [details] [diff] [review]
> > Fix window colors (active/inactive) in fullscreen mode.
> > 
> > Review of attachment 8336461 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/themes/osx/browser.css
> > @@ +2537,5 @@
> > >    min-height: @tabHeight@;
> > >  }
> > >  
> > > +#main-window:not(:-moz-any([privatebrowsingmode=temporary],[customizing],[tabsintitlebar])) #navigator-toolbox:not(:-moz-lwtheme)::before,
> > > +#main-window:not(:-moz-any([privatebrowsingmode=temporary],[customizing])) #navigator-toolbox[inFullscreen]:not(:-moz-lwtheme)::before {
> ...
> > Isn't this the only change required to fix this bug? i.e. what breaks
> > without the rest of the changes? Aren't the others for bug 878436? If so,
> > can we split the patch to make reviews and regression-finding easier.
> 
> With only this change, there will be a top padding, but only for
> non-lwtheme, non-PB windows. That's why the first patch removed the top
> padding, to have no padding for all for all windows, like it it right now
> (and also pre-Australis incl. Firefox Stable).
> Then you asked me to actually keep the padding and fix it for the other
> windows, which I did, hence the other changes.

LWT already had the top-padding prior to this patch and PB can be addressed in a separate bug (bug 944229) since it's a different issue. You can see screenshots of various configurations of release, nightly and nightly with a patch to just the CSS from comment 10 in this zip: https://people.mozilla.org/~mnoorenberghe/bugs/939010.zip
The only one that changed with that change is 3_noLWT_fullScreen and it is the desired look. I will r+ a patch with just the addition of the rule from comment 10 (put it on top to avoid chaning blame please).
 
> So either remove the padding here, and fix that in bug 878436, or fix it
> here, and resolve bug 878436 as a duplicate of this one?

We can continue to fix the LWT issue in bug 878436. Lets keep this bug just about the titlebar colour and if it happens to add padding above the tabs in non-LWT windows without causing regressions, that's fine.

> > I would feel much better about the vertical tab position changes if there
> > was a test for them but it seems like those changes may move ot bug 878436.
> 
> How would I test this?

I've filed bug 944228 for adding tests for this since it's not related to the bug summary.

Comment 13

[Nils Maier [:nmaier]]

Attachment: Fix normal window colors in OSX native fullscreen

OK, here is a patch with just the rule from comment #10, that will add a padding to normal windows as a side-effect.

Requesting re-review, to make sure I didn't misunderstand you.

Comment 14

[Nils Maier [:nmaier]]

PS: The re-enabled padding is incorrect, of course. I assume you mean bug 878436 / bug to address this as well.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8339744 [details] [diff] [review]
Fix normal window colors in OSX native fullscreen

(In reply to Nils Maier [:nmaier] from comment #14)
> PS: The re-enabled padding is incorrect, of course. I assume you mean bug
> 878436 / bug to address this as well.

Good catch. Bug 878436 was more focused on the LWT image shifting so if you want to include a fix like the following, you can:

@@ -3587,7 +3588,8 @@ toolbarbutton.chevron > .toolbarbutton-m
 /* Lion Fullscreen window styling */
 @media (-moz-mac-lion-theme) {
   #navigator-toolbox[inFullscreen]:not(:-moz-lwtheme)::before {
-    height: calc(@tabHeight@ + 11px) !important;
+    /* The 1px accounts for a -1px margin-bottom from the non-lion rule */
+    height: calc(@tabHeight@ + @spaceAboveTabbar@ + 1px) !important;
   }
   #main-window[inFullscreen]:-moz-lwtheme {
     /* This additional padding matches the change in height in the pseudo-element


If you have commit access, remember to include "Australis" somewhere in the commit message so we know to back it out for the Holly branch. If it doesn't naturally fit, we have been putting "[Australis]" after the bug #.

Thanks and btw. you can take bug 878436 too if you want as I won't get to it within the next week.

Comment 16

[Nils Maier [:nmaier]]

Attachment: [Australis] Fix normal window colors in OSX native fullscreen. r=MattN

For checkin. Carrying over r=MattN

Comment 17

[Nils Maier [:nmaier]]

No, only L1 access for me so far.
Need somebody to check this in for me.

Comment 18

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/752458fcf4d6

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/752458fcf4d6

Comment 20

[Jordan]

I'm not sure where to post this, but the top border line of the navigation bar (where the tabs connect) is not the same color in fullscreen as it is when the broswer is windowed.  It actually looks better in fullscreen in my opinion where it is more grey than the darker black color it usually is.  Maybe this is a known bug posted elsewhere, but I couldn't find it...

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Jordan:

Open a new bug.  Include screenshots of the different colors.  Test both with today's mozilla-central nightly and the current release (FF 25.0.1), to see if that makes a difference.  Also report the version of OS X that you're using.