1180584 - Support multiple badges on menu button and add one for Sync authentication errors.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

In bug 1177589, we are looking to communicate "please reauthenticate with FxAccounts" via the hamburger menu rather than notifications at the bottom of the window. As part of this we want to "badge" the button in the same way that update notifications do.  

It looks like that badge was added in bug 1080406, but the code supporting it doesn't have the concept of multiple badges - but we are almost certainly going to want some kind of "priority" for these badges (eg, I'd assume if we have both pending updates and an authentication error, the update badge would win).

CC Gijs as he was instrumental in the landing of that bug and the followups - Gijs, we'd welcome any thoughts on the matter you might have.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Doh - forgot to CC Gijs - please see comment 0.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Gijs, here's a strawman for your enjoyment ;) What do you think?

As shouldn't be surprising, gMenuButtonUpdateBadge() doesn't do much with the badge itself - CSS does. Almost all the code is directly related to the update and the menu itself (eg, adjusting the *content* of the menu, not the badge itself)

So it sounds like we want a kind of "manager" for the badge itself, which is responsible for tracking multiple "badge requests" (badgers?). It would decide which has priority and would only need to update the attributes as badges are added and removed. It's not clear we need to make this too generic and support arbitrary badgers - maybe that would make more sense when we get a 3rd ;) - but keeping everything hard-coded seems to make sense for now. So we can use a couple of hard-coded "badge ids", with a relative priority, plus "addBadge(badgeId, attributeInfo)" and "removeBadge(badgeId)" functions with the 2 callers being gMenuButtonUpdateBadge and browser-fxaccounts.js.

The .css would still hard-code the attribute names - so we'd stick with:
> #PanelUI-update-status[update-status="succeeded"]
and add:
> #PanelUI-update-status[fxa-status="needs-authentication"]

or, possibly better, we decide now on a single attribute name - so it might look something like:
> #PanelUI-update-status[badge-status="update-succeeded"]
and add:
> #PanelUI-update-status[badge-status="fxa-needs-authentication"]

but either way, we've only 2 badgers and can manage the attributes without getting too clever. The "badge manager" would track the attribute(s?) currently in use for each badge ID and update the element as addBadge/removeBadge is called.

There's also a pref 'app.update.badge' to control whether a badge is shown. I suggest this shouldn't be part of this "manager" - gMenuButtonUpdateBadge() would just do the right thing (and we'd probably add a similar pref for this new badge). However, this implies we'd unconditionally set the menu as badged (ie, add 'badged-button' on the button class directly in the xul) - I can't see why that would cause a problem, but could be wrong.

Comment 3

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #2)
> Gijs, here's a strawman for your enjoyment ;) What do you think?
> 
> As shouldn't be surprising, gMenuButtonUpdateBadge() doesn't do much with

We should change the name though, because now it sounds like something that updates badges. :-)

> the badge itself - CSS does. Almost all the code is directly related to the
> update and the menu itself (eg, adjusting the *content* of the menu, not the
> badge itself)
> 
> So it sounds like we want a kind of "manager" for the badge itself, which is
> responsible for tracking multiple "badge requests" (badgers?). It would
> decide which has priority and would only need to update the attributes as
> badges are added and removed. It's not clear we need to make this too
> generic and support arbitrary badgers - maybe that would make more sense
> when we get a 3rd ;) - but keeping everything hard-coded seems to make sense
> for now. So we can use a couple of hard-coded "badge ids", with a relative
> priority, plus "addBadge(badgeId, attributeInfo)" and "removeBadge(badgeId)"
> functions with the 2 callers being gMenuButtonUpdateBadge and
> browser-fxaccounts.js.
> 
> The .css would still hard-code the attribute names - so we'd stick with:
> > #PanelUI-update-status[update-status="succeeded"]
> and add:
> > #PanelUI-update-status[fxa-status="needs-authentication"]
> 
> or, possibly better, we decide now on a single attribute name - so it might
> look something like:
> > #PanelUI-update-status[badge-status="update-succeeded"]
> and add:
> > #PanelUI-update-status[badge-status="fxa-needs-authentication"]
> 
> but either way, we've only 2 badgers and can manage the attributes without
> getting too clever. The "badge manager" would track the attribute(s?)
> currently in use for each badge ID and update the element as
> addBadge/removeBadge is called.
> 
> There's also a pref 'app.update.badge' to control whether a badge is shown.
> I suggest this shouldn't be part of this "manager" -
> gMenuButtonUpdateBadge() would just do the right thing (and we'd probably
> add a similar pref for this new badge). However, this implies we'd
> unconditionally set the menu as badged (ie, add 'badged-button' on the
> button class directly in the xul) - I can't see why that would cause a
> problem, but could be wrong.

This all sounds fine to me. Generic attribute probably easier from a CSS perspective and from making sure we only ever try to do one badge at a time. Manager JS also sounds fine. Pref story sounds fine. As for downsides there, well, slight perf impact. Probably too small to be picked up by our talos infra though, and probably too small to care about apart from adding another notch to the "argh, XUL, why are you so..." stick.

We should be able to do this and not touch XBL.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #3)
> As for downsides there, well, slight perf impact. Probably too small ...

The badged-button class is added at .init time rather than when the badge is added, so I don't see a concern there. What did you have in mind?

> We should be able to do this and not touch XBL.

That thought didn't even enter my mind ;)

Thanks!

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Chris, you said you were going to talk to eoger about possibly taking this.

Comment 6

[Chris Karlof [:ckarlof]]

eoger is gonna take it! 

eoger, you can start working on this. Please self-assign whenever you're ready, and coordinate with :markh on how it works with bug 1180587. I'll get it added to backlog.

Comment 7

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Draft patch, what do you think of this?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8631270 [details] [diff] [review]
bug-1180584.patch

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

looking ok. Probably worth getting some tests up sooner rather than later too (I'm not sure tests already exist - I sure hope they do, and knowing Gijs, he would have ensured they do - but either way we definitely want tests here.

I think you can also have browser-fxaccounts do the badge thing now - it will mean such errors are shown on both the button *and* the bottom infobar, but that's OK - we will probably just coordinate the landing of the infobar removal with this.

::: browser/base/content/browser.js
@@ +2565,5 @@
> +    PanelUI.menuButton.removeAttribute("badge");
> +    PanelUI.menuButton.removeAttribute("badge-status");
> +
> +    if (badgeToShow) {
> +      if (badgeToShow.badgeId == this.BADGEID_UPDATE && badgeToShow.attributeInfo == "failed") {

I think we can kill the need to set the "badge" attribute with css like:

#PanelUI-menu-button[update-status="failed"] .toolbarbutton-badge::after {
  content: "!";
}

Also, I don't think we should get too clever with the attribute names - just have the caller pass it in. Seeing as we are going towards a single attribute name, I think addBadge can just take the full attribute value.

And while it's probably not possible today to remove the "update" badge without restarting, I think we might as well make this manager not assume that - so addBadge(BADGEID_SYNC, ...); addBadge(BADGEID_UPDATE, ...); // now showing update badge
removeBadge(BADGEID_UPDATE); // now showing original sync badge.

(and let's call the "sync" badge an "fxa" badge.

@@ -2653,5 @@
>      updateButton.hidden = false;
> -
> -    PanelUI.panel.addEventListener("popupshowing", this, true);
> -  },
> -

Like you, I don't seem to see this fire, but I think we need to dig a little more into why (eg, I wonder if there is a test covering this?) Otherwise we should dig back through blame and ping whoever added it.

Comment 9

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Tests added and comments addressed.

Right now the Sync badge is just a ! (pretty much like update error, but with a blue background). Maybe we should check with someone else for colors and/or and icon.

I also asked :zer0 (Matteo Ferretti) about the about the "popupshowing" event listener: apparently the UX guys suggested that we clear the badges when we click the hamburger menu button. I re-implemented that and it works pretty well.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #9)
> Right now the Sync badge is just a ! (pretty much like update error, but
> with a blue background). Maybe we should check with someone else for colors
> and/or and icon.

Ryan's mockups uses an exclamation mark in a yellow, triangle-shaped background. I haven't looked yet, but from what you describe, the exclamation mark will be in a square background.

Ryan, can you please weigh-in here, either indicating a square background is OK and specifying the exact color, or by supplying a suitable svg?

> I also asked :zer0 (Matteo Ferretti) about the about the "popupshowing"
> event listener: apparently the UX guys suggested that we clear the badges
> when we click the hamburger menu button. I re-implemented that and it works
> pretty well.

Hmm. It sounds like the intended functionality didn't actually work and now I'm not convinced we should fix it here :) For one thing, I don't think that functionality actually makes sense - if I use the hamburger menu for something unrelated to the pending update I don't think the indicator should be removed as I'll now forget the update is pending (and similarly, I'll now forget Sync is in that error state). And worse, it makes the behaviour strange when there are *both* "update" and "sync" badges - should the first click on the hamburger menu replace the "update" badge with the "sync" badge, then a second click clear that Sync badge? IMO that would be weird.

Matteo, are you happy with us leaving this functionality broken for the time being and addressing that in a different bug? If so, do you mind opening it?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8631808 [details] [diff] [review]
bug-1180584.patch

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

Looking good!

::: browser/base/content/browser.js
@@ +2559,5 @@
> +  BADGEID_UPDATE: "update",
> +  BADGEID_FXA: "fxa",
> +
> +  fxaBadge: null,
> +  updateBadge: null,

this is really subjective, but I think the name "updateBadge" is a little misleading to the casual reader - I reading _updateBadge() below I thought it was something like "the badge to update". I guess I'm just suggesting this be renamed to, say, "appUpdateBadge" (and maybe BADGEID_UPDATE should be BADGEID_APPUPDATE)

Not too bothered by this though if you think I'm being too pedantic ;)

@@ +2568,5 @@
> +
> +    if (badgeToShow) {
> +      PanelUI.menuButton.setAttribute("badge-status", badgeToShow);
> +      PanelUI.panel.addEventListener("popupshowing", this, true);
> +    }

please cuddle all the "elses" here (ftr, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures)

@@ +2575,5 @@
> +    }
> +  },
> +
> +  addBadge: function(badgeId, attributeInfo) {
> +    let badge = attributeInfo || null;

attributeInfo should probably be renamed to something like "badgeAttrValue" or something - "info" is vague and what I was suggesting when it might have been an object or array with both name and value.

I also think that instead of allowing attribute to be null here we should have an internal helper - maybe something like "_changeBadge(badgeId, attributeValue)", and addBadge checks attributeValue is not null (throwing if it is) before calling _changeBadge, while removeBadge just calls _changeBadge explicitly passing null as the 2nd param.

Feel free to come up with a better name than "_changeBadge" ;)

@@ +2598,5 @@
> +    this.fxaBadge = null;
> +    this._updateBadge();
> +  },
> +
> +  handleEvent: function(e) {

see my other comment - I'm hoping we will be told not to bother with this here!

::: browser/base/content/test/general/browser_menuButtonBadgeManager.js
@@ +25,5 @@
> +  gMenuButtonBadgeManager.addBadge(gMenuButtonBadgeManager.BADGEID_UPDATE, "update-succeeded");
> +  yield PanelUI.show();
> +
> +  is(menuButton.hasAttribute("badge-status"), false, "Should not have a badge status (Hamburger menu opened)");
> +

might as well test clearBadges too.

Comment 12

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #8)
> Comment on attachment 8631270 [details] [diff] [review]
> bug-1180584.patch
> 
> Review of attachment 8631270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looking ok. Probably worth getting some tests up sooner rather than later
> too (I'm not sure tests already exist - I sure hope they do, and knowing
> Gijs, he would have ensured they do - but either way we definitely want
> tests here.

I don't think we have tests because of how they'd need to interact with the update IDL stuff, and because the initial landing here was slightly rushed.

Comment 13

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

(In reply to Mark Hammond [:markh] from comment #10)

> > I also asked :zer0 (Matteo Ferretti) about the about the "popupshowing"
> > event listener: apparently the UX guys suggested that we clear the badges
> > when we click the hamburger menu button. I re-implemented that and it works
> > pretty well.
> 
> Hmm. It sounds like the intended functionality didn't actually work and now
> I'm not convinced we should fix it here :)

The intended functionality worked when it was implemented; the fact that was broken is a regression – I believe when the arrow's svg image was introduced.

> For one thing, I don't think that
> functionality actually makes sense - if I use the hamburger menu for
> something unrelated to the pending update I don't think the indicator should
> be removed as I'll now forget the update is pending (and similarly, I'll now
> forget Sync is in that error state).

From Stephen Horlander, in the original bug:
>> Once you open and close the panel we should clear the badge.
>> Otherwise it will feel persistent and heavy.
>> Which is probably the opposite of what you want in a "silent" update.
( https://bugzilla.mozilla.org/show_bug.cgi?id=1080406#c29 )

> strange when there are *both* "update" and "sync" badges - should the first
> click on the hamburger menu replace the "update" badge with the "sync"
> badge, then a second click clear that Sync badge? IMO that would be weird.

At this point it's probably better checking with UX – maybe Stephen directly, due the fact he was originally involved with that design – providing the new information we want to display to the user, and see how the behaviour should be evolved.

> Matteo, are you happy with us leaving this functionality broken for the time
> being and addressing that in a different bug? If so, do you mind opening it?

I don't mind – it was a lower priority anyway, and it seems to me that is broken since a while so it shouldn't definitely be a blocker.

Comment 14

[Ryan Feeley [:rfeeley]]

Attachment: restart-firefox.png

I have the warning (yellow triangle exclamation mark) icon, but would like to match the exact style of this restart badge. What is the file? Is it SVG? Is the shadow in the file or is styled?

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ryan Feeley from comment #14)
> Created attachment 8633233 [details]
> restart-firefox.png
> 
> I have the warning (yellow triangle exclamation mark) icon, but would like
> to match the exact style of this restart badge. What is the file? Is it SVG?
> Is the shadow in the file or is styled?

Existing one at https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/update-badge.svg - it looks trivial! Re-adding ni? so it doesn't get lost ;)

Comment 16

[Ryan Feeley [:rfeeley]]

Attachment: stinkin-badges.gif

Here is the mockup. Animated GIF showing no badge, update badge and sync warning badge. SVG comes next.

Comment 17

[Ryan Feeley [:rfeeley]]

Attachment: warning-16.svg

Basic warning icon. Requires 1px 15% shadow (copy restart icon style if possible).

Comment 18

[Edouard Oger [:eoger]]

Attachment: Screen Shot 2015-07-13 at 8.01.08 PM.png

Here's what I've been able to do.
I wasn't able to reproduce the inset shadows because the css property "filter: drop-shadow" cannot do inset shadows. If we want these we have to hard-code them in the SVG file.

Comment 19

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

ˆ Patch for this

Comment 20

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Removed the "clear badges on popup open" feature and updated tests.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8633298 [details] [diff] [review]
bug-1180584.patch

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

::: browser/base/content/browser.js
@@ +2581,5 @@
> +    }
> +  },
> +
> +  _changeBadge: function (badgeId, badgeStatus) {
> +    let badge = badgeStatus || null;

I'd be inclined to explicitly pass null in removeBadge rather than (or as well as?) the '||null' check.

@@ -2665,5 @@
> -
> -    PanelUI.panel.addEventListener("popupshowing", this, true);
> -  },
> -
> -  handleEvent: function(e) {

we must make sure a bug is on file for this before landing.

::: browser/themes/linux/jar.mn
@@ +482,5 @@
>    skin/classic/browser/[e-mail] (../shared/[e-mail])
>  #endif
>    skin/classic/browser/warning16.png                        (../shared/warning16.png)
>    skin/classic/browser/[e-mail]                     (../shared/[e-mail])
> +  skin/classic/browser/warning-16.svg                       (../shared/warning-16.svg)

ack - sorry,but if these 2 .pngs are perfect (apart from not being .svg) and there's no other issues, we should probably see if we can use them rather than a new .svg. I think :)

::: browser/themes/shared/warning-16.svg
@@ +6,5 @@
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     width="16"
> +     height="16"
> +     viewBox="0 0 16 16">
> +       

trailing whitespace.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8633298 [details] [diff] [review]
bug-1180584.patch

Gijs, all feedback welcome, but in particular see comment 21 where we are adding a 'warning16' svg alongside the same .png variants. The svg is small and seems better long term, but is also redundant - what do you think?

Comment 23

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #22)
> Comment on attachment 8633298 [details] [diff] [review]
> bug-1180584.patch
> 
> Gijs, all feedback welcome, but in particular see comment 21 where we are
> adding a 'warning16' svg alongside the same .png variants. The svg is small
> and seems better long term, but is also redundant - what do you think?

If you're going to say all feedback, then I'm going to say:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#645

645   list-style-image: url(chrome://browser/skni/warning16.png);

read closely.

Yeah.

Why don't we just remove the PNG and use the SVG file? We're heading in the SVG direction anyway.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8633298 [details] [diff] [review]
bug-1180584.patch

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

Otherwise, the CSS is all about to get bitrotted to smithereens by bug 1029937.

::: browser/base/content/browser.js
@@ +2581,5 @@
> +    }
> +  },
> +
> +  _changeBadge: function (badgeId, badgeStatus) {
> +    let badge = badgeStatus || null;

Besides, that, nitpick:

_changeBadge(badgeId, badgeStatus=null) {
}

We have all the syntactic sugar now, let's use it. Same for the other methods.

@@ +2587,5 @@
> +      this.appUpdateBadge = badge;
> +    } else if (badgeId == this.BADGEID_FXA) {
> +      this.fxaBadge = badge;
> +    } else {
> +      throw new Error("This badge ID is unknown!");

Errors in JS are ugly. Any reason not to Cu.reportError and bail silently?

@@ +2594,5 @@
> +  },
> +
> +  addBadge: function (badgeId, badgeStatus) {
> +    if (!badgeStatus) {
> +      throw new Error("badgeStatus must be defined!");

Ditto. Though I'd actually vote for defaulting badgeStatus to "true".

@@ -2591,5 @@
>        this.timer.cancel();
>      if (this.enabled) {
>        Services.obs.removeObserver(this, "update-staged");
>        Services.obs.removeObserver(this, "update-downloaded");
> -      PanelUI.panel.removeEventListener("popupshowing", this, true);

So while I'm sympathetic to removing imperfect code, I would prefer you check with UX (:phlsa or someone) before just removing this outright.

::: browser/themes/windows/jar.mn
@@ +582,5 @@
>          skin/classic/browser/[e-mail] (../shared/[e-mail])
>  #endif
>          skin/classic/browser/warning16.png                            (../shared/warning16.png)
>          skin/classic/browser/[e-mail]                         (../shared/[e-mail])
> +        skin/classic/browser/warning-16.svg                           (../shared/warning-16.svg)

So yes, if we're adding this, we should rm the PNGs.

Comment 25

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Here's the updated patch addressing your comments while waiting for UX team to answer about the popupshowing event. You have to apply the attached patch in bug 1183730 first.

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #25)
> while waiting for UX team to answer about the popupshowing event.

FTR, that's bug 1183212 (which blocks this, but I missed it :)

Comment 27

[Ryan Feeley [:rfeeley]]

Looks great!

Comment 28

[Tim Nguyen :ntim]

Comment on attachment 8633679 [details] [diff] [review]
bug-1180584.patch

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

Some tips for cleaning up the SVG, see https://etherpad.mozilla.org/svg-guidelines

::: browser/themes/shared/warning16.svg
@@ +1,3 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +

- This needs a license header
- The DOCTYPE can be removed
- This blank line too.

@@ +2,5 @@
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +
> +<svg version="1.1"
> +     xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"

The version and the xmlns:xlink attributes can be removed.

@@ +6,5 @@
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     width="16"
> +     height="16"
> +     viewBox="0 0 16 16">
> +  <path id="background" fill="#ffbf00" d="M14.8,12.5L9.3,1.9C9,1.3,8.5,1,8,1C7.5,1,7,1.3,6.7,1.9L1.2,12.5c-0.3,0.6-0.3,1.2,0,1.7C1.5,14.7,2,15,2.6,15h10.8 c0.6,0,1.1-0.3,1.4-0.8C15.1,13.7,15.1,13.1,14.8,12.5z"/>

This id can be removed.

Comment 29

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

I moved the SVG replacement into another bug, because some other bugs depend on that new image and I want them to land quickly (and it's also easier to review this patch now).

Comment 30

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

And I posted the wrong patch...

Comment 31

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Changed warning16.svg to warning.svg (see blocking bug 1184627)

Comment 32

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Reworked on the patch since bug 1185725 changed the display implementation of the badges.

Comment 33

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Rebased and also removed a bug where it would break the hamburger menu update button css.

Comment 34

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8638803 [details] [diff] [review]
bug-1180584.patch

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

LGTM! But let's not land this until bug 1180587 is ready (or if that ends up being a PITA, we could just land this without the browser-fxaccount changes)

::: browser/base/content/browser-fxaccounts.js
@@ +280,5 @@
>            let tooltipDescription = this.strings.formatStringFromName("reconnectDescription", [userData.email], 1);
>            this.panelUIFooter.setAttribute("fxastatus", "error");
>            this.panelUILabel.setAttribute("label", errorLabel);
>            this.panelUIStatus.setAttribute("tooltiptext", tooltipDescription);
> +          gMenuButtonBadgeManager.addBadge(gMenuButtonBadgeManager.BADGEID_FXA, "fxa-needs-authentication");

I'm slightly worried about flicker here. It may be better to keep an, eg, showBadge boolean and at the end of this function either remove the badge or addBadge (which if I'm reading the code correctly will do the right thing if the badge already exists). I'm just trying to avoid us removing the badge and immediately re-adding it every time this is called.

::: browser/base/content/browser.js
@@ +2607,5 @@
> +    }
> +    this._showBadge();
> +  },
> +
> +  addBadge: function (badgeId, badgeStatus = "true") {

I don't think we need the default for badgeStatus here, but maybe should throw/log if it is not specified.

@@ +2689,5 @@
>      this.uninit();
>    },
>  
>    displayBadge: function (succeeded) {
>      let status = succeeded ? "succeeded" : "failed";

If we make this line |let status = "update-" + (succeeded ? "succeeded" : "failed");| we end up with 2 lines of reasonable length.

Comment 35

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Comments addressed except the last one: the status variable is used in |updateButton.setAttribute("update-status", status);|.
This how I broke the update button earlier.
I settled on using a second variable badgeStatus to shorten the addBadge() line.

Comment 36

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8639975 [details] [diff] [review]
bug-1180584.patch

After testing these patches, I notice the "update failed" icon looks different enough that I think we should be the OK from Shorlander. I'll attach screen-shots.

Comment 37

[Mark Hammond [:markh] [:mhammond]]

Attachment: update-failed-button-before.png

This is how the button looks on "update failed" before this patch.

Comment 38

[Mark Hammond [:markh] [:mhammond]]

Attachment: update-failed-button-after.png

Steven, this is what the "update failed" button looks like after this patch. Note that the button is squarer than it was - but see also the following attachment, where we show the same icon in the menu itself, so being squarer actually makes sense IMO - but I'm not sure this version is as clear as the old one.

Comment 39

[Mark Hammond [:markh] [:mhammond]]

Attachment: update-failed-menu-after.png

This change is actually from bug 1186521, but I'm attaching it here for completeness. This is how the hamburger menu looks in an "update failed" state, where the intent is for the badge on the button and on the menu to be the same(ish?)

Comment 40

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #38)
> Steven,

Sorry - make that Stephen!

Comment 41

[Stephen Horlander [:shorlander]]

Attachment: Badge/Icon Issues - 01

(In reply to Mark Hammond [:markh] from comment #38)
> Created attachment 8640860 [details]
> update-failed-button-after.png
> 
> Steven, this is what the "update failed" button looks like after this patch.
> Note that the button is squarer than it was - but see also the following
> attachment, where we show the same icon in the menu itself, so being squarer
> actually makes sense IMO - but I'm not sure this version is as clear as the
> old one.

I am fine with squarer, that's probably the right thing to do. Just a couple of comments:
- The height of the badge is too short compared to the other toolbar badges
- It isn't aligned with the other badges
- The Exclamation Mark "icon" for failed downloads is fuzzy and off-center
  - It should use an icon image like the others, e.g. chrome://browser/skin/update-badge.svg

Comment 42

[Stephen Horlander [:shorlander]]

Comment on attachment 8640860 [details]
update-failed-button-after.png

See comment 41.

Comment 43

[Stephen Horlander [:shorlander]]

Comment on attachment 8640863 [details]
update-failed-menu-after.png

See comment 41.

Comment 44

[Stephen Horlander [:shorlander]]

Attachment: update-badge-failed.svg

Icon for failed update.

Comment 45

[Edouard Oger [:eoger]]

Attachment: bug-1180584.patch

Patch amended with the new update-failed icon.

Comment 46

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8643176 [details] [diff] [review]
bug-1180584.patch

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

Looks good - I'll check this in with its companions tomorrow!

Comment 47

[Pulsebot]

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

Comment 48

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

https://hg.mozilla.org/mozilla-central/rev/8d243e99ae6e