1105768 - The reason for showing a badge on the hamburger menu should be obvious when opening the menu

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Marco Bonardo [:mak]]

So, I just noticed the badge on the hamburger button, I clicked on it cause I was curious... I looked at the menu and I didn't notice anything new...

At the third inspection I noticed the "Restart Nightly to apply updates" entry... it is grey-ish. barely distinguishable from the Firefox Account entry.
I think once we notify the user that something needs his attention in the menu, the reason for the notification should be clearly visible, possibly even recalling the badge itself (either the color or having the badge on itself).

The thing I hate more in smartphones badges is when you open an app and it's unclear WHAT you should do to handle the notification. This is not very different.

Comment 1

[:Gijs (he/him)]

Next time you see this, can you take a screenshot of the color? It's meant to be green- or red-ish, not just grey... :-\

Comment 2

[Marco Bonardo [:mak]]

sure, it is grey-green-ish... barely green.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

It my case, it was green on green. No text was visible. I only thought to click on it because I had seen this bug...

Comment 4

[:Gijs (he/him)]

(In reply to David Rajchenbach-Teller [:Yoric] (away until November 17th - use "needinfo") from comment #3)
> It my case, it was green on green. No text was visible. I only thought to
> click on it because I had seen this bug...

If this reproduces again, can you check what's going on in terms of whether the label property/value is actually set, and/or if there are errors in the console?

Comment 5

[Marco Bonardo [:mak]]

Attachment: screenshot

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Screen Shot 2014-11-28 at 22.13.26.png

This can, of course, be due to an extension.

Browser console:

TypeError: can't access dead object
Stack trace:
getInnerId@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/david/Library/Application%20Support/Firefox/Profiles/l5suw4b1.default/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/window/utils.js:77:3
initialize/model.observe@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/david/Library/Application%20Support/Firefox/Profiles/l5suw4b1.default/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/content/worker-parent.js:62:27
Observer<.observe@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/david/Library/Application%20Support/Firefox/Profiles/l5suw4b1.default/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/system/events.js:72:7
 events.js:79
OpenGL compositor Initialized Succesfully.
Version: 2.1 INTEL-8.28.32
Vendor: Intel Inc.
Renderer: Intel Iris OpenGL Engine
FBO Texture Target: TEXTURE_2D

Comment 7

[Francesco Lodolo [:flod]]

(In reply to David Rajchenbach-Teller [:Yoric] (away until November 17th - use "needinfo") from comment #6)
> Created attachment 8530422 [details]
> Screen Shot 2014-11-28 at 22.13.26.png
> 
> This can, of course, be due to an extension.

Just seen the same issue. Besides Adblock Plus, I don't have many add-ons installed.
I'm thinking of Profilist, since it works around the same area, and it looks like you're using it too.

Comment 8

[Francesco Lodolo [:flod]]

* disabled Profilist, first update was still empty, after a restart I saw the text. 
* enabled Profilist, next update was empty again.

Comment 9

[:Gijs (he/him)]

noitidart, can you have a look at the issues here with profilist?

Comment 10

[noitidart]

Thanks for the note. Will look into and get back asap.

Comment 11

[noitidart]

Does anyone know any code off the top of their head to force trigger the badge to show?
Thx

Comment 12

[noitidart]

Oh actually I just need to code that triggers "restart toolbarbutton" in menu. Just wondering if you know off the top of your head otherwise ill dig it out.

Comment 13

[:Gijs (he/him)]

(In reply to noitidart from comment #12)
> Oh actually I just need to code that triggers "restart toolbarbutton" in
> menu. Just wondering if you know off the top of your head otherwise ill dig
> it out.

Services.obs.notifyObservers(null, "update-staged", "pending");

Comment 14

[noitidart]

Thanks man you're awesome! :)

Comment 15

[noitidart]

Ok I think I found steps to reproduce. It is related to profilist.

Can users please test:

1. Have a window where you have never opened the panel yet (profilist does not have to be installed at this time)
2. Get update notified - badge and toolbarbutton triggered (`Services.obs.notifyObservers(null, "update-staged", "pending");`) (profilist does not have to be installed at this time)
3. If profilist was not installed, install it now, if it was installed then continue. In the window where you never opened the panel before, click the tri strip menu icon to open the panel, you will see the restart button under profilist button, but no visible text on restart button

Comment 16

[noitidart]

Oh in step 1. if you alrady opened the panel in your current window, just open a new window from panel or with keyboard shortcut or any other way.

Comment 17

[:Gijs (he/him)]

(In reply to noitidart from comment #16)
> Oh in step 1. if you alrady opened the panel in your current window, just
> open a new window from panel or with keyboard shortcut or any other way.

I've not tested, but I think we're all aware that this is broken - but it's not broken without profilist. Can you fix profilist so it doesn't break? :-)

Comment 18

[noitidart]

Hahaha!

Will do. When does this feature land? I was just curious because I had the next release version planned for mid to late january. But if this lands before then I can do small release to fix this.

Also in nightly on expand of profilist the scrollbar becomes visilble, but thats my problem just putting here for note to self.

Comment 19

[:Gijs (he/him)]

(In reply to noitidart from comment #18)
> Hahaha!
> 
> Will do. When does this feature land? I was just curious because I had the
> next release version planned for mid to late january. But if this lands
> before then I can do small release to fix this.
> 
> Also in nightly on expand of profilist the scrollbar becomes visilble, but
> thats my problem just putting here for note to self.

It will stay on nightly/aurora, and is automatically turned off on beta/release. I don't believe there are plans to change that yet, but it's a simple change to make (pref). If it's a simple fix to make on your side the people affected would probably prefer to have it sooner rather than later. :-)

Comment 20

[noitidart]

Oh man its a beautiful feature we should bring it to the masses! The badge is kind of ugly though :P

True true. Ok Ill report back once I figure out what specifically is causing it. Ill do that later tonight plz, i was curious to figure out if it was profilist or not and reproducibility steps, i have to get back to work >_<

Comment 21

[noitidart]

Attachment: 0.1 opacity on no hover, greyish comment.png

also this is re makk's comment of the grayishness. some computers have bad graphics, on my computer at work its barely noticable that its green because of the 0.1 alpha. on my computer at home the 0.1 alpha green is more noticeable.

here is what 0.1 alpha looks like on my computer at work, basically looks gray. it may be that when you view this image, its teh same case of gfx rendering so you might not be able to see difference.

Comment 22

[noitidart]

Hey all I can't explain this.

If you go:
document.getElementById("PanelUI-update-status").hidden = false
document.getElementById("PanelUI-update-status").hidden = true

it takes affect and changes the hidden attribute value

but doing:
document.getElementById("PanelUI-update-status").label = 'rawr';

does not work regardless of if profilist is there or not.

So on DXR if you change this line:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2553

from: `updateButton.label = updateButtonText;`
to: `updateButton.setAttribute('label', updateButtonText);`

it will fix it, but i have no idea why, i cant figure out how this is related to profilist? because the .label vs .setAttribute('label behavior is same regardless of it. can this be because no label attribute is given when the xul element is created? http://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#19

Comment 23

[noitidart]

im not saying its not profilists fault it definitely is. im just having a hard time connecting this real odd behavior. its especially odd because doing updateButton.label = blah on other windows doesnt work i have to do setAttribute but then how on earth is it applying that text? mind is boggled man

Comment 24

[:Gijs (he/him)]

label is a XBL property. It only works if the XBL binding is applied. I should have caught that in review, I think, and we should change it regardless of profilist.

Comment 25

[:Gijs (he/him)]

Michael, can you suggest new colours for us to use here, that are a bit more vibrant? Can we just up the opacity to 0.4 for the base case, and have hover and active be .6 and .7, respectively, or something?

Comment 26

[noitidart]

but it seems to work sometimes, as in so many other cases we are seeing the toolbarbutton properly has its label value set. how is that possible?

Comment 27

[noitidart]

(In reply to :Gijs Kruitbosch from comment #25)
> Michael, can you suggest new colours for us to use here, that are a bit more
> vibrant? Can we just up the opacity to 0.4 for the base case, and have hover
> and active be .6 and .7, respectively, or something?

Colors would be nice. But what about some extra creativity. Like `transition:transform 2000ms; transform:translate(0,100px);` on the badge. We can transform it reach the toolbarbutton, then after transition show that icon in the toolbarbutton.

Comment 28

[:Gijs (he/him)]

Attachment: fix sometimes not showing label on update button because XBL binding isn't attached,

Let's at least fix XBL binding silliness.

Comment 29

[Allison Naaktgeboren :ally]

Comment on attachment 8533440 [details] [diff] [review]
fix sometimes not showing label on update button because XBL binding isn't attached,

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

indeed. I wish xbl binding gotchas were easier to spot in reviews too.

Comment 30

[Marco Bonardo [:mak]]

(In reply to noitidart from comment #21)
> also this is re makk's comment of the grayishness. some computers have bad
> graphics, on my computer at work its barely noticable that its green because
> of the 0.1 alpha. on my computer at home the 0.1 alpha green is more
> noticeable.

Honestly, I have an high end graphics card and an IPS screen. I am quite confident of what I see and it's faint. Surely I could setup a wrong icc profile on my screen just to get some contrast there and break all of the other colors of the system :)

Comment 31

[Marco Bonardo [:mak]]

and fwiw, I'm not really happy that my bug has been hijacked to fix another bug, the color IS an issue here (just by comparing the color of the star and the color of the notification, they are miles away to create any sort of visual relation), if there is another issue either we fix both here or we file another bug.

Comment 32

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #31)
> and fwiw, I'm not really happy that my bug has been hijacked to fix another
> bug, the color IS an issue here (just by comparing the color of the star and
> the color of the notification, they are miles away to create any sort of
> visual relation), if there is another issue either we fix both here or we
> file another bug.

I'm still waiting on mmaslaney to provide better colors. In the meantime I intend to land the r+'d patch and mark this bug leave-open. I don't really think it's worth filing a separate bug at this point.

Comment 33

[:Gijs (he/him)]

Comment on attachment 8533440 [details] [diff] [review]
fix sometimes not showing label on update button because XBL binding isn't attached,

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

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0087b6c1a7c9

Comment 35

[noitidart]

is it the xbl is not attaching at all? or xbl is attaching after the code `.label = blah` executes>

Comment 36

[:Gijs (he/him)]

(In reply to noitidart from comment #35)
> is it the xbl is not attaching at all? or xbl is attaching after the code
> `.label = blah` executes>

Considering that the icon shows up, the xbl must be attaching, so it's the latter that's the case here.

When .label is set without the binding present, that creates an expando, and the expando never gets "included" into the XBL binding's state - instead, in fact, the XBL property breaks (so setting it after the binding has been attached, if you set it before that happened too, will also not do anything).

Comment 37

[noitidart]

Thanks for that thas good knowledge :) Im working a bit more then usual with xbl lately :)

ill try to figure out why profilist is delaying the xbl and let you guys know :)

Comment 38

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

Attachment: Updated colors patch

(In reply to Gijs Kruitbosch from comment #32)
> I'm still waiting on mmaslaney to provide better colors. In the meantime I
> intend to land the r+'d patch and mark this bug leave-open. I don't really
> think it's worth filing a separate bug at this point.

I talked with Sevaan over IRC and he said that the attached colors look better. This only changes the alpha values from (0.1, 0.4, 0.8) to (0.5, 0.8, 1.0), respectively.

Comment 39

[:Gijs (he/him)]

(In reply to (Behind on reviews/needinfos) Jared Wein [:jaws] (please needinfo? me) from comment #38)
> Created attachment 8544807 [details] [diff] [review]
> Updated colors patch
> 
> (In reply to Gijs Kruitbosch from comment #32)
> > I'm still waiting on mmaslaney to provide better colors. In the meantime I
> > intend to land the r+'d patch and mark this bug leave-open. I don't really
> > think it's worth filing a separate bug at this point.
> 
> I talked with Sevaan over IRC and he said that the attached colors look
> better. This only changes the alpha values from (0.1, 0.4, 0.8) to (0.5,
> 0.8, 1.0), respectively.

While we're here... is the foreground color fixed to black on all platforms here, and if so, does that provide enough contrast for all these shades?

And is the inset box shadow still useful for the active state when the colors have an opacity of 1 anyway?

Comment 40

[:Gijs (he/him)]

Comment on attachment 8544807 [details] [diff] [review]
Updated colors patch

rs=me assuming the questions are addressed

Comment 41

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

The foreground color is -moz-dialogText on Windows and Linux, and OSX doesn't have it specified in toolbarbutton.css. I've added an explicit color of black to this button, which matches was done for the customize-exit button when in Customization Mode (except white).

I removed the inset box shadows on this button for now. We can add it back later if their absence is noticed.

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

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/359db230dfb1