Status

()

For bugs in Firefox Desktop, the Mozilla Foundation's web browser. For Firefox user interface issues in menus, developer tools, bookmarks, location bar, and preferences. Many Firefox bugs will either be filed here or in the Core product. (more info)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
If the Unfiled Bookmarks folder is moved into the Bookmarks Menu folder, bookmarks in the Unfiled folder cannot be viewed via the Bookmarks Menu until the browser has been restarted.
Reproducible: Always
Steps to Reproduce:
1. Make sure "Unfiled Bookmarks" is under "All Bookmarks" (in the default place), and start the browser.
2. Use the star button to make an unfiled bookmark
3. Move the "Unfiled Bookmarks" folder into the "Bookmarks Menu" folder
Actual Results:
Try to use the bookmarks menu to view what is in the Unfiled Bookmarks folder - the Unfiled Bookmarks folder appears empty.
Expected Results:
You should be able to view what is in the "Unfiled Bookmarks" folder by using the Bookmarks menu.
Once the browser has been restarted, bookmarks appear correctly when viewing the unfiled bookmarks folder in this way, even if you add new ones, so there is no longer a problem.
The problem can be reproduced by moving the Unfiled Bookmarks folder back into "All Bookmarks", and then back in to "Bookmarks Menu" again.
If it is not moved again the problem does not seem to re-occur.

(In reply to comment #0)
> 3. Move the "Unfiled Bookmarks" folder into the "Bookmarks Menu" folder
Hrm, here's the bug. You should not be able to do this. You should be able to copy, but not move it.
I can accomplish this by clicking on All Bookmarks in the organizer, then drag-and-dropping Unfiled on the Menu folder in the right-side pane.
Drivers: Requesting blocking. The special folders should not be able to moved, only copied. And maybe not even copied, since the action would not result in what the user expected.

Hm, thanks for the reply. Glad I was able to find an actual bug!
However, may I suggest that it be made so that you can do this, as it makes things much easier if you're able to quickly get to bookmarks via the bookmarks menu which you may have quickly saved as unfiled.
The full bookmark manager is great for filing them later, but I would not want to have to keep accessing that in order to find things that I've bookmarked with the star button and just not got around to filing yet. That is why I moved it to the Bookmarks Menu folder in the first place, as it's much quicker to get to them that way.
As it can already be moved there and does work properly (if the browser is restarted after it is moved), I think it would be more beneficial to allow that to happen if that is where the user wants it. Obviously better if you don't need to restart the browser before they show up properly though.
Suggestion: Allow the special folder to be moved so it can be accessed from the bookmarks menu if the user wants it to be there, but agree that copying (not moving) is a bad idea due to the action perhaps not resulting in what was expected.

I was just about to report the same bug. Actually, the same behavior happens when moving the Unfiled Bookmarks item to the Bookmarks Toolbar. I guess that different people trying to move the Unfilled Bookmarks item means it's a feature we want, so please don't block it :)
(In reply to comment #0)
> Drivers: Requesting blocking. The special folders should not be able to moved,
> only copied. And maybe not even copied, since the action would not result in
> what the user expected.

Created attachment 313500[details][diff][review]
conservative fix
This fix disallows dragging of any of the "special" roots, tag containers and the All Bookmarks folder. It does allow dnd reordering of tag containers within the Tags folder.
The change fixes this bug, bug 426805 and bug 426846.
There are a number of useful dnd interactions possible in the Library. Until these interactions are spec'd, it's better to conservatively disable dnd of these folders. Primum non nocere!
Regarding comment #6 and #7: drag-to-shortcut for roots sounds like it might be workable, please file an enhancement bug for that.

Nice work on the fix. Was hoping that we would still be able to move it into the bookmarks menu folder, it really does make the unfiled bookmarks a lot easier to access, but since there's a number of bugs with dnd of these folders I suppose this way makes sense for now. Hopefully this will be considered for future tweaks/changes.

(In reply to comment #9)
> Nice work on the fix. Was hoping that we would still be able to move it into
> the bookmarks menu folder, it really does make the unfiled bookmarks a lot
> easier to access, but since there's a number of bugs with dnd of these folders
> I suppose this way makes sense for now. Hopefully this will be considered for
> future tweaks/changes.
>
Please file an enhancement request for this.
See bug 423617 for changing the default location of newly starred+unfiled bookmarks, which might address your concern without needing to re-arrange the special folders.

A similar problem (but with copying, not moving):
1. Create a new profile, since it is dangerous in a way.
2. Go to Library.
3. Copy “All Bookmarks”.
4. Paste it into “Unfiled Bookmarks” or another subfolder of “All Bookmarks”.
Now you have an infinite recursive tree you cannot just delete or cut.

Created attachment 314163[details][diff][review]
fix v2
There were several scenarios not covered in the previous patch. Here's a more complete fix:
- address copy
- fix for items w/o concreteId property (eg: All Bookmarks)
- disallow for all immediate children of the left pane folder

Created attachment 314249[details][diff][review]
fix v3
implementing a test for the fix we talked about over irc exposed a bug and a reproduce-able crash scenario.
the bug is that when a query result contains a folder shortcut, and that folder shortcut is deleted, the notification sends the item id, which doesn't match any in the result node (since it needs to match on the query item id).
the crash occurs if we assert when not finding the node. changing that to warn stopped the crashing. i'll file a followup to figure out why asserting there causes xpcshell to crash (stack below).
changes in the new patch:
- remove hasCopyableSelection, etc
- add support to force copying of shortcuts as folders
- fix FindChildById() to support folder shortcuts
- add a test that flexes forcing copy for shortcuts, as well as making sure we still properly handle non-folder-shortcut queries (i'll rename this on check-in)
- s/NS_NOTREACHED/NS_WARNING/ to stop the crashing during the test
ftr, i still do not like that we're allowing copy of special folders. it will come to no good.
###!!! ASSERTION: Removing item we don't have: 'node', file /Users/dietrich/moz/trunk/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 3371
nsPrintfCString::~nsPrintfCString()+0x00022485 [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libplaces.dylib +0x0005C6C3]
nsPrintfCString::~nsPrintfCString()+0x000212EE [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libplaces.dylib +0x0005B52C]
nsXPCOMCycleCollectionParticipant::nsXPCOMCycleCollectionParticipant()+0x00012B00 [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libplaces.dylib +0x00075472]
NS_InvokeByIndex_P+0x00000063 [/Users/dietrich/moz/trunk/build-dbg/xpcom/build/libxpcom_core.dylib +0x0009EFFB]
nsSupportsArray::AppendElement(nsISupports*)+0x0000540E [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libxpconnect.dylib +0x0004EAEE]
nsCRT::free(unsigned short*)+0x00004FD9 [/Users/dietrich/moz/trunk/build-dbg/dist/bin/components/libxpconnect.dylib +0x000590D9]
js_Invoke+0x00000A21 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x00077383]
JS_CompareValues+0x0000F2A4 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x0006A348]
js_Invoke+0x00001324 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x00077C86]
JS_ExecuteScript+0x00000083 [/Users/dietrich/moz/trunk/build-dbg/js/src/libmozjs.dylib +0x000172A1]
start+0x000017A0 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x00002D58]
start+0x00001AD7 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x0000308F]
start+0x00002131 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000036E9]
start+0x00003343 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000048FB]
start+0x000000FC [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000016B4]
start+0x00000029 [/Users/dietrich/moz/trunk/build-dbg/toolkit/components/places/tests/../../../../dist/bin/xpcshell +0x000015E1]

Created attachment 315573[details][diff][review]
wip patch
Updated to fix comments, as well as to address a number of scenarios not covered by the previous patch.
One problem I found is that setting the drag action is not persistent: It's set to COPY in tree.xml.startDrag(), but at any point during the drag, if you get the current drag session and dump dragAction, it's set to MOVE. I took a tour through nsDragAndDrop.js, and it's setting it properly, though I stopped tracing when I hit the OS-specific widget code.

Created attachment 316098[details][diff][review]
fix
two problems i found while fixing this that i'll file followups for:
- during drags, getting the current drag session does *not* reflect the dragAction set when the drag began
- even if cmd_cut is not enabled, and returns false, cuts via kb shortcut are still allowed (!)

if (PlacesUtils.nodeIsReadOnly(parent) ||
- PlacesUtils.nodeIsTagQuery(parent)) {
+ PlacesUtils.nodeIsTagQuery(parent) ||
+ !PlacesControllerDragHelper.canMoveContainerNode(node)) {
+ // XXX DOES NOTHING! dragAction doesn't persist
dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
break;
i'm not sure that this does nothing at all, IIRC when i put the check for nodeIsTagQuery here, it was correctly forcing a copy op, while that was a move op before.

+ // Disallow moving tag containers unless they're being moved
+ // within the Tags folder.
+ if (aParentId == PlacesUtils.tagsFolderId) {
+ if (aTargetId == -1 || aTargetId != PlacesUtils.tagsFolderId)
+ return false;
+ }
this will not work, tag containers are no more children of the tags folder, they are generated by a RESULTS_AS_TAG_QUERY query. or better, will work only for old profiles until we upgrade their left pane folder

> >+ !PlacesControllerDragHelper.canMoveContainerNode(node)) {
> >+ // XXX DOES NOTHING! dragAction doesn't persist
> > dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
>
> hrm, when and why doesn't it persist?
in the treebinding, in onDragStart(), we set the drag action to copy if the dragged node cannot be moved. however, *during* the drag, controller.canDrop() gets the drag action from the current session, which returns DRAGDROP_ACTION_MOVE. i traced it through nsDragAndDrop.js, up into mac widgets code, iirc.
> >+ var concreteId = PlacesUtils.getConcreteItemId(aPlacesNode);
> >+ if (aJSNode.id != -1 && (PlacesUtils.nodeIsQuery(aPlacesNode) ||
> >+ (concreteId != aPlacesNode.itemId && !aResolveShortcuts))) {
>
> You could simply if it's a shortcut.
>
hm, actually folder shortcuts should be removable even if they're a shortcut to a "special" folder. the only case in which they should not be removable is if the shortcut is a child of a read-only container. this a more consistent and simpler policy, resulting in less special-casing in the code. changes for this in the new patch:
- make left folder root read-only
- don't check concrete id of folder shortcuts at all in canMoveContainerNode()
- check if parent is read-only in canMoveContainerNode()

Created attachment 317764[details][diff][review]
bump left pane version
same patch, but also bumps left pane version number, so that existing users get the change to make the left pane root folder read-only.

Comment on attachment 317743[details][diff][review]
fix + tests
aTargetId is no longer used, please remove that from the function, from its header and from all of its callers.
r=mano with that fixed, plus reving the left-pane version.

(In reply to comment #20)
> Created an attachment (id=316098) [details]
> fix
>
> two problems i found while fixing this that i'll file followups for:
>
> - during drags, getting the current drag session does *not* reflect the
> dragAction set when the drag beganbug 431699>
> - even if cmd_cut is not enabled, and returns false, cuts via kb shortcut are
> still allowed (!)
> bug 431671 (thanks marco)

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".
In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body contains places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.
Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.
Gerv