When looking at bug 890039 I noticed we don't have a blue-ish icon for the downloads button for when its panel is open (like for the menu button, the charset button, etc.). On regular nightly, we just have button outlines so you can *see* that the buttons are in a "pressed" state. However, as Australis got rid of those, there's really no visual indication of what's going on. Now, this would be a question of just adding a blue icon to the sprite. Unfortunately, we do also already use blue to indicate that there are downloads. As Stephen so eloquently put it... we need more colors! Note that Stephen also rightly pointed out to me that we'll likely have similar issues with add-ons, which don't necessarily ship extra icons for pressed state especially as Windows still has button outlines... (P3 mostly because of the add-ons, but I could be convinced otherwise)

Stephen, could you be more eloquent as to point us in the right direction here? Which color should we use for the Download arrow in pressed state? Maybe the color of the indicator should change to be different than blue? Add-ons are a different story for which a technical solution from our end is not possible, I think.

(In reply to Mike de Boer [:mikedeboer] from comment #1) > Stephen, could you be more eloquent as to point us in the right direction > here? Which color should we use for the Download arrow in pressed state? > Maybe the color of the indicator should change to be different than blue? > > Add-ons are a different story for which a technical solution from our end is > not possible, I think. Working on an alternative approach here. Leaving needinfo for tracking.

(In reply to Mike de Boer [:mikedeboer] from comment #1) > Add-ons are a different story for which a technical solution from our end is > not possible, I think. SVG/CSS mask-image could solve this (though it may not be that trivial, AFAICS it also needs an inner shadow for the path applied on top, I don't know if this works with in inset shadow on the masked image). It may "break" addons that do ship with a different hover/active state (like Firebug). Masks could also be used to reducing the assets required to make and keep updated for the whole Australis design. Also, dynamically changing SVG paths (rather, their backgrounds). But that feels somehow hacky.

Presumably this isn't actually a regression wrt add-ons? I think we should focus this bug on getting out own buttons working correctly, which should be pretty simple as a short-term fix. If we want to then spin-off a bug on finding a better long-term approach for this that [also] works for addons, then let's do that separately.

Is there any update about this bug? I believe that the depressed state in Windows is the one displayed in the mockups (https://github-camo.global.ssl.fastly.net/18bd7a0956a9babbe13258ccaaa263c0c33f6068/687474703a2f2f70656f706c652e6d6f7a696c6c612e636f6d2f7e73686f726c616e6465722f66696c65732f6164646f6e732d696e2d746f6f6c6261722d6930312f696d616765732f30382e706e67); can't we have something similar to OS X too?

(In reply to Justin Dolske [:Dolske] from comment #4) > Presumably this isn't actually a regression wrt add-ons? I *think* it is, because in the "old" OS X theme buttons have button outlines which can have a pressed state. We got rid of the outlines so the pressed state is non-obvious. I don't have any add-ons that use toolbarbuttons (or perhaps I've just customized them all away) so I don't know for sure that we have styling for this state, but I would (naively, perhaps) imagine so...

Apparently having an hover effect also on OSX is also the plan : http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/osx.html

Uh, whoops, wrong bug.

I'm hoping this gets shipped, the SDK's toggleButton api is pretty useless without it :)

The mockup shown is adding a border and background-effect to buttons, is that going to have any effect to the toolbar size or any other kind of fallout? I'm a bit wary of this becoming a bigger change than it might seem at first glance.

taking this for next week!

Looks like this just started working for the downloads button. Not sure what fixed it, but that would seem to make this bug just about style for add-on buttons?

Correction on comment 12: it's ok the first time, but after having opened the panel it stops working. We might want to split this bug -- I think was want our own, high-visibility primary UI by-default buttons to work properly, but it's less important wrt addon buttons.

Mike, do you remember in which bug no. you removed the OSX navbar buttons hover state? I can't find it using a bug search :/

(In reply to Mike de Boer [:mikedeboer] from comment #15) > Mike, do you remember in which bug no. you removed the OSX navbar buttons > hover state? I can't find it using a bug search :/ bug 856665?

Aye, that one! Thanks, Gijs.

This patch also removes the blue icons from Toolbar.png

Comment on attachment 8379095 [details] [diff] [review] Patch v1.1: re-introduce a hover and active state Review of attachment 8379095 [details] [diff] [review]: ----------------------------------------------------------------- Hey Mike - this looks really good, quite a bit better than what we've currently got on OS X, so kudos! Just a few questions / suggestions below. ::: browser/base/content/browser-places.js @@ +1149,4 @@ > } > + > +#ifdef XP_MACOSX > + this.button.classList.add("toolbarbutton-1"); I'm curious - why does this need to be special-cased for OS X? ::: browser/themes/osx/browser.css @@ +371,4 @@ > } > > @keyframes animation-bookmarkAddedToBookmarksBar { > + from { transform: rotate(0deg) translateX(-8px) rotate(0deg) scale(1); opacity: 0; } Hm - I think this might be off the mark a little. Testing the bookmarks menu button on the bookmarks toolbar, the star seems to fall short of the dropmarker by a few pixels. @@ +468,5 @@ > +toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-button, > +toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker, > +#restore-button { > + border: 1px solid transparent; > + border-radius: @toolbarbuttonCornerRadius@; I've been noticing that things with border-radius has been burning our perf some in terms of painting, at least on Windows. Let's keep an eye on our ts_paint and tpaint numbers after this lands. @@ +1504,5 @@ > #cut-button { > -moz-margin-end: 0; > } > > +toolbar #cut-button { I think this can be: #edit-controls[cui-areatype="toolbar"] > #cut-button { @@ +1542,5 @@ > -moz-margin-start: 0; > + -moz-border-start: 0; > +} > + > +toolbar #zoom-out-button { As above, I think this can be: #zoom-controls[cui-areatype="toolbar"] > #zoom-X-button { And that'll save us some descendant selectors.

Hey Mike, I know you're already working on the Win8 stuff and the tip panel for customization mode... want me to push this one over the line?

Thanks for the offer, Mike! But I'll hack on this until I have a Windows VM here to continue with the other bugs. So this one comes first! :)

(In reply to Mike Conley (:mconley) from comment #20) > Comment on attachment 8379095 [details] [diff] [review] > Patch v1.1: re-introduce a hover and active state > > Review of attachment 8379095 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hey Mike - this looks really good, quite a bit better than what we've > currently got on OS X, so kudos! > > Just a few questions / suggestions below. > > ::: browser/base/content/browser-places.js > @@ +1149,4 @@ > > } > > + > > +#ifdef XP_MACOSX > > + this.button.classList.add("toolbarbutton-1"); > > I'm curious - why does this need to be special-cased for OS X? Well, because I'm a scaredy-cat I suppose... too afraid to regress Win & Lin themes with this OSX theme change...

Comment on attachment 8385331 [details] [diff] [review] Patch v1.2: re-introduce a hover and active state Review of attachment 8385331 [details] [diff] [review]: ----------------------------------------------------------------- I think the special-casing in browser-places.js is the only thing that's really bugging me here. Otherwise, the code looks sound. shorlander did bring up that the buttons are too tall when hovered - they should be the height of the text inputs, if possible. I also noticed that SDK provided buttons are slightly wider than the default toolbar buttons. Those last two points are probably OK for follow-ups, but let's see if we can get that special-casing into the CSS. ::: browser/base/content/browser-places.js @@ +1146,5 @@ > } > > if (onPersonalToolbar) { > this.button.classList.add("bookmark-item"); > +#ifndef XP_MACOSX Yeah, to me this special-casing looks like we should, instead, just make sure that the toolbarbutton-1 rules are always applied to the bookmarks menu button. That's a thing, I think, we should do in CSS, and not here.

Tested this on Windows too, doesn't regress AFAICT.

Comment on attachment 8386417 [details] [diff] [review] Patch v1.3: re-introduce a hover and active state Cancelling review, since (as discussed in IRC), we want to persist the toolbarbutton-1 class on the bookmarks menu button all the time, instead of magically adding and removing it (which currently happens to the current implementation).

Comment on attachment 8386781 [details] [diff] [review] Patch v1.5: re-introduce a hover and active state Review of attachment 8386781 [details] [diff] [review]: ----------------------------------------------------------------- So, I'm good with this - but please file these two follow-up bugs: 1) Make the toolbarbuttons shorter - right now, they exceed the height of the URL and search containers, and that's weird. shorlander thinks it looks funky. 2) In a new window, when clicking on the downloads button, the button changes size (it's slight, but noticeable). That second one is probably a P5. The first miiiiiight be a P3. Probably depends on who you ask. :)

Thanks! Filing bugs coming right up... Pushed as: https://hg.mozilla.org/integration/fx-team/rev/ccbe695b5f40

Follow-ups filed as bug 980370 and bug 980374.

Comment on attachment 8386781 [details] [diff] [review] Patch v1.5: re-introduce a hover and active state [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis User impact if declined: no hover and active (depressed) states for toolbar buttons on OSX Testing completed (on m-c, etc.): merged to m-c Risk to taking this patch (and alternatives if risky): minor String or IDL/UUID changes made by this patch: n/a

This is marked as RESOLVED FIXED, but <toolbarbutton type="checkbox" checked="true"> doesn't keep any depressed state, has it was in pre-australis build. As Jeff states in comment 9, this is pretty important to SDK ToggleButton.

Depressed state is rather different than a checked state, so that's what we went with. Adding a [checked=true] to the selector is rather straightforward, though.

I thought was included – see jeff's and mine comments. However, I can fill a new bug if you prefer, it seems to me that having <toolbarbutton type="checkbox" checked="true"> broken needs to be fixed anyway.

Can you do that for me? Please cc me and I'll try to get to it asap. Thanks!