1098420 - App update menu panel strip should be disabled in customization mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Comment 1

[Mike Hoye [:mhoye]]

Tim, can we get a DXR link and a sentence or two describing this fix?

Thanks.

Comment 2

[Tim Nguyen :ntim]

The relevant code is here : http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js

So basically, we need set the disabled attribute to the panel strip when appropriate (entering the customization mode). 
Gijs might have more precise indications here though.

Comment 3

[:Gijs (he/him)]

TBH, I think the devtools team should be fixing this themselves, rather than making this a "good first bug".

Anyway. The code is here:

http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2450

It should add listeners in the init method if it's enabled:

gNavToolbox.addEventListener("customizationstarting", this);
gNavToolbox.addEventListener("customizationending", this);

and remove them in uninit.

In handleEvent, the code should check for these types of events, and disable and enable the button, respectively (by setting/removing the "disabled" attribute from JS).

It's possible that some work will need to be done to ensure that the button's disabled state is accurately visually reflected, probably by adding the appropriate selector here:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#617

Comment 4

[Swapnil R Patil [:swaprp]]

Hi, I would like to work on this.

Comment 5

[Tim Nguyen :ntim]

(In reply to Swapnil R Patil from comment #4)
> Hi, I would like to work on this.

You can find tutorials to set up your environment at http://codefirefox.com .

Once you've done that, you can follow the instructions in comment 3.

Comment 6

[Swapnil R Patil [:swaprp]]

Thanks Tim. I will go through the tutorial and start working.

Comment 7

[Swapnil R Patil [:swaprp]]

Hi Tim,
 
I have successfully built Firefox. Now, I am trying to reproduce the issue. I entered the customization mode, but I don't know what 'App update menu panel strip' is. Could you please let me know. A screenshot would help.

Also could you please let me know which IRC channel I should connect to contact you?

Thanks.

Comment 8

[Tim Nguyen :ntim]

I don’t know how this can be tested for custom firefox builds. Needinfo'ed someone for that question. 
But you can reproduce this bug on a nightly :
- make sure there's a pending firefox update
- wait for a green badge to appear
- open the menu panel, you'll see a new strip related to updating firefox

I can’t provide a screen right now, but I'll do once I can.

Comment 9

[Tim Nguyen :ntim]

(In reply to Swapnil R Patil [:swaprp] from comment #7)
> 
> Also could you please let me know which IRC channel I should connect to
> contact you?
> 
> Thanks.

I won't be available on IRC at least for the 2 next weeks. But feel free to ask in #devtools or #introduction for help.

Comment 10

[Swapnil R Patil [:swaprp]]

Thanks Tim for the information.

Comment 11

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

In order to generate an update badge/notification, put the following two lines in a file (say, update.js) and load it in Scratchpad:

// -sp-context:browser
Services.obs.notifyObservers(null, "update-staged", "pending");

The comment will enable the Browser Environment in Scratchpad, but you can also do it manually from the menu. Running it will get you the update badge.

Comment 12

[:Gijs (he/him)]

Hi Swapnil, are you still working on this? If so, do you need more information from any of us in order to fix this?

Comment 13

[Swapnil R Patil [:swaprp]]

Hi Gijs,

Sorry, I was busy with my academics so couldn't update on this earlier.

I did try steps mentioned by Panos. But didn't work quite well. I need to get in touch with him. I did try to contact him on IRC.

@Panos: Hi, the lines you told did work for the first time. I mean I added those and opened Firefox. Then I saw the update dialog. But when I ran that script in Scratchpad I did not get any update badge/notification. Just to cross check I restarted Firefox with those two lines present in update.js. But now that update dialog didn't appear at the start up. Am I missing something?

Thanks.

Comment 14

[Tim Nguyen :ntim]

(In reply to Swapnil R Patil [:swaprp] from comment #13)
> Hi Gijs,
> 
> Sorry, I was busy with my academics so couldn't update on this earlier.
> 
> I did try steps mentioned by Panos. But didn't work quite well. I need to
> get in touch with him. I did try to contact him on IRC.
> 
> @Panos: Hi, the lines you told did work for the first time. I mean I added
> those and opened Firefox. Then I saw the update dialog. But when I ran that
> script in Scratchpad I did not get any update badge/notification. Just to
> cross check I restarted Firefox with those two lines present in update.js.
> But now that update dialog didn't appear at the start up. Am I missing
> something?
> 
> Thanks.

Have you switched to the browser environnement ?

Comment 15

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

Attachment: update-notify.js

Here is the file that I'm using, in case it is of any help.

(In reply to Swapnil R Patil [:swaprp] from comment #13)
> @Panos: Hi, the lines you told did work for the first time. I mean I added
> those and opened Firefox. Then I saw the update dialog. But when I ran that
> script in Scratchpad I did not get any update badge/notification. Just to
> cross check I restarted Firefox with those two lines present in update.js.
> But now that update dialog didn't appear at the start up. Am I missing
> something?

It only works the first time you run it. It's something crude and simple, you have to restart Firefox before running it again. However, I don't understand what you mean that you restarted Firefox with the update.js present. As Tim says, this code is only executed when you are in a Browser Environment Scratchpad and execute it.

The point of this file is to help you see what it is that this bug is talking about.

Comment 16

[shreyas]

Hi, I would like to work on this.

Comment 17

[:Gijs (he/him)]

(In reply to shreyas from comment #16)
> Hi, I would like to work on this.

Swapnil R Patil is still working on this (at least, he was 3 days ago) - maybe you could consider working on bug 1122430 instead?

Comment 18

[Swapnil R Patil [:swaprp]]

Hi Panos,

Initially I thought I have to add those two lines in update.js file of firefox (/toolkit/mozapps/extensions/content/update.js). SO I added there and launched the firefox. There I did get a update notification.

But when I load that file in Scratchpad, with those lines present in the file and run it, nothing happens. I don't get to see the update badge.

Plus if I restart the browser again I don't get that update dialog that I got first time. (Even if those two lines are present in the update.js)

But now I understood that I just need to create new sample file, put these lines and load it. I hope I got it correct. I did this. I am sure that the scratchpad is executing in browser environment. (I can see the message "This scratchpad executes in Browser Context."). But still when I click on run. Nothing happens. I don't get to see the update badge.

I am assuming that the update badge will appear on the menu button as per the link http://www.ghacks.net/2014/11/10/display-an-update-badge-in-firefox/. This link also mentions property 'app.update.badge'. Does it need to be set to true?

PS: My locally built Firefox version is 35.

Comment 19

[Tim Nguyen :ntim]

(In reply to Swapnil R Patil [:swaprp] from comment #18)
> Hi Panos,
> 
> Initially I thought I have to add those two lines in update.js file of
> firefox (/toolkit/mozapps/extensions/content/update.js). SO I added there
> and launched the firefox. There I did get a update notification.
> 
> But when I load that file in Scratchpad, with those lines present in the
> file and run it, nothing happens. I don't get to see the update badge.
> 
> Plus if I restart the browser again I don't get that update dialog that I
> got first time. (Even if those two lines are present in the update.js)
> 
> But now I understood that I just need to create new sample file, put these
> lines and load it. I hope I got it correct. I did this. I am sure that the
> scratchpad is executing in browser environment. (I can see the message "This
> scratchpad executes in Browser Context."). But still when I click on run.
> Nothing happens. I don't get to see the update badge.
> 
> I am assuming that the update badge will appear on the menu button as per
> the link
> http://www.ghacks.net/2014/11/10/display-an-update-badge-in-firefox/. This
> link also mentions property 'app.update.badge'. Does it need to be set to
> true?
Yes, but it should be true on nightly builds already.

> PS: My locally built Firefox version is 35.

Can you build mozilla-central instead ?

Comment 20

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

What Tim said. Update your mozilla-central clone, or switch to one if you cloned the release repo.

Comment 21

[Swapnil R Patil [:swaprp]]

Thanks. I could reproduce the issue. I have attached a screenshot. I assume that the strip that I have marked in red should be disabled in customization mode. So it will be there, but nothing should happen if user clicks on it. Am I right?

Comment 22

[Swapnil R Patil [:swaprp]]

Attachment: Screenshot of update strip.

Comment 23

[Tim Nguyen :ntim]

(In reply to Swapnil R Patil [:swaprp] from comment #21)
> Thanks. I could reproduce the issue. I have attached a screenshot. I assume
> that the strip that I have marked in red should be disabled in customization
> mode. So it will be there, but nothing should happen if user clicks on it.
> Am I right?

Yeah, and it should as well look disabled.
See comment 3 for how to do this.

Comment 24

[Swapnil R Patil [:swaprp]]

Thanks Tim. Working on it.

Comment 25

[Swapnil R Patil [:swaprp]]

Attachment: Color of update strip doesn't change on hover

Comment 26

[Swapnil R Patil [:swaprp]]

Hi,

I could make code changes to disable the update strip in customization mode. Now a click on the strip doesn't restart firefox plus color of the strip doesn't change on hover. Is this what we need?

@Gijs : You mentioned that we need to ensure that the button's disabled state is visually reflected, probably by adding the appropriate selector. Is it required now given that color of the strip doesn't change on hover? Or we need something else also to happen?

Thanks.

Comment 27

[:Gijs (he/him)]

(In reply to Swapnil R Patil [:swaprp] from comment #26)
> Hi,
> 
> I could make code changes to disable the update strip in customization mode.
> Now a click on the strip doesn't restart firefox plus color of the strip
> doesn't change on hover. Is this what we need?
> 
> @Gijs : You mentioned that we need to ensure that the button's disabled
> state is visually reflected, probably by adding the appropriate selector. Is
> it required now given that color of the strip doesn't change on hover? Or we
> need something else also to happen?

If hover/active doesn't change the appearance of the button, that sounds good enough to me.

Comment 28

[Tim Nguyen :ntim]

Comment on attachment 8551458 [details]
Color of update strip doesn't change on hover

Can you lower the opacity ? It doesn't seem too obvious at first look that the strip is disabled.

Comment 29

[:Gijs (he/him)]

Thinking about it more (and actually looking at the screenshot)... is there any reason not to just hide the strip completely in customize mode? (display: none)

That seems the simplest to me, both code-wise and in terms of getting the visual feedback right - having a thing that says "you have an update" with a different color that you can't interact with is confusing. Even moreso because it shares a color with the "Exit customize" button and draws attentions away from it, which we shouldn't do.

The more I think about it, the more I think that will be the simplest solution. Swapnil, sorry for leading you astray on that point, but could just hide it? You could even use element.hidden = true / false for this.

Comment 30

[Swapnil R Patil [:swaprp]]

Okay. No problem. I will hide it. It seems a better way.

Thanks.

Comment 31

[Swapnil R Patil [:swaprp]]

Attachment: This is how it looks now with hidden update strip.

Shall I attach the patch?

Comment 32

[Tim Nguyen :ntim]

(In reply to Swapnil R Patil [:swaprp] from comment #31)
> Created attachment 8551647 [details]
> This is how it looks now with hidden update strip.
> 
> Shall I attach the patch?

Yes, please do.

Comment 33

[Swapnil R Patil [:swaprp]]

Attachment: Bug_1098420_update_menu_strip_disabled.diff

Comment 34

[:Gijs (he/him)]

Comment on attachment 8551885 [details] [diff] [review]
Bug_1098420_update_menu_strip_disabled.diff

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

So, this is not quite so simple, unfortunately.

For one, right now it seems like the code in browser.js is unnecessary; it doesn't do anything apart from adding and removing a listener which then doesn't do anything...

For another, you're unconditionally setting hidden to false after leaving customize mode... but there might not be an update at all, so that's not right.

Then there's also the case of the update becoming available while you're in customize mode, which this code doesn't take care about (it's racy).

While this problem could be solved in JS, solving it in CSS is a lot easier. Just add:

#main-window[customizing=true] #PanelUI-update-status {
  display: none;
}

after this line:

http://hg.mozilla.org/mozilla-central/annotate/c2df1daf74c3/browser/base/content/browser.css#l1187

and doublecheck that this works even after the update code unhides the item by setting .hidden = false (if not, you might need to add !important)

Then you don't need any JS at all, as far as I can tell.

Comment 35

[Swapnil R Patil [:swaprp]]

Attachment: Set display: none to update status panel

Only css changes work fine.

Comment 37

[:Gijs (he/him)]

Comment on attachment 8554377 [details] [diff] [review]
Set display: none to update status panel

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

Yup, that's it. Thanks for the patch!

Comment 38

[:Gijs (he/him)]

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

This is now on fx-team; all being well, it'll merge to mozilla-central sometime today or tomorrow. Thanks for your help! Maybe you'd like to look at, say, bug 964944 next?

Comment 39

[Swapnil R Patil [:swaprp]]

Thanks Gijs. Yes, will surely look at Bug 964944.

Comment 40

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/8248339059d1