Closed Bug 1098420 Opened 7 years ago Closed 7 years ago

App update menu panel strip should be disabled in customization mode

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.1 - 26 Jan

People

(Reporter: ntim, Assigned: swaprp, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js][lang=css])

Attachments

(5 files, 1 obsolete file)

No description provided.
Whiteboard: [good first bug]
Tim, can we get a DXR link and a sentence or two describing this fix?

Thanks.
Flags: needinfo?(ntim007)
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.
Mentor: gijskruitbosch+bugs
Flags: needinfo?(ntim007) → needinfo?(gijskruitbosch+bugs)
Whiteboard: [good first bug] → [good first bug][lang=css]
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
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [good first bug][lang=css] → [good first bug][lang=js][lang=css]
Hi, I would like to work on this.
(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.
Thanks Tim. I will go through the tutorial and start working.
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.
Flags: needinfo?(ntim007)
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.
Flags: needinfo?(ntim007) → needinfo?(past)
(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.
Thanks Tim for the information.
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.
Flags: needinfo?(past)
Hi Swapnil, are you still working on this? If so, do you need more information from any of us in order to fix this?
Flags: needinfo?(swapnil.rp15)
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.
Flags: needinfo?(swapnil.rp15) → needinfo?(past)
(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 ?
Attached file 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.
Flags: needinfo?(past)
Hi, I would like to work on this.
(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?
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.
Flags: needinfo?(past)
(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 ?
What Tim said. Update your mozilla-central clone, or switch to one if you cloned the release repo.
Flags: needinfo?(past)
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?
(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.
Thanks Tim. Working on it.
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.
(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 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.
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.
Flags: needinfo?(swapnil.rp15)
Okay. No problem. I will hide it. It seems a better way.

Thanks.
Flags: needinfo?(swapnil.rp15)
Shall I attach the patch?
(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.
Attachment #8551885 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8551885 - Flags: review?(gijskruitbosch+bugs)
Only css changes work fine.
Attachment #8551885 - Attachment is obsolete: true
Attachment #8554377 - Flags: review?(gijskruitbosch+bugs)
Duplicate of this bug: 1125763
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!
Attachment #8554377 - Flags: review?(gijskruitbosch+bugs) → review+
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?
Assignee: nobody → swapnil.rp15
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
Points: --- → 2
Flags: qe-verify-
Flags: in-testsuite-
Flags: firefox-backlog+
Thanks Gijs. Yes, will surely look at Bug 964944.
https://hg.mozilla.org/mozilla-central/rev/8248339059d1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.