348698 - Menu bar no longer expands to accomodate icons when toolbars are customized

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Majken Connor [:Kensie]]

In 1.5.0.x if you choose to move icons (large, I believe) to the menu bar (where File Edit etc. are) it makes the menu bar as tall as the toolbar that generally has the back, forward, refresh etc. icons.

Under the new theme, the bar does not expand and the icons reach the top and bottom edges of the bar.  Can see this on Windows, don't have Linux to test on.

Comment 1

[Majken Connor [:Kensie]]

note- this also depends on what font size you're using, i.e. if your menu fonts are large enough you won't see that the menu bar is too small (and therefor doesn't expand) as in your case it won't be.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Attachment: Screenshot of the problem

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I don't see this on Linux (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060815 BonEcho/2.0b1).

Comment 4

[Ria Klaassen (not reading all bugmail)]

*** Bug 349156 has been marked as a duplicate of this bug. ***

Comment 5

[Ria Klaassen (not reading all bugmail)]

Attachment: screenshot

Trying to imitate the appearance of the browser in the mock-up, if I change padding to 3px for top and 2px for bottom, also the other toolbars (navigation toolbar, extra toolbar) will look better. This appears to be forgotten in the code (or it is left out for some reason).

.toolbarbutton-1, .toolbarbutton-menubutton-button {
  padding-top: 3px !important;
  padding-bottom: 2px !important;
}

Comment 6

[Mike Connor [:mconnor]]

Gavin, please take a look at the old padding/margin values and see how we compare.  I'm not sure we want to go back completely, but we can stand a bit bigger clickable areas vertically.

Comment 7

[Mike Connor [:mconnor]]

*** Bug 350526 has been marked as a duplicate of this bug. ***

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This patch removes the overriding of the standard toolbarbutton padding across all toolbarbuttons, and adds two rules for the back and forward buttons specifically. This results in more padding around all the toolbarbuttons than there currently is. I picked 5px to match the previous theme, but perhaps 3 or 4 pixels would be preferred - I'll attach a screenshot of all the options for comparison.

Myk tested this on linux, and it has no negative effects since the native themeing overrides these rules there anyways. I've tested this with Windows XP Luna and Classic. I also tested the back/forward button changes in RTL mode.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: comparison screenshot

Comment 10

[Majken Connor [:Kensie]]

is it just me or do the toolbars have padding that add on to the button padding?  Notice if you drag the url bar to the current menubar the menubar expands a little for it.  It also seems in Gavin's screenshots that the toolbar with the url and search bars is slightly taller than the menubar with the added icon.

Comment 11

[Majken Connor [:Kensie]]

never mind, it's an optical illusion as the toolbar has a bevel on top whereas the menubar has the dark blue on top.

Comment 12

[Bryan]

Gavin,

Don't forget to test adding links to the toolbar.  If I move a link from the bookmark toolbar to the navigation toolbar, the navigation toolbar "grows" vertically (at least this occurs under classic view.  Make sure the min-height takes that into account.

~B

Comment 13

[Bryan]

BTW, IMO 3px looks just about right!  Thanks for the time you're putting into this...

~B

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 235967 [details] [diff] [review]
patch

r+a=me on this theme patch.  went over the options with beltzner, and we're going to go with 5px for now.  thanks for cleaning up the margin/padding thing so the targets are bigger again.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Checked in on the branch.
mozilla/browser/themes/winstripe/browser/browser.css 	1.17.2.45

Comment 16

[Bryan]

(In reply to comment #14)
> (From update of attachment 235967 [details] [diff] [review] [edit])
> r+a=me on this theme patch.  went over the options with beltzner, and we're
> going to go with 5px for now.  

Could we possibly go with 3px as 5px seems offly "off", especially in Classic?  I believe there is a bug coming up for this.

~B

Comment 17

[ronin.achilles]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 235967 [details] [diff] [review] [edit] [edit])
> > r+a=me on this theme patch.  went over the options with beltzner, and we're
> > going to go with 5px for now.  
> 
> Could we possibly go with 3px as 5px seems offly "off", especially in Classic? 
> I believe there is a bug coming up for this.
> 
> ~B
> 

Bug 350736 (Since 0830 build, the Toolbars look very large and ugly)

Comment 18

[Peter Kasting]

*** Bug 351019 has been marked as a duplicate of this bug. ***