909349 - Australis, OS X: downloads and add-on buttons don't have a depressed state

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[:Gijs (out for now; he/him)]

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)

Comment 1

[Mike de Boer [:mikedeboer]]

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.

Comment 2

[Stephen Horlander [:shorlander]]

(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.

Comment 3

[Florian Bender]

(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.

Comment 4

[Justin Dolske [:Dolske]]

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.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

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?

Comment 6

[:Gijs (out for now; he/him)]

(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...

Comment 7

[u428464]

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

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Uh, whoops, wrong bug.

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 10

[Justin Dolske [:Dolske]]

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.

Comment 11

[Mike de Boer [:mikedeboer]]

taking this for next week!

Comment 12

[Justin Dolske [:Dolske]]

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?

Comment 13

[Justin Dolske [:Dolske]]

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.

Comment 15

[Mike de Boer [:mikedeboer]]

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 :/

Comment 16

[:Gijs (out for now; he/him)]

(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?

Comment 17

[Mike de Boer [:mikedeboer]]

Aye, that one! Thanks, Gijs.

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: re-introduce a hover and active state

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: re-introduce a hover and active state

This patch also removes the blue icons from Toolbar.png

Comment 20

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 22

[Mike de Boer [:mikedeboer]]

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! :)

Comment 23

[Mike de Boer [:mikedeboer]]

(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 24

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: re-introduce a hover and active state

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: re-introduce a hover and active state

Comment 26

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.3: re-introduce a hover and active state

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

Comment 28

[Mike Conley (:mconley) (:⚙️)]

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 29

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.4: re-introduce a hover and active state

Comment 30

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.5: re-introduce a hover and active state

Comment 31

[Mike Conley (:mconley) (:⚙️)]

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. :)

Comment 32

[Mike de Boer [:mikedeboer]]

Thanks! Filing bugs coming right up...

Pushed as: https://hg.mozilla.org/integration/fx-team/rev/ccbe695b5f40

Comment 33

[Mike de Boer [:mikedeboer]]

Follow-ups filed as bug 980370 and bug 980374.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ccbe695b5f40

Comment 35

[Mike de Boer [:mikedeboer]]

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

Comment 36

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 37

[Mike de Boer [:mikedeboer]]

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.

Comment 38

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 39

[Mike de Boer [:mikedeboer]]

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

Comment 40

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45cbc102a414