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

RESOLVED FIXED in Firefox 42

Status

()

Firefox
Toolbars and Customization
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markh, Assigned: eoger)

Tracking

unspecified
Firefox 42
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [fxsync])

Attachments

(10 attachments, 11 obsolete attachments)

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
(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Doh - forgot to CC Gijs - please see comment 0.
(Reporter)

Updated

2 years ago
Blocks: 1180587
(Reporter)

Comment 2

2 years ago
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)
(Reporter)

Comment 4

2 years ago
(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!
(Reporter)

Comment 5

2 years ago
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)

Updated

2 years ago
Assignee: nobody → edouard.oger
Flags: needinfo?(edouard.oger)
(Assignee)

Comment 7

2 years ago
Created attachment 8631270 [details] [diff] [review]
bug-1180584.patch

Draft patch, what do you think of this?
Attachment #8631270 - Flags: feedback?(markh)
(Reporter)

Comment 8

2 years ago
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+
(Assignee)

Comment 9

2 years ago
Created attachment 8631808 [details] [diff] [review]
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.
Attachment #8631270 - Attachment is obsolete: true
Attachment #8631808 - Flags: feedback?(markh)

Updated

2 years ago
Blocks: 1182288

Updated

2 years ago
Rank: 15

Updated

2 years ago
Iteration: --- → 42.1 - Jul 13
(Reporter)

Comment 10

2 years ago
(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)
(Reporter)

Comment 11

2 years ago
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)
Blocks: 1183212
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?
Flags: needinfo?(rfeeley)
(Reporter)

Comment 15

2 years ago
(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)
Created attachment 8633242 [details]
stinkin-badges.gif

Here is the mockup. Animated GIF showing no badge, update badge and sync warning badge. SVG comes next.
Flags: needinfo?(rfeeley)
Created attachment 8633243 [details]
warning-16.svg

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

Comment 18

2 years ago
Created attachment 8633278 [details]
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.
Attachment #8633278 - Flags: feedback?(rfeeley)
(Assignee)

Comment 19

2 years ago
Created attachment 8633281 [details] [diff] [review]
bug-1180584.patch

ˆ Patch for this
Attachment #8631808 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8633298 [details] [diff] [review]
bug-1180584.patch

Removed the "clear badges on popup open" feature and updated tests.
Attachment #8633281 - Attachment is obsolete: true
(Reporter)

Comment 21

2 years ago
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.
(Reporter)

Comment 22

2 years ago
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+
(Assignee)

Updated

2 years ago
Depends on: 1183730
(Assignee)

Comment 25

2 years ago
Created attachment 8633679 [details] [diff] [review]
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.
Attachment #8633298 - Attachment is obsolete: true
(Reporter)

Comment 26

2 years ago
(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!
(Assignee)

Updated

2 years ago
Blocks: 1183924
(Assignee)

Updated

2 years ago
Blocks: 1184184

Comment 28

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1184627
(Assignee)

Updated

2 years ago
No longer blocks: 1184184
(Assignee)

Updated

2 years ago
No longer blocks: 1183924
(Assignee)

Comment 29

2 years ago
Created attachment 8634825 [details] [diff] [review]
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).
Attachment #8633679 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
Created attachment 8634826 [details] [diff] [review]
bug-1180584.patch

And I posted the wrong patch...
Attachment #8634825 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8633278 - Flags: feedback?(rfeeley) → feedback+
(Assignee)

Comment 31

2 years ago
Created attachment 8635456 [details] [diff] [review]
bug-1180584.patch

Changed warning16.svg to warning.svg (see blocking bug 1184627)
Attachment #8634826 - Attachment is obsolete: true

Updated

2 years ago
Whiteboard: [fxsync]
(Assignee)

Updated

2 years ago
No longer blocks: 1183212
(Assignee)

Updated

2 years ago
Blocks: 1186521
(Assignee)

Updated

2 years ago
Depends on: 1185725
(Assignee)

Comment 32

2 years ago
Created attachment 8637658 [details] [diff] [review]
bug-1180584.patch

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)
(Assignee)

Comment 33

2 years ago
Created attachment 8638803 [details] [diff] [review]
bug-1180584.patch

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)
(Reporter)

Comment 34

2 years ago
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+
(Assignee)

Comment 35

2 years ago
Created attachment 8639975 [details] [diff] [review]
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.
Attachment #8638803 - Attachment is obsolete: true
Attachment #8639975 - Flags: review+
(Reporter)

Comment 36

2 years ago
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)
(Reporter)

Comment 37

2 years ago
Created attachment 8640859 [details]
update-failed-button-before.png

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

Comment 38

2 years ago
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.
Attachment #8640860 - Flags: ui-review?(shorlander)
(Reporter)

Comment 39

2 years ago
Created attachment 8640863 [details]
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?)
Attachment #8640863 - Flags: ui-review?(shorlander)
(Reporter)

Comment 40

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

Sorry - make that Stephen!

Updated

2 years ago
Iteration: 42.1 - Jul 13 → 42.3 - Aug 10
Flags: firefox-backlog?
Priority: -- → P1
Flags: firefox-backlog? → firefox-backlog+
Created attachment 8643075 [details]
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-
Comment on attachment 8640863 [details]
update-failed-menu-after.png

See comment 41.
Attachment #8640863 - Flags: ui-review?(shorlander)
Created attachment 8643080 [details]
update-badge-failed.svg

Icon for failed update.
(Assignee)

Comment 45

2 years ago
Created attachment 8643176 [details] [diff] [review]
bug-1180584.patch

Patch amended with the new update-failed icon.
Attachment #8639975 - Attachment is obsolete: true
Attachment #8639975 - Flags: review?(markh)
Attachment #8643176 - Flags: review?(markh)
(Reporter)

Comment 46

2 years ago
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+

Comment 47

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/8d243e99ae6e
https://hg.mozilla.org/mozilla-central/rev/8d243e99ae6e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
(Reporter)

Updated

2 years ago
Depends on: 1199355
No longer depends on: 1199355
You need to log in before you can comment on or make changes to this bug.