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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Theme
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mconley, Assigned: Gijs)

Tracking

Trunk
Firefox 28
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M6], URL)

Attachments

(1 attachment, 3 obsolete attachments)

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.
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?
Flags: needinfo?(shorlander)
Hardware: x86_64 → All
Whiteboard: [Australis:M6]
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.
Flags: needinfo?(shorlander)
Assignee: nobody → mnoorenberghe+bmo
Whiteboard: [Australis:M6] → [Australis:M5]
Duplicate of this bug: 874864
(Assignee)

Comment 4

4 years ago
I should have a patch for this shortly.
Whiteboard: [Australis:M5] → [Australis:M6]
(Assignee)

Comment 5

4 years ago
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.
Assignee: mnoorenberghe+bmo → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #753288 - Flags: review?(mnoorenberghe+bmo)
Attachment #753288 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Blocks: 874819
(Assignee)

Comment 6

4 years ago
(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 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.
Attachment #753288 - Flags: review?(mconley) → review+
(Assignee)

Comment 8

4 years ago
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).
Attachment #753288 - Flags: review?(mnoorenberghe+bmo) → review?(dao)
(Assignee)

Comment 9

4 years ago
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? :-)
Attachment #753288 - Attachment is obsolete: true
Attachment #753288 - Flags: review?(dao)
Attachment #754857 - Flags: review?(dao)
(Assignee)

Comment 10

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

Carrying over r=mconley
Attachment #754857 - Flags: review+
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.
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
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.
Attachment #754857 - Attachment is obsolete: true
Attachment #754963 - Flags: review?(dao)
Attachment #754857 - Flags: review?(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"].
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
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
Attachment #754963 - Attachment is obsolete: true
Attachment #754963 - Flags: review?(dao)
Attachment #756143 - Flags: review?(dao)
(Assignee)

Comment 17

4 years ago
(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
(Assignee)

Comment 18

4 years ago
(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.

Updated

4 years ago
Attachment #756143 - Flags: review?(dao) → review+
(Assignee)

Comment 19

4 years ago
Pushed: https://hg.mozilla.org/projects/ux/rev/ccd9c2be155c
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
(Assignee)

Updated

4 years ago
Blocks: 879162

Updated

4 years ago
No longer blocks: 879162
Depends on: 879162
(Assignee)

Comment 20

4 years ago
https://hg.mozilla.org/mozilla-central/rev/ccd9c2be155c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.