Closed Bug 525926 Opened 12 years ago Closed 12 years ago

[MacOSX] Sub-menus in native Bookmarks menu have "Empty" menuitem

Categories

(SeaMonkey :: Bookmarks & History, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

(Keywords: fixed-seamonkey2.0.1, platform-parity, regression)

Attachments

(2 files, 1 obsolete file)

The "Empty" menuitem is hidden with css - this doesn't work in mac native menus.
Flags: blocking-seamonkey2.0.1?
Version: SeaMonkey 2.0 Branch → Trunk
Keywords: pp
Summary: Sub-menus in native Bookmarks menu has "Empty" menuitem → [MacOSX] Sub-menus in native Bookmarks menu have "Empty" menuitem
I think it should work if we just play with the hidden attribute when the popupshowing event occurs. It should also be enough to do this for the main menu only.
Attached patch needs testing (obsolete) — Splinter Review
This patch works for me. However, I discovered some strange behaviour:

1) Create an empty folder in the PT.
2) Open the (XUL) bm menu in the the PT and move (d & d) some bookmarks into the newly created folder
3) Open the native bm menu, look for the newly added bookmarks - they're there
4) Open the bm menu in the PT, drag & drop one bm out of the folder
5) Open the native bm menu

--> the folder still contains 2 bookmarks

I can't see how this would be related to the patch, though - need some further investigation.
I see the following with the patch applied on Linux:
1) Create an empty folder in bookmarks menu
2) Drag and drop link onto empty folder

Expected result
1) Link replaces "empty" entry

Actual result
1) Link shows below "empty" entry

When you re-show the bookmarks, "empty" entry has gone.
Hmm, maybe I should keep the css.
This should address comment #3, asking Ian to get Linux confirmation. Regarding my comment #2, I managed to reproduce this in 2.0 so it doesn't look related to my patch. I guess the native menu sometimes doesn't re-build (or something like that) properly.
Assignee: nobody → stefanh
Attachment #411011 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #412414 - Flags: superreview?(neil)
Attachment #412414 - Flags: review?(mnyromyr)
Attachment #412414 - Flags: review?(iann_bugzilla)
Comment on attachment 412414 [details] [diff] [review]
Keep class name and stop the event bubbling

You might want to restore the comment (but change "will" to "may").

>+                     onpopupshowing="this.firstChild.hidden = (this.childNodes.length > 1); event.stopPropagation();">
Nit: doesn't need ()s around the boolean expression.
Attachment #412414 - Flags: superreview?(neil) → superreview+
(In reply to comment #6)
> (From update of attachment 412414 [details] [diff] [review])
> You might want to restore the comment (but change "will" to "may").

Ah, right - will do.
> 
> >+                     onpopupshowing="this.firstChild.hidden = (this.childNodes.length > 1); event.stopPropagation();">
> Nit: doesn't need ()s around the boolean expression.
OK.
Comment on attachment 412414 [details] [diff] [review]
Keep class name and stop the event bubbling

Works fine on linux r=me
Attachment #412414 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 412414 [details] [diff] [review]
Keep class name and stop the event bubbling

r=me for the Mac side.
Attachment #412414 - Flags: review?(mnyromyr) → review+
http://hg.mozilla.org/comm-central/rev/ee7d93c37a69
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
Attached patch What I landedSplinter Review
I think we need this for 2.0.1 - it's a safe fix that fixes a highly visible problem on mac.
Attachment #414912 - Flags: approval-seamonkey2.0.1?
Attachment #414912 - Flags: approval-seamonkey2.0.1? → approval-seamonkey2.0.1+
Flags: blocking-seamonkey2.0.1?
You need to log in before you can comment on or make changes to this bug.