312527 - Need to reduce padding for bookmark menu items

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Aaron Slunt]

Now that (unfortunately) bug 243078 has been backed out, the old wallpaper patch
is around now and inflates the size of the bookmarks menu. I go from 1/2 of the
screen that the menu takes up to now 3/4. I have no screenshots, but it doesn't
take a rocket scientist to figure out inflated padding inflates the height of
the bookmark items.

I propose reducing the padding of teh bookmark items by 1 or 2px, so teh
bookmarks menu isn't killed.

Comment 1

[Bill Gianopoulos [:WG9s]]

This is NOT just an issue for classic!

There is at least 1 px too much padding between items in the bookmarks menu
under luna, if you comapre it to either the trunk builds with the native widgets
patch or IE.

Comment 2

[Bill Gianopoulos [:WG9s]]

Sorry to SPAM the bug, but wanted to clarify the issue here.

With WIndows/XP and the default XP theme (Luna) and the default Firefox theme,
the dropdown menu from the Bookmarks selection on the menu toolbar as well as
dropdown menus from the personal bookmarks toolbar (if you have folders in the
bookmark toolbar) exhibit the issue of extra padding.

The bookmarks sidebar has correct padding between items.

Comment 3

[Alfred Kayser]

It is not just the bookmarks menu, any menu now has too much padding.
Also the menuitems on the menubar should not expand to the full height of the
toolbar. They should just have enough padding.

Comment 4

[Alfred Kayser]

Attachment: Sort of patch showing what needs to be done to get the menu's right

This is to get the padding of the menu much more like other apps in the WinXP
Luna/WinXP themes...

Comment 5

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

This bug is about bookmark menu items, they are treated differently. Any other
issues are not this bug.

Comment 6

[Alfred Kayser]

Attachment: Screenshots of before, after, and how others look in WinXP

unpatched.gif: how it currently looks (in WinXP theme, using defaults)
patched.gif: how it looks with this patch (in WinXP theme, using defaults)
luna_with_ie.gif: how IE looks in luna
xp_with_ie.gif: how IE looks in WinXP (notice how close patched.gif looks to
this!)
littlefox.gif: how it looks in my theme (not so relevant for this patch ;-) )

If you compare patched.gif with luna_with_ie.gif, the only difference remaining
is that the checkmark for xp_with_ie.gif is one pixel lower (but that looks a
bit too low for me anyway).

So, this will solve the bookmark menu issue, and as well get the menu styling
much closer to the (default) WinXP styling.

Comment 7

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

Comment on attachment 199691 [details] [diff] [review]
Sort of patch showing what needs to be done to get the menu's right

Doug isn't a Firefox peer. It'd be better if you could provide a cvs diff, too.
Thanks for taking this! :)

Comment 8

[Alfred Kayser]

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

Comment 9

[Alfred Kayser]

Attachment: cvs diff -u8Np edition of the menu patch

Comment 10

[Alfred Kayser]

Comment on attachment 199691 [details] [diff] [review]
Sort of patch showing what needs to be done to get the menu's right

Moving review to cvs-version of the patch

Comment 11

[Alfred Kayser]

Comment on attachment 199694 [details] [diff] [review]
cvs diff -u8Np  edition of the menu patch

Also removed a tabstop.
Note, the border statements in menubar > menu were removed because they don't
have any effect because of the '  border: 1px solid transparent !important;
' above for menubar>menu.

Comment 12

[Asa Dotzler [:asa]]

not going to block the release on this but we'd consider a fully reviewed, trunk
verified patch for inclusion in this release.

Comment 13

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

Attachment: bookmark menu only

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to comment #12)
> not going to block the release on this but we'd consider a fully reviewed, trunk
> verified patch for inclusion in this release.

This is a branch only issue, so trying to test a fix on the turnk, although
ususally a good idea will not really work in this case.

However, it is not necessary in this case.  If you create a userChrome.css file
with the code in attachment 199835 [details] [diff] [review] you can easily test it that way.  It works
just fine for me and fixes the issue.

I have tested with variouws themes and extension under windows.  I don't have
the facilites to test under other Operating systems.

Comment 15

[Aaron Slunt]

Asa, this is for the branch only right now (as trunk has native menu rendering
on it).

Alfred, I filed this bug with the intentions of only getting the bookmarks menu
taken care of, since that's really the thing that can be so negatively affected
with that wallpaper crap. Nice patch, but I doubt it can land on the branch this
close in the game sense rc1 is maybe a week away. Gavin's patch is significantly
safer, and looks to be 0 risk to me, as well as it fixes the bug I reported, not
anything else. Still, you submitted a nice patch.

Re-nominating for rc1 since gavin's attached patch is for branch only, and is
extremely safe.

Asa, please remember it's a branch-only patch and cannot be tested on trunk
unless bug 243078 is backed out of it (and I hope it isn't).

Comment 16

[Joshua Welderson]

(In reply to comment #15)
> Asa, please remember it's a branch-only patch and cannot be tested on trunk
> unless bug 243078 is backed out of it (and I hope it isn't).
I would actually like to see the trunk patch for bug 243078 backed out, as it's
apparently caused needless regressions (GDI resource leaks) on Win9x and isn't
complete anyway.  I asked for the backout a few times, but nobody was available.
 If anyone reading this could do so, I'd be much obliged.

Comment 17

[Alfred Kayser]

Note that allthough the last patch may be safer, it will create inconsistency
between bookmarks menus and the other menus. 
As for testing, the code of my patch has been tested allready a lot in
forums.mozilla.org, section themes: 
http://forums.mozillazine.org/viewtopic.php?t=315361&postdays=0&postorder=asc&postsperpage=15&highlight=menu+padding&start=375

Comment 18

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

(In reply to comment #17)
> Note that allthough the last patch may be safer, it will create inconsistency
> between bookmarks menus and the other menus.

Have you tested the patch? Using Classic, the bookmarks menu items currently
have 3px of extra padding, my patch removes that extra padding and makes that
menu consistent with the others.

Comment 19

[Bill Gianopoulos [:WG9s]]

(In reply to comment #18)
> (In reply to comment #17)
> > Note that allthough the last patch may be safer, it will create inconsistency
> > between bookmarks menus and the other menus.
> 
> Have you tested the patch? Using Classic, the bookmarks menu items currently
> have 3px of extra padding, my patch removes that extra padding and makes that
> menu consistent with the others.

I tested using classic and luna with variu themes and it looks much better in
all cases.  This seems like a safe patch that fixes all the major issues and
looks like the way to go for 1.5.

A more perfect solution is in the works for a future release. 

Comment 20

[Asa Dotzler [:asa]]

we need the mikes, beltzner and connor, to say that removing that padding is
desirable before this goes anywhere.

Comment 21

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

Attachment: better patch

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Asa, mconnor, gavin and I talked this through and looked at a bunch of options,
and gavin's latest patch reflects what we feel is the right thing to do for 1.5

Comment 23

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

Trunk: mozilla/browser/themes/winstripe/browser/browser.css; new revision: 1.20; 

Comment 24

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

Backed out on trunk (oops!), checked in on branch.