Last Comment Bug 870849 - Gap between the tab-strip and top of titlebar is too large when in restored mode
: Gap between the tab-strip and top of titlebar is too large when in restored mode
Status: RESOLVED FIXED
[Australis:M6]
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All Windows 7
-- normal (vote)
: Firefox 28
Assigned To: :Gijs
:
: Dão Gottwald [:dao]
Mentors:
https://people.mozilla.com/~shorlande...
: 874864 (view as bug list)
Depends on: 879162
Blocks: australis-tabs-win 874819
  Show dependency treegraph
 
Reported: 2013-05-10 09:00 PDT by Mike Conley (:mconley)
Modified: 2013-11-18 13:09 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.24 KB, patch)
2013-05-23 07:51 PDT, :Gijs
mconley: review+
Details | Diff | Splinter Review
Patch with OS X hunk reverted, too (2.46 KB, patch)
2013-05-28 08:53 PDT, :Gijs
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
Patch v1.2 (2.75 KB, patch)
2013-05-28 12:48 PDT, :Gijs
no flags Details | Diff | Splinter Review
Patch v1.3 (2.65 KB, patch)
2013-05-30 12:42 PDT, :Gijs
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Mike Conley (:mconley) 2013-05-10 09:00:01 PDT
Now that the AppMenu button is gone, the gap between the top of the tab-strip and the top of the window is too big. According to the spec here:

https://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windows7-mainWindow.html

It's supposed to be 16px. I think it's quite a few pixels greater than that at the moment.
Comment 1 User image Mike Conley (:mconley) 2013-05-10 09:02:48 PDT
Steven, it looks like we're revisiting this gap again. :)

So, looking at this: http://cl.ly/image/423u0z1O0c3x

It looks like the gap between the menubar or window buttons (whichever is lowest) is supposed to be 4px. We had agreed upon that before, and that's the current state of things.

So when the menu bar is hidden... *then* is the gap between the tabstrip and the top of the window 16px? If so, that means that, depending on the OS font size, when displaying the menubar, the tabstrip will get pushed down. Is that OK?
Comment 2 User image Mike Conley (:mconley) 2013-05-17 10:50:16 PDT
From Stephen via IRC:

"...it should be 16px from the top or 4px from the bottom of the menubar. If a user has both increased their system font size AND turned back on the menu bar then whatever the result is will have to be fine."

Cool - thanks Stephen.
Comment 3 User image Mike Conley (:mconley) 2013-05-22 07:05:33 PDT
*** Bug 874864 has been marked as a duplicate of this bug. ***
Comment 4 User image :Gijs 2013-05-23 07:18:26 PDT
I should have a patch for this shortly.
Comment 5 User image :Gijs 2013-05-23 07:51:29 PDT
Created attachment 753288 [details] [diff] [review]
Patch

As far as I can tell, this makes everything match the design (tested with overlaid layers in GIMP). Keep in mind we get 1 pixel of space because of the reeeeaaaally light outside shadow/border/thing on the tabs.

This reverts some of the changes from bug 813802, which as a side-effect will fix bug 874819.
Comment 6 User image :Gijs 2013-05-23 09:50:38 PDT
(Moving this conversation here so we can adjust the patch if necessary)

(In reply to Dão Gottwald [:dao] from bug 874819 comment #9)
> (In reply to :Gijs Kruitbosch from bug 874819 comment #8)
> > Comment on attachment 752770 [details] [diff] [review]
> > Patch v1
> > 
> > (In reply to Dão Gottwald [:dao] from bug 874819 comment #7)
> > > Comment on attachment 752770 [details] [diff] [review]
> > > Patch v1
> > > 
> > > >--- a/toolkit/content/xul.css
> > > >+++ b/toolkit/content/xul.css
> > > >@@ -272,20 +272,24 @@ toolbar[customizing="true"][hidden="true
> > > > 
> > > > %ifdef XP_MACOSX
> > > > toolbar[type="menubar"] {
> > > >   visibility: collapse;
> > > > }
> > > 
> > > Should this be reverted as well?
> > 
> > I don't think so. OS X isn't really affected by any of this.
> 
> What was the point of changing this for OS X in the first place, if not
> consistency with the autohiding behavior on other platforms, which you
> reverted?

Good point, I don't know! If consistency is the only reason, and if that's more important than hg blame, let's change that back, too. The only other thing I can think of is that visibility:collapse might be a hair's breadth faster for the layout engine to deal with.
Comment 7 User image Mike Conley (:mconley) 2013-05-23 10:07:55 PDT
Comment on attachment 753288 [details] [diff] [review]
Patch

Yes, this is way better. Thanks for getting this fixed! r=me for the browser/ changes. You'll want dao's r+ on the changes to toolkit stuff.
Comment 8 User image :Gijs 2013-05-24 01:41:21 PDT
Comment on attachment 753288 [details] [diff] [review]
Patch

Shifting this review to Dao then. If this gets review, please feel free to land it; I won't be back until Tuesday (CEST).
Comment 9 User image :Gijs 2013-05-28 08:53:39 PDT
Created attachment 754857 [details] [diff] [review]
Patch with OS X hunk reverted, too

Reverting the OS X hunk as well. Dao, I take it you prefer this? :-)
Comment 10 User image :Gijs 2013-05-28 08:54:08 PDT
Comment on attachment 754857 [details] [diff] [review]
Patch with OS X hunk reverted, too

Carrying over r=mconley
Comment 11 User image Dão Gottwald [:dao] 2013-05-28 09:27:13 PDT
Comment on attachment 754857 [details] [diff] [review]
Patch with OS X hunk reverted, too

>-#main-window[sizemode="normal"] #toolbar-menubar,
> /* We want a 4px gap between the TabsToolbar and the toolbar-menubar when the
>    toolbar-menu is displayed in either restored or maximized mode. */
> #main-window:not([sizemode="fullscreen"]) #toolbar-menubar:-moz-any(:not([autohide="true"]), :not([inactive="true"])) {
>-  margin-bottom: 4px;
>+  margin-bottom: 3px;
>+}

The comment still says 4px.

Is #main-window:not([sizemode="fullscreen"]) needed, given that the menu bar is collapsed in fullscreen mode?

Is :-moz-any(:not([autohide="true"]), :not([inactive="true"])) still needed here?

> toolbar[type="menubar"][autohide="true"] {
>   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide");
>+  overflow:hidden;
> }

As I mentioned before, add a space after the colon.

> toolbar[type="menubar"][autohide="true"][inactive="true"]:not([customizing="true"]) {
>-  visibility: collapse;
>+  border-style: none !important;
>+  -moz-appearance: none !important;
>+  min-height: 0 !important;
>+  height: 0 !important;
> }
> %endif

Please restore how these properties were ordered originally. min-height and height should come first as they're the gist of this code.
Comment 12 User image :Gijs 2013-05-28 11:26:37 PDT
(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 754857 [details] [diff] [review]
> Patch with OS X hunk reverted, too
> 
> >-#main-window[sizemode="normal"] #toolbar-menubar,
> > /* We want a 4px gap between the TabsToolbar and the toolbar-menubar when the
> >    toolbar-menu is displayed in either restored or maximized mode. */
> > #main-window:not([sizemode="fullscreen"]) #toolbar-menubar:-moz-any(:not([autohide="true"]), :not([inactive="true"])) {
> >-  margin-bottom: 4px;
> >+  margin-bottom: 3px;
> >+}
> 
> The comment still says 4px.

I will add a new copy of the patch adding more commentary regarding this, and also address the other nits


> Is #main-window:not([sizemode="fullscreen"]) needed, given that the menu bar
> is collapsed in fullscreen mode?

I thought so as well but didn't want to touch this. If you also think it can be removed, I shall do so.

> 
> Is :-moz-any(:not([autohide="true"]), :not([inactive="true"])) still needed
> here?

Yes, because on aero in maximized mode with no menubar visible (autohide=true and inactive=true), we don't want any margin at all.

Alternatively, we could remove the :not() part and add another rule here specifying margin-bottom: 0 for that situation, which may be clearer.
Comment 13 User image :Gijs 2013-05-28 12:48:45 PDT
Created attachment 754963 [details] [diff] [review]
Patch v1.2

This should take care of the nits. Dao, let me know if you prefer to have a separate rule to avoid the -moz-any(:not(), :not()) bits.
Comment 14 User image Dão Gottwald [:dao] 2013-05-30 08:01:29 PDT
Comment on attachment 754963 [details] [diff] [review]
Patch v1.2

>+#main-window #toolbar-menubar:-moz-any(:not([autohide="true"]), :not([inactive="true"])) {

#main-window shouldn't be needed here.

I'm somewhat confused by :-moz-any(:not([autohide="true"]), :not([inactive="true"])). Isn't :not([inactive]) sufficient here?

>+#main-window[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive="true"] {
>+  margin-bottom: 15px;
> }

Should this be an em value, i.e. should this gap grow with larger fonts?

By the way, you can just use [inactive] instead of [inactive="true"].
Comment 15 User image :Gijs 2013-05-30 08:12:24 PDT
(In reply to Dão Gottwald [:dao] from comment #14)
> Comment on attachment 754963 [details] [diff] [review]
> Patch v1.2
> 
> >+#main-window #toolbar-menubar:-moz-any(:not([autohide="true"]), :not([inactive="true"])) {
> 
> #main-window shouldn't be needed here.

Oops, good point.

> 
> I'm somewhat confused by :-moz-any(:not([autohide="true"]),
> :not([inactive="true"])). Isn't :not([inactive]) sufficient here?

I'm not sure. Logically, I guess it shouldn't be (as anything that's not autohidden shouldn't ever be inactive) but I'll test to be sure.

> >+#main-window[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive="true"] {
> >+  margin-bottom: 15px;
> > }
> 
> Should this be an em value, i.e. should this gap grow with larger fonts?

I don't think so, because the gap is there for the user to be able to drag the window; not to make space for text.

> By the way, you can just use [inactive] instead of [inactive="true"].

Fair point.
Comment 16 User image :Gijs 2013-05-30 12:42:02 PDT
Created attachment 756143 [details] [diff] [review]
Patch v1.3

This should address the feedback from comment 14, but it turns out my build env (VS2012) can't build things for Windows XP, which means I ended up sending this to try so I can check that it doesn't break anything on XP where the menubar is always present: https://tbpl.mozilla.org/?tree=Try&rev=b8e2ad57703d
Comment 17 User image :Gijs 2013-05-30 13:45:59 PDT
(In reply to :Gijs Kruitbosch from comment #16)
> https://tbpl.mozilla.org/?tree=Try&rev=b8e2ad57703d

Something weird happened, no builds happened, I repushed: https://tbpl.mozilla.org/?tree=Try&rev=6b29eee4d4e1
Comment 18 User image :Gijs 2013-05-30 15:36:14 PDT
(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to :Gijs Kruitbosch from comment #16)
> > https://tbpl.mozilla.org/?tree=Try&rev=b8e2ad57703d
> 
> Something weird happened, no builds happened, I repushed:
> https://tbpl.mozilla.org/?tree=Try&rev=6b29eee4d4e1

I can confirm this works as expected on Windows XP.

Note You need to log in before you can comment on or make changes to this bug.