656269 - Add link to Mozilla plugin check from Add-ons Manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Kev Needham [:kev]]

Attachment: Mockup of link to plugin check from add-ons manager

Adding plugin check functionality is a goal for the Add-ons Manager which requires some development work (see bug 613305). In the interim between that feature being ready, we should add a link to Mozilla's plugin check page (e.g. https://www.mozilla.com/en-US/plugincheck) from the Plugins section of the Add-ons Manager. Until there is plugincheck functionality in Firefox proper, this will provide users with a simple method to determine if their plugins are up-to-date.

Additional information for this feature can be found on the Wiki at https://wiki.mozilla.org/Features/Firefox/PlugincheckLinkInAOM

UX has provided a mockup (see attached), and we'll need to make l10n aware as well.

Comment 1

[Dave Townsend [:mossop]]

One problem I see here is that that text is extremely long and we're going to run into problems as the window is resized unless we crop it in that case

Comment 2

[Axel Hecht [:Pike]]

There are two lengthy strings that compete for screen estate in localized build in particular, the plugin check string on top, and the "Recent Updates" string in the tab selector on the left, which in German is "Zuletzt durchgeührte Updates", and leads to a lot of (usually blank) space on the left.

Also, does this depend on merging some of the info of about:plugins into the addons manager? The tab on the left looks like it.

I'm not really sold on this design. Also unclear on when this link should be shown, i.e., does that depend on the tab selection?

Comment 3

[Kev Needham [:kev]]

(In reply to comment #1)
> One problem I see here is that that text is extremely long and we're going
> to run into problems as the window is resized unless we crop it in that case

Do we want to consider (and can we) consider redirecting only if the user checks for updates from the plugin page through the settings button? Would adding the check as a list item make it simpler with the wrapping it does?

(In reply to comment #2)
> Also, does this depend on merging some of the info of about:plugins into the
> addons manager? The tab on the left looks like it.

I don't think it does, the intent is to just give users a way to check if their installed plugins are up to date via the plugin check page.

> I'm not really sold on this design. Also unclear on when this link should be
> shown, i.e., does that depend on the tab selection?

It should, I think.

Comment 4

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup of link to plugin check as drop-down notification in add-ons manager

(In reply to comment #3)
> (In reply to comment #1)
> > One problem I see here is that that text is extremely long and we're going
> > to run into problems as the window is resized unless we crop it in that case
> 
> Do we want to consider (and can we) consider redirecting only if the user
> checks for updates from the plugin page through the settings button? Would
> adding the check as a list item make it simpler with the wrapping it does?

As a pure list item with similar height, it could be easily confused as a kind of plugin check-service add-on.  However, if string length is the crux of the issue in this temporary solution, we could display it as we display notifications within list view: as a notification drop-down at the top of the list (see attached).

> (In reply to comment #2)
> > Also, does this depend on merging some of the info of about:plugins into the
> > addons manager? The tab on the left looks like it.
> 
> I don't think it does, the intent is to just give users a way to check if
> their installed plugins are up to date via the plugin check page.
> 
> > I'm not really sold on this design. Also unclear on when this link should be
> > shown, i.e., does that depend on the tab selection?
> 
> It should, I think.

It would depend on tab selection.  So switching tabs mean the link appears and then disappears.  It's weird but temporary behavior, perhaps mitigated by showing the link within list view rather than outside of it, as in the attached.

The only other notification that appears here, as far as I know, says that add-ons are disabled due to safe mode.  So there may be two notifications showing in that case.

Comment 5

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup of link to plugin check as drop-down notification in add-ons manager

Comment 6

[Dave Townsend [:mossop]]

I think that second option works pretty well actually

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Jennifer, how would that look like with safe mode enabled or the disabled compatibility checks?

Comment 8

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #7)
> Jennifer, how would that look like with safe mode enabled or the disabled
> compatibility checks?

When would compatibility checks be disabled?  Safe mode?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to comment #8)
> When would compatibility checks be disabled?  Safe mode?

Wait, we have disabled that bar for the plugin pane. So it wouldn't affect us here.

Comment 10

[Axel Hecht [:Pike]]

Yeah, the second mockup is much better.

Still confused to see a "plugins" tab showing a host of add-ons plus a pluginchecker notification. For which tab should we show the pluginchecker?

Comment 11

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Screenshot: Currently plugins are not disabled in safe mode

(In reply to comment #7)
> Jennifer, how would that look like with safe mode enabled or the disabled
> compatibility checks?

Right now, it appears plugins aren't disabled in safe mode (see attached).  Is this by design or by accident?  If it's design, no problem, no double notifications!  If it's by accident, we at least don't have a problem until this gets fixed...

Comment 12

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #9)
> (In reply to comment #8)
> > When would compatibility checks be disabled?  Safe mode?
> 
> Wait, we have disabled that bar for the plugin pane. So it wouldn't affect
> us here.

Oddly fortunate, I suppose... 

(In reply to comment #10)
> Yeah, the second mockup is much better.
> 
> Still confused to see a "plugins" tab showing a host of add-ons plus a
> pluginchecker notification. For which tab should we show the pluginchecker?

Is your concern that this link should be in a global management area of UI rather than displayed alongside plugins?  If so, I'd disagree - both because we have no global management UI, and because the check link takes users directly to a list of their own plugins, which I'd argue relates directly to this screen and no others.  Also, once we properly integrate plugin check, it would be available as an action per plugin item.

Comment 13

[Axel Hecht [:Pike]]

Oops, I got confused by the mockup showing the plugins tab at a different location where it is right now, and the pane just showing add-ons. We have a plugins section, and yes, showing the notification box there should work out fine.

Technical question, can we allow the text to wrap in that box for small windows and long localizations?

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> Oops, I got confused by the mockup showing the plugins tab at a different
> location where it is right now, and the pane just showing add-ons. We have a
> plugins section, and yes, showing the notification box there should work out
> fine.
> 
> Technical question, can we allow the text to wrap in that box for small
> windows and long localizations?

I believe so yes.

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup of link to plugin check as drop-down notification in add-ons manager with additional notification that user is in safe mode

(In reply to comment #9)
> (In reply to comment #8)
> > When would compatibility checks be disabled?  Safe mode?
> 
> Wait, we have disabled that bar for the plugin pane. So it wouldn't affect
> us here.

Mossop points out that this is a problem being addressed in bug 342333.  Assuming that this bug gets fixed, we'd still need a way to show (groan) two notifications - one for safe mode and one for the plugin update link.  The incompatible add-on notification is currently hidden when the safe mode notification is present, and this behavior is fine - the former is a fairly rare user-set preference that is constantly activated, while safe mode is usually a one-off run.  Luckily, both of these are user-set, so a normal user who does not change preferences would only ever see the notification for plugin updating and would never run into a double-notification scenario.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Jennifer, can you please update your mockups so they reflect the latest state of the add-ons manager, specifically colors and styles? That would be really appreciated.

Comment 17

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup of link to plugin check as drop-down notification in add-ons manager with additional notification, current UI (Windows 7 5.11.11)

(In reply to comment #16)
> Jennifer, can you please update your mockups so they reflect the latest
> state of the add-ons manager, specifically colors and styles? That would be
> really appreciated.

Sure

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #11)
> Right now, it appears plugins aren't disabled in safe mode (see attached). 
> Is this by design or by accident?  If it's design, no problem, no double
> notifications!  If it's by accident, we at least don't have a problem until
> this gets fixed...

A little bit of both. Right now, plugins don't get disabled in safe-mode - so it's intentional that we don't show the safe-mode warning in the plugins pane (bug 492271). However, bug 342333 will eventually make it so plugins are disabled in safe-mode.

Comment 19

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: initial patch

Well, first approach to solve this. I'm attaching a proposed patch, but have a few questions:

* I Don't know if I chose the right entities for the message, nor if the strings are the correct ones.

* the screenshot attached to comment #11 shows that when in safemode the "addons disabled" message is not properly hidden. Fixing that too.

* Don't know if I chose the right names for the classes for the container of the message, as I think this is a new kind of notification there.

* When coding the cmd_ action, I noticed that in extension.js some preferences are used with constants, but others are just accessed directly. Is this OK?

Comment 20

[Dave Townsend [:mossop]]

We don't track features until after they have landed.

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 531986 [details] [diff] [review]
initial patch

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

This is great. Only one trivial comment on the patch so far however it isn't quite finished yet. We should have an automated test for this, it should check that the message is visible on the plugins pane and not on another one and that clicking on it does open the correct webpage. I'd also like you to sit down with Boriss and show her what it looks like right now and find out what styling changes she'd like. You'll probably have to trigger a try build once you have the test written and you can use the generated builds to see what it looks like on other platforms.

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
@@ +22,5 @@
>  <!ENTITY warning.updatesecurity.label              "Add-on update security checking is disabled. You may be compromised by updates.">
>  <!ENTITY warning.updatesecurity.enable.label       "Enable">
>  <!ENTITY warning.updatesecurity.enable.tooltip     "Enable add-on update security checking">
> +<!ENTITY warning.plugincheck.label                 "Check to see if your Plugins are up to date">
> +<!ENTITY warning.plugincheck.tooltip               "Check to see if your Plugins are up to date">

I would use info.plugincheck.* here just because as you've rightly done elsewhere these aren't warning messages.

Comment 22

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: added automated test

changed the strings location to info.plugincheck.* and added automated test

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 532130 [details] [diff] [review]
added automated test

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

Couple of tweaks for the test, otherwise the code is all good, going to wait to see the styling parts before granting this review though.

::: toolkit/mozapps/extensions/test/browser/browser_globalinformations.js
@@ +43,5 @@
> +  gLoadCompleteCallback = function() {
> +    browser.removeProgressListener(gProgressListener);
> +    aCallback(aManager);
> +  };
> +}

There is actually a simpler way to do this without the need for the progress listener, look here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_purchase.js#139

@@ +58,5 @@
> +      var button = aManager.document.querySelector("#list-view hbox.global-info-plugincheck button.button-link");
> +      is_element_visible(button, "Plugin Check message button should be visible");
> +
> +      info("Clicking 'Plugin Check' button");
> +      EventUtils.synthesizeMouse(button, 2, 2, { }, aManager);

Use synthesizeMouseAtCenter here

Comment 24

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: patch with corrections

Comment 25

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 533023 [details] [diff] [review]
patch with corrections

This looks great!  A couple changes that have been made here (discussed via email):  

- Background color for OSX changed to #e3e6eb
- Background color for Windows changed to #f3f7fb
- String capitalization corrected ("plugins" in lowercase)

Comment 26

[obsolete.fax]

Is this going to come in Firefox 5?

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 533023 [details] [diff] [review]
patch with corrections

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

Great work!

Comment 28

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/e816b1abd0e0

(In reply to comment #26)
> Is this going to come in Firefox 5?

Firefox 5 is already in beta testing, so no it is unlikely to make it there.

Comment 29

[Kev Needham [:kev]]

Nicely done, Hernán. Thanks for pushing this through, and thanks to all for your input and help.

(In reply to comment #26)
> Is this going to come in Firefox 5?

Nope. It should be in the following release if it lands in the next week (I believe).

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Verified fixed across platforms with builds like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110529 Firefox/6.0a2 ID:20110529042002

It would have been great if the link element would have gotten an id assigned to. I will raise a follow-up bug.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 533023 [details] [diff] [review]
patch with corrections

>+            <hbox class="global-info-plugincheck" flex="1" align="center"
>+                  tooltiptext="Update plugins">
>+              <button class="button-link"
>+                      label="&info.plugincheck.label;"
>+                      tooltiptext="&info.plugincheck.tooltip;"

I will also take care of this hard-coded tooltiptext property which seems to be not necessary at all.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Oh, and I think we should have a manual test to at least test that we are able to open the live web page and show its content. That's not something we can test with the automated test.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Bug 661134 is now covering the localization of the plugincheck web page.

Comment 34

[Ben Bucksch (:BenB)]

xref Bug 929474 Add plugin check natively into Firefox