Some notes: - This removes the backforward dropmarker because it isn't present in the mockups. Stephen, was its ommission intentional? - Extensions' icons are stretched to 20x20px, because that's the size of the Toolbar.png icons. I'm waiting for the outcome of bug 547419 before changing that. - This needs some platform-color-ization. I'd like to do that in a follow-up bug. Most importantly, the white shadow under the buttons has a different opacity on Leopard vs. Snow Leopard, so we'll definitely need a platform color for that. - The toolbarbutton[type="menu-button"] setup is really really nasty. I haven't found a better solution for them that works in icon/text mode with the label under the button.

(In reply to comment #0) > - This removes the backforward dropmarker because it isn't present in the > mockups. Please save it for a separate bug with extra ui-review and all that.

Also please explain why you hide the dropmarker and use ::before instead.

(In reply to comment #2) > Also please explain why you hide the dropmarker i.e. the menu button dropmarker

That was to position it correctly in icon + text mode. It should be adjacent to the icon, which is centered above the text. But I'll just drop that hack and position the dropmarker outside the button in icon + text mode in the next version of the patch. Nobody uses icon + text mode anyway.

I suspected that, and I was about to suggest the same solution.

This re-adds the back/forward dropmarker, gets rid of the type="menu-button" hack, and adds styling for in-tabbar buttons.

(In reply to comment #1) > (In reply to comment #0) > > - This removes the backforward dropmarker because it isn't present in the > > mockups. > > Please save it for a separate bug with extra ui-review and all that. We can short-circuit this a little here. This gets my ui-review, with a sidenote that we might put the drop-marker back in. Recent Test Pilot data shows that the drop-down *is* used pretty frequently, but we haven't sliced for Windows v. OSX yet, and my suspicion is that it's not as frequently used on Windows. As for the design rationale: while drop-markers exist in OSX, they're always on the button and for when the user must pick one of the entries in the drop-down. It's rarely-to-never the case that you see a drop-marker that pops down an optional list as one does on Windows. So we're dropping it because it looks non-native and the press-and-hold behaviour is more expected/consistent with OSX. As I said, if it turns out that beta feedback is resoundingly negative, we'll re-evaluate, but for now the behavioural change gets my ui-r+

Without dropmarker again.

Comment on attachment 452030 [details] [diff] [review] v3 > #nav-bar { >- padding: 0 4px; >+ -moz-box-align: center; Please explain this. Is the lack of -moz-box-align: center; going to cause problems for other toolbars?

I assume you're not using a reduced opacity for the disabled state because it would affect the background and border as well as the icon? And you're setting the background and border on the icon rather than the button because of the text and icons+text modes? This seems unfortunate, since we don't care that much about these modes...

(In reply to comment #9) > (From update of attachment 452030 [details] [diff] [review]) > > #nav-bar { > >- padding: 0 4px; > >+ -moz-box-align: center; > > Please explain this. -moz-box-align:stretch extended the buttons' hit rect beyond their visual rect, which I don't want. I'd rather have that space available for window dragging, as Boriss suggested in attachment 320238 [details]. > Is the lack of -moz-box-align: center; going to cause > problems for other toolbars? Not really problems, but it would be good to be consistent. I'll set it on them, too. The bookmarks toolbar already has -moz-box-align:center, but custom toolbars don't. (In reply to comment #10) > I assume you're not using a reduced opacity for the disabled state because it > would affect the background and border as well as the icon? And you're setting > the background and border on the icon rather than the button because of the > text and icons+text modes? This seems unfortunate, since we don't care that > much about these modes... Exactly. It's unfortunate, but we offer that mode so I think we shouldn't make it look completely ridiculous.

spacing fixes for custom toolbars

How would things look without any button appearance in icons+text mode?

Not good.

Good enough, imho, except that the textboxes should probably be centered rather than being aligned with the icons.

(In reply to comment #15) > Good enough, imho Sorry, not good. Why not completely drop support for icons + text then ? (Safari and Chrome on OS X don't support it at all)

Because that would be way beyond the scope of this bug... Why do you think it's not good enough? That is, for a mode that hardly anyone uses, let alone on OS X? Obviously attachment 452304 [details] isn't what we'd want to ship by default, but that's not the question.

Comment on attachment 452304 [details] Icons + text mode without button appearance Whether it's good enough is a question that should be answered by a UI reviewer.

There's another place where we use icon + text mode: The toolbar customization panel. This is what it looks like in Safari. Not having a button appearance there would be a visual regression. (Our customization panel obviously isn't where we want it to be visually, but that doesn't mean we should regress it.) I strongly believe that the code simplicity gained by using opacity for the disabled state does not justify this regression. In the customization palette, toolbarbuttons are wrapped in toolbarpaletteitems, which have their own label, which is currently empty. Maybe we could put the toolbarbutton description into that label and hide the label in the toolbarbutton empty. However, just like getting rid of icons + text mode, that's beyond the scope of this bug. How about this: Keep the current approach, file new bugs on removing icons + text mode and the label fixup in the customization palette, and then simplify the CSS once those bugs have been fixed?

Some more spacing tweaks and using #navigator-toolbox > .chromeclass-toolbar, #PersonalToolbar instead of #navigator-toolbox > toolbar:not(#TabsToolbar) because I want to exclude the menubar, too.

Patch for discussion. It gets rid of about 60 lines of CSS. I haven't modified Toolbar.png yet. I've thought of a more convincing reason to use opacity than just code simplicity: Add-on developers don't need to create disabled versions of their icons on for the Windows theme (because it uses opacity), so we shouldn't require them to do so for Mac. So this becomes a compatibility issue. And looking at it in a different way, not having a button appearance in the toolbar customization panel actually makes sense to a certain degree: The button might be dropped in the tab bar where it doesn't get a button appearance. I guess this is beltzner's call now.

> because I want to exclude the menubar, too. Why is this relevant? The menu toolbar is never rendered, is it?

Comment on attachment 452304 [details] Icons + text mode without button appearance I actually think that Shorlander gets the final say, here, as I lack a good amount of context on the specific discussion. Some notes, though: - assuming that this only happens in icons & text mode, I think it's fine for beta 1 - I think that (in a separate bug) we may want to get rid of icons & text mode, as well

(In reply to comment #22) > > because I want to exclude the menubar, too. > > Why is this relevant? The menu toolbar is never rendered, is it? It's not rendered because its height is zero. Both toolbar#toolbar-menubar and toolbaritem#menubar-items in fact are visible. menubar#main-menubar however is *not* (it behaves like display:none), but I haven't found out what hides it (nothing sets display:none on it). This is all very confusing and I'm not sure why it's set up the way it is.

Well then #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar) ? I'm not sure we can rely on chromeclass-toolbar. Also: > -moz-box-align:stretch extended the buttons' hit rect beyond their visual rect, > which I don't want. Doesn't sound like this would be relevant when the background is set on the toolbarbutton.

(In reply to comment #25) > I'm not sure we can rely on chromeclass-toolbar. Oh, we can't? I'll use your suggestion, sure. > > -moz-box-align:stretch extended the buttons' hit rect beyond their visual rect, > > which I don't want. > > Doesn't sound like this would be relevant when the background is set on the > toolbarbutton. It's still relevant: we wouldn't only stretch the button's hit rect, but the whole button appearance. But we always want them to be 22px high (except for the circle back button), no matter what else is in the toolbar.

Can items offered by Firefox cause this, though, or only oversized extension items? Have you tested a few popular third-party toolbars to see how they handle the centered alignment?

(In reply to comment #27) > Can items offered by Firefox cause this, though, or only oversized extension > items? The circle back button could cause it, if we don't add negative margins to it (or positive margins to all other buttons). > Have you tested a few popular third-party toolbars to see how they handle the > centered alignment? Not yet, I'll do so. How is this handled in the Windows theme?

The Windows theme stretches the items and sets a negative margin on the back button.

(In reply to comment #19) > How about this: Keep the current approach, file new bugs on removing icons + > text mode and the label fixup in the customization palette, and then simplify > the CSS once those bugs have been fixed? This is what I would prefer. It looks broken with no button background. Even if we remove the text/label options from the toolbar(which we probably should) they still need to be in the customization pane.

(In reply to comment #30) > Even if we remove the text/label options from the toolbar(which we probably > should) they still need to be in the customization pane. No, the styling there can be just like in icons mode, and with a separate label, similar to the other items. We need to fix this for winstripe anyway.

Comment on attachment 452304 [details] Icons + text mode without button appearance This allows us to simplify the CSS quite a bit, which is why "land now and change later" is undesirable, when the change we need to make is already at hand.

(In reply to comment #19) > file new bugs on removing icons + > text mode and the label fixup in the customization palette I filed bug 573329 and bug 573326.

(In reply to comment #32) > (From update of attachment 452304 [details]) > This allows us to simplify the CSS quite a bit, which is why "land now and > change later" is undesirable, when the change we need to make is already at > hand. Maybe I am just not clear on what you are proposing. So you are saying to leave the simplified CSS and then fix the underlying issues (Bug 573329 and Bug 573326)? I am ok with that. Just as long as the end result is that buttons in the customization panel look like buttons with labels and not free floating glyphs with labels :)

(In reply to comment #34) > So you are saying to leave the simplified CSS and then fix the underlying > issues (Bug 573329 and Bug 573326)? Yes, except that I'm not sure we should do bug 573329. Supporting text+icons with minimal effort might be better than not supporting it at all. I suspect those few using it would be more annoyed if we dropped it than by the free glyphs.

Comment on attachment 452304 [details] Icons + text mode without button appearance (In reply to comment #35) > (In reply to comment #34) > > So you are saying to leave the simplified CSS and then fix the underlying > > issues (Bug 573329 and Bug 573326)? > > Yes, except that I'm not sure we should do bug 573329. Supporting text+icons > with minimal effort might be better than not supporting it at all. I suspect > those few using it would be more annoyed if we dropped it than by the free > glyphs. I don't have a strong opinion on bug 573329. The complexity the additional modes add seem disproportionate to the amount of use they get. Still if we do support the modes I feel they should look as expected. UI review + since it's a low visibility problem. Will wait and see what the outcome of 573329 is.

With opacity and adapted Toolbar.png. I've tested some addon toolbars and they all coped fine with -moz-box-align: center. For some it was even an improvement; for example, one toolbar contained an editable menulist that was stretched before.

Comment on attachment 453109 [details] [diff] [review] v6 >+toolbar:not([mode="icons"]) toolbarbutton[type="menu-button"]:not([open="true"]) > .toolbarbutton-menubutton-dropmarker { >+ list-style-image: url(chrome://browser/skin/toolbarbutton-dropmarker.png); >+ opacity: .7; >+} >+ >+toolbar:not([mode="icons"]) toolbarbutton[type="menu-button"][open="true"] > .toolbarbutton-menubutton-dropmarker { >+ opacity: 1; >+} The list style image seems redundant, and it looks like you could use :not([open="true"]). >-#nav-bar { >- padding: 0 4px; >+#navigator-toolbox > #nav-bar { >+ padding-top: 2px !important; > } What's the idea behind this selector change? >+#TabsToolbar > toolbarbutton > .toolbarbutton-icon, >+#TabsToolbar > toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon, >+#TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon, >+#TabsToolbar > toolbarpaletteitem > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon { > padding: 0; >+ width: auto; >+ height: auto; What's the goal of the width and height? >--- a/toolkit/themes/pinstripe/global/toolbarbutton.css >+++ b/toolkit/themes/pinstripe/global/toolbarbutton.css >+toolbarbutton[open="true"], > toolbarbutton:not([disabled="true"]):active:hover { > text-shadow: none; > } Does this still make sense? This regresses the lightweight theme appearance. Any plans for that?

(In reply to comment #38) > (From update of attachment 453109 [details] [diff] [review]) > >+toolbar:not([mode="icons"]) toolbarbutton[type="menu-button"]:not([open="true"]) > .toolbarbutton-menubutton-dropmarker { > >+ list-style-image: url(chrome://browser/skin/toolbarbutton-dropmarker.png); > >+ opacity: .7; > >+} > >+ > >+toolbar:not([mode="icons"]) toolbarbutton[type="menu-button"][open="true"] > .toolbarbutton-menubutton-dropmarker { > >+ opacity: 1; > >+} > > The list style image seems redundant, and it looks like you could use > :not([open="true"]). Done. > > >-#nav-bar { > >- padding: 0 4px; > >+#navigator-toolbox > #nav-bar { > >+ padding-top: 2px !important; > > } > > What's the idea behind this selector change? Specificity, but that was before I added the !important. Reverted the selector. > > >+#TabsToolbar > toolbarbutton > .toolbarbutton-icon, > >+#TabsToolbar > toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon, > >+#TabsToolbar > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon, > >+#TabsToolbar > toolbarpaletteitem > toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon { > > padding: 0; > >+ width: auto; > >+ height: auto; > > What's the goal of the width and height? Right, it doesn't have any goal any more. Removed. > > >--- a/toolkit/themes/pinstripe/global/toolbarbutton.css > >+++ b/toolkit/themes/pinstripe/global/toolbarbutton.css > > >+toolbarbutton[open="true"], > > toolbarbutton:not([disabled="true"]):active:hover { > > text-shadow: none; > > } > > Does this still make sense? Yes it does. > This regresses the lightweight theme appearance. Any plans for that? Yes, and it's going to look better than ever before. Follow-up bug. I tweaked the paddings again, and I removed the negative top and bottom margin on the circle back button. It didn't feel right to me that selecting "small icons" doesn't save you any vertical space. I think I'm finished with the alignments now.