Some of the toolbar icons in Toolbar.png have uneven padding built into the icon image region. For example, the reload and stop icons have two pixels of padding on the right-hand side of the image region but no padding on the left-hand side. And all of the icons appear to have one pixel of padding on the bottom of the image region and no padding on the top. Ideally these images should contain no built-in padding, so that we have the maximum flexibility to apply padding via CSS when using the images in the theme. If that's difficult or has undesirable consequences, however, then we should at least give images an even amount of padding where possible. For example, in the aforementioned reload and stop icons, there should be one pixel of padding on the left and one pixel of padding on the right instead of none on the left and two on the right.

This screenshot shows the stop, reload, and home icons magnified 800%, with ruler guides showing the edges of the image regions. All three icons have no pixels of padding on top and one pixel on the bottom (it may look like two pixels on the bottom, but there's a light drop shadow taking up one of those pixels). The stop and reload icons also have the aforementioned uneven left-right padding, with two pixels on the right and none on the left.

The problem with icons that have uneven padding is that they don't appear centered on the toolbar buttons. Here are those three icons as they appear in Firefox on Linux in the hover state. Both the stop and the reload icons appear uncentered in their buttons.

This seems more like a "would take patch" than a blocker, but it's worth having drivers take a look at it, if only so they know what they'll be shipping with.

Jay: he's right, the stop and reload icons should both be 1px to the right, and we should make sure that all icons are centered within their moz-regions in these types of files.

The latest Toolbar.png in the current nightly has all the icons centered. The space below them, which you can't see in the screenshot of magnified icons, is because they all have drop shadows directly under them.

fixed1.8.1

(In reply to comment #5) > The latest Toolbar.png in the current nightly has all the icons centered. The > space below them, which you can't see in the screenshot of magnified icons, is > because they all have drop shadows directly under them. That's true for the second to last row of the 24px icons, but the last row of those icons is completely transparent in almost every case. So the icons still look uncentered vertically, even though they now look centered horizontally. Given the composition of the icons, presumably it would be pretty hard to recreate them all to use that last row of the space allotted to them, so we should consider implementing one of the following two CSS solutions: 1. Change the -moz-image-regions to include only the first 23 rows of each icon instead of all 24 rows. 2. Add an additional pixel of padding to the tops of the icons. As for the 16px icons, they appear to be centered vertically, but the rightmost columns of many of them are completely transparent, so they actually only occupy 15 of the 16 columns allotted to them. Here again, we could either change the -moz-image-regions to include only the first 15 columns of each icon, or we could add an additional pixel of padding to the left sides of the icons.

(In reply to comment #7) > Given the composition of the icons, presumably it would be pretty hard to > recreate them all to use that last row of the space allotted to them That would be pretty much impossible without redesigning them all :) The icons are pretty much square, so they can't use more vertical space than horizontal (or vice versa). Extending them all down by one pixel means redoing them all and that's a lot of icons. I agree that the best thing here is a CSS fix, but I'm concerned about what it means to Extension developers who expect to use the full 24px of space and find they can't. I hadn't noticed the lack of a pixel at the bottom of the icons and I'm wondering if anyone who wasn't so intimately involved in the implementation would - in other words, could we just leave it? > 2. Add an additional pixel of padding to the tops of the icons. That's an easier change than actually extending them all and could be arranged, though it's still a few hours to go through all the icons and do it cleanly. > As for the 16px icons, they appear to be centered vertically, but the rightmost > columns of many of them are completely transparent, so they actually only > occupy 15 of the 16 columns allotted to them. In those cases, is the leftmost column transparent as well? If it is, then they're still centered, but I'm guessing it isn't or you wouldn't have mentioned it :) I think padding the icons is the better approach, again, so that the CSS is still 16x16.

Here's a patch that implements the simpler of the two CSS solutions: it adds one pixel of top padding to the 24px icons and one pixel of left padding to the 16px icons. This works reasonably well for the 24px icons, almost every one of which contains one pixel of bottom padding (and the rest of which contain only a very faint drop-shadow in that row). But it doesn't work as well for the 16px icons, which aren't nearly as consistent. For example, the back and forward 16px buttons actually contain one extra pixel of *left* padding, while the bookmark sidebar button contains *two* pixels of right padding. Furthermore, because of the lightness/transparency of their drop shadows, in a few cases the icons look the most centered when they're actually one pixel off. Thus I fear that the right fix for this bug is to optimize each icon individually, changing each one's -moz-image-region coordinates or adding padding to center it within its button. And if we care about the relative size of buttons (do we?), then we need to take that into account, probably by making every 24px icon consume 25 pixels of space and every 16px icon consume 17 pixels of space.

> I agree that the best thing here is a CSS fix, but I'm concerned about what > it means to Extension developers who expect to use the full 24px of space > and find they can't. I hadn't noticed the lack of a pixel at the bottom of > the icons and I'm wondering if anyone who wasn't so intimately involved in > the implementation would - in other words, could we just leave it? Well, I wasn't intimately involved in the implementation until recently, and I initially noticed the misalignment in the reload icon when I wasn't looking for it. In fact, to me it seems more pronounced the more casually I glance at it. Once I start to stare at the icon intently I don't see the misalignment quite as well. > > 2. Add an additional pixel of padding to the tops of the icons. > > That's an easier change than actually extending them all and could be > arranged, though it's still a few hours to go through all the icons > and do it cleanly. Err, I meant to suggest that we could do this via CSS. And based on some experimentation (see comment 9), it seems that we can do it via CSS, although for the 16px icons we'll have to have icon-specific rules to account for variation in the amount of padding within the images themselves. > > As for the 16px icons, they appear to be centered vertically, but the > > rightmost columns of many of them are completely transparent, so they > > actually only occupy 15 of the 16 columns allotted to them. > > In those cases, is the leftmost column transparent as well? If it is, then > they're still centered, but I'm guessing it isn't or you wouldn't have > mentioned it :) Right. In a couple cases the padding is even (f.e. the Cut button has 2px of padding on each side, while the new window button has no pixels of padding on either side). But in most cases there is no padding on the left and either one or two pixels of padding on the right. > I think padding the icons is the better approach, again, so that the CSS > is still 16x16. When you say that you want the CSS to still be 16x16, are you referring to the area specified by the -moz-image-region tag? Seems like we can do that, although we'll have to have region-specific rules to add the padding, since different regions have different amounts of padding (0, 1, or 2 pixels worth), and a few icons have it on their left-hand side.

Removing fixed1.8.1 since this was reopened, setting target milestone

Here's a patch that applies padding to buttons via CSS to even out their padding. It applies button-specific left and right padding for the 24px and 16px icons as well as a blanket 1px top padding for 24px icons.

Comment on attachment 238382 [details] [diff] [review] patch v2: button-specific evening padding looks good, please get this in tonight

Fix checked into the branch.