888044 - When the menu bar is shown, the spacer below the caption buttons is unnecessary

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot of unnecessary spacer below the caption buttons

I believe the tabs and/or toolbar buttons would fit under the window controls in this case but I could be wrong about how much spacing is there.

Comment 1

[Brandon Cheng]

We should think about the different sizes the window buttons can be with different themes and DPI settings.

Comment 2

[:Gijs (he/him)]

(In reply to Brandon Cheng from comment #1)
> We should think about the different sizes the window buttons can be with
> different themes and DPI settings.

It sounds like we could just add code to the titlebar size calculations that, when the size of the actual titlebar content is smaller than that of the menubar + padding. In particular, here:

https://mxr.mozilla.org/projects-central/source/ux/browser/base/content/browser.js#4363

we probably want another if block that checks if titlebarContentHeight - fullMenuHeight <= 0, in which case we can hide the placeholder.

Comment 3

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

It's probably simpler - we can just use some CSS to display: none the placeholder if the menu is visible.

Comment 4

[Dulanja Wijethunga [:dwij]]

Hi Mike,
I'm new to this, I would like to work on this . Please some one assign me. 
Please tell me what to do first.

Comment 5

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

I believe Allan was interested in fixing this one, but never got assigned. Let's just clear it with him before we move forward.

Allan - were you still interested in hacking on this one?

Comment 6

[Allan Gardner (:Mathnerd314)]

I was, but it's finals week and I'm pretty busy... so if Dulanja wants to do it that's fine.

Comment 7

[Brandon Cheng]

(In reply to Mike Conley (:mconley) from comment #3)
> It's probably simpler - we can just use some CSS to display: none the
> placeholder if the menu is visible.

It depends on if the menubar size changes with DPI settings (which it doesn't).

This is actually beyond the scope of the reported bug, but DPI does cause problems like this -> http://i.imgur.com/K2o8zHc.png

I like Gijs's solution, but it doesn't account for larger than regular window buttons (but I guess neither did the code before).

Comment 8

[Brandon Cheng]

So a solution would probably be to calculate the size of that padding based on window control width. Then have an additional check for window control height (if it's shorter than where the tab top starts, padding will be 0).

Comment 9

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

Ok, going to let this percolate in my head for a little bit...

Comment 10

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

(In reply to Brandon Cheng from comment #7)
> (In reply to Mike Conley (:mconley) from comment #3)
> > It's probably simpler - we can just use some CSS to display: none the
> > placeholder if the menu is visible.
> 
> It depends on if the menubar size changes with DPI settings (which it
> doesn't).
> 
> This is actually beyond the scope of the reported bug, but DPI does cause
> problems like this -> http://i.imgur.com/K2o8zHc.png
> 
> I like Gijs's solution, but it doesn't account for larger than regular
> window buttons (but I guess neither did the code before).

Hm, I'm having no luck getting into the state with the massive window buttons. How did you do that, Brandon?

Comment 11

[Brandon Cheng]

(In reply to Mike Conley (:mconley) from comment #10)
> (In reply to Brandon Cheng from comment #7)
> > I like Gijs's solution, but it doesn't account for larger than regular
> > window buttons (but I guess neither did the code before).
> 
> Hm, I'm having no luck getting into the state with the massive window
> buttons. How did you do that, Brandon?

Let me correct myself quickly. The current Nightly handles DPI fine. Changing titlebar text size was what I was doing (I got confused between the two).

On Windows 8: Control Panel -> Appearance and Personalization -> Display -> Make text and other items larger or smaller (I set title bars to 24px).

I definitely saw the same option in Vista and 7. Not sure if Windows XP allows the user to do this.

Comment 12

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

(In reply to Brandon Cheng from comment #11)
> (In reply to Mike Conley (:mconley) from comment #10)
> > (In reply to Brandon Cheng from comment #7)
> > > I like Gijs's solution, but it doesn't account for larger than regular
> > > window buttons (but I guess neither did the code before).
> > 
> > Hm, I'm having no luck getting into the state with the massive window
> > buttons. How did you do that, Brandon?
> 
> Let me correct myself quickly. The current Nightly handles DPI fine.
> Changing titlebar text size was what I was doing (I got confused between the
> two).
> 
> On Windows 8: Control Panel -> Appearance and Personalization -> Display ->
> Make text and other items larger or smaller (I set title bars to 24px).
> 
> I definitely saw the same option in Vista and 7. Not sure if Windows XP
> allows the user to do this.

Ah, OK - I see. That overlapping issue in your image seems to be due to our caching of the original titlebar height on first window open. I am able to reproduce if I change the titlebar font size with Firefox open. This applies to the opened Firefox window and subsequent window opens in the same session.

The issue goes away once we close and re-open Firefox - then, the new titlebar size is sampled, and the proper height is maintained.

I'm pretty sure we should be OK discounting the edge-case whereby the user changes the titlebar font size with Firefox still open.

So I *think* the titlebar calculations should take care of the majority of the cases, and we can still go with hiding the placeholder if the menubar is being shown.

Comment 13

[Akshat Kedia [:aks]]

Can I work on this bug ?

Comment 14

[Dulanja Wijethunga [:dwij]]

Well, I think, I understood the problem ..  but I'm not clear about your suggestion to remove the space below the caption buttons .  What do you mean by hiding the placeholder ..

Comment 15

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #3)
> It's probably simpler - we can just use some CSS to display: none the
> placeholder if the menu is visible.

So, this doesn't work because, as people pointed out, depending on the height of the menubar and the height of the titlebar buttons, the menubar being visible might not be enough to be able to 'push down' the tabs far enough to be able to fit below the titlebar buttons.

(In reply to Dulanja Wijethunga [:dwij] from comment #14)
> Well, I think, I understood the problem ..  but I'm not clear about your
> suggestion to remove the space below the caption buttons .  What do you mean
> by hiding the placeholder ..

You can set the "hidden" attribute on the placeholders for the titlebar buttons to "true" (and remove the attribute when the menubar is hidden). We probably shouldn't change anything when the menubar is shown only temporarily (ie when it's autohidden and the user presses 'alt').

Comment 16

[:Gijs (he/him)]

> (In reply to Dulanja Wijethunga [:dwij] from comment #14)
> You can set the "hidden" attribute on the placeholders for the titlebar

In particular, this node:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#521

(you'd need to do this at runtime, though)

Comment 17

[Dulanja Wijethunga [:dwij]]

So you mean .. 
1) add hidden attribute to browser.xul .. default set to "false" 
2) add if statement in browser.js 
      if (menuTitlebarDelta <= 0) {
           // hide the placeholders
       }

I have no idea change the hidden attribute in browser.js file. Can you help me on this?

Comment 18

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

Sorry for the delays here - holiday happened.

So, akshat - looking at the bug comment history, I think dwij got here first, so I'm going to assign the bug to dwij. Feel free to hunt down another bug via Bugs Ahoy![1]

dwij: Gijs and I just talked this over, and we *think* this is the rule that needs to be added to browser/themes/windows/browser.css:

#toolbar-menubar[autohide="false"] + #TabsToolbar > .titlebar-placeholder[type="caption-buttons"] {
  display: none;
}

We *think* this is going to do the job, but we need you to implement and test it to be sure.

Would you mind trying that out in a Firefox build, and running it through some tests? Test with the menu hidden and not hidden, and then try some different OS font sizes.

Let me know if you need guidance on how to move forward on this.

[1]: http://www.joshmatthews.net/bugsahoy/?

Comment 19

[Dulanja Wijethunga [:dwij]]

Thanks Mike .. 

It is working. :) , I suppose that now I have to make a patch for this. I'm following video tutorial in this link [1]

Is it OK? Please give me some guidance on patch making and submitting.

[1]: http://codefirefox.com/

Comment 20

[Dulanja Wijethunga [:dwij]]

Attachment: rev1 - added a rule to themes/windows/browser.css in order to hide place holder

Comment 21

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

Hey dwij! I'll look at your patch soon - probably within the next 24 hours or so. I'm on vacation though, so my availability will be a bit spotty.

Comment 22

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

Comment on attachment 8355831 [details] [diff] [review]
rev1 -  added a rule to themes/windows/browser.css  in order to hide place holder

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

Looking good! Just some polish requests.

::: browser/themes/windows/browser.css
@@ +30,5 @@
>  #toolbar-menubar:not([autohide="true"]) ~ #TabsToolbar,
>  #toolbar-menubar[autohide="true"]:not([inactive]) ~ #TabsToolbar {
>    margin-top: 3px;
>  }
> +/*Hide the titlebar-placeholder when menubar is hidden- fixes the bug 888044 */

We're going to want newlines above and below this new rule.

You've flipped the signs in the comment - we're hiding the titlebar-placeholder when the menubar is _not_ autohidden.

For comments like this, we put a space after the first *, and include punctuation.  I think I'd prefer a comment like:

/* Hides the titlebar-placeholder underneath the window caption buttons when we're not autohiding the menubar. */

I don't think including the bug number adds much value in this case.

Comment 23

[Dulanja Wijethunga [:dwij]]

Attachment: rev2 - added a rule to themes/windows/browser.css in order to hide place holder

changed the comment.

Comment 24

[Dulanja Wijethunga [:dwij]]

(In reply to Mike Conley (:mconley) - vacationing from Jan 4 - 12 from comment #21)
> Hey dwij! I'll look at your patch soon - probably within the next 24 hours
> or so. I'm on vacation though, so my availability will be a bit spotty.
:) It's ok! take your vacation, sorry if I bother you much !

(In reply to Mike Conley (:mconley) - vacationing from Jan 4 - 12 from comment #22)

> 
> For comments like this, we put a space after the first *, and include
> punctuation.  I think I'd prefer a comment like:
> 
> /* Hides the titlebar-placeholder underneath the window caption buttons when
> we're not autohiding the menubar. */
> 
> I don't think including the bug number adds much value in this case.

So I added the comment like you mentioned about. I have attached another patch. Please review it when you are free.

Comment 25

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

Comment on attachment 8355877 [details] [diff] [review]
rev2 - added a rule to themes/windows/browser.css in order to hide place holder

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

Just styling nits now.

::: browser/themes/windows/browser.css
@@ +30,5 @@
>  #toolbar-menubar:not([autohide="true"]) ~ #TabsToolbar,
>  #toolbar-menubar[autohide="true"]:not([inactive]) ~ #TabsToolbar {
>    margin-top: 3px;
>  }
> +/* Hides the titlebar-placeholder underneath the window caption buttons when we're not autohiding the menubar. */

Break this onto two lines after the 80th character.

@@ +33,5 @@
>  }
> +/* Hides the titlebar-placeholder underneath the window caption buttons when we're not autohiding the menubar. */
> +#toolbar-menubar[autohide="false"] + #TabsToolbar > .titlebar-placeholder[type="caption-buttons"] {
> +  display: none;
> +}

Needs newlines before and after the rule.

Comment 26

[Dulanja Wijethunga [:dwij]]

You mean breaking the only comment ? 
just like
/* Hides the titlebar-placeholder underneath the window caption buttons when we
   are not autohiding the menubar. */

Comment 27

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

(In reply to Dulanja Wijethunga [:dwij] from comment #26)
> You mean breaking the only comment ? 
> just like
> /* Hides the titlebar-placeholder underneath the window caption buttons when
> we
>    are not autohiding the menubar. */

Correct!

Comment 28

[Dulanja Wijethunga [:dwij]]

Attachment: rev3 - added a rule to themes/windows/browser.css in order to hide place holder

Separated the comment from the 80th character and add 2 lines before and after the rule

Comment 29

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

Comment on attachment 8355889 [details] [diff] [review]
rev3 - added a rule to themes/windows/browser.css in order to hide place holder

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

Ok, this looks good to me. Thanks dwij!

Comment 30

[Dulanja Wijethunga [:dwij]]

Thank you very much Mike,highly appreciate your help ! .. :) 
If you can please vouch me!

https://mozillians.org/en-US/u/dulanja33/

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 8355889 [details] [diff] [review]
rev3 - added a rule to themes/windows/browser.css in order to hide place holder

Can you please not add this in the middle of two related rules?

By the way, does this take care of large caption buttons? E.g. does this work as expected on XP?

Comment 32

[Dulanja Wijethunga [:dwij]]

(In reply to Dão Gottwald [:dao] from comment #31)

> Can you please not add this in the middle of two related rules?
> 
Please explain it , and show me what to do.
> By the way, does this take care of large caption buttons? E.g. does this
I think it is ok with large caption. Is any thing wrong? 
> work as expected on XP?
well I have not checked this in XP, I'm using windows 8

Comment 33

[Dão Gottwald [:dao]]

(In reply to Dulanja Wijethunga [:dwij] from comment #32)
> (In reply to Dão Gottwald [:dao] from comment #31)
> 
> > Can you please not add this in the middle of two related rules?
> > 
> Please explain it , and show me what to do.

I mean these two rules:

> /* We want a 4px gap between the TabsToolbar and the toolbar-menubar when the
>    toolbar-menu is displayed, and a 16px gap when it is not. 1px is taken care
>    of by the (light) outer shadow of the tab, the remaining 3/15 are these margins. */
> #toolbar-menubar:not([autohide="true"]) ~ #TabsToolbar,
> #toolbar-menubar[autohide="true"]:not([inactive]) ~ #TabsToolbar {
>   margin-top: 3px;
> }
...
> #main-window[tabsintitlebar][sizemode="normal"][chromehidden~="menubar"] #toolbar-menubar ~ #TabsToolbar,
> #main-window[tabsintitlebar][sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] ~ #TabsToolbar {
>   margin-top: 15px;
> }

The comment even refers to both of them.

> > By the way, does this take care of large caption buttons? E.g. does this
> I think it is ok with large caption. Is any thing wrong?

I don't know, that's why I'm asking. Do the tabs fit under the caption buttons if those buttons are taller than they are by default on Windows 7 and 8?

Comment 34

[Dulanja Wijethunga [:dwij]]

I got it! I'll put the rule above the existing rule which having a comment like below.

>/* Hides the titlebar-placeholder underneath the window caption buttons when we
>   are not autohiding the menubar. */
>#toolbar-menubar[autohide="false"] + #TabsToolbar > .titlebar-placeholder[type="caption-buttons"] {
>  display: none;
>}

> /* We want a 4px gap between the TabsToolbar and the toolbar-menubar when the
>    toolbar-menu is displayed, and a 16px gap when it is not. 1px is taken care
>    of by the (light) outer shadow of the tab, the remaining 3/15 are these margins. */
> #toolbar-menubar:not([autohide="true"]) ~ #TabsToolbar,
> #toolbar-menubar[autohide="true"]:not([inactive]) ~ #TabsToolbar {
>   margin-top: 3px;
> }

(In reply to Dão Gottwald [:dao] from comment #33)

> I don't know, that's why I'm asking. Do the tabs fit under the caption
> buttons if those buttons are taller than they are by default on Windows 7
> and 8?

I set the titlebar to 24px. Then there is a gap between tabs and menubar.But tabs fit with caption button. Is there any thing to fix?

Comment 35

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #33)
> (In reply to Dulanja Wijethunga [:dwij] from comment #32)
> > (In reply to Dão Gottwald [:dao] from comment #31)
> > > By the way, does this take care of large caption buttons? E.g. does this
> > I think it is ok with large caption. Is any thing wrong?
> 
> I don't know, that's why I'm asking. Do the tabs fit under the caption
> buttons if those buttons are taller than they are by default on Windows 7
> and 8?

The TabsInTitlebar code pads the menubar at the bottom or the tabs bar at the top (I forget which, but I think it's the menubar) if the titlebar is taller than the menubar - see also Dulanja's mention of a 'gap' between the menubar and the tabbar. This was part of my discussion with mconley which led to comment #22 (which doesn't seem to reference the nature of our discussion, sorry about that). So, yes, we should be fine even with large caption buttons.

Comment 36

[Dulanja Wijethunga [:dwij]]

So, is there any thing I have to do ?
Shall I submit a patch again fixing what Dao mentioned in Comment 33

Comment 37

[Dão Gottwald [:dao]]

(In reply to Dulanja Wijethunga [:dwij] from comment #36)
> Shall I submit a patch again fixing what Dao mentioned in Comment 33

Yes, please.

Comment 38

[Dulanja Wijethunga [:dwij]]

Attachment: rev4 - added a rule to themes/windows/browser.css in order to hide place holder

Put the rule above the commented rule as shown in comment 33

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 8356557 [details] [diff] [review]
rev4 - added a rule to themes/windows/browser.css in order to hide place holder

Sorry, I just noticed another issue: You need to use :not([autohide="true"]) instead of [autohide="false"], because the autohide attribute isn't present at all by default on Windows XP.

Comment 40

[Dulanja Wijethunga [:dwij]]

Attachment: rev5 - added a rule to themes/windows/browser.css in order to hide place holder

changed the attribute [autohide="false"] to :not([autohide="true"])

Comment 41

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

Thanks dwij! And thanks Dao for setting us straight. :)

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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