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.
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.
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
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
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.
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)
*** Bug 307820 has been marked as a duplicate of this bug. ***
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 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.
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
(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.
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?
(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.
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.
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
Target Milestone: --- → Firefox1.5rc1
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?
Attachment #200031 - Flags: approval1.8rc1? → approval1.8rc1+
Backed out on trunk (oops!), checked in on branch.
You need to log in before you can comment on or make changes to this bug.