994280 - It would be nice to have ability to add badge over button

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[felix.shnir]

This is a very common feature for all other browsers that allows developers to have a customizable badge over the toolbar icon of the addon. I know some addons do this anyway, but adding support for this into Addon-SDK will bring Firefox on par with all other browsers.

Thanks.

Comment 1

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

+1 geberally, this was already part of the spec and SocialApi does it. I defer to Matteo on how complicated it might be. Also, feels like it should be possible for both action and toggle buttons.

Comment 2

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

Attachment: Badge

Yes, it was part of the original API: one of the reason because we removed was exactly the Social APIs: they have already bagdes as XBL but their badges are a bit different from the UX design and our specifications.
We should probably modify the XBL we already have in Firefox to match what we need.

Here how it should looks like:
<http://people.mozilla.org/~shorlander/addOn-notificationBadge/addOn-notificationBadge.html>

It seems also that the toolbarbutton with badge is a bit broken in OS X, if we trying to create the button from XBL at least, see the snapshot attached.

We should probably contact who implement that for the Social API and working together in order to be sure we do not break anything on their side, or doing anything they do not want.

In my opinion, in addition to the look & feel, we have another big issue: currently the XBL for badge button use <toolbarbutton type="badge"> to define the binding. That means that we can't have a `ToggleButton` with badge, because `ToggleButton` are <toolbarbutton type="checkbox">.

Comment 3

[Shane Caraveo (:mixedpuppy)]

I'd be happy to see improvements to the socialapi badge button that would fit the design in comment #2.  I also recently noticed the problem on osx, likely a regression from the australis changes.

Comment 4

[andornaut]

It'd be great if button/icon badges were supported for both ToggleButton and ActionButton and exposed via the sdk/ui/button/* APIs.

Comment 5

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> I'd be happy to see improvements to the socialapi badge button that would
> fit the design in comment #2.  I also recently noticed the problem on osx,
> likely a regression from the australis changes.

Do you know who implemented the badge in the Social API? I'd like to implement this feature use a common ground, but the badge in Social API doesn't fit our purpose and design as explained in my previous comment; so we need probably to improve the XBL and change the way is used in the Social API as well.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #5)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> > I'd be happy to see improvements to the socialapi badge button that would
> > fit the design in comment #2.  I also recently noticed the problem on osx,
> > likely a regression from the australis changes.
> 
> Do you know who implemented the badge in the Social API? I'd like to
> implement this feature use a common ground, but the badge in Social API
> doesn't fit our purpose and design as explained in my previous comment; so
> we need probably to improve the XBL and change the way is used in the Social
> API as well.

I did.  Like I said, improvements welcome. go for it.

Comment 7

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

+1 for this, this has always been something I wanted for these apis.

Comment 8

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

Attachment: badge.patch

Added the style defined in the Australis mockup for badge, and binding the XBL using `badge` attribute instead of `type`.

Comment 9

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

Shane, I notice a bug about badged button and Australis during my development.
Basically if you're moving a badged button in the Panel UI, it brokes the Panel UI itself the second time you're trying to open it: the first time just immediately close, and the second time it opens without the icons area.

I filed a new bug about it: bug 1037528

Any idea why is happening?

Comment 10

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

Attachment: https://github.com/mozilla/addon-sdk/pull/1549

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8454535 [details] [diff] [review]
badge.patch

LGTM, asking Gijs for an additional look at the css changes.

Comment 12

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

Comment on attachment 8454535 [details] [diff] [review]
badge.patch

Review of attachment 8454535 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really understand. Comment 2 seems to imply we're breaking type="badged" (NB: this will break add-ons!) in order to use it with type="checkbox" as well. However, the patch does not reflect this change, and in particular, seems to provide no styling for type="checkbox" combined with [badge]. Does that just work as-is? From looking at the CSS, I would expect it to overlap the border, which would make it harder to see whether the button is pressed or not. There doesn't seem to be a spec about this?

There's something else here... you don't set the 'badge' attribute in social.jsm anywhere else. Which means that it will first be deemed a "regular" toolbar button, and will change XBL binding when you add/remove the badge attribute (which I presume happens elsewhere?). That is both not free and seems strange. Seems to me like you always want the other XBL binding to be there and therefore shouldn't be overloading the attribute like this, and if we really needed a different attribute from type="checkbox", maybe we should add a boolean of the form badged="true" ?

You're also not keeping the type="badge" case functional for add-ons which are already using it, which is another concern.

Altogether (see also the concerns below as well), I'm not comfortable giving this r+.

::: browser/base/content/browser.css
@@ +798,4 @@
>  toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
>    max-width: 16px;
>  }
> +toolbarpaletteitem[place="palette"] > toolbarbutton[badge] > .toolbarbutton-badge-container > .toolbarbutton-icon {

If there isn't CSS for the panel case already, there should be. If there is, why wasn't it updated in this patch?

::: browser/themes/osx/browser.css
@@ +4198,4 @@
>  }
>  
>  .toolbarbutton-badge[badge]:not([badge=""]):-moz-window-inactive {
> +  background-color: rgb(230,230,230)!important;

Nit: needs a space.

Why was this necessary? That is, why !important?

::: browser/themes/windows/browser.css
@@ +2697,5 @@
>    display: none;
>  }
>  .toolbarbutton-badge[badge]:not([badge=""])::after {
>    /* The |content| property is set in the content stylesheet. */
> +  position: absolute;

Nit: can you rearrange these lines so that the original order is preserved? Right now you're touching blame on lots of bits that are staying the same, which makes the patch harder to read/review and will make regression hunting more difficult in future.

Same thing for the other browser.css changes.

Comment 13

[Shane Caraveo (:mixedpuppy)]

@Gijs: Are addons actually using this button?  It's not documented on MDN anywhere I can find, so I would be surprised that it is used outside of socialapi.

Given the checkbox button requirement, I wonder if this bug should just add a badge to the base toolbarbutton xbl. (but fixing the socialapi css would still be appreciated.)

Comment 14

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> @Gijs: Are addons actually using this button?  It's not documented on MDN
> anywhere I can find, so I would be surprised that it is used outside of
> socialapi.

Huh. Yeah, searching through the add-ons mxr, I don't see any hits... Still, the other concerns remain...

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #14)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> > @Gijs: Are addons actually using this button?  It's not documented on MDN
> > anywhere I can find, so I would be surprised that it is used outside of
> > socialapi.
> 
> Huh. Yeah, searching through the add-ons mxr, I don't see any hits... Still,
> the other concerns remain...

BTW, badge is set in browser-social.js, but it should also be set in social.jsm for the status widget to avoid the binding change.  Any CSS that has badge="" should be looked at as well (I recall one or two places that happens).

Gijs, what do you think of adding a badge to the base toolbarbutton xbl?

Comment 16

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #14)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> > > @Gijs: Are addons actually using this button?  It's not documented on MDN
> > > anywhere I can find, so I would be surprised that it is used outside of
> > > socialapi.
> > 
> > Huh. Yeah, searching through the add-ons mxr, I don't see any hits... Still,
> > the other concerns remain...
> 
> BTW, badge is set in browser-social.js, but it should also be set in
> social.jsm for the status widget to avoid the binding change.  Any CSS that
> has badge="" should be looked at as well (I recall one or two places that
> happens).
> 
> Gijs, what do you think of adding a badge to the base toolbarbutton xbl?

It's an interesting suggestion. There are a lot of bindings in there (4, IIRC). IME adding stuff to XBL bindings makes them more expensive. We did it with multiline labels, because any type of button can be moved to the panel. I don't know that any button should be badge-able, and even where it should be, if it makes sense to add this ability to the main bindings. We should just ensure that badgeable buttons use the right XBL binding at all times.

I think badgeable buttons are so rare that we should move the binding to toolkit, but not update the existing bindings. AFAICT there isn't a special binding for checkbox buttons. You'd need toolkit reviews for that, though.

Comment 17

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

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #12)

> I don't really understand. Comment 2 seems to imply we're breaking
> type="badged"

Not sure in which way we're going to break `type="badged"`; do you have some scenario in mind?
However I agreed with Shane – also because badged button are already pretty broken since Australis is released; we're trying to fix that too.

> However, the patch does not reflect this change,
> and in particular, seems to provide no styling for type="checkbox" combined
> with [badge]. Does that just work as-is?

Yes, pretty much.

> You're also not keeping the type="badge" case functional for add-ons which
> are already using it, which is another concern.

I'm not sure I got this point. A button with `type="badged"` is virtually identical to a regular button if it hasn't a badge attribute set. If it has a badge attribute set, then the XBL is applied also in this new case.

> > +toolbarpaletteitem[place="palette"] > toolbarbutton[badge] > .toolbarbutton-badge-container > .toolbarbutton-icon {
> 
> If there isn't CSS for the panel case already, there should be. If there is,
> why wasn't it updated in this patch?

Probably there wasn't, 'cause I searched for badge="" in order to replace it; and I didn't had any issue with panel 'cause it wasn't possible to actually test, a badged button in panel – see bug 1029937.
I'll add the CSS rule for panel too.

> >  .toolbarbutton-badge[badge]:not([badge=""]):-moz-window-inactive {
> > +  background-color: rgb(230,230,230)!important;
> 
> Nit: needs a space.
> 
> Why was this necessary? That is, why !important?

Because otherwise, if the background-color is set by javascript, that would take the precedence also for the inactive style; where, by default, it shouldn't happens – in SDK API developers can change the color of the badge dynamically, and we don't want to ruin the inactive style for that.

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)

> Gijs, what do you think of adding a badge to the base toolbarbutton xbl?

If it's doable, I totally agree: in fact, I already do that for SDK button – I always create a badged button even if `badge` property is not specified at creation time – it would makes sense to me having the same stuff on platform side.

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #16)

> I think badgeable buttons are so rare that we should move the binding to
> toolkit, but not update the existing bindings. AFAICT there isn't a special
> binding for checkbox buttons. You'd need toolkit reviews for that, though.

So, in that case, how we should proceed?

Comment 18

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #17)
> (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #12)
> 
> > I don't really understand. Comment 2 seems to imply we're breaking
> > type="badged"
> 
> Not sure in which way we're going to break `type="badged"`; do you have some
> scenario in mind?
> However I agreed with Shane – also because badged button are already pretty
> broken since Australis is released; we're trying to fix that too.

I'm not sure how "badged button are [sic] already pretty broken" and how this patch fixes that; it's certainly not part of this bug as summarized. As for why type='badged' breaks, your patch removes the -moz-binding CSS declaration, so they don't get the right XBL binding, and so setting the attribute type to 'badged' no longer works.

> > However, the patch does not reflect this change,
> > and in particular, seems to provide no styling for type="checkbox" combined
> > with [badge]. Does that just work as-is?
> 
> Yes, pretty much.
> 
> > You're also not keeping the type="badge" case functional for add-ons which
> > are already using it, which is another concern.
> 
> I'm not sure I got this point. A button with `type="badged"` is virtually
> identical to a regular button if it hasn't a badge attribute set. If it has
> a badge attribute set, then the XBL is applied also in this new case.

Shane and I went over this one earlier...

> > > +toolbarpaletteitem[place="palette"] > toolbarbutton[badge] > .toolbarbutton-badge-container > .toolbarbutton-icon {
> > 
> > If there isn't CSS for the panel case already, there should be. If there is,
> > why wasn't it updated in this patch?
> 
> Probably there wasn't, 'cause I searched for badge="" in order to replace
> it; and I didn't had any issue with panel 'cause it wasn't possible to
> actually test, a badged button in panel – see bug 1029937.
> I'll add the CSS rule for panel too.
> 
> > >  .toolbarbutton-badge[badge]:not([badge=""]):-moz-window-inactive {
> > > +  background-color: rgb(230,230,230)!important;
> > 
> > Nit: needs a space.
> > 
> > Why was this necessary? That is, why !important?
> 
> Because otherwise, if the background-color is set by javascript, that would
> take the precedence also for the inactive style; where, by default, it
> shouldn't happens – in SDK API developers can change the color of the badge
> dynamically, and we don't want to ruin the inactive style for that.

This sounds like an SDK oddity that doesn't need to be !important for everyone, and also not something that would need to be important only on OSX but nowhere else.

In particular, it also sounds like something that you could/should implement in the SDK in some other way if this is the behaviour you want (like adding styles to a stylesheet that use the correct selector, for instance). This is assuming that you're not passing the raw node reference to the SDK consumer, which I thought the SDK never did because it implies privilege escalation beyond whatever modules are require()d...

> (In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> 
> > Gijs, what do you think of adding a badge to the base toolbarbutton xbl?
> 
> If it's doable, I totally agree: in fact, I already do that for SDK button –
> I always create a badged button even if `badge` property is not specified at
> creation time – it would makes sense to me having the same stuff on platform
> side.

What you're doing is orthogonal to what Shane is suggesting, and wouldn't work for other types of buttons unless we updated all the bindings (e.g. also menu-button/menu/...), which as I've said, I don't really think is the best option here.

> (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #16)
> 
> > I think badgeable buttons are so rare that we should move the binding to
> > toolkit, but not update the existing bindings. AFAICT there isn't a special
> > binding for checkbox buttons. You'd need toolkit reviews for that, though.
> 
> So, in that case, how we should proceed?

You should move the binding from the urlbarbindings to the toolkit/content/widgets/toolbarbutton.xml file and make it styled by a different attribute than badge="", in the same css file that the other button types are bound from, and then request review from a toolkit peer like Dão or Neil or Jared or ...

Comment 19

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

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #18)

> I'm not sure how "badged button are [sic] already pretty broken" and how
> this patch fixes that;

On OS X they basically are centered, covered the button itself. That patch fixed that.
Plus there is the panel bug, as mentioned – that is not fixed by this patch – but shows as they're already pretty broken since Australis.

> for why type='badged' breaks, your patch removes the -moz-binding CSS
> declaration, so they don't get the right XBL binding, and so setting the
> attribute type to 'badged' no longer works.

Not sure how that's broke the stuff, 'cause if you set only the type "badged" but not the "badge" attribute you basically get a regular button – you do not have any differences with or without XBL until you set also the "badge" attribute, in both cases. We discussed that on IRC with Shane, and we agreed that it wasn't a break change – we couldn't find any other use of this button and it's an edge case that an add-on trying to access to the anonymous content of such button if there isn't a "badge" to be displayed.
Say that, if we introduce the badge in the base toolbarbutton, it's useless discuss further.

> > I'm not sure I got this point. A button with `type="badged"` is virtually
> > identical to a regular button if it hasn't a badge attribute set. If it has
> > a badge attribute set, then the XBL is applied also in this new case.

> Shane and I went over this one earlier...

Me too, but earlier so probably I missed something? I'm not sure what we're discussion here, can you be more explicit, maybe with a common and concrete scenario of an add-on that is broken by that?

> > Because otherwise, if the background-color is set by javascript, that would
> > take the precedence also for the inactive style; where, by default, it
> > shouldn't happens – in SDK API developers can change the color of the badge
> > dynamically, and we don't want to ruin the inactive style for that.
 
> This sounds like an SDK oddity that doesn't need to be !important for
> everyone,

It's not, in my opinion: the inactivity should have the priority on inline style, unless the developers specify otherwise explicitly. Set the inline style will breaks the inactivity, where it's definitely important in SDK, I think should be done in general. Where usually you have author / user / agent stylesheet for that, in this case I thought that use `!important` did the trick. However, I don't mind to introduce SDK specific rules.

> and also not something that would need to be important only on OSX
> but nowhere else.

I believe because only on OS X you have such look & feel in the theme, when a window becomes inactive: it's platform specific. We don't change the color of the badge in other OSes, there isn't such rule. I searched in the CSS when I wrote the patch, and I didn't find anything similar for other OSes; but I could be wrong.

> In particular, it also sounds like something that you could/should implement
> in the SDK in some other way

There is no other way to implement this thing without some hack, in SDK.
If you think it's better, we can add SDK specific rules, I don't mind, as soon as it works. But they've to be in the CSS, like we did for the other buttons.

> > If it's doable, I totally agree: in fact, I already do that for SDK button –
> > I always create a badged button even if `badge` property is not specified at
> > creation time – it would makes sense to me having the same stuff on platform
> > side.
> 
> What you're doing is orthogonal to what Shane is suggesting,

What I've done is implementing badged button always as "base button" in SDK, despite if it's actually a badged button or not; if I understood correctly Shane is suggesting to do basically the same but on the platform side, so that "base buttons" have always the badge. That would works perfectly for me, if we're not to break too many things. :)

> > > I think badgeable buttons are so rare that we should move the binding to
> > > toolkit, but not update the existing bindings. AFAICT there isn't a special
> > > binding for checkbox buttons. You'd need toolkit reviews for that, though.
> > 
> > So, in that case, how we should proceed?
> 
> You should move the binding from the urlbarbindings to the
> toolkit/content/widgets/toolbarbutton.xml file and make it styled by a
> different attribute than badge="",

Sorry, I'm not following you here. Why we can't reuse badge=""?

Comment 20

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #19)

> > > Because otherwise, if the background-color is set by javascript, that would
> > > take the precedence also for the inactive style; where, by default, it
> > > shouldn't happens – in SDK API developers can change the color of the badge
> > > dynamically, and we don't want to ruin the inactive style for that.
>  
> > This sounds like an SDK oddity that doesn't need to be !important for
> > everyone,

Let's put it other way around, maybe we're looking at it from the wrong view point: according to UX mockups, the badge button should have at least the capability to change its color. How we can do that from JavaScript? One alternative I considered was also adding another attribute to the XBL, like "badge-color", but to me was a bit mixing the CSS domain, here. However, even if we implement such attribute, we still need to ensure that wouldn't affect the look & feel of the theme once the window is inactive.

What do you think, Gijs?

Comment 21

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #19)
> Say that, if we introduce the badge in the base toolbarbutton, it's useless
> discuss further.

Right, but as I've already said, I don't think that's a good idea. It should remain a separate binding - but it should be added in toolkit's toolbarbutton.xml

> > > I'm not sure I got this point. A button with `type="badged"` is virtually
> > > identical to a regular button if it hasn't a badge attribute set. If it has
> > > a badge attribute set, then the XBL is applied also in this new case.
> 
> > Shane and I went over this one earlier...
> 
> Me too, but earlier so probably I missed something? I'm not sure what we're
> discussion here, can you be more explicit, maybe with a common and concrete
> scenario of an add-on that is broken by that?

Please read comment #13 and comment #14. There are no AMO add-ons that rely on type="badged", as far as I can tell.

> > > Because otherwise, if the background-color is set by javascript, that would
> > > take the precedence also for the inactive style; where, by default, it
> > > shouldn't happens – in SDK API developers can change the color of the badge
> > > dynamically, and we don't want to ruin the inactive style for that.
>  
> > This sounds like an SDK oddity that doesn't need to be !important for
> > everyone,
> 
> It's not, in my opinion: the inactivity should have the priority on inline
> style, unless the developers specify otherwise explicitly. Set the inline
> style will breaks the inactivity, where it's definitely important in SDK, I
> think should be done in general. Where usually you have author / user /
> agent stylesheet for that, in this case I thought that use `!important` did
> the trick. However, I don't mind to introduce SDK specific rules.

The SDK shouldn't be getting the author to provide inline styles. We've had this issue before with the 'widget' library (which set inline margin styles with !important). It makes it impossible for the core Firefox code to adjust the appearances of buttons without adding !important everywhere (and even then sometimes that is not enough), which is something we should always avoid.

> > In particular, it also sounds like something that you could/should implement
> > in the SDK in some other way
> 
> There is no other way to implement this thing without some hack, in SDK.
> If you think it's better, we can add SDK specific rules, I don't mind, as
> soon as it works. But they've to be in the CSS, like we did for the other
> buttons.

You cut out the bit of my reply where I suggested how to implement this in the SDK, so I will repeat myself once more:

You can dynamically manipulate stylesheets from the SDK itself, so if you let the author declare a badgecolor property on their button specification then you can just insert the correct rule in a custom stylesheet for them.

> > > > I think badgeable buttons are so rare that we should move the binding to
> > > > toolkit, but not update the existing bindings. AFAICT there isn't a special
> > > > binding for checkbox buttons. You'd need toolkit reviews for that, though.
> > > 
> > > So, in that case, how we should proceed?
> > 
> > You should move the binding from the urlbarbindings to the
> > toolkit/content/widgets/toolbarbutton.xml file and make it styled by a
> > different attribute than badge="",
> 
> Sorry, I'm not following you here. Why we can't reuse badge=""?

Because then the XBL binding that is applied changes when the badge is set or unset, which is not free, loses any changes made to the anon content, and therefore should generally be avoided. See e.g. bug 984455 for the kind of headaches XBL binding rebindings get you into.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #20)
> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #19)
> 
> > > > Because otherwise, if the background-color is set by javascript, that would
> > > > take the precedence also for the inactive style; where, by default, it
> > > > shouldn't happens – in SDK API developers can change the color of the badge
> > > > dynamically, and we don't want to ruin the inactive style for that.
> >  
> > > This sounds like an SDK oddity that doesn't need to be !important for
> > > everyone,
> 
> Let's put it other way around, maybe we're looking at it from the wrong view
> point: according to UX mockups, the badge button should have at least the
> capability to change its color. How we can do that from JavaScript? One
> alternative I considered was also adding another attribute to the XBL, like
> "badge-color", but to me was a bit mixing the CSS domain, here. However,
> even if we implement such attribute, we still need to ensure that wouldn't
> affect the look & feel of the theme once the window is inactive.
> 
> What do you think, Gijs?

As I've said above, you can manipulate stylesheets to do what you want without involving XBL. I don't actually think the XBL route is viable because CSS' attr() only works on pseudo content, last I checked.

Comment 22

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

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #21)
> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #19)
> > Say that, if we introduce the badge in the base toolbarbutton, it's useless
> > discuss further.
> 
> Right, but as I've already said, I don't think that's a good idea. It should
> remain a separate binding - but it should be added in toolkit's
> toolbarbutton.xml

To be 100% clear here, though, I am not a toolkit peer, so if you are sure that your way is better, you might write a patch and request review from a toolkit peer as suggested earlier and ignore my opinion. :-)

Comment 23

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

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #21)

> Right, but as I've already said, I don't think that's a good idea. It should
> remain a separate binding - but it should be added in toolkit's
> toolbarbutton.xml

Sorry to misunderstood you. Then, what's the point to move on toolkit's toolbarbutton.xml? I'm not against that, I'm trying to understand what is the discriminant.

> > > > I'm not sure I got this point. A button with `type="badged"` is virtually
> > > > identical to a regular button if it hasn't a badge attribute set. If it has
> > > > a badge attribute set, then the XBL is applied also in this new case.
> > 
> > > Shane and I went over this one earlier...
> > 
> > Me too, but earlier so probably I missed something? I'm not sure what we're
> > discussion here, can you be more explicit, maybe with a common and concrete
> > scenario of an add-on that is broken by that?
> 
> Please read comment #13 and comment #14. There are no AMO add-ons that rely
> on type="badged", as far as I can tell.

Exactly, so, still, I'm not sure what we're talking about here.

> > > > Because otherwise, if the background-color is set by javascript, that would
> > > > take the precedence also for the inactive style; where, by default, it
> > > > shouldn't happens – in SDK API developers can change the color of the badge
> > > > dynamically, and we don't want to ruin the inactive style for that.

> > > This sounds like an SDK oddity that doesn't need to be !important for
> > > everyone,
> > 
> > It's not, in my opinion: the inactivity should have the priority on inline
> > style, unless the developers specify otherwise explicitly. Set the inline
> > style will breaks the inactivity, where it's definitely important in SDK, I
> > think should be done in general. Where usually you have author / user /
> > agent stylesheet for that, in this case I thought that use `!important` did
> > the trick. However, I don't mind to introduce SDK specific rules.
 
> The SDK shouldn't be getting the author to provide inline styles.

It isn't. But changing dynamically the color of the badge means set the style attribute of it. The badge's color can be set contextually to the browser (over all the windows and tabs), or to the window, or even contextually to the a specific tab.
In SDK the API take care of the last specific case, so that the author doesn't have to write code for that.

> > > In particular, it also sounds like something that you could/should implement
> > > in the SDK in some other way

> > There is no other way to implement this thing without some hack, in SDK.
> > If you think it's better, we can add SDK specific rules, I don't mind, as
> > soon as it works. But they've to be in the CSS, like we did for the other
> > buttons.

> You cut out the bit of my reply where I suggested how to implement this in
> the SDK, 

I cut it because it's not a viable way in SDK at the moment. We have a platform bug open to make something like that easier (apply a style only for a DOM element, something similar we'll have with shadow DOM, a sort of scoped stylesheet without actually modifying the DOM, that sometimes is not possible), but we do not want to block this bug for something like that.

> You can dynamically manipulate stylesheets from the SDK itself, so if you
> let the author declare a badgecolor property on their button specification
> then you can just insert the correct rule in a custom stylesheet for them.

We can't. First of all the color should be applied only to the specific node, so for instance we need an ID. The author is not aware of the button's ID because is auto-generated. Assuming we load the stylesheet, ad automatically add the id, and then register as data:url stylesheet, we broke the relative URLs in the stylesheet. We could also try to replace that with absolute URL dynamically; but we took this approach in consideration time ago for other APIs and we discarded as "hack" – and that's what I said before.
Also, there is the tab's context (so we could have different badge color depends by the current tab / document) that makes heavier and a bit more complex manipulate the stylesheet all the time.

Comparing that approach to just add "!important" to the OS X rule, to ensure that this rule will be respected, seems just better.

> > > You should move the binding from the urlbarbindings to the
> > > toolkit/content/widgets/toolbarbutton.xml file and make it styled by a
> > > different attribute than badge="",
> > 
> > Sorry, I'm not following you here. Why we can't reuse badge=""?
> 
> Because then the XBL binding that is applied changes when the badge is set
> or unset,

Not totally true. So the XBL binding should be applied when the attribute is present, even if is not "set". Just the presence of the "badge" attribute makes it a "badged" button, then if there is no value, the badge is not "visually" displayed, but the XBL is properly bound. So even set the badge to empty string doesn't remove the XBL, only remove explicitly the badge attribute make that so.

> which is not free, loses any changes made to the anon content, and
> therefore should generally be avoided. See e.g. bug 984455 for the kind of
> headaches XBL binding rebindings get you into.

The rebindings shouldn't happen if the badge value is modified or set to empty, only if the attribute is explicitly removed.

> > What do you think, Gijs?
> 
> As I've said above, you can manipulate stylesheets to do what you want
> without involving XBL.

But that's not easily done, as explained above.

> I don't actually think the XBL route is viable
> because CSS' attr() only works on pseudo content, last I checked.

So we're at the beginning.

(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #22)

> To be 100% clear here, though, I am not a toolkit peer, so if you are sure
> that your way is better, you might write a patch and request review from a
> toolkit peer as suggested earlier and ignore my opinion. :-)

Well, it's always good have another point of view, I'll check also with a toolkit peer if it makes sense move the badged button there, and what about the stylesheet. Thanks!

Comment 24

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #23)
> (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #21)
> > which is not free, loses any changes made to the anon content, and
> > therefore should generally be avoided. See e.g. bug 984455 for the kind of
> > headaches XBL binding rebindings get you into.
> 
> The rebindings shouldn't happen if the badge value is modified or set to
> empty, only if the attribute is explicitly removed.

Sure, but creating an artificial and hard-to-understand distinction between:

<toolbarbutton />
and
<toolbarbutton badge=""/>

is a recipe for disaster. None of the other attributes we use have a purposeful distinction like this, and I don't think we should start having one.

Comment 25

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

Attachment: badge-toolkit.patch

Comment 26

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

Attachment: badge-screenshot.png

Currently – even with the patch – if the icon is smallest in height (e.g. 8px), we have the badge positioned lower than expected.

This is particular an issue on OS X, because it seems mispositioned. On Windows and Linux the button's height reflect the content's height, where in OS X is always the same height (see the attachment), so the effect is somehow understandable for the user.

I'm not sure why this discrepancy in button's height across the OSes in first place (with or without badges); and I'm not sure if we should fix that or just the badge.
If we want to keep the current visual status, we probably need to set an height for the badge's container only on OS X.

Comment 27

[Neil Deakin]

Comment on attachment 8472296 [details] [diff] [review]
badge-toolkit.patch

OK, finally looked at this.

diff --git a/toolkit/content/widgets/toolbarbutton.xml b/toolkit/content/widgets/toolbarbutton.xml
     <content>
       <xul:image class="toolbarbutton-icon" xbl:inherits="src=image"/>
     </content>
   </binding>
+
+  <binding id="toolbarbutton-badged" display="xul:button"
+           extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton">

The display attribute is inherited from the base binding so is not strictly necessary here.


+toolbarbutton.badged-button {
+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-badged");
+}
+
+toolbarbutton.badged-button .toolbarbutton-badge[badge]:not([badge=""])::after {
+  content: attr(badge);
+}

Since you added the :not([badge=""]) here, my understanding is that the after content isn't generated at all when badge="", so the checks for this in various selectors in the theme files shouldn't be necessary.


diff --git a/toolkit/themes/linux/global/toolbarbutton.css b/toolkit/themes/linux/global/toolbarbutton.css
+.toolbarbutton-badge-container {
+  margin: 0;
+  padding: 0;
+  position: relative;
+}

toolbarbutton-badge-container already doesn't have a margin or padding. Similar for the other themes.


+toolbar[iconsize="small"] .toolbarbutton-badge-container {
+  margin: 0;
+}

The margin is already 0.

When I try this patch on a simple test file with only this button, the badge is partially not visible as it is offset above the boundary of the toolbarbutton and hidden behind the top window border. Perhaps you should either extend the top margin around toolbarbutton-badge-container, or move the badge lower.

Comment 28

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

Attachment: badge-toolkit-2.patch

(In reply to Neil Deakin from comment #27)

> OK, finally looked at this.

Thanks Neil!
I think I addressed all the review's comment; however:

> When I try this patch on a simple test file with only this button, the badge
> is partially not visible as it is offset above the boundary of the
> toolbarbutton and hidden behind the top window border.

Could you attach a screenshot of that, and give a way to reproduce the issue? In which platform it's happening? Is it different from the snapshot I attached (https://bug994280.bugzilla.mozilla.org/attachment.cgi?id=8472301)?
I fully tested the previous patch on Firefox in Windows 8, OS X 10.9.2 and Linux (Ubuntu 14) and at that time the results was the one the snapshot shows – Linux was behaving exactly as Windows.
Currently I can't test on Linux and Windows – I will be able to do so on Monday – but at least on OS X the behavior is still the same.

To test the button, I executed in scratchpad the following code, in browser context:

CustomizableUI.createWidget({
    id: 'test-button',
    type: 'custom',
    removable: true,
    defaultArea: CustomizableUI.AREA_NAVBAR,
    onBuild: function(aDocument) {
      let node = aDocument.createElement('toolbarbutton');
      node.id = this.id;
      node.setAttribute('class', 'toolbarbutton-1 chromeclass-toolbar-additional badged-button');
      node.style.listStyleImage = "url(resource:///chrome/browser/skin/classic/browser/Geolocation-16.png)";
      node.setAttribute("badge", "1");
      node.setAttribute("label", "button");
      return node;
    }
  });

Comment 29

[Neil Deakin]

Attachment: Screen Shot 2014-09-19 at 11.29.06 AM.png

A toolbarbutton is the only thing in the window:

<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <toolbarbutton class="badged-button" badge="23" label="Button" image="happy.png"/>
</window>

Comment 30

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

(In reply to Neil Deakin from comment #29)
> Created attachment 8492213 [details]
> Screen Shot 2014-09-19 at 11.29.06 AM.png
> 
> A toolbarbutton is the only thing in the window:

I see. So, it works as expected, the end result is exactly the one wanted by UX: the badge needs to be partially "outside" the button itself, as shown in the screenshot I attached. Of course, if the badge is directly aligned on the window's top, it doesn't have enough space to be fully displayed.

So, given that at least in Firefox, we need to have the badge in the position is now, I guess we could either:

1. Lowering the badge in the toolkit's stylesheet and having it upper in the browser's one; but then devs will have badges in a different position than the usual place (see other app that supports badges), plus the badge would cover totally the icon if there is more text. 

2. Leave as is, the devs that wants to use badged button – that anyway is a new feature so is not going to break anything – needs to add a bit of padding on the button's container, if there isn't already: the issue is only when the button is attached on the top's edge of the window, that is "cutting" partially the badge.

But if you have other suggestions, are more than welcome!

Comment 31

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

Attachment: badge-toolkit-3.patch

Made a minor change to badge's background color; removed `!important` from OS X style too.

Comment 32

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

Comment on attachment 8492194 [details] [diff] [review]
badge-toolkit-2.patch

forgot to mark as obsolete. :/

Comment 33

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

Comment on attachment 8454548 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1549

Erik, as mentioned previously I assigned the review to you: notice that if you want to run the unit test from that pull, you need to build Firefox with the badge patch here provided (the latest one, badge-toolkit-3).

Irakli, I'm asking a feedback from you because the API design change; let me know what do you think and if you have question about that.

Comment 34

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8454548 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1549

This looks good! great progress.

I think we should add a few more tests though which we can discuss, and there are a few things that I'm not clear on and would like some more information about, so have a look at the pr with you have a chance and r? me again when you're ready!

Comment 35

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8454548 [details] [review]
https://github.com/mozilla/addon-sdk/pull/1549

After talking to Matteo it seems like most of the stuff I nitting about was in the patch by mistake, so one test was reverted by mistake and there was some inline css changes that were not necessary.  Once that is removed I'm alright with landing.

Comment 36

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f17b7cd8b1d3
There is one orange at the moment, but it seems unrelated so far.

Comment 37

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

Attachment: badge-toolkit-final.patch

this is the same patch but exported with the proper commit message

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f467df214797

For future patches, please include your full name in your commit information.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d52e6cf9683e5721342c99edb1dedde1882f5eff
Bug 994280 - It would be nice to have ability to add badge over button

- Added badge API
- Added unit test

https://github.com/mozilla/addon-sdk/commit/6af9d62830bed15130918fa868ccefb066effac9
Merge pull request #1549 from ZER0/badge/994280

fix Bug 994280 - It would be nice to have ability to add badge over button r=@erikvold

Comment 41

[Mike Kaply [:mkaply]]

This was not an add-on sdk specific patch was it?

Is it documented somewhere?

Comment 42

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

(In reply to Mike Kaply [:mkaply] from comment #41)
> This was not an add-on sdk specific patch was it?
Matteo could probably answer more correctly, but it looks like there was both a toolkit patch and an add-on sdk patch.

> Is it documented somewhere?
Looks like https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/ui_button_action#Badged_buttons and https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/ui_button_toggle#Badged_buttons for the sdk patch.

Comment 43

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

(In reply to Mike Kaply [:mkaply] from comment #41)
> This was not an add-on sdk specific patch was it?

It was.

> Is it documented somewhere?

https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/ui_button_action#Badged_buttons

Comment 44

[Mike Kaply [:mkaply]]

Looking at the code, there's no reason this can't be used by regular add-ons. We should document it for them.

Comment 45

[[e-mail]]

(In reply to Gijs Kruitbosch from comment #21)
> because CSS' attr() only works on pseudo content, last I checked.

This is XBL: we have a solution for that. It looks something like this:
<xul:stack ...>
  <xul:image .../>
  <xul:label xbl:inherits="xbl:text=badge" .../>
</xul:stack>

You'd still need a bunch of CSS to position the label of course, so I'm not going to suggest you undo all the work you've already done here.