Last Comment Bug 525926 - [MacOSX] Sub-menus in native Bookmarks menu have "Empty" menuitem
: [MacOSX] Sub-menus in native Bookmarks menu have "Empty" menuitem
Status: RESOLVED FIXED
: fixed-seamonkey2.0.1, pp, regression
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Stefan [:stefanh]
:
Mentors:
Depends on:
Blocks: 513275
  Show dependency treegraph
 
Reported: 2009-11-02 10:59 PST by Stefan [:stefanh]
Modified: 2009-11-29 02:36 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
needs testing (1.23 KB, patch)
2009-11-07 17:10 PST, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
Keep class name and stop the event bubbling (1.04 KB, patch)
2009-11-14 12:13 PST, Stefan [:stefanh]
mnyromyr: review+
iann_bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review
What I landed (1.16 KB, patch)
2009-11-28 04:47 PST, Stefan [:stefanh]
iann_bugzilla: approval‑seamonkey2.0.1+
Details | Diff | Splinter Review

Description Stefan [:stefanh] 2009-11-02 10:59:31 PST
The "Empty" menuitem is hidden with css - this doesn't work in mac native menus.
Comment 1 Stefan [:stefanh] 2009-11-07 08:45:24 PST
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] 2009-11-07 17:10:02 PST
Created attachment 411011 [details] [diff] [review]
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 2009-11-10 16:01:27 PST
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] 2009-11-11 03:00:07 PST
Hmm, maybe I should keep the css.
Comment 5 Stefan [:stefanh] 2009-11-14 12:13:20 PST
Created attachment 412414 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-11-14 15:41:59 PST
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] 2009-11-14 15:50:51 PST
(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 2009-11-17 16:24:09 PST
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 2009-11-27 16:44:49 PST
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] 2009-11-28 04:45:19 PST
http://hg.mozilla.org/comm-central/rev/ee7d93c37a69
Comment 11 Stefan [:stefanh] 2009-11-28 04:47:10 PST
Created attachment 414912 [details] [diff] [review]
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.

Note You need to log in before you can comment on or make changes to this bug.