358149 - "Expand" item on context menu for bookmark groups has no function

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Krang]

When you right-click a bookmark group, its context menu contains the item "Expand."  This is useful for the Bookmark Manager and Sidebar, but when you select it anywhere else (for example, on either the Personal Toolbar or the Bookmarks menu), nothing happens, and this error shows up in the Error Console:

Error: target has no properties
Source File: chrome://communicator/content/bookmarks/bookmarksMenu.js
Line: 257

This bug was tested using SeaMonkey 1.0.5 and SeaMonkey 1.1 nightlies.

Comment 1

[Krang]

Attachment: Patch to remove "Expand" item

This patch removes the "Expand" item for bookmark groups from context menus generated from anywhere that doesn't use a tree view.  Ideally, it would be nice to generate a context menu containing the bookmarks within the group instead, but that would probably be unnecessary, and I'm not sure if it would even be possible.

Comment 2

[[e-mail]]

Comment on attachment 243577 [details] [diff] [review]
Patch to remove "Expand" item

I don't like the way this works based on a technical difference between navigatorOverlay.xul and bookmarksTree.xml

Comment 3

[Krang]

Attachment: Patch to remove "Expand" item v2

Here is an updated patch.  It uses the ID to distinguish between the two types rather than the tagName.

Comment 4

[[e-mail]]

Comment on attachment 243826 [details] [diff] [review]
Patch to remove "Expand" item v2

Sorry for the delay. I couldn't find anyone to care about the bookmarks context menus to get an opinion as to whether the Expand item should appear at all, since these days the context menu is also uselessly available on submenus.

I'm also undecided as to the best way to remove the Expand item. If you use the (hidden and possibly unworking) dynamic system bookmarks preference, then the system bookmarks folder only has the Expand and Delete options. You would have to be careful to remove the separator as well as the Expand option.

Comment 5

[Krang]

Attachment: Patch to remove "Expand" item v3

Here is a new patch that uses a different (and more flexible) approach to remove the Expand item.

(In reply to comment #4)
> Sorry for the delay. I couldn't find anyone to care about the bookmarks context
> menus to get an opinion as to whether the Expand item should appear at all,
> since these days the context menu is also uselessly available on submenus.

You're right, Expand doesn't have much of a use for submenus.  I removed it in this updated patch.

> I'm also undecided as to the best way to remove the Expand item. If you use the
> (hidden and possibly unworking) dynamic system bookmarks preference, then the
> system bookmarks folder only has the Expand and Delete options. You would have
> to be careful to remove the separator as well as the Expand option.

System bookmarks didn't pull up the correct context menus when I tested them (in fact, they didn't really work at all other than displaying the correct files), but I removed Expand and the separator from their menus in case they are fixed in the future.

Comment 6

[[e-mail]]

Comment on attachment 245369 [details] [diff] [review]
Patch to remove "Expand" item v3

This is a nice idea but I think it would be better to test for the popup inside each of the switch cases instead of tweaking the array afterwards.

Comment 7

[Krang]

Attachment: Patch to remove "Expand" item v4

(In reply to comment #6)
> This is a nice idea but I think it would be better to test for the popup inside
> each of the switch cases instead of tweaking the array afterwards.

Fixed in this updated patch.

Comment 8

[[e-mail]]

I was just going to say this looks good, when I got confused. When looking at the context menu for a groupmark in the bookmark manager, there are *three* entries before the separator: open, open in new window and expand. But when looking at the context menu in the personal toolbar, there is only open in new window. So something else must already be removing the "open" option...

Comment 9

[Krang]

(In reply to comment #8)
> I was just going to say this looks good, when I got confused. When looking at
> the context menu for a groupmark in the bookmark manager, there are *three*
> entries before the separator: open, open in new window and expand. But when
> looking at the context menu in the personal toolbar, there is only open in new
> window. So something else must already be removing the "open" option...

Hmm, I'm not sure what I'm doing differently, but I still see all three of those items on the context menus for groupmarks on the personal toolbar (tested using the latest trunk and branch builds on both Windows and Linux).  After applying the patch, the "Expand" item is removed, leaving "Open" and "Open in New Window".

Comment 10

[[e-mail]]

Comment on attachment 245528 [details] [diff] [review]
Patch to remove "Expand" item v4

OK, I found it was a conflict with an extension that makes it possible to select individual bookmarks from folder groups on the personal toolbar.

Comment 11

[[e-mail]]

Actually one final possibility occurs to me: you could pass an initial list of permitted common commands into createContextMenu, which would then act as an additional filter (in addition to the existing findCommonNodes filter).

Comment 12

[Krang]

Attachment: Patch to remove "Expand" item v5

(In reply to comment #11)
> Actually one final possibility occurs to me: you could pass an initial list of
> permitted common commands into createContextMenu, which would then act as an
> additional filter (in addition to the existing findCommonNodes filter).

Here is a new patch that uses this method.  Not obsoleting the previous one yet in case this new one doesn't work out.

Comment 13

[[e-mail]]

Comment on attachment 245683 [details] [diff] [review]
Patch to remove "Expand" item v5

I'm thinking that the caller should pass in the default value for commonCommands (assuming that's possible, which I thought it was).

Comment 14

[Krang]

Attachment: Patch to remove "Expand" item v5.1

(In reply to comment #13)
> I'm thinking that the caller should pass in the default value for
> commonCommands (assuming that's possible, which I thought it was).

Fixed in this patch.

Comment 15

[[e-mail]]

Comment on attachment 245761 [details] [diff] [review]
Patch to remove "Expand" item v5.1

But I think line 195 is no longer necessary.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment 15 makes it sound like the patch needs to be updated, I'd rather let you do that than risk doing it incorrectly when I land it.

Comment 17

[Krang]

Attachment: Patch to remove "Expand" item v5.2

(In reply to comment #16)
> Comment 15 makes it sound like the patch needs to be updated, I'd rather let
> you do that than risk doing it incorrectly when I land it.

No problem.  I had better re-request r+ also just in case.

Comment 18

[[e-mail]]

Comment on attachment 246305 [details] [diff] [review]
Patch to remove "Expand" item v5.2

>       if (!commonCommands.length) commonCommands = commands;
Sorry, my line numbers must have been off, this was the line that I meant.

Comment 19

[Krang]

Attachment: Patch to remove "Expand" item v5.3

Thanks, I should have seen that earlier.  Carrying over r+ from v5.1.

Comment 20

[[e-mail]]

Comment on attachment 246330 [details] [diff] [review]
Patch to remove "Expand" item v5.3

Didn't bookmark submenu context menus land on the branch? Then I guess we need this too.

Comment 21

[Robert Kaiser]

Comment on attachment 246330 [details] [diff] [review]
Patch to remove "Expand" item v5.3

a=me for SeaMonkey 1.1