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

RESOLVED FIXED in seamonkey1.1final

Status

SeaMonkey
Bookmarks & History
--
minor
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Krang, Assigned: Krang)

Tracking

({fixed-seamonkey1.1, useless-UI})

1.8 Branch
seamonkey1.1final
x86
Windows XP
fixed-seamonkey1.1, useless-UI

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 243577 [details] [diff] [review]
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.
Attachment #243577 - Flags: review?(neil)

Comment 2

12 years ago
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
Attachment #243577 - Flags: review?(neil) → review-
(Assignee)

Comment 3

12 years ago
Created attachment 243826 [details] [diff] [review]
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.
Attachment #243577 - Attachment is obsolete: true
Attachment #243826 - Flags: review?(neil)

Comment 4

12 years ago
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.
Attachment #243826 - Flags: review?(neil) → review-
(Assignee)

Comment 5

12 years ago
Created attachment 245369 [details] [diff] [review]
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.
Attachment #243826 - Attachment is obsolete: true
Attachment #245369 - Flags: review?(neil)

Comment 6

12 years ago
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.
Attachment #245369 - Flags: review?(neil) → review-
(Assignee)

Comment 7

12 years ago
Created attachment 245528 [details] [diff] [review]
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.
Attachment #245369 - Attachment is obsolete: true
Attachment #245528 - Flags: review?(neil)

Comment 8

12 years ago
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...
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Target Milestone: seamonkey1.1beta → seamonkey1.1final
(Assignee)

Comment 9

12 years ago
(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

12 years ago
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.
Attachment #245528 - Flags: review?(neil) → review+

Comment 11

12 years ago
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).
(Assignee)

Comment 12

12 years ago
Created attachment 245683 [details] [diff] [review]
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.
Attachment #245683 - Flags: review?(neil)

Comment 13

12 years ago
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).
Attachment #245683 - Flags: review?(neil) → review-
(Assignee)

Comment 14

12 years ago
Created attachment 245761 [details] [diff] [review]
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.
Attachment #245683 - Attachment is obsolete: true
Attachment #245761 - Flags: review?(neil)

Comment 15

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

But I think line 195 is no longer necessary.
Attachment #245761 - Flags: review?(neil) → review+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed] - see comment #15
(Assignee)

Updated

12 years ago
Attachment #245528 - Attachment is obsolete: true
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.
Whiteboard: [checkin needed] - see comment #15
(Assignee)

Comment 17

12 years ago
Created attachment 246305 [details] [diff] [review]
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.
Attachment #245761 - Attachment is obsolete: true
Attachment #246305 - Flags: review?(neil)

Comment 18

12 years ago
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.
Attachment #246305 - Flags: review?(neil) → review-
(Assignee)

Comment 19

12 years ago
Created attachment 246330 [details] [diff] [review]
Patch to remove "Expand" item v5.3

Thanks, I should have seen that earlier.  Carrying over r+ from v5.1.
Attachment #246305 - Attachment is obsolete: true
Attachment #246330 - Flags: superreview?(neil)
Attachment #246330 - Flags: review+

Updated

12 years ago
Attachment #246330 - Flags: superreview?(neil) → superreview+

Comment 20

12 years ago
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.
Attachment #246330 - Flags: approval-seamonkey1.1?

Comment 21

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

a=me for SeaMonkey 1.1
Attachment #246330 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed-seamonkey1.1
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Updated

9 years ago
Duplicate of this bug: 86649
You need to log in before you can comment on or make changes to this bug.