596811 - "Show more information" in the context menu of add-ons shouldn't be displayed in details view

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[c221752]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100915 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100915 Firefox/4.0b7pre

 

Reproducible: Always

Steps to Reproduce:
1. Open add on manager.
2. Right click on any extension on choose "Show more information".
3. You're seeing this information now. Now right click again inside the window. 
Actual Results:  
The context menu offers you "Show more information" again.

Expected Results:  
Shouldn't see this.

Comment 1

[Bobby Johnson [:rjohnson19]]

Attachment: proposed patch

This hides the details menu item when you are in a details view.

I also hide the menuseparator for details of themes, because with an active default theme, the only menu item visible is About.

Tested on Windows with TEST_PATH=toolkit/mozapps/extensions/test/browser/ make mochitest-browser-chrome

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 476643 [details] [diff] [review]
proposed patch

>+      if (gViewController.currentViewObj == gDetailView) {
>+        contextMenu.setAttribute("view", "detail");
>+      } else {
>+        contextMenu.setAttribute("view", "list");
>+      }

This works, but it should be fixing the underlying cause - which is that the cmd_showItemDetails command is enabled when it shouldn't be.
See http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#597 

The isEnabled() function there should be returning false if the current view is the details pane. That will cause the menuitem to automatically be disabled, and the existing CSS rules will hide it.

>+      is_element_hidden(gManagerWindow.document.getElementById("menuitem_showDetails"), "'Show More Information' should be hidden in details view");

Move this extra check into the check_contextmenu() function, so that its checked every time (ie, so that it also checks that the menuitem is visible when in list view).

(In reply to comment #1)
> I also hide the menuseparator for details of themes, because with an active
> default theme, the only menu item visible is About.

The trouble is that it also hides the separator for all themes, where it actually makes sense to show it (because they'll have the usual menuitems shown). Could you file a separate bug for that, and fix it there?
As a side-note, that menuseperator could probably use an "id" attribute (if someone ever adds additional separators, you don't want to be hiding those additional ones too).

Comment 3

[Bobby Johnson [:rjohnson19]]

Attachment: revised patch

Revised patch based on comment 2.

Filed bug 597983 for the menu separator issue.

Comment 4

[aicitman]

I don't think this should be removed. It should just change into a button that brings you back to the list.

Comment 5

[Bobby Johnson [:rjohnson19]]

(In reply to comment #4)
> I don't think this should be removed. It should just change into a button that
> brings you back to the list.

Sounds like you want a new menu item for returning to the list view, which is entirely separate from this bug.

I'd suggest you file an enhancement request.

Comment 6

[aicitman]

(In reply to comment #5)
> (In reply to comment #4)
> > I don't think this should be removed. It should just change into a button that
> > brings you back to the list.
> 
> Sounds like you want a new menu item for returning to the list view, which is
> entirely separate from this bug.
> 
> I'd suggest you file an enhancement request.

filed bug 598515

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 476765 [details] [diff] [review]
revised patch

Great - looks good!

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

And my apologies for the huge lag in getting that reviewed - I was out sick for a week.

Comment 9

[Dave Townsend [:mossop]]

Fixed, thanks for the patch: http://hg.mozilla.org/mozilla-central/rev/9c6b838ccef7

Comment 10

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

Verified fixed with builds on OS X and Windows like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101008 Firefox/4.0b8pre

Dave, do we have automated tests for the details pane which check the presence of context menu entries? The litmus part I will cover in the general test for the details pane.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> Verified fixed with builds on OS X and Windows like Mozilla/5.0 (Macintosh;
> Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101008 Firefox/4.0b8pre
> 
> Dave, do we have automated tests for the details pane which check the presence
> of context menu entries? The litmus part I will cover in the general test for
> the details pane.

Blair? I thought we did?

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yes, browser_bug591465.js tests all the context menu items (both list view and details view), and the patch here added to it.