1117722 - Replace the »update star« icon

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

For a few weeks, we've had an indicator on the menu button when an update for Firefox is available (http://cl.ly/image/2q3L0U3z341C)

One issue with the icon is that it uses the same symbol that we already use for bookmarks. Stephen had a different icon (http://cl.ly/image/0l0Y1M3L0N3l) that seems better suited here.

Stephen, could you post the assets (correct size, inactive version) here so that we can swap the assets?

Comment 1

[Mike Hoye [:mhoye]]

This looks like a good first bug, if Shorlander or yourself are willing to mentor.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

It looks like the badge is enabled at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2510 but the fact that we're using a badge means that it's not a great first bug, unless we want to stick with the square-ish badge and change the "\u2605" to "\u2191" (↑ which is a little thin) or "\u21E7" (⇧ which is hollow) or "\u2B06\uFE0E" (⬆︎ which may have inconsistent platform support).

We can get it looking like https://www.dropbox.com/s/s7t27wma9z4gisn/Screenshot%202015-01-27%2016.38.50.png?dl=0 without a lot of trouble.  Making it more like the image would be trickier.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

(Sorry, forgot to needinfo you on this, Philipp…)

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Re-routing to visuals expert in residence, shorlander.

What do you think of this as an update notification?
https://www.dropbox.com/s/s7t27wma9z4gisn/Screenshot%202015-01-27%2016.38.50.png?dl=0

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Okay, so I chatted with shorlander on irc, and he says he's happy with the up-arrow mockup Philipp posted, but we should change the background to #74BF43 to match our other green button.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

(And as a side note, just setting it to '\u2B06' (Upwards Black Arrow) works on Mac, Linux, and Windows, so I suggest going with that.)

Comment 7

[John Tungul]

Hello Blake! So for this bug, we are changing the current update button to a newer button? This only requires a switch in the file name where it is referenced in the javascript function?

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Hi John!  It turns out the button is just some text and styles, created at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2537 so instead of changing a filename, you'll need to change the backgroundColor and the "badge" attribute.

Comment 9

[John Tungul]

Okay I'll be on it. I just have to figure out how to update my local Mozilla build and create a patch again. My browser.js is different from the one you linked. The line #2537 in the link you posted is on my line 2510 and the badge attribute is already set to \u2B06.

Comment 10

[John Tungul]

Hmm, I've changed my local copy, and did the hg qnew command to create a diff file, but when i access the diff file, it does not contain any changes. All it contains is my bug message that I added with -m command. Any ideas what I am doing wrong? Should I ask in #introduction?

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Asking in #introduction is usually a good way to get a faster answer than waiting for me.  ;)

Did you do an 'hg qrefresh' before your 'hg qdiff'?  (That's the part that tell MQ to update the diff file.)

Comment 12

[John Tungul]

Attachment: bug1117722_NewUpdateButton3.diff

This patch changes the 'Updates Available' button. in browser.js, the following changes were made: 
badge.style.backgroundColor was set to '74BF43', from 'green'
PanelUI.menuButton.setAttribute("badge","\u2B06", from "\u2605"

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8561068 [details] [diff] [review]
bug1117722_NewUpdateButton3.diff

The code looks good to me, but I'm not an official reviewer, so I'm going to redirect it to (picking someone mostly at random…) jaws.  :)

The one thing I might change is to use double quotes around '#74BF43', to make it more like the other strings.  (The previous code also used single quotes, but we might as well be consistent, right?)

For the record, this is what it looks like (on Mac): https://www.dropbox.com/s/u0y0ozju0js7gaj/Screenshot%202015-02-08%2013.53.31.png?dl=0
(to get it to show up, I opened the Browser Console with Cmd-Shift-J and then ran:
  gMenuButtonUpdateBadge.observe(null, null, "pending");
)

Stephen, should we lighten the colour a little to make it match the Restart Nightly button?  (XScope suggests #CFE8BE, as opposed to the #72C139 it's showing up as.)

Comment 14

[John Tungul]

Oh yeah, it should be single quoted. Should we wait for the other guys' responses before submitting another patch, so I can make all the suggested changes in 1 patch?

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Yeah, good idea.  Did you want to look at bug 1073234 while we're waiting?
(I suspect shorlander won't respond until he gets to work on Monday…)

Comment 16

[John Tungul]

I checked it out and it looks a little complicated. I am not too familiar with xul but it looks the same as css. I can take it up, depending on how much hand holding you are willing to do!

Comment 17

[Blake Winton (:bwinton) (:☕️)]

I'ld be happy to give you as much help as you need.  Let's continue the discussion over there…  :)

Comment 18

[Stephen Horlander [:shorlander]]

Comment on attachment 8561068 [details] [diff] [review]
bug1117722_NewUpdateButton3.diff

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

Looks good to me. Thank you!

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8561068 [details] [diff] [review]
bug1117722_NewUpdateButton3.diff

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

r=me

Looks good, thanks!

Comment 20

[John Tungul]

Do we need to run this through try before putting the "Checkin-Needed" keyword? If so, can someone do that for me since I do not have commit access? Thank you very much!

Comment 21

[Blake Winton (:bwinton) (:☕️)]

In general, yes, but in this case the code is small and simple enough that I don't think you need to (unless or until one of the sheriffs asks for it).

Comment 22

[John Tungul]

Oh alright! Thank you for your guys' help! I missed that other bug you recommended, Blake. Do you have any others that may be suitable for beginners that are not currently being worked on?

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Sure, did you want to take a stab at bug 1109477?  Or, if you're on Windows, I could use a hand investigating bug 1123657.

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[UK92]

The upwards arrow doesn't look good on Windows, it look like a stick.

Screenshot: https://www.dropbox.com/s/s64v3y4gqz7fqow/Screenshot%202015-02-13%2020.18.08.png?dl=0

Comment 27

[Blake Winton (:bwinton) (:☕️)]

(In reply to UK92 from comment #26)
> The upwards arrow doesn't look good on Windows, it look like a stick.

Thanks for your feedback, UK92!  Do you have any suggestions on a character that would look better on Windows?  I'ld be happy to help you fix the Firefox code so that it works well on all platforms…

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, I've noticed this too. I'm not sure why it didn't look odd during review. We should get this fixed on Windows. Blake, do you think we should file a new bug for this or back this out until it is fixed?

Comment 29

[Blake Winton (:bwinton) (:☕️)]

I think we have time to fix it before the next merge date, so I'ld lean towards filing a new bug.
(It would be awesome if you could suggest a different character, though, since you're on Windows.  ;)
 There are a few options in comment 2.)

Comment 30

[Blake Winton (:bwinton) (:☕️)]

(For the record, https://dl.dropboxusercontent.com/u/2301433/UpTriangle.png is "\u25B2", Mac on the left, Windows on the right.  Stephen?  Is that better enough to check in on both platforms?)

Comment 31

[John Tungul]

Okay so do I need to change anything to my original patch or is someone just going to change that part? or will this be a separate patch all together?

Comment 32

[Blake Winton (:bwinton) (:☕️)]

It'll be a separate patch, since yours already landed.  (It may even be a separate bug…)  I might just change it, but if you wanted to, and Stephen likes the triangle, feel free to switch it to "\u25B2", and post the patch yourself.  :)

Comment 33

[Tim Nguyen :ntim]

(In reply to Blake Winton (:bwinton) from comment #27)
> (In reply to UK92 from comment #26)
> > The upwards arrow doesn't look good on Windows, it look like a stick.
> 
> Thanks for your feedback, UK92!  Do you have any suggestions on a character
> that would look better on Windows?  I'ld be happy to help you fix the
> Firefox code so that it works well on all platforms…

An SVG could be used as well.

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Can it?  In a badge label?  Could you post a sample diff that we can modify to get the arrow shape we want?

Comment 35

[Tim Nguyen :ntim]

Stephen is already needinfo'd in bug 1138630