591465 - Context menu of add-ons miss context related state change entries

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100827 Minefield/4.0b5pre

Compared to older branches of Firefox we do not have any entries in the context menu of add-ons which can be used to change the state. Formerly we had:

Extensions:
* Disable / Enable
* Uninstall

Themes:
* Use Theme
* Uninstall

Plugins:
* Disable / Enable

As talked with Dave on IRC we will have to figure out if we want to have those entries or not. It's not that much time until beta 6 and then it will probably be too late. Asking for UX review from Jennifer.

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

There's a bit more functionality in the add-ons manager, and the context menus should reflect that.  

Proposing:

Extensions:
* Enable / Disable
* Preferences
* More Information
* Remove
* ✓ Update Automatically

Plugins:
* Enable / Disable
* More Information
* Remove

Themes:
* Wear Theme / Stop Wearing Theme
* More Information
* Remove
* ✓ Update Automatically

Backgrounds:
* Wear Background / Stop Wearing Background
* More Information
* Remove
* ✓ Update Automatically

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

Actually, forget "✓ Update Automatically".  That shouldn't be in the context menu. 

Current proposal:

Extensions:
* Enable / Disable
* Preferences
* More Information
* Remove

Plugins:
* Enable / Disable
* More Information

Themes:
* Wear Theme / Stop Wearing Theme
* More Information
* Remove

Backgrounds:
* Wear Background / Stop Wearing Background
* More Information
* Remove

Comment 3

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

(In reply to comment #2)
> Extensions:
> * Enable / Disable
> * Preferences
> * More Information
> * Remove

Can we group/sort those entries by actions? IMO Enable/Disable should be placed right next to remove:

Find updates
Enable / Disable
Remove
------------
More Information
Preferences
About

> Themes:
> * Wear Theme / Stop Wearing Theme
> * More Information
> * Remove

Do we really need "Stop Wearing Theme"? If yes, in such a case we should fallback to the default theme.

You missed "About".

Wear Theme / Stop Wearing Theme
Remove
------------
More Information
About

> Backgrounds:
> * Wear Background / Stop Wearing Background
> * More Information
> * Remove

You also missed "About".

Wear Background / Stop Wearing Background
Remove
------------
More Information
About

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Things to note:
* This doesn't differentiate between themes and backgrounds, because the UI doesn't see any difference. Bug 520124 would have added that difference, but now its a lot of work just for a wording change on one menuitem. So both themes and backgrounds have "Wear Theme" / "Stop Wearing Theme". IMO, this works fine, as currently there are no other differences between themes and backgrounds.
* "Stop Wearing Theme" is never visible, since currently you can't explicitly disable a theme. But I gather this is part of whats being carried over from the patches in bug 520124.
* The ordering doesn't follow any of the comments in this bug so far. Mostly because "Show More Information" needs to be at the top for consistency with all other context menus, since its the default action (because its the double click action). I worked backwards from there. If necessary, ordering can be easily changed any time in the future.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Seems the test is failing on OSX - don't know why yet.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 471427 [details] [diff] [review]
Patch v1

Looks good, just a couple of tweaks for the test.

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_bug591465.js b/toolkit/mozapps/extensions/test/browser/browser_bug591465.js

>+  open_manager(null, function(aWindow) {
>+    gManagerWindow = aWindow;
>+    gContextMenu = aWindow.document.getElementById("addonitem-popup");
>+    run_next_test();
>+  });

Make sure that this opens the extensions list otherwise it will depend on whether the previous test last opened it there.

Can you add some tests that check the context menu works from the details view too?

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

* Makes the context menu popup in the detail view (it never did, previously)
* Adds "Install" menuitem for remote addons
* Adds a bunch to the automated test, including testing in the details view


(In reply to comment #5)
> Seems the test is failing on OSX - don't know why yet.

Turns out it was fine on OSX after all - it was just the build system not updating my files.

Comment 8

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

Comment on attachment 471753 [details] [diff] [review]
Patch v2

>+    var contextMenu = document.getElementById("addonitem-popup");
>+    contextMenu.addEventListener("popupshowing", function() {
>+      var addon = gViewController.currentViewObj.getSelectedAddon();
>+      contextMenu.setAttribute("addontype", addon.type);
>+    }, false);

Gets this event listener automatically removed when we close the add-ons manager?

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #8)
> Gets this event listener automatically removed when we close the add-ons
> manager?

Yea, it gets removed automatically when the document gets unloaded.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/mozilla-central/rev/f032fd9b166b

Comment 11

[Aaron Train [:aaronmt]]

Verified fixed w/  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre

Comment 12

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

A testcase I have created also covers those state changes. All that stuff is fairly new so lets also have manual test coverage:

https://litmus.mozilla.org/show_test.cgi?id=10220

The only thing which doesn't work right now is "Stop Wearing Theme" for Personas. I have filed bug 597607 for it.