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

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Stefan [:stefanh]]

The "Empty" menuitem is hidden with css - this doesn't work in mac native menus.

Comment 1

[Stefan [:stefanh]]

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.

Comment 2

[Stefan [:stefanh]]

Attachment: needs testing

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.

Comment 3

[Ian Neal]

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.

Comment 4

[Stefan [:stefanh]]

Hmm, maybe I should keep the css.

Comment 5

[Stefan [:stefanh]]

Attachment: Keep class name and stop the event bubbling

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.

Comment 6

[[e-mail]]

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.

Comment 7

[Stefan [:stefanh]]

(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 8

[Ian Neal]

Comment on attachment 412414 [details] [diff] [review]
Keep class name and stop the event bubbling

Works fine on linux r=me

Comment 9

[Karsten Düsterloh]

Comment on attachment 412414 [details] [diff] [review]
Keep class name and stop the event bubbling

r=me for the Mac side.

Comment 10

[Stefan [:stefanh]]

http://hg.mozilla.org/comm-central/rev/ee7d93c37a69

Comment 11

[Stefan [:stefanh]]

Attachment: What I landed

I think we need this for 2.0.1 - it's a safe fix that fixes a highly visible problem on mac.

Comment 12

[Stefan [:stefanh]]

http://hg.mozilla.org/releases/comm-1.9.1/rev/1f90b2bf7dc7