598923 - add-on bar should be made visible if there are any add-ons installed

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dietrich Ayala (:dietrich)]

the patch originally had mutation event listeners that toggled the visibility of the bar as needed. however, those were removed due to perf/security concerns.

for now we should check at window load, and if there's a way to listen for addon installation, then too.

Comment 1

[Paul [pwd]]

This doesn't make sense to me. I thought the Addon bar was meant to be purely optional? Perhaps it'd be better to offer a door-hanger notification that will allow users to turn it on?

Comment 2

[Dietrich Ayala (:dietrich)]

The interaction I'm talking about is just when a new add-on is installed that puts itself in the bar. We'd open the bar then. This gives the user visual confirmation of what happened to the add-on they just installed.

We could also do something fancy like flash the new one and then slide the bar out of view after a second. But that leads to frustrations for users that then have to go figure out how to turn it back on (hidden in a submenu!).

Comment 3

[Dietrich Ayala (:dietrich)]

Requesting blocking - without this, users might install add-ons that use the add-on bar, and never ever see them. Ever.

Comment 4

[Paul [pwd]]

(In reply to comment #3)
> Requesting blocking - without this, users might install add-ons that use the
> add-on bar, and never ever see them. Ever.

That's just given me a thought. Would it be possible to have Addon ask users whether they want their button added to the navigation bar or the Addon bar on Installation? Apologies if I'm over-thinking things.

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: wip

Comment 6

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> That's just given me a thought. Would it be possible to have Addon ask users
> whether they want their button added to the navigation bar or the Addon bar on
> Installation? Apologies if I'm over-thinking things.

I'd prefer zero interruption + post-install customization over interrupting the user. If it's totally confusing to people then let's revisit our approach at that point.

Comment 7

[Paul [pwd]]

(In reply to comment #6)
> (In reply to comment #4)
> > That's just given me a thought. Would it be possible to have Addon ask users
> > whether they want their button added to the navigation bar or the Addon bar on
> > Installation? Apologies if I'm over-thinking things.
> 
> I'd prefer zero interruption + post-install customization over interrupting the
> user. If it's totally confusing to people then let's revisit our approach at
> that point.

I'm a "power user" so I'll think to check the customisation palette, however for users that aren't using the nightlies and haven't followed the development of the Addon Bar, the buttons will be easily lost.

Alternatives are Doorhangers, but they disappear if you even sneeze on the browser, so you can't notify users of the ability to add a button there. Where as prompting users as to where they'd like to place the button follows established Microsoft installation familiarity.

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> I'm a "power user" so I'll think to check the customisation palette, however
> for users that aren't using the nightlies and haven't followed the development
> of the Addon Bar, the buttons will be easily lost.

I don't understand. The whole point of this bug is about showing the bar after an add-on that puts itself on the bar is installed. That's what this patch does, exactly so what you described does not happen.

I'm not saying it's perfect, nor what we'll ultimately ship with, however it achieves two goals:

1. Shows users that the add-on bar exists

2. Shows users where that add-on they just installed went to

Once this lands, let's take a look at the user feedback.

Also, you bring up a great point: For add-ons that don't pre-install themselves anywhere, so are only on the palette, we might want to do something for discoverability - maybe a doorhanger as you suggest. Can you file a separate bug for that?

> Alternatives are Doorhangers, but they disappear if you even sneeze on the
> browser, so you can't notify users of the ability to add a button there.

I like this idea - to not only show the add-on bar, but to also put a doorhanger off the new add-on, that summarizes the customization options.

File a separate bug for the shortcomings of doorhangers please.

> Where as prompting users as to where they'd like to place the button follows
> established Microsoft installation familiarity.

That doesn't make it right. And I don't want to go down the road we've been speeding away from: prompting the user, interrupting their workflow, instead of achieving a design that just works in most scenarios.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 482125 [details] [diff] [review]
wip

In order to persist the 'collapsed' state, you should call setToolbarVisibility instead of setting 'collapsed' on the toolbar.

Comment 10

[Paul [pwd]]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm a "power user" so I'll think to check the customisation palette, however
> > for users that aren't using the nightlies and haven't followed the development
> > of the Addon Bar, the buttons will be easily lost.
> 
> I don't understand. The whole point of this bug is about showing the bar after
> an add-on that puts itself on the bar is installed. That's what this patch
> does, exactly so what you described does not happen.
> 
> I'm not saying it's perfect, nor what we'll ultimately ship with, however it
> achieves two goals:
> 
> 1. Shows users that the add-on bar exists
> 
> 2. Shows users where that add-on they just installed went to
> 
> Once this lands, let's take a look at the user feedback.
> 
> Also, you bring up a great point: For add-ons that don't pre-install themselves
> anywhere, so are only on the palette, we might want to do something for
> discoverability - maybe a doorhanger as you suggest. Can you file a separate
> bug for that?
Bug 603185

> > Alternatives are Doorhangers, but they disappear if you even sneeze on the
> > browser, so you can't notify users of the ability to add a button there.
> 
> I like this idea - to not only show the add-on bar, but to also put a
> doorhanger off the new add-on, that summarizes the customization options.
Bug 603187 

> File a separate bug for the shortcomings of doorhangers please.
Bug 603188 

Not sure if I was eloquent enough, but all filed.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: v1

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: v1.1

patch with test.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 487543 [details] [diff] [review]
v1.1

>+  finish();

Shouldn't be needed.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: v1.2 - for checkin

Comment 15

[Erik Vold]

From what I understand from just quickly reading the comments is that this patch will show the add-on bar after a add-on that puts itself in to the add-on bar is installed; so then my question would be how should a add-on put itself into the add-on bar?

If I overlay it, then the user can't move the toolbar button I inserted; if I use this hack https://developer.mozilla.org/en/Code_snippets/Toolbar#Adding_button_by_default for the add-on bar then will your patch understand that my add-on has put itself into the add-on bar?

Also, is there or will there be some better way than the mentioned hack to add a toolbar button to the add-on bar by default?

Comment 16

[Dietrich Ayala (:dietrich)]

Page for best-practices in adding to the add-on bar is started here, not done yet though. https://wiki.mozilla.org/User:Dietrich/Scratchpad

This patch listens for add-on install/uninstall events, counts the child elements before and after, to determine whether to show/hide. This should handle any method of adding to the add-on bar.

It doesn't handle scenarios unrelated to add-on install/uninstall.

Comment 17

[Erik Vold]

(In reply to comment #16)
> This patch listens for add-on install/uninstall events, counts the child
> elements before and after, to determine whether to show/hide. This should
> handle any method of adding to the add-on bar.
> 
> It doesn't handle scenarios unrelated to add-on install/uninstall.

You are referring to the install/uninstall events which trigger the install/uninstall methods of a extension with a bootstrap.js I assume? or can a regular extension hook into these events?

Comment 18

[Erik Vold]

(In reply to comment #17)
> You are referring to the install/uninstall events which trigger the
> install/uninstall methods of a extension with a bootstrap.js I assume? or can a
> regular extension hook into these events?

Nvm, I forgot for a moment that a reg extension would req a restart, so these events would have to be fired then, and I'm pretty certain that's not being done.

Comment 19

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/6b25735eebd0

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101117 Firefox/4.0b8pre ID:20101117030837

A Litmus and Mozmill test will be part of the basic add-ons manager tests.

Comment 21

[Matthew Turnbull [Bluefang]]

Perhaps I'm missing something, but I can't get this to work.

I've tried installing an uninstalling extensions that add elements directly to both the addon-bar and status-bar via overlay. However, the addon-bar is never shown or hidden on restart.

Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101120 Firefox/4.0b8pre

STR:

1. Start Minefield with a new profile
2. Install Status-4-Evar
3. Restart Minefield

Expected results:

The Addon-Bar is shown

Actual results:

The Addon-Bar is not shown

Comment 22

[Ian Nartowicz]

Same here, just doesn't work.  I checked and the patch exists in my build, but a quick scan suggests that it will only work for restartless addons?

Comment 23

[Dietrich Ayala (:dietrich)]

I just tested this with an add-on that adds a Jetpack widget, and the add-on bar became visible after installation, and was hidden when I uninstalled.

I am seeing some odd behavior though: I noticed that I seeing a "lastAddonBarCount is undefined" error in onUninstalling. So I though dump() statements in each method of the addons manager listener. The install methods dump. The uninstall methods do not! But I know they're getting called, since the bar does go away on uninstall. Still investigating.

Comment 24

[Matthew Turnbull [Bluefang]]

I haven't tested this with JetPacks. The problem is with regular extensions. My guess is that:
A) The listener is being registered after the install events for regular add-ons occur
B) The install events happen before the toolbar items are overlaid onto the toolbar

Rather than using an AddonManager listener, wouldn't it be simpler and more robust to persist the expected item count as an attribute on the toolbar, then handle this directly in BrowserStartup(), delayedStartup(), or on window load/showing?

Comment 25

[Adam Kowalczyk]

Yeah, this patch clearly doesn't work for non-restartless extensions. They
insert their toolbar buttons on window load after a restart - long after
onInstalled is called and when lastAddonBarCount isn't even remembered any more.

Comment 26

[Dietrich Ayala (:dietrich)]

Filed bug 616419 for the non-restartless case.

Comment 27

[Ioana (away)]

Testcase #45481 has been added in Litmus for:
aurora basic functional tests (bfts) (276) -> fx aurora bft - add-ons (2173)
aurora full functional tests (ffts) (275) -> fx aurora fft - add-ons (2174).

Henrik, please let me know if you want me to add the testcase to any other test suites.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Ioana Budnar [QA] from comment #27)
> Testcase #45481 has been added in Litmus for:
> aurora basic functional tests (bfts) (276) -> fx aurora bft - add-ons (2173)
> aurora full functional tests (ffts) (275) -> fx aurora fft - add-ons (2174).

Thanks Ioana, but this test should go under Toolbar and not Add-ons. Can you please move it to that subgroup? Thanks.

Comment 29

[Ioana (away)]

Henrik, there is no Toolbar subgroup under BFTs. The only subgroup close to that is Toolbar Customization which only contains testcases about customizing toolbars. This is why I added the testcase under Add-ons. Please let me know if you want me to move the testcase under Toolbar Customization.