Closed Bug 1180584 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- fixed

People

(Reporter: markh, Assigned: eoger)

References

Details

(Whiteboard: [fxsync])

Attachments

(10 files, 11 obsolete files)

18.96 KB, image/png
Details
13.25 KB, image/gif
Details
750 bytes, image/svg+xml
Details
47.13 KB, image/png
rfeeley
: feedback+
Details
855 bytes, image/png
Details
851 bytes, image/png
shorlander
: ui-review-
Details
3.91 KB, image/png
Details
107.26 KB, image/png
Details
472 bytes, image/svg+xml
Details
17.53 KB, patch
markh
: review+
Details | Diff | Splinter Review
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.
Doh - forgot to CC Gijs - please see comment 0.
Blocks: 1180587
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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!
Chris, you said you were going to talk to eoger about possibly taking this.
Flags: needinfo?(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.
Flags: needinfo?(ckarlof) → needinfo?(edouard.oger)
Assignee: nobody → edouard.oger
Flags: needinfo?(edouard.oger)
Attached patch bug-1180584.patch (obsolete) — Splinter Review
Draft patch, what do you think of this?
Attachment #8631270 - Flags: feedback?(markh)
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.
Attachment #8631270 - Flags: feedback?(markh) → feedback+
Attached patch bug-1180584.patch (obsolete) — Splinter Review
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.
Attachment #8631270 - Attachment is obsolete: true
Attachment #8631808 - Flags: feedback?(markh)
Blocks: 1182288
Rank: 15
Iteration: --- → 42.1 - Jul 13
(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?
Flags: needinfo?(zer0)
Flags: needinfo?(rfeeley)
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.
Attachment #8631808 - Flags: feedback?(markh) → feedback+
(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.
(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.
Flags: needinfo?(zer0)
Attached image 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?
Flags: needinfo?(rfeeley)
(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 ;)
Flags: needinfo?(rfeeley)
Attached image stinkin-badges.gif
Here is the mockup. Animated GIF showing no badge, update badge and sync warning badge. SVG comes next.
Flags: needinfo?(rfeeley)
Attached image warning-16.svg
Basic warning icon. Requires 1px 15% shadow (copy restart icon style if possible).
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.
Attachment #8633278 - Flags: feedback?(rfeeley)
Attached patch bug-1180584.patch (obsolete) — Splinter Review
ˆ Patch for this
Attachment #8631808 - Attachment is obsolete: true
Attached patch bug-1180584.patch (obsolete) — Splinter Review
Removed the "clear badges on popup open" feature and updated tests.
Attachment #8633281 - Attachment is obsolete: true
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/e10s-64@2x.png (../shared/e10s-64@2x.png)
>  #endif
>    skin/classic/browser/warning16.png                        (../shared/warning16.png)
>    skin/classic/browser/warning16@2x.png                     (../shared/warning16@2x.png)
> +  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 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?
Attachment #8633298 - Flags: feedback?(gijskruitbosch+bugs)
(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 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/e10s-64@2x.png (../shared/e10s-64@2x.png)
>  #endif
>          skin/classic/browser/warning16.png                            (../shared/warning16.png)
>          skin/classic/browser/warning16@2x.png                         (../shared/warning16@2x.png)
> +        skin/classic/browser/warning-16.svg                           (../shared/warning-16.svg)

So yes, if we're adding this, we should rm the PNGs.
Attachment #8633298 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Depends on: 1183730
Attached patch bug-1180584.patch (obsolete) — Splinter Review
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.
Attachment #8633298 - Attachment is obsolete: true
(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 :)
Looks great!
Blocks: 1183924
Blocks: 1184184
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.
Depends on: 1184627
No longer blocks: 1184184
No longer blocks: 1183924
Attached patch bug-1180584.patch (obsolete) — Splinter Review
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).
Attachment #8633679 - Attachment is obsolete: true
Attached patch bug-1180584.patch (obsolete) — Splinter Review
And I posted the wrong patch...
Attachment #8634825 - Attachment is obsolete: true
Attachment #8633278 - Flags: feedback?(rfeeley) → feedback+
Attached patch bug-1180584.patch (obsolete) — Splinter Review
Changed warning16.svg to warning.svg (see blocking bug 1184627)
Attachment #8634826 - Attachment is obsolete: true
Whiteboard: [fxsync]
No longer blocks: 1183212
Blocks: 1186521
Depends on: 1185725
Attached patch bug-1180584.patch (obsolete) — Splinter Review
Reworked on the patch since bug 1185725 changed the display implementation of the badges.
Attachment #8635456 - Attachment is obsolete: true
Attachment #8637658 - Flags: review?(markh)
Attached patch bug-1180584.patch (obsolete) — Splinter Review
Rebased and also removed a bug where it would break the hamburger menu update button css.
Attachment #8637658 - Attachment is obsolete: true
Attachment #8637658 - Flags: review?(markh)
Attachment #8638803 - Flags: review?(markh)
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.
Attachment #8638803 - Flags: review?(markh) → review+
Attached patch bug-1180584.patch (obsolete) — Splinter Review
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.
Attachment #8638803 - Attachment is obsolete: true
Attachment #8639975 - Flags: review+
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.
Attachment #8639975 - Flags: review+ → review?(markh)
This is how the button looks on "update failed" before this patch.
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.
Attachment #8640860 - Flags: ui-review?(shorlander)
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?)
Attachment #8640863 - Flags: ui-review?(shorlander)
(In reply to Mark Hammond [:markh] from comment #38)
> Steven,

Sorry - make that Stephen!
Iteration: 42.1 - Jul 13 → 42.3 - Aug 10
Flags: firefox-backlog?
Priority: -- → P1
Flags: firefox-backlog? → firefox-backlog+
Attached image 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 on attachment 8640860 [details]
update-failed-button-after.png

See comment 41.
Attachment #8640860 - Flags: ui-review?(shorlander) → ui-review-
Patch amended with the new update-failed icon.
Attachment #8639975 - Attachment is obsolete: true
Attachment #8639975 - Flags: review?(markh)
Attachment #8643176 - Flags: review?(markh)
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!
Attachment #8643176 - Flags: review?(markh) → review+
https://hg.mozilla.org/mozilla-central/rev/8d243e99ae6e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1199355
You need to log in before you can comment on or make changes to this bug.