870849 - Gap between the tab-strip and top of titlebar is too large when in restored mode

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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

[Mike Conley (:mconley) (:⚙️)]

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

[Mike Conley (:mconley) (:⚙️)]

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 4

[:Gijs (he/him)]

I should have a patch for this shortly.

Comment 5

[:Gijs (he/him)]

Attachment: 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

[:Gijs (he/him)]

(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

[Mike Conley (:mconley) (:⚙️)]

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

[:Gijs (he/him)]

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

[:Gijs (he/him)]

Attachment: Patch with OS X hunk reverted, too

Reverting the OS X hunk as well. Dao, I take it you prefer this? :-)

Comment 10

[:Gijs (he/him)]

Comment on attachment 754857 [details] [diff] [review]
Patch with OS X hunk reverted, too

Carrying over r=mconley

Comment 11

[Dão Gottwald [:dao]]

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

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

Attachment: 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

[Dão Gottwald [:dao]]

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

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

Attachment: 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

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

(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.

Comment 19

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/ccd9c2be155c

Comment 20

[:Gijs (he/him)]

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