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

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: MattN, Assigned: dwij)

Tracking

unspecified
Firefox 29
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M?][Australis:P4][good first bug][lang=css][lang=xul][mentor=mconley])

Attachments

(2 attachments, 4 obsolete attachments)

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.
We should think about the different sizes the window buttons can be with different themes and DPI settings.
(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.
It's probably simpler - we can just use some CSS to display: none the placeholder if the menu is visible.
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M?][Australis:P4][good-first-bug][lang=css][lang=xul][mentor=mconley]
Whiteboard: [Australis:M?][Australis:P4][good-first-bug][lang=css][lang=xul][mentor=mconley] → [Australis:M?][Australis:P4][good first bug][lang=css][lang=xul][mentor=mconley]
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.
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(mathnerd314...gph+mozilla)
I was, but it's finals week and I'm pretty busy... so if Dulanja wants to do it that's fine.
Flags: needinfo?(mathnerd314...gph+mozilla)
(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).
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).
Ok, going to let this percolate in my head for a little bit...
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(bcheng.gt)
(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.
Flags: needinfo?(bcheng.gt)
(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.
Can I work on this bug ?
Flags: needinfo?(mconley)
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 ..
Flags: needinfo?
(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').
Flags: needinfo?
> (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)
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?
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/?
Flags: needinfo?(mconley)
Assignee: nobody → dulanja33
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/
Status: NEW → ASSIGNED
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8355831 - Flags: review?(mconley) → review-
changed the comment.
Attachment #8355877 - Flags: review?(mconley)
(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 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.
Attachment #8355877 - Flags: review?(mconley) → review-
You mean breaking the only comment ? 
just like
/* Hides the titlebar-placeholder underneath the window caption buttons when we
   are not autohiding the menubar. */
(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!
Separated the comment from the 80th character and add 2 lines before and after the rule
Attachment #8355877 - Attachment is obsolete: true
Attachment #8355889 - Flags: review?(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!
Attachment #8355889 - Flags: review?(mconley) → review+
Thank you very much Mike,highly appreciate your help ! .. :) 
If you can please vouch me!

https://mozillians.org/en-US/u/dulanja33/
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?
Attachment #8355889 - Flags: review-
(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
(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?
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?
(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.
So, is there any thing I have to do ?
Shall I submit a patch again fixing what Dao mentioned in Comment 33
(In reply to Dulanja Wijethunga [:dwij] from comment #36)
> Shall I submit a patch again fixing what Dao mentioned in Comment 33

Yes, please.
Put the rule above the commented rule as shown in comment 33
Attachment #8356557 - Flags: review?(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.
Attachment #8356557 - Flags: review?(dao) → review-
changed the attribute [autohide="false"] to :not([autohide="true"])
Attachment #8356566 - Flags: review?(dao)
Attachment #8356566 - Flags: review?(dao) → review+
Attachment #8356557 - Attachment is obsolete: true
Attachment #8355889 - Attachment is obsolete: true
Thanks dwij! And thanks Dao for setting us straight. :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c7136dd407a6
Keywords: checkin-needed
Whiteboard: [Australis:M?][Australis:P4][good first bug][lang=css][lang=xul][mentor=mconley] → [Australis:M?][Australis:P4][good first bug][lang=css][lang=xul][mentor=mconley][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c7136dd407a6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M?][Australis:P4][good first bug][lang=css][lang=xul][mentor=mconley][fixed-in-fx-team] → [Australis:M?][Australis:P4][good first bug][lang=css][lang=xul][mentor=mconley]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.