Closed Bug 596811 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect, minor)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: c221752, Assigned: rjohnson19)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Version: unspecified → Trunk
Summary: Shouldn't see "Show more information" in add on context menu when you're inside "more information" view → Shouldn't see "Show more information" in add on context menu when you're already inside "more information" view
Status: UNCONFIRMED → NEW
Component: Menus → Add-ons Manager
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Toolkit
QA Contact: menus → add-ons.manager
Hardware: x86 → All
Whiteboard: [good first bug]
blocking2.0: --- → final+
Attached patch proposed patch (obsolete) — Splinter Review
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
Attachment #476643 - Flags: review?(dtownsend)
Assignee: nobody → rjohnson19
Status: NEW → ASSIGNED
Summary: Shouldn't see "Show more information" in add on context menu when you're already inside "more information" view → "Show more information" in the context menu of add-ons shouldn't be displayed in details view
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).
Attachment #476643 - Flags: review?(dtownsend) → review-
Attached patch revised patchSplinter Review
Revised patch based on comment 2.

Filed bug 597983 for the menu separator issue.
Attachment #476643 - Attachment is obsolete: true
Attachment #476765 - Flags: review?(bmcbride)
I don't think this should be removed. It should just change into a button that brings you back to the list.
(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.
(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
Whiteboard: [good first bug] → [good first bug][has patch][needs review Unfocused]
Comment on attachment 476765 [details] [diff] [review]
revised patch

Great - looks good!
Attachment #476765 - Flags: review?(bmcbride) → review+
Whiteboard: [good first bug][has patch][needs review Unfocused] → [has patch][checkin-needed-post-b7]
And my apologies for the huge lag in getting that reviewed - I was out sick for a week.
Blocks: 597983
Fixed, thanks for the patch: http://hg.mozilla.org/mozilla-central/rev/9c6b838ccef7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][checkin-needed-post-b7]
Target Milestone: --- → mozilla2.0b8
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus-
(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?
Yes, browser_bug591465.js tests all the context menu items (both list view and details view), and the patch here added to it.
Flags: in-testsuite? → in-testsuite+
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.