Need to reduce padding for bookmark menu items

RESOLVED FIXED in Firefox1.5rc1

Status

()

Firefox
Menus
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Aaron Slunt, Assigned: Gavin)

Tracking

({fixed1.8, regression})

1.5.0.x Branch
Firefox1.5rc1
fixed1.8, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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.
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.
Flags: blocking1.8rc1?
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.
(Reporter)

Updated

13 years ago
Keywords: regression

Comment 3

13 years ago
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.
Summary: need to reduce padding for bookmark menu items → Need to reduce padding for (bookmark) menu items

Comment 4

13 years ago
Created attachment 199691 [details] [diff] [review]
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...
This bug is about bookmark menu items, they are treated differently. Any other
issues are not this bug.
OS: Windows XP → All
Hardware: PC → All
Summary: Need to reduce padding for (bookmark) menu items → Need to reduce padding for bookmark menu items

Comment 6

13 years ago
Created attachment 199693 [details]
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.

Updated

13 years ago
Attachment #199691 - Flags: review?(dougt)
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! :)
Attachment #199691 - Flags: review?(dougt) → review?(mconnor)

Comment 8

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

Comment 9

13 years ago
Created attachment 199694 [details] [diff] [review]
cvs diff -u8Np  edition of the menu patch

Updated

13 years ago
Attachment #199691 - Attachment is obsolete: true

Comment 10

13 years ago
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
Attachment #199691 - Flags: review?(mconnor)

Comment 11

13 years ago
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.
Attachment #199694 - Flags: review?(mconnor)

Comment 12

13 years ago
not going to block the release on this but we'd consider a fully reviewed, trunk
verified patch for inclusion in this release.
Flags: blocking1.8rc1? → blocking1.8rc1-
Created attachment 199835 [details] [diff] [review]
bookmark menu only
Attachment #199835 - Flags: review?(mconnor)
(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.
(Reporter)

Comment 15

13 years ago
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).
Flags: blocking1.8rc1- → blocking1.8rc1?

Comment 16

13 years ago
(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

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

13 years ago
we need the mikes, beltzner and connor, to say that removing that padding is
desirable before this goes anywhere.
Created attachment 200031 [details] [diff] [review]
better patch
Assignee: nobody → gavin.sharp
Attachment #199694 - Attachment is obsolete: true
Attachment #199835 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #200031 - Flags: review?(mconnor)
Attachment #199694 - Flags: review?(mconnor)
Attachment #199835 - Flags: review?(mconnor)
Target Milestone: --- → Firefox1.5rc1

Updated

13 years ago
Attachment #200031 - Flags: review?(mconnor) → review+
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
Trunk: mozilla/browser/themes/winstripe/browser/browser.css; new revision: 1.20; 
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Attachment #200031 - Flags: approval1.8rc1?

Updated

13 years ago
Attachment #200031 - Flags: approval1.8rc1? → approval1.8rc1+

Updated

13 years ago
Flags: blocking1.8rc1?
Backed out on trunk (oops!), checked in on branch.
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.