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

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: whimboo, Assigned: Unfocused)

Tracking

Trunk
mozilla2.0b7
Points:
---
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [strings][AOMTestday][in-litmus-bug-week])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
blocking2.0: ? → beta6+
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
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
Assignee: nobody → bmcbride
(Reporter)

Comment 3

7 years ago
(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
Whiteboard: [strings]
Created attachment 471427 [details] [diff] [review]
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.
Attachment #471427 - Flags: review?(dtownsend)
Seems the test is failing on OSX - don't know why yet.
Status: NEW → ASSIGNED
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?
Attachment #471427 - Flags: review?(dtownsend) → review-
Created attachment 471753 [details] [diff] [review]
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.
Attachment #471427 - Attachment is obsolete: true
Attachment #471753 - Flags: review?(dtownsend)
(Reporter)

Comment 8

7 years ago
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?
(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.
Attachment #471753 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/f032fd9b166b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: uiwanted
Resolution: --- → FIXED
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla2.0b6
Verified fixed w/  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre
Status: RESOLVED → VERIFIED
(Reporter)

Updated

7 years ago
Depends on: 597607
(Reporter)

Comment 12

7 years ago
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.
Flags: in-litmus- → in-litmus+
Whiteboard: [strings] → [strings][AOMTestday][in-litmus-bug-week]
You need to log in before you can comment on or make changes to this bug.