286115 - Display a separator in the bookmark bar

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, enhancement)

Description

[J.D. Myers]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050313 Camino/0.8+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050313 Camino/0.8+

When managing bookmarks, I've added separators to the bookmark bar, but these do
not show up in the actual bookmark bar of the browser. If you do this in
Firefox, you get a little vertical separator in the bookmark bar where you've
added them. It would be nice to have this functionality in Camino.

Reproducible: Always

Steps to Reproduce:
1.Show All Bookmarks
2.Select Bookmark Bar
3.Add a separator

Actual Results:  
Separator is seen in bookmark view within the bookmark bar, but not shown on
actuall bookmark bar on the browser window.

Expected Results:  
Displayed a vertical separator on the bookmark bar of the browser.

Comment 1

[Jasper]

Yeah I agree that this would be very nice to have. I like the way FF does this.
This would be a nice polish for 1.0.

Comment 2

[Jasper]

Attachment: 16px icon

Attching the 16px dotted icon. It uses the same size pixels and color as a
regular  toolbar separator. Make sure the spacing looks good.

Comment 3

[Bruce Davidson]

Taking this bug. I've got a basic implementation working using Jasper's icon.
When you drag and drop it however it displays the icon and the text "<Menu
Separator>", which is less than ideal. Trying to fix this behaviour.

Comment 4

[Bruce Davidson]

Attachment: Patch

This patch displays bookmark separators in the bookmark toolbar. I haven't
worked out how to get round the drag showing "<Menu Spacer>" yet.

To make this patch work you will need to save Jasper's icon (as
bm_vseparator.tif - note slight change in filename to make the vertical
orientation clear) to (your camino dir)/mozilla/camino/resources/images/chrome
and add it to BrowserWindow.nib.

Comment 5

[Samuel Sidler (old account; do not CC)]

Wevah, care to review this patch? ;)

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 182941 [details] [diff] [review]
Patch

looks good to me

Comment 7

[Wevah (Nate Weaver)]

Comment on attachment 182941 [details] [diff] [review]
Patch

Me too. Though, does the icon need to be 16 px wide to work with our custom
button? Doesn't matter to me, though, as long as it looks good. ;)

Comment 8

[Wevah (Nate Weaver)]

I just checked; it can be any width we want (even 100px, if we were completely
insane, or something). However, the custom button cell adds an extra 3px of
space to the right of the image, as if there was still text there, so I've found
that an icon 4px wide, with the dots on the far right actually looks the best
(no, I am not kidding).

Comment 9

[Wevah (Nate Weaver)]

Oh, we should also modify the code for the contextual menu; "Open in New Tab"
and "Open in New Window" don't make much sense for a separator. We might also
want to make the separator unclickable, or at least not highlight when clicked.

Comment 10

[Mike Pinkerton (not reading bugmail)]

wevah, agreed on both counts. should we make a new patch?

Comment 11

[Bruce Davidson]

Attachment: Revised patch

This patch disables the "Open in New Tab" and "Open in New Window" menu items
in the context menu for a bookmark separator.

I don't quite understand the comment about making separators unclickable. On my
build nothing happens when I [left] click a separator in the bookmark bar. Any
more details available?

Comment 12

[Bruce Davidson]

Attachment: Revised patch

Revised patch (suggested by Ludo) to only enable the delete action in the
context menu for a separator.

Comment 13

[Simon Fraser [no longer active]]

Self ping to check in.

Comment 14

[Jasper]

Attachment: 4/16px icon

new 4/16px icon per comment 8

Comment 15

[Simon Fraser [no longer active]]

Landed, branch and trunk. Thanks!

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 191823 [details] [diff] [review]
Revised patch

Clearing obsolete review request.