Closed Bug 597983 Opened 10 years ago Closed 10 years ago

Menu separator should be hidden when there is only one item in the context menu

Categories

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

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla2.0b7

People

(Reporter: rjohnson19, Assigned: rjohnson19)

References

Details

Attachments

(1 file, 1 obsolete file)

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

After bug 596811 is fixed, in the details view of an active full theme, the only menu item left will be About.

This means an extraneous menu separator appears above it, and it should be hidden in this case.

Reproducible: Always

Steps to Reproduce:
1. Right click in the details area of an active theme (not persona)
Actual Results:  
After bug 596811 is resolved, the only menu item showing will be About with a menu separator above it.

Expected Results:  
No menu separator should be shown.
Per bug 596811 comment 2, also should give the menuseparator an id so we can hide just it and not any other menuseparators.
Assignee: nobody → rjohnson19
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Version: unspecified → Trunk
Attached patch patch (obsolete) — Splinter Review
This hides the menu separator by counting the number of menu items with enabled commands.
Attachment #477013 - Flags: review?(bmcbride)
Comment on attachment 477013 [details] [diff] [review]
patch

>+        if (contextMenu.children[i].nodeName == "menuitem" && gViewController.isCommandEnabled(contextMenu.children[i].command)) {

Wrap this line, so it's no longer than 80 characters.

Code looks good, but I think it would benefit from an automated test. It should fit nicely into browser_bug591465.js (like you did with bug 596811). I think you should be able to just set gProvider.addons[x].permissions = 0; to get all but the "About" menuitem hidden.
Attachment #477013 - Flags: review?(bmcbride) → review-
Attached patch patch v2Splinter Review
I made this patch on top of the patch for 596811, and added testing in essentially the same way, adding a new test to get the single menu item case covered.
Attachment #477013 - Attachment is obsolete: true
Attachment #479221 - Flags: review?(bmcbride)
Depends on: 596811
Comment on attachment 479221 [details] [diff] [review]
patch v2

Great - thanks!
Attachment #479221 - Flags: review?(bmcbride) → review+
Whiteboard: [has patch][checkin-needed-post-b7]
Comment on attachment 479221 [details] [diff] [review]
patch v2

Approved to land after b7
Attachment #479221 - Flags: approval2.0? → approval2.0+
Fixed, thanks for the patch: http://hg.mozilla.org/mozilla-central/rev/4e480cb2362f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][checkin-needed-post-b7]
Target Milestone: --- → mozilla2.0b8
Verified fixed with builds on Windows and OS X like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101008 Firefox/4.0b8pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Flags: in-litmus-
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.