1080406 - Add an optional badge to the hamburger menu when updates are available

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: update-badge.patch

If silent updates are enabled, it would be useful to have some non-intrusive indication in the hamburger menu that a update is ready to be installed (or background updates have failed and a manual update is required). This patch adds such a badge on the hamburger menu using the recent API from bug 994280.

The behavior is disabled by default as silent updates are also disabled by default.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: update-ready.png

This is what a successful update looks like.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: update-failed.png

This is what a failed background update looks like.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8502351 [details] [diff] [review]
update-badge.patch

I should probably gate this on both app.update.badge and app.update.silent, but does it otherwise look sane?

Comment 4

[Dão Gottwald [:dao]]

It seems odd to have the indicator on the button while that button's command action (i.e. the menu it opens) has nothing to do with that indicator's intent.

Anyway, this should get ui-review before code review.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Good point, I should have mentioned that in offline discussions we are planning to add an extra button on the menu (probably as the first one on the list) to carry out the relevant action (reload or open the downloads page). This version is meant a step towards that goal.

Comment 6

[:Gijs (he/him)]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #5)
> Good point, I should have mentioned that in offline discussions we are
> planning to add an extra button on the menu (probably as the first one on
> the list) to carry out the relevant action (reload or open the downloads
> page). This version is meant a step towards that goal.

Where is the work for the button? I'm also not sure what "first one on the list" means here.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8502351 [details] [diff] [review]
update-badge.patch

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

::: browser/base/content/browser.js
@@ +1252,5 @@
> +    try {
> +      useBadges = Services.prefs.getBoolPref("app.update.badge");
> +    } catch (e) {}
> +    if (useBadges) {
> +      let hamburger = document.getElementById("PanelUI-menu-button");

Don't do this, use PanelUI.menuButton instead.

@@ +1257,5 @@
> +      hamburger.classList.add("badged-button");
> +      Services.obs.addObserver(function updateObserver(subject, topic, status) {
> +        // Update the UI when the background updater is finished.
> +        if (status == "applied" || status == "applied-service" ||
> +            status == "pending" || status == "pending-service") {

Are there no constants for this on the update service? I'm wary of having the same strings hardcoded here.

Nit: IMO this would be clearer as a switch statement.

@@ +1263,5 @@
> +          // to non-staged updates, add a badge to the hamburger menu to indicate an
> +          // update will be applied once the browser restarts.
> +          let badge = document.getAnonymousElementByAttribute(hamburger,
> +                                                              "class",
> +                                                              "toolbarbutton-badge");

We should instead set an attribute on the button and use CSS to style the background of the badge.

@@ +1271,5 @@
> +          hamburger.setAttribute("tooltiptext", tooltip);
> +        } else if (status == "failed") {
> +          // Background update has failed, let's show the UI responsible for
> +          // prompting the user to update manually.
> +          hamburger.setAttribute("badge", "!");

Does this not need to be localizable? I don't know how universal a symbol this is... (thinking of languages which use it upside down at the start of sentences, I would not be surprised if there were others that used different punctuation?)

Similar question for the star unicode character. Also, have you thought about specifying the font for the badge? Different (Windows) systems can use different fonts for symbols, sometimes with and sometimes without color (see e.g. bug 1054780 )

@@ +1279,5 @@
> +          // We've fallen back to downloading the full update because the partial
> +          // update failed to get staged in the background. Therefore we need to keep
> +          // our observer.
> +          return;
> +        }

Is the desired UI here really that the button stays badged until you restart? That seems like it'd get really annoying for people on Nightly who habitually get rid of all their notifications.

I defer to Stephen here, but I would have thought that the badge should clear (perhaps with some delay) when opening the menu - as Dão said, normally the badge should be on a thing which lets you act, which would then remove the notification.

Comment 8

[Stephen Horlander [:shorlander]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #5)
> Good point, I should have mentioned that in offline discussions we are
> planning to add an extra button on the menu (probably as the first one on
> the list) to carry out the relevant action (reload or open the downloads
> page). This version is meant a step towards that goal.

I agree that it's weird to add an indicator to the menu button when it is not directly related to updates.

It also isn't clear that adding a new button to the menu is necessarily what we would want to do as that has other implications.

Before we start badging things I feel like we should at least have a better understanding of what the ideal end state is.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

AIUI the goal is to use silent updates, but provide users with a non-intrusive notification of a pending or failed background update. This should result in a seamless upgrade experience, which would ensure that we keep the aurora population always up to date. Badging the hamburger button came up when we thought about displaying the notification in a place users might be familiar with, since Chrome does something similar.

The two possible messages need to provide some affordance for carrying out the suggested action (restarting or downloading). Assuming we can't think of a better notification mechanism, it would seem natural to put buttons in the hamburger menu, since clicking it would naturally open it and its contents would be immediately visible and demanding user attention.

Joe might be able to provide more context on the subject.

Comment 10

[Madhava Enros [:madhava]]

I think sevaan had also done some thinking on notifying about version updates in the menu.

Comment 11

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #9)
> AIUI the goal is to use silent updates, but provide users with a
> non-intrusive notification of a pending or failed background update. This
> should result in a seamless upgrade experience, which would ensure that we
> keep the aurora population always up to date. Badging the hamburger button
> came up when we thought about displaying the notification in a place users
> might be familiar with, since Chrome does something similar.
> 
> The two possible messages need to provide some affordance for carrying out
> the suggested action (restarting or downloading). Assuming we can't think of
> a better notification mechanism, it would seem natural to put buttons in the
> hamburger menu, since clicking it would naturally open it and its contents
> would be immediately visible and demanding user attention.
> 
> Joe might be able to provide more context on the subject.

This is a UX decision, but I agree that we shouldn't have a badge with a call-to-action, but no obvious way to take action.

The simplest possible solution is a button that says "Download update" or "Restart $APP_NAME" depending on the situation.

Could we land just those 2 strings?

Comment 12

[Sevaan Franks [:sevaan]]

(In reply to Madhava Enros [:madhava] from comment #10)
> I think sevaan had also done some thinking on notifying about version
> updates in the menu.

My thinking was to use a promotional strip in the menu, rather than an icon: http://cl.ly/image/212V390f2W3S

This strip could be used for a number of things in the future.

Bug 1062930 is the implementation bug for it.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add an optional badge to the hamburger menu when updates are available v2

I wanted to take care of Gijs's comments, but I ran out of time and Matteo agreed to pick this up, so I wanted to post what I've managed to finish.

(In reply to :Gijs Kruitbosch from comment #7)
> Don't do this, use PanelUI.menuButton instead.

Done.

> Are there no constants for this on the update service? I'm wary of having
> the same strings hardcoded here.

There are, but they aren't exported. I did copy the constants however, as that seems to be common practice in other places in the tree.

> Nit: IMO this would be clearer as a switch statement.

Done.

> @@ +1263,5 @@
> > +          // to non-staged updates, add a badge to the hamburger menu to indicate an
> > +          // update will be applied once the browser restarts.
> > +          let badge = document.getAnonymousElementByAttribute(hamburger,
> > +                                                              "class",
> > +                                                              "toolbarbutton-badge");
> 
> We should instead set an attribute on the button and use CSS to style the
> background of the badge.

I didn't get to that, yet.

> Does this not need to be localizable? I don't know how universal a symbol
> this is... (thinking of languages which use it upside down at the start of
> sentences, I would not be surprised if there were others that used different
> punctuation?)
> 
> Similar question for the star unicode character. Also, have you thought
> about specifying the font for the badge? Different (Windows) systems can use
> different fonts for symbols, sometimes with and sometimes without color (see
> e.g. bug 1054780 )

The star and exclamation mark characters were meant more like placeholders until UX provides us with nice icons, at which point localization wouldn't be an issue.

> Is the desired UI here really that the button stays badged until you
> restart? That seems like it'd get really annoying for people on Nightly who
> habitually get rid of all their notifications.
> 
> I defer to Stephen here, but I would have thought that the badge should
> clear (perhaps with some delay) when opening the menu - as Dão said,
> normally the badge should be on a thing which lets you act, which would then
> remove the notification.

Agreed, but I didn't get to this point yet.

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #11)
> Could we land just those 2 strings?

The strings I was anticipating to use in this bug landed on time in bug 1080690.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: interdiff - Fix leak in update badges

The leak is quite simple, we are not removing the observer listener when we close the window,
that ends up leaking the window itself...
I ended up refactoring the code to put badges in its own singleton,
as most other browses.js features...
But as I don't know how to test this code, I don't know if I somehow broke it or not :/

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Add an optional badge to the hamburger menu when updates are available v3

with the leak fix.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

It still works, but it's based on v1, not v2, since I hadn't pushed that to gum yet. That's not a big deal though, I'll update the patch once we verify the leak fixes.

Comment 17

[Sevaan Franks [:sevaan]]

Attachment: Updated Badges + Menu Bar Strip

Updated badges attached, with the informational strip in the menu.

When an update is available, the green badge appears. When the user clicks on the menu, the green strip can be clicked to restart the browser to install the update.

When an update fails, the red badge appears. When the user clicks on the menu, the red strip can be clicked to restart the download. Once the download is complete, the green badge is displayed.

Comment 18

[Sevaan Franks [:sevaan]]

Matej, can you review the strings in the strips of the above attachment?: https://bugzilla.mozilla.org/attachment.cgi?id=8507062

Update available. Click to restart!
Update failed. Click to redownload.

I'm trying to keep it short and pithy so it all fits on one line.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Sevaan Franks [:sevaan] from comment #18)
> Matej, can you review the strings in the strips of the above attachment?:
> https://bugzilla.mozilla.org/attachment.cgi?id=8507062
> 
> Update available. Click to restart!
> Update failed. Click to redownload.
> 
> I'm trying to keep it short and pithy so it all fits on one line.

What's short in one language will be longer in others. FYI, in German those two lines would be something like this:

> Update verfügbar. Klicken um neu zu starten!
> Update fehlgeschlagen. Klicken um erneut herunterzulanden.

Comment 20

[Sevaan Franks [:sevaan]]

Then I suppose pi(In reply to Dão Gottwald [:dao] from comment #19)

> > Update verfügbar. Klicken um neu zu starten!
> > Update fehlgeschlagen. Klicken um erneut herunterzulanden.

Then I suppose pithiness doesn't matter! Two lines works as well:

http://cl.ly/image/3s043d1y112i

Comment 22

[Matej Novak [:matej]]

(In reply to Sevaan Franks [:sevaan] from comment #18)
> Matej, can you review the strings in the strips of the above attachment?:
> https://bugzilla.mozilla.org/attachment.cgi?id=8507062
> 
> Update available. Click to restart!
> Update failed. Click to redownload.
> 
> I'm trying to keep it short and pithy so it all fits on one line.

Can we say "try again" instead of "redownload"?

I would also prefer not to say click. Maybe the following:

Update available. Restart now!
Update failed. Please try again.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v4

This is the combined patch that I had promised and then forgotten to attach. It incorporates Alex's leak fix on top of v2, which had addressed some (but not all) of Gijs' review comments. Comment 13 mentions which parts are still missing.

Comment 24

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

Attachment: strip-button.patch

The patch is working on top of the v4. Not sure if `gBadges` should be renamed now, or maybe I should create another object.

Comment 25

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

Almost forgot. I reused the strings landed in  bug 1080690, as discussed with joe. However there is only one string for the failed scenario, and it's not a button's one, so it's currently too long to be displayed entirely. Not sure how to address that – beside adding a tooltip, that probably is worthy to add anyway.

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8514068 [details] [diff] [review]
strip-button.patch

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

Looking good! I spotted 3 issues so far, but I'm not a browser peer, so someone like Gijs should do the actual review.

One is that the style changes are only made for OS X. We need to make sure Windows and Linux also look right.

Two is that we probably want to remove the hamburger badge once the menu is open. See the previous discussion about this in the bug.

Three is:

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #25)
> Almost forgot. I reused the strings landed in  bug 1080690, as discussed
> with joe. However there is only one string for the failed scenario, and it's
> not a button's one, so it's currently too long to be displayed entirely. Not
> sure how to address that – beside adding a tooltip, that probably is worthy
> to add anyway.

I think we want to make the button label wrap around when the text is too long. In addition to that, we should probably have this version that just reuses the strings already present in Aurora, and an additional patch that replaces those with the UX-provided strings above. Then we only uplift the version without string changes and let the rest ride the trains.

Comment 27

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #26)
> Two is that we probably want to remove the hamburger badge once the menu is
> open. See the previous discussion about this in the bug.

But this part feels like it's low priority to me - don't risk not getting this done for that.

Comment 28

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

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #26)

> Looking good! I spotted 3 issues so far, but I'm not a browser peer, so
> someone like Gijs should do the actual review.

Ok!

> One is that the style changes are only made for OS X. We need to make sure
> Windows and Linux also look right.

The style changes are also made for the other platforms (see: panelUIOverlay.inc.css); it's just that osx has some specific tweaks.

> Two is that we probably want to remove the hamburger badge once the menu is
> open. See the previous discussion about this in the bug.

Maybe I missed a comment there, because I can't find the Stephen's decision about that. I read the Gijs' comment about it, that defer the decision to UX. I'll ping Stephen later about that, but in case I missed something let me know!

> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #25)
> > Almost forgot. I reused the strings landed in  bug 1080690, as discussed
> > with joe. However there is only one string for the failed scenario, and it's
> > not a button's one, so it's currently too long to be displayed entirely. Not
> > sure how to address that – beside adding a tooltip, that probably is worthy
> > to add anyway.
 
> I think we want to make the button label wrap around when the text is too
> long.

So you mean having a multiline text? I'm not familiar with that in XUL, maybe Gijs can give a hint how to do that – so far, anything I tried in the PanelUI to having the multiline text, didn't work well.

> In addition to that, we should probably have this version that just
> reuses the strings already present in Aurora, and an additional patch that
> replaces those with the UX-provided strings above. Then we only uplift the
> version without string changes and let the rest ride the trains.

Ok, how do you suggest to update the strings? Adding a property for the button's text when is failed, and update the one when is succeed? What about the long descriptions, should I leave as is or update them too?

Comment 29

[Stephen Horlander [:shorlander]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #28)
> (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> comment #26)
> > Two is that we probably want to remove the hamburger badge once the menu is
> > open. See the previous discussion about this in the bug.
> 
> Maybe I missed a comment there, because I can't find the Stephen's decision
> about that. I read the Gijs' comment about it, that defer the decision to
> UX. I'll ping Stephen later about that, but in case I missed something let
> me know!

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.

Comment 30

[Joe Walker [:jwalker]]

(In reply to Stephen Horlander [:shorlander] from comment #29)
> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #28)
> > (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> > comment #26)
> > > Two is that we probably want to remove the hamburger badge once the menu is
> > > open. See the previous discussion about this in the bug.
> > 
> > Maybe I missed a comment there, because I can't find the Stephen's decision
> > about that. I read the Gijs' comment about it, that defer the decision to
> > UX. I'll ping Stephen later about that, but in case I missed something let
> > me know!
> 
> 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.

TL;DR. Can we do it in a follow-up?

You could argue that this is like an ios folder badge where the badges bubble up. Also it's a bit like the mould on the chrome hamburger which has the exact same purpose, and I'm fairly sure is persistent. (can't double check now).

You do UX not me, so I'm very happy to do it your way, my point is more that we shouldn't risk not getting this in for that tweak.

Comment 31

[:Gijs (he/him)]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #30)
> (In reply to Stephen Horlander [:shorlander] from comment #29)
> > (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #28)
> > > (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> > > comment #26)
> > > > Two is that we probably want to remove the hamburger badge once the menu is
> > > > open. See the previous discussion about this in the bug.
> > > 
> > > Maybe I missed a comment there, because I can't find the Stephen's decision
> > > about that. I read the Gijs' comment about it, that defer the decision to
> > > UX. I'll ping Stephen later about that, but in case I missed something let
> > > me know!
> > 
> > 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.
> 
> TL;DR. Can we do it in a follow-up?
> 
> You could argue that this is like an ios folder badge where the badges
> bubble up. Also it's a bit like the mould on the chrome hamburger which has
> the exact same purpose, and I'm fairly sure is persistent. (can't double
> check now).
> 
> You do UX not me, so I'm very happy to do it your way, my point is more that
> we shouldn't risk not getting this in for that tweak.

As far as I can tell this is essentially just something along the lines of:

PanelUI.popup.addEventListener("popupshowing", /* or popuphiding, depending on when you want to clear this*/
() => PanelUI.menuButton.removeAttribute("badge"));

in the right place... potentially removing it when the window closes to avoid leaks.

Comment 32

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

(In reply to :Gijs Kruitbosch from comment #31)

> As far as I can tell this is essentially just something along the lines of:
> 
> PanelUI.popup.addEventListener("popupshowing", /* or popuphiding, depending
> on when you want to clear this*/
> () => PanelUI.menuButton.removeAttribute("badge"));

Yeah, it should be an issue. What's the best place to clean the listener? Do we have already a similar code, when the window is closed? I'm not familiar with that.

Comment 33

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #32)
> Yeah, it should be an issue. 

There is a "n't" missing here, of course. :)

Comment 34

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

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

> Yeah, it should be an issue. What's the best place to clean the listener? Do
> we have already a similar code, when the window is closed? I'm not familiar
> with that.

I guess in `gBadges.uninit`. I'll have a patch ready for that asap.

Comment 35

[:Gijs (he/him)]

Comment on attachment 8514068 [details] [diff] [review]
strip-button.patch

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

Where is the code to handle starting the actual update, ie to use the string:

appmenu.downloadUpdateButton.label

?

::: browser/base/content/browser.js
@@ +2402,5 @@
>  
> +  onMenuPanelCommand: function(event) {
> +    let button = event.originalTarget;
> +
> +    if (button.classList.contains("update-succeed")) {

Nit: don't reuse 'button', so might as well just use event.originalTarget here.

@@ +2405,5 @@
> +
> +    if (button.classList.contains("update-succeed")) {
> +      // restart the app
> +      let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"]
> +                         .createInstance(Ci.nsISupportsPRBool);

General nit: please be consistent in your indenting of multi-line statements. I don't much care how you do it, but achieve consistency, please. Right now I've spotted second lines which are 64, 71 and 72 characters long (if they all were 100 or 80, that would make sense), none of which line up exactly with the previous line (would also be OK by me), none of which have a consistent prefixed-indent (ie 4 space indent from the start of the previous line), either. Please pick one of those three (or explain how your indenting is currently consistent, in case I've missed something).

@@ +2431,5 @@
>      let hamburger = window.PanelUI.menuButton;
> +    let updateButton = document.getElementById("PanelUI-update-status");
> +
> +    let brandBundle = document.getElementById("bundle_brand");
> +    let brandShortName = brandBundle.getString("brandShortName");

Nit: put these in the case where you actually need them.

@@ +2451,5 @@
>          badge.style.backgroundColor = 'green';
>          hamburger.setAttribute("badge", "\u2605");
> +
> +        updateButtonText = gNavigatorBundle.getFormattedString(
> +                                  "appmenu.restartBrowserButton.label",

Why this instead of appmenu.restartNeeded.description, which seems more appropriate? Otherwise, what context does the user have as to why there's suddenly a restart button there?

More generally: where is the UX design for this feature?

@@ +2456,5 @@
> +                                  [brandShortName]);
> +
> +        updateButton.label = updateButtonText;
> +        updateButton.hidden = false;
> +        updateButton.classList.add("update-succeed");

This should be an attribute, not a class. Also, "update-success" or "update-succeeded" (probably the latter to be consistent with update-failed) - "update-succeed" doesn't really work in English.

@@ +2467,5 @@
> +
> +        updateButtonText = gNavigatorBundle.getString(
> +                                  "appmenu.updateFailed.description");
> +
> +        updateButton.label = updateButtonText;

This isn't going to work; this string is very long and will need to wrap in some locales. You can try setting wrap="true" in the XUL, but I expect you'll need to do more CSS work for that to look good.

@@ +2469,5 @@
> +                                  "appmenu.updateFailed.description");
> +
> +        updateButton.label = updateButtonText;
> +        updateButton.hidden = false;
> +        updateButton.classList.add("update-failed");

This should be an attribute too...

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +16,5 @@
>        </vbox>
>  
>        <footer id="PanelUI-footer">
> +        <toolbarbutton id="PanelUI-update-status"
> +                       oncommand="gBadges.onMenuPanelCommand(event)"

Nit: ';'

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +21,5 @@
>      background-image: url(chrome://browser/skin/customizableui/[e-mail]),
>                        linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
>    }
>  
> +  #PanelUI-update-status {

This hunk doesn't apply cleanly.

@@ +49,5 @@
>    #PanelUI-quit {
>      list-style-image: url(chrome://browser/skin/[e-mail]);
>    }
>  
> +  #PanelUI-update-status,

This isn't necessary; there's no other bits of the image that need to be excluded.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +585,5 @@
>    -moz-border-end-style: none;
>    list-style-image: url(chrome://browser/skin/menuPanel-exit.png);
>  }
>  
> +#PanelUI-update-status,

Get rid of this too.

@@ +614,5 @@
>  #PanelUI-quit[disabled] {
>    opacity: 0.4;
>  }
>  
> +#PanelUI-update-status:not([disabled]):hover,

So... you added this here, and at some selectors below, but then you override the background color and also add this to the selector for outline: none?

So in effect, none of these properties are applied. No need to add this, then, is there?

@@ +623,5 @@
>    outline: 1px solid hsla(210,4%,10%,.07);
>    background-color: hsla(210,4%,10%,.07);
>  }
>  
> +#PanelUI-update-status:not([disabled]):hover:active,

This gets you a grey-ish inset box shadow... does that actually make sense here?

If you really need it, even though you actually define active background colors, I think at this point it makes more sense to add it to the selectors you add below, and not modify this selector, nor the one below.

@@ +633,5 @@
>    background-color: hsla(210,4%,10%,.12);
>    box-shadow: 0 1px 0 hsla(210,4%,10%,.05) inset;
>  }
>  
> +#PanelUI-update-status:not([disabled]):hover,

Definitely don't need the :hover selector here.

@@ +644,5 @@
> +#PanelUI-update-status.update-succeed {
> +  background-color: hsla(96, 65%, 75%, 0.1);
> +}
> +
> +#PanelUI-update-status:not([disabled]):hover.update-succeed {

Ew. For all of these, classes (and attributes) before :not() and pseudoclasses, please.

Also, where do all of these colors come from?

And, as a rule, you should define the foreground color where you define the background color, unless you are sure the foreground color is going to be a hardcoded, matching color already. I'm fairly sure that in this case, the color inherits through from a -moz-fieldText or similar, and therefore this needs some foreground color help.

Comment 36

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

(In reply to :Gijs Kruitbosch from comment #35)

> Where is the code to handle starting the actual update, ie to use the string:
> 
> appmenu.downloadUpdateButton.label

We don't need to handle them, in this scenario. We only handle when is the download is succeeded or failed.

> ::: browser/base/content/browser.js
> @@ +2402,5 @@
> >  
> > +  onMenuPanelCommand: function(event) {
> > +    let button = event.originalTarget;
> > +
> > +    if (button.classList.contains("update-succeed")) {
> 
> Nit: don't reuse 'button', so might as well just use event.originalTarget
> here.

I tried to be consistent; I reuse the same code we had already: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-fxaccounts.js#208


> General nit: please be consistent in your indenting of multi-line
> statements. I don't much care how you do it, but achieve consistency,
> please. Right now I've spotted second lines which are 64, 71 and 72
> characters long (if they all were 100 or 80, that would make sense), none of
> which line up exactly with the previous line (would also be OK by me), none
> of which have a consistent prefixed-indent (ie 4 space indent from the start
> of the previous line), either. Please pick one of those three (or explain
> how your indenting is currently consistent, in case I've missed something).

I usually don't have such long lines, so I indent based on the context. If you could give me the code style guidelines link for that, I'll stick to them.

> 
> @@ +2451,5 @@
> >          badge.style.backgroundColor = 'green';
> >          hamburger.setAttribute("badge", "\u2605");
> > +
> > +        updateButtonText = gNavigatorBundle.getFormattedString(
> > +                                  "appmenu.restartBrowserButton.label",
> 
> Why this instead of appmenu.restartNeeded.description, which seems more
> appropriate?

Because that one was decided for the button – unfortunately, we don't have one for the failed case.
I don't mind to use the description in both cases.

> More generally: where is the UX design for this feature?

About the strip button, there is the mockup attached in this bug by Sevaan.

> This should be an attribute, not a class. Also, "update-success" or
> "update-succeeded" (probably the latter to be consistent with update-failed)
> - "update-succeed" doesn't really work in English.

Not sure what's the naming convention for such cases; could you give me a suggestion? I'll be consistent with that, if we have a convention.

> 
> @@ +2467,5 @@
> > +
> > +        updateButtonText = gNavigatorBundle.getString(
> > +                                  "appmenu.updateFailed.description");
> > +
> > +        updateButton.label = updateButtonText;
> 
> This isn't going to work; this string is very long and will need to wrap in
> some locales.

We know, it said in previous comments. And as I said, unfortunately my attempts to have a multi line label in the panel didn't work well. I'm not familiar with this aspect of XUL, so if you could help I would appreciate!

> You can try setting wrap="true" in the XUL, but I expect
> you'll need to do more CSS work for that to look good.

I tried, and it didn't work in fact. The problem is, XUL and HTML handles this thing really differently, and I have no past experience for that thing. I'm not sure what I should do here.

> > +#PanelUI-update-status:not([disabled]):hover,

> So... you added this here, and at some selectors below, but then you
> override the background color and also add this to the selector for outline:
> none?

I applied the same base selectors of the rest, and then I modified only the bit needed. I can get rid of them.

> @@ +644,5 @@
> > +#PanelUI-update-status.update-succeed {
> > +  background-color: hsla(96, 65%, 75%, 0.1);
> > +}
> > +
> > +#PanelUI-update-status:not([disabled]):hover.update-succeed {
> 
> Ew. For all of these, classes (and attributes) before :not() and
> pseudoclasses, please.

If I do that, the rule doesn't have enough specificity in such context, and I need to put `!important` to override. However, given the fact that I'm not going to use classes, but attributes instead, and remove some previous rules, I don't think this matters anymore. 

> Also, where do all of these colors come from?

From Sevaan, who made the mockup.

> And, as a rule, you should define the foreground color where you define the
> background color, unless you are sure the foreground color is going to be a
> hardcoded, matching color already. I'm fairly sure that in this case, the
> color inherits through from a -moz-fieldText or similar, and therefore this
> needs some foreground color help.

I'm not sure what do you what I should do; the button's foreground color should be the same of the default ones. The background color was chosen by UX to be soft to match the other background color, and keep the default color. I guess I can hard code the foreground color with the same value of the default color; is that what you want?

Comment 37

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #36)
> (In reply to :Gijs Kruitbosch from comment #35)
> 
> > Where is the code to handle starting the actual update, ie to use the string:
> > 
> > appmenu.downloadUpdateButton.label
> 
> We don't need to handle them, in this scenario. We only handle when is the
> download is succeeded or failed.

That's surprising to me. Why was the string added, then?

> > ::: browser/base/content/browser.js
> > @@ +2402,5 @@
> > >  
> > > +  onMenuPanelCommand: function(event) {
> > > +    let button = event.originalTarget;
> > > +
> > > +    if (button.classList.contains("update-succeed")) {
> > 
> > Nit: don't reuse 'button', so might as well just use event.originalTarget
> > here.
> 
> I tried to be consistent; I reuse the same code we had already:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> fxaccounts.js#208

That code reuses the button variable, and you don't. Consistency with that code isn't really a goal, considering that they're in completely different places, too.

> > General nit: please be consistent in your indenting of multi-line
> > statements. I don't much care how you do it, but achieve consistency,
> > please. Right now I've spotted second lines which are 64, 71 and 72
> > characters long (if they all were 100 or 80, that would make sense), none of
> > which line up exactly with the previous line (would also be OK by me), none
> > of which have a consistent prefixed-indent (ie 4 space indent from the start
> > of the previous line), either. Please pick one of those three (or explain
> > how your indenting is currently consistent, in case I've missed something).
> 
> I usually don't have such long lines, so I indent based on the context. If
> you could give me the code style guidelines link for that, I'll stick to
> them.

The prevailing style is "stick to what the code around you does". We don't have a style guide for JS that I know of.

You have three cases. I would probably make the .createInstance line up with "Cc", stick the label ID in a temp var and make the string fetch use 2 lines instead of 3, with the second line lining up with the opening (. The last one can use a temp var to avoid the multiline statement altogether.


> > @@ +2451,5 @@
> > >          badge.style.backgroundColor = 'green';
> > >          hamburger.setAttribute("badge", "\u2605");
> > > +
> > > +        updateButtonText = gNavigatorBundle.getFormattedString(
> > > +                                  "appmenu.restartBrowserButton.label",
> > 
> > Why this instead of appmenu.restartNeeded.description, which seems more
> > appropriate?
> 
> Because that one was decided for the button – unfortunately, we don't have
> one for the failed case.

Where was this decided? The design says "Update available! Click to restart!". Your string just says "Restart Firefox".

> I don't mind to use the description in both cases.

Yes please.


> > This should be an attribute, not a class. Also, "update-success" or
> > "update-succeeded" (probably the latter to be consistent with update-failed)
> > - "update-succeed" doesn't really work in English.
> 
> Not sure what's the naming convention for such cases; could you give me a
> suggestion? I'll be consistent with that, if we have a convention.

I don't understand. The bit you quoted says you should go with "update-succeeded". Not so much convention rather than "use correct English where possible".

> > @@ +2467,5 @@
> > > +
> > > +        updateButtonText = gNavigatorBundle.getString(
> > > +                                  "appmenu.updateFailed.description");
> > > +
> > > +        updateButton.label = updateButtonText;
> > 
> > This isn't going to work; this string is very long and will need to wrap in
> > some locales.
> 
> We know, it said in previous comments. And as I said, unfortunately my
> attempts to have a multi line label in the panel didn't work well. I'm not
> familiar with this aspect of XUL, so if you could help I would appreciate!
> 
> > You can try setting wrap="true" in the XUL, but I expect
> > you'll need to do more CSS work for that to look good.
> 
> I tried, and it didn't work in fact.

What did you try and what does "didn't work" mean? I would be surprised if "Restart Firefox" would need to wrap. You should set the wrap=true attribute on the toolbarbutton and use a label that is sufficiently long so as to need wrapping.

> > @@ +644,5 @@
> > > +#PanelUI-update-status.update-succeed {
> > > +  background-color: hsla(96, 65%, 75%, 0.1);
> > > +}
> > > +
> > > +#PanelUI-update-status:not([disabled]):hover.update-succeed {
> > 
> > Ew. For all of these, classes (and attributes) before :not() and
> > pseudoclasses, please.
> 
> If I do that, the rule doesn't have enough specificity in such context, and

What? No! I'm asking you to do:

#foo.bar:hover

rather than

#foo:hover.bar

which doesn't affect specificity at all: http://jsbin.com/lisuvitane/1/edit?html,css,output
(try swapping the rules if you don't believe me - the last one to be defined always wins)

> > Also, where do all of these colors come from?
> 
> From Sevaan, who made the mockup.
> 
> > And, as a rule, you should define the foreground color where you define the
> > background color, unless you are sure the foreground color is going to be a
> > hardcoded, matching color already. I'm fairly sure that in this case, the
> > color inherits through from a -moz-fieldText or similar, and therefore this
> > needs some foreground color help.
> 
> I'm not sure what do you what I should do; the button's foreground color
> should be the same of the default ones. The background color was chosen by
> UX to be soft to match the other background color, and keep the default
> color. I guess I can hard code the foreground color with the same value of
> the default color; is that what you want?

The mockup isn't clear here (it's been compressed to hell and back, and in any case seems to use a darker color than any of the other buttons), but either way the contrast of the foreground color should be sufficient given the background color. The current foreground color depends on the OS theme, and therefore might not be sufficient (for instance, on dark themes with light text). Therefore, you should specify a different foreground color that works considering the red/green used on the button. If you don't know what colors to use, ask Sevaan.

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gijs Kruitbosch from comment #37)
> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #36)
> > (In reply to :Gijs Kruitbosch from comment #35)
> > 
> > > Where is the code to handle starting the actual update, ie to use the string:
> > > 
> > > appmenu.downloadUpdateButton.label
> > 
> > We don't need to handle them, in this scenario. We only handle when is the
> > download is succeeded or failed.
> 
> That's surprising to me. Why was the string added, then?

> > > @@ +2451,5 @@
> > > >          badge.style.backgroundColor = 'green';
> > > >          hamburger.setAttribute("badge", "\u2605");
> > > > +
> > > > +        updateButtonText = gNavigatorBundle.getFormattedString(
> > > > +                                  "appmenu.restartBrowserButton.label",
> > > 
> > > Why this instead of appmenu.restartNeeded.description, which seems more
> > > appropriate?
> > 
> > Because that one was decided for the button – unfortunately, we don't have
> > one for the failed case.
> 
> Where was this decided? The design says "Update available! Click to
> restart!". Your string just says "Restart Firefox".
> 
> > I don't mind to use the description in both cases.
> 
> Yes please.

The confusion in both cases is my fault. The strings from bug 1080690 landed in time for 35, but at the time the UI I had in mind had a separate label and a button. What Savaan suggested later is of course far more elegant and only needs one string. So yes, let's stick with the long ones.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #28)
> (In reply to Panos Astithas [:past] (overloaded, please needinfo) from
> comment #26)
> > In addition to that, we should probably have this version that just
> > reuses the strings already present in Aurora, and an additional patch that
> > replaces those with the UX-provided strings above. Then we only uplift the
> > version without string changes and let the rest ride the trains.
> 
> Ok, how do you suggest to update the strings? Adding a property for the
> button's text when is failed, and update the one when is succeed? What about
> the long descriptions, should I leave as is or update them too?

In the additional patch you would just remove the two unused strings and modify the other two. Remember to update the property name to something like foo.bar2.description. Use the strings from comment 22.

Comment 39

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

(In reply to :Gijs Kruitbosch from comment #37)

> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #36)
> > (In reply to :Gijs Kruitbosch from comment #35)
> > 
> > > Where is the code to handle starting the actual update, ie to use the string:
> > > 
> > > appmenu.downloadUpdateButton.label
> > 
> > We don't need to handle them, in this scenario. We only handle when is the
> > download is succeeded or failed.
> 
> That's surprising to me. Why was the string added, then?
[..]
> Where was this decided? The design says "Update available! Click to
> restart!". Your string just says "Restart Firefox".

See the reply from Panos.

> > Not sure what's the naming convention for such cases; could you give me a
> > suggestion? I'll be consistent with that, if we have a convention.
> 
> I don't understand. The bit you quoted says you should go with
> "update-succeeded". Not so much convention rather than "use correct English
> where possible".

I was talking about the attribute's name, obviously. Anyway, in my next patch I just used `update-status` as attribute, and "succeeded" and "failed" as values.

> > > You can try setting wrap="true" in the XUL, but I expect
> > > you'll need to do more CSS work for that to look good.

> > I tried, and it didn't work in fact.
> 
> What did you try and what does "didn't work" mean?

I tried `wrap="true"` and it didn't work – it messed up with the width of the panel, increasing it and showing the subview item on the right.
I also tried with different approaches in CSS and modifying the xul:label elements, but without success (as result I got an empty label, for instance).

> I would be surprised if
> "Restart Firefox" would need to wrap. You should set the wrap=true attribute
> on the toolbarbutton and use a label that is sufficiently long so as to need
> wrapping.
> 
> > > @@ +644,5 @@
> > > > +#PanelUI-update-status.update-succeed {
> > > > +  background-color: hsla(96, 65%, 75%, 0.1);
> > > > +}
> > > > +
> > > > +#PanelUI-update-status:not([disabled]):hover.update-succeed {
> > > 
> > > Ew. For all of these, classes (and attributes) before :not() and
> > > pseudoclasses, please.
> > 
> > If I do that, the rule doesn't have enough specificity in such context, and
> 
> What? No! I'm asking you to do:
> 
> #foo.bar:hover
> 
> rather than
> 
> #foo:hover.bar

Yeah. That's what I said. I've done the first one, and it didn't work unless I used !important. I was surprised too. I had to do the same in my next patch. To be clear:

#PanelUI-update-status[update-status="succeeded"]:not([disabled]):hover

It's not applied, where:

#PanelUI-update-status:not([disabled])[update-status="succeeded"]:hover

it is.
The first one is applied only if I used `!important`.

> (try swapping the rules if you don't believe me - the last one to be defined
> always wins)

I know, that's why I was surprised to have this behavior in `panelUIOverlay`. You can try yourself if you don't believe me. At least on gum build, on OS X, I have this result.

> > > Also, where do all of these colors come from?
> > 
> > From Sevaan, who made the mockup.
> > 
> > > And, as a rule, you should define the foreground color where you define the
> > > background color, unless you are sure the foreground color is going to be a
> > > hardcoded, matching color already. I'm fairly sure that in this case, the
> > > color inherits through from a -moz-fieldText or similar, and therefore this
> > > needs some foreground color help.
> > 
> > I'm not sure what do you what I should do; the button's foreground color
> > should be the same of the default ones. The background color was chosen by
> > UX to be soft to match the other background color, and keep the default
> > color. I guess I can hard code the foreground color with the same value of
> > the default color; is that what you want?
> 
> The mockup isn't clear here (it's been compressed to hell and back, and in
> any case seems to use a darker color than any of the other buttons), but
> either way the contrast of the foreground color should be sufficient given
> the background color. The current foreground color depends on the OS theme,
> and therefore might not be sufficient (for instance, on dark themes with
> light text). Therefore, you should specify a different foreground color that
> works considering the red/green used on the button. If you don't know what
> colors to use, ask Sevaan.

I'll do.

Comment 40

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

Attachment: strip-button-v2.patch

I think I addressed most of the comments of the previous patch. I submit what I have now even if there is still one big thing missing: I wasn't able to have a proper multiline button. I can work again on it tomorrow, but if anyone want to help on that earlier is more than welcome.
I also didn't add the foreground color – I contacted Sevaan but he was in a meeting and he wasn't sure.

I added the behavior to hide the badge once the panel is displayed.

Comment 41

[:Gijs (he/him)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #39)
> (In reply to :Gijs Kruitbosch from comment #37)
> 
> > (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #36)
> > > Not sure what's the naming convention for such cases; could you give me a
> > > suggestion? I'll be consistent with that, if we have a convention.
> > 
> > I don't understand. The bit you quoted says you should go with
> > "update-succeeded". Not so much convention rather than "use correct English
> > where possible".
> 
> I was talking about the attribute's name, obviously.

Not obvious to me! update-status="succeeded|failed" sounds fine to me.
 
> > > > You can try setting wrap="true" in the XUL, but I expect
> > > > you'll need to do more CSS work for that to look good.
> 
> > > I tried, and it didn't work in fact.
> > 
> > What did you try and what does "didn't work" mean?
> 
> I tried `wrap="true"` and it didn't work – it messed up with the width of
> the panel, increasing it and showing the subview item on the right.

I would offer to help, but my build is crashing non-stop.

> > > > @@ +644,5 @@
> > > > > +#PanelUI-update-status.update-succeed {
> > > > > +  background-color: hsla(96, 65%, 75%, 0.1);
> > > > > +}
> > > > > +
> > > > > +#PanelUI-update-status:not([disabled]):hover.update-succeed {
> > > > 
> > > > Ew. For all of these, classes (and attributes) before :not() and
> > > > pseudoclasses, please.
> > > 
> > > If I do that, the rule doesn't have enough specificity in such context, and
> > 
> > What? No! I'm asking you to do:
> > 
> > #foo.bar:hover
> > 
> > rather than
> > 
> > #foo:hover.bar
> 
> Yeah. That's what I said. I've done the first one, and it didn't work unless
> I used !important. I was surprised too. I had to do the same in my next
> patch. To be clear:
> 
> #PanelUI-update-status[update-status="succeeded"]:not([disabled]):hover
> 
> It's not applied, where:
> 
> #PanelUI-update-status:not([disabled])[update-status="succeeded"]:hover
> 
> it is.
> The first one is applied only if I used `!important`.
> 
> > (try swapping the rules if you don't believe me - the last one to be defined
> > always wins)
> 
> I know, that's why I was surprised to have this behavior in
> `panelUIOverlay`. You can try yourself if you don't believe me. At least on
> gum build, on OS X, I have this result.

I don't use gum, but on fx-team tip my build crashes literally within 10 seconds, so I can't try this.

On beta as well as a bog-standard nightly, using:

#PanelUI-fxa-status[defaultlabel="Sign in to Sync"]:not([disabled]):hover {
background: red;
}

works fine to override the default styling.

I suspect something else is wrong, either with the same or a previous selector. Use the toolbox inspector to see what's happening.

Comment 42

[Sevaan Franks [:sevaan]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #40)
> Created attachment 8514440 [details] [diff] [review]
> strip-button-v2.patch
> 
> I also didn't add the foreground color – I contacted Sevaan but he was in a
> meeting and he wasn't sure.


Was just a little confused about what colour is needed, exactly. Are you looking for the grey that makes up those bottom rows (http://cl.ly/image/0E1k0v1r2Q10), upon which the color is overlayed?  The hexcode is #e8e8e8, I believe.

Comment 43

[Victor Porof [:vporof][:vp]]

Joe asked me to chime in here. Now, I never had to build multiline buttons, and doing multiline stuff in XUL is hard in the first place. Generally, you'd want to use a <description> node. A little history:
> <label value="FOOBAR"/>
will never *ever* know how to do line breaks. At best, you can crop the contents, but never break text. As far as I know, <button>s use labels internally in their XBL, so there's no way you can achieve this without using something else. Using
> <description value="LONG FOOBAR"/>
will also never break, but
> <description>LONG FOOBAR</description>
will break on whitespace when the available width is not sufficient. Now, I don't know if your source text *itself* contains newlines, in which case there's a few more mystical incantations you need to write to get this to work properly. Let me know.

My suggestion is to use a <description>{text}</description> and make it look like a button using -moz-appearance: button in your css. Alternatively, you might be able to do <button><description>{text}</description></button> and set display: none on the inner label.

Comment 44

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #43)
> -moz-appearance: button in your css.

It's actually moz-appearance: button-bevel. Meh.

Comment 45

[:Gijs (he/him)]

Actually, toolbarbuttons have a multiline label now that uses <label>foo</label>, which does support multiline text. It gets used if you set the wrap="true" attribute. I expect the element needs to have its max-width set sensibly, though, and it's also possible the width: 0 that is added in the patch interferes with it.

Comment 46

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #45)
> Actually, toolbarbuttons have a multiline label now that uses
> <label>foo</label>, which does support multiline text. It gets used if you
> set the wrap="true" attribute. I expect the element needs to have its
> max-width set sensibly, though, and it's also possible the width: 0 that is
> added in the patch interferes with it.

(this, fwiw, is how all the multiline labels in the other 'square' buttons in the panel work - and they certainly do work, so it'd be surprising if it couldn't be made to work here)

Comment 47

[Victor Porof [:vporof][:vp]]

(In reply to :Gijs Kruitbosch from comment #46)
> (In reply to :Gijs Kruitbosch from comment #45)

Oh, I didn't know that using <toolbarbutton>s was a possibility here. In that case, yes, this is a more sensible approach.

Comment 48

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

Attachment: strip-button-v3.patch

Comment 49

[:Gijs (he/him)]

Comment on attachment 8514933 [details] [diff] [review]
strip-button-v3.patch

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

::: browser/base/content/browser.js
@@ +2396,5 @@
>    uninit: function () {
>      if (this.enabled) {
>        Services.obs.removeObserver(this, "update-staged");
> +
> +      window.PanelUI.panel.removeEventListener("popupshowing", this);

No need for "window." here, please correct throughout. Also, please pass 'true' here as the last parameter so that this works. Finally, please remove the extra newlines before and after this.

@@ +2416,5 @@
> +    } else {
> +      // open the page for manual update
> +      let url = Services.urlFormatter.formatURLPref("app.update.url.manual");
> +
> +      openUILinkIn(url, "tab");

Nit: please nix the newline above this

@@ +2453,5 @@
> +        let brandBundle = document.getElementById("bundle_brand");
> +        let brandShortName = brandBundle.getString("brandShortName");
> +
> +        stringId = "appmenu.restartNeeded.description";
> +

Nit: nix newlines before and after this

@@ +2455,5 @@
> +
> +        stringId = "appmenu.restartNeeded.description";
> +
> +        updateButtonText = gNavigatorBundle.getFormattedString(stringId,
> +                                                              [brandShortName]);

Nit: shift right 1 space please. :-)

@@ +2461,5 @@
> +        updateButton.label = updateButtonText;
> +        updateButton.hidden = false;
> +        updateButton.setAttribute("update-status", "succeeded");
> +
> +        window.PanelUI.panel.addEventListener("popupshowing", this._onPopupShowing);

Instead, use (..., this, true), which works in my testing. As above, no need for "window." here (throughout)

@@ +2471,5 @@
>          hamburger.setAttribute("badge", "!");
> +
> +        stringId = "appmenu.updateFailed.description";
> +
> +        updateButtonText = gNavigatorBundle.getString(stringId);

Nit: nix newline before this.

@@ +2489,5 @@
>      }
>      this.uninit();
> +  },
> +
> +  _onPopupShowing: function({type}) {

Correct to handleEvent, and use:

    if (e.type == "popupshowing") {
      PanelUI.menuButton.removeAttribute("badge");
      PanelUI.panel.removeEventListener("popupshowing", this, true);
    }

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +644,5 @@
> +#PanelUI-update-status[update-status="succeeded"] {
> +  background-color: hsla(96, 65%, 75%, 0.1);
> +}
> +
> +#PanelUI-update-status:not([disabled])[update-status="succeeded"]:hover {

I tested this locally and it works for me with the [update-status="succeeded"] before the :not([disabled]). Ditto for the "failed" case below.

Comment 50

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

Attachment: strip-button-v4.patch

Notice that this patch still have the conflict with m-c; but it can't be helped: the conflict is due the dark theme of gum, and this patch is intended to land in gum in first place. In accordance with Joe, once we land the dark theme on m-c too, it shouldn't be any issue.

I'm definitely missing the smart merging of git… *sigh*.

Comment 51

[:Gijs (he/him)]

Comment on attachment 8515009 [details] [diff] [review]
strip-button-v4.patch

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

Almost there, but you forgot comments from the last review...

::: browser/base/content/browser.js
@@ +2391,1 @@
>        hamburger.classList.add("badged-button");

Nit: might as well update to single-line:

PanelUI.menuButton.classList.add("badged-button")

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +19,5 @@
>    #PanelUI-help[panel-multiview-anchor="true"]:-moz-locale-dir(rtl)::after,
>    toolbarbutton[panel-multiview-anchor="true"]:-moz-locale-dir(rtl) {
>      background-image: url(chrome://browser/skin/customizableui/[e-mail]),
>                        linear-gradient(rgba(255,255,255,0.3), rgba(255,255,255,0));
>    }

I don't think this has anything to do with the dark theme. On fx-team and m-c, this looks like this:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/customizableui/panelUIOverlay.css#11

Because bug 1088578 fixed the gradients here, and gum is now tracking aurora (albeit with quite some lag), this is out of sync. However, this patch will need to land on fx-team; gum is just a test-bed. Ideally the patches on this bug should reflect that, but whatever.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +648,5 @@
> +#PanelUI-update-status[update-status="succeeded"]:not([disabled]):hover {
> +  background-color: hsla(96, 65%, 75%, 0.4);
> +}
> +
> +#PanelUI-update-status:not([disabled])[update-status="succeeded"]:hover:active {

You forgot this

@@ +657,5 @@
> +#PanelUI-update-status[update-status="failed"] {
> +  background-color: hsla(359, 69%, 84%, 0.1);
> +}
> +
> +#PanelUI-update-status:not([disabled])[update-status="failed"]:hover {

And this

@@ +661,5 @@
> +#PanelUI-update-status:not([disabled])[update-status="failed"]:hover {
> +  background-color: hsla(359, 69%, 84%, 0.4);
> +}
> +
> +#PanelUI-update-status:not([disabled])[update-status="failed"]:hover:active {

And this

Comment 52

[:Gijs (he/him)]

Gavin, I noticed bug 1072181 comment 33 . Do you want to clarify here?

Comment 53

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

Attachment: strip-button-v5.patch

Hoping this is the last version of the patch, without any leftovers – I'm start to lose track with all this repos. :)

Comment 54

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

(In reply to :Gijs Kruitbosch from comment #51)

> Almost there, but you forgot comments from the last review...
> 
> ::: browser/base/content/browser.js
> @@ +2391,1 @@
> >        hamburger.classList.add("badged-button");
> 
> Nit: might as well update to single-line:
> 
> PanelUI.menuButton.classList.add("badged-button")

That wasn't in my patch; I just kept the previous code as. I updated that in the v5.

> ::: browser/themes/osx/customizableui/panelUIOverlay.css

> I don't think this has anything to do with the dark theme.

I double checked and it has nothing to do with dark theme, so I reuse the fx-team's file as base. We'll sync gum later.

Comment 55

[:Gijs (he/him)]

Comment on attachment 8515066 [details] [diff] [review]
strip-button-v5.patch

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #53)
> Created attachment 8515066 [details] [diff] [review]
> strip-button-v5.patch
> 
> Hoping this is the last version of the patch, without any leftovers – I'm
> start to lose track with all this repos. :)

This has exactly the same CSS problems as the previous one in panelUIOverlay.inc.css

Comment 56

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

Attachment: strip-button-v5.patch

My apologies, I probably mixed up with fx-team repo. That should be OK, now.

Comment 57

[Panos Astithas (he/him) [:past] (please ni?)]

We still need an r+ for the first patch.

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 8513466 [details] [diff] [review]
v4

Drive-by comment... this needs a more descriptive name:

>+let gBadges = {

Comment 59

[Panos Astithas (he/him) [:past] (please ni?)]

Indeed, part 2 renames it to gMenuButtonUpdateBadge.

Comment 60

[:Gijs (he/him)]

Most of comment #7 still applies to the first patch. Matteo, can you unify the patches, apply feedback from comment 7, and re-request review? Thanks!

Comment 61

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

Attachment: badge-and-strip-button.patch

Comment 62

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

Attachment: badge-and-strip-button.patch

Done! I attached the wrong one before, that should be the correct one.

Comment 63

[:Gijs (he/him)]

Comment on attachment 8515580 [details] [diff] [review]
badge-and-strip-button.patch

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

So for what this tries to do, I think the code is essentially perfect. That means you get r+.

However, I'm still worried about the l10n/image-badge side, see below. Please test carefully at least on some windows versions and corresponding default locales to see if the glyphs in use work if we're not going to use images when landing this - or specify the font you want used here to avoid confusion/broken glyphs. Alternatively, ask UX for icons and please re-request review (as that's not a trivial change, I suspect).

Finally, I'm still waiting on the needinfo on gavin about this plan...

::: browser/base/content/browser.js
@@ +2478,5 @@
> +        let badge = document.getAnonymousElementByAttribute(PanelUI.menuButton,
> +                                                            "class",
> +                                                            "toolbarbutton-badge");
> +        badge.style.backgroundColor = 'green';
> +        PanelUI.menuButton.setAttribute("badge", "\u2605");

So on what OS/locale combinations have we checked that this works? We don't seem to be setting the font here, and the 'we will use images for this for the real patch' plan from comment 13 seems to have not happened, either.

Comment 64

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

(In reply to :Gijs Kruitbosch from comment #63)

> However, I'm still worried about the l10n/image-badge side, see below.
> Please test carefully at least on some windows versions and corresponding
> default locales to see if the glyphs in use work if we're not going to use
> images when landing this - or specify the font you want used here to avoid
> confusion/broken glyphs. Alternatively, ask UX for icons and please
> re-request review (as that's not a trivial change, I suspect).

I share your concerns, and definitely in the next iteration we're going to replace this glyph with something more appropriate. I've tested on both OS X and Windows (8), and so far there is no issue. I would probably need to tests more locales that I had, but for the time being that should be enough – we have time constraints.

Comment 65

[Joe Walker [:jwalker]]

Also tested on Windows. The glyph issue isn't something I'd like to hold things up for.

Comment 66

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=d9461cde34da

Comment 67

[Carsten Book [:Tomcat]]

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

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8515580 [details] [diff] [review]
badge-and-strip-button.patch

Please stick to using the checkin-needed keyword in the future. It plays more nicely with our bug marking tools.

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43fa04c541b0

Comment 70

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8515580 [details] [diff] [review]
badge-and-strip-button.patch

Approval Request Comment
[Feature/regressing bug #]: new feature
[User impact if declined]: users will be presented with the existing, more intrusive notification when an update is ready to be installed
[Describe test coverage new/current, TBPL]: tested on m-c, gum
[Risks and why]: not terribly risky, straightforward patch that is disabled by default (although we will presumably enable it in Aurora soon)
[String/UUID change made/needed]: none

Comment 71

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

Attachment: update-strings.patch

Updated the strings, and changed the code that uses the brandname (now useless).

I used the string suggested by Matej in comment 22; however I think the "failed" string is a bit misleading. With "Update failed. Please try again." seems that the user initiate an action, that ended up in a failure, and he needs to repeat the same action. However, this is a silent update: so the user didn't "try" to do anything, therefore it could be confusing about what should "try again".
In my opinion we should be more explicit about what is the action needed – the user needs to download / update the browser manually – and makes the text more consistent with the action associate to the button (as we have for the "success" scenario).

Comment 72

[:Gijs (he/him)]

Comment on attachment 8516959 [details] [diff] [review]
update-strings.patch

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #71)
> Created attachment 8516959 [details] [diff] [review]
> update-strings.patch
> 
> Updated the strings, and changed the code that uses the brandname (now
> useless).
> 
> I used the string suggested by Matej in comment 22; however I think the
> "failed" string is a bit misleading. With "Update failed. Please try again."
> seems that the user initiate an action, that ended up in a failure, and he
> needs to repeat the same action. However, this is a silent update: so the
> user didn't "try" to do anything, therefore it could be confusing about what
> should "try again".
> In my opinion we should be more explicit about what is the action needed –
> the user needs to download / update the browser manually – and makes the
> text more consistent with the action associate to the button (as we have for
> the "success" scenario).

This patch is just for trunk, right? Why are we updating the strings if we're not sure they're right yet?

r- because you should change the string IDs if you're sure you want to go ahead with this. Ideally I also think this should all happen in a different bug, this bug is already marked FIXED, and it looks like we need more discussion just about the strings, so it makes more sense to file a new bug for that.

Comment 73

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8515580 [details] [diff] [review]
badge-and-strip-button.patch

Approving this for uplift, the other patch will have to ride the trains as it changes strings (I know it's not nominated, but just reminding).

Comment 74

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

(In reply to :Gijs Kruitbosch from comment #72)

> r- because you should change the string IDs if you're sure you want to go
> ahead with this. Ideally I also think this should all happen in a different
> bug, this bug is already marked FIXED, and it looks like we need more
> discussion just about the strings, so it makes more sense to file a new bug
> for that.

I added the patch to this bug because it was suggested to do so; I wasn't sure about opening a new bug or not. And yes, I think we should discuss a bit more about the strings. So I'll file a new bug tomorrow.
Not sure, however, what do you mean with "change the string IDs": those strings are going to replace the previous ones.

Comment 75

[Robert Strong (they/them - no direct email)]

By changing the string ID l10n is able to tell that the string has changed so they can localize the new string. So, the following string ID's should be changed
appmenu.restartNeeded.description
appmenu.updateFailed.description

Comment 76

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #75)
> By changing the string ID l10n is able to tell that the string has changed
> so they can localize the new string.

I see! Thanks, that makes perfectly sense.

Comment 77

[Panos Astithas (he/him) [:past] (please ni?)]

I suggested posting the patch in this bug, because it already contains some useful discussion about the strings that we wouldn't want to forget. If we make sure that happens, a new bug is fine.

Comment 78

[Ryan VanderMeulen [:RyanVM]]

This landed with the gum uplift.
https://hg.mozilla.org/releases/mozilla-aurora/rev/1cc7838d4c35