Drag and drop in Bookmarks Sidebar allows bookmarks to be placed in top level ("All Bookmarks") folder

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P2
normal
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: abillings, Assigned: dietrich)

Tracking

Trunk
Firefox 3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
If a user opens the bookmarks sidebar, they can drag and drop a bookmark into the top level folder. This places the bookmark at the same level as the "Bookmarks Toolbar", "Bookmarks Menu", and "Unfiled Bookmarks" folders. Users should not be able to do this.

Note: In the Bookmarks Organizer, this drag and drop action will fail. It only works in sidebar.

Found in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007112004 Minefield/3.0b2pre
Still happen with latest nightly builds.
Flags: blocking-firefox3?
Why is that bad? The bookmarks menu is longer the "top lever" folder in there.
Mano i agree with Henrik, imho we shouldn't allow users to put folders into All Bookmarks, what's the use for that? All bookmarks should be the root to separate Bookmarks by type, and we could in future add other meta-folders like smart bookmarks or livemark collection...

For archiving bookmarks we already have unfiled folder. And i'm not sure that bookmarks under all bookmarks are exported / backup at all
Mano as Marco already said, we have a nice and clean way of storing folders/bookmarks in given meta-folders. One for the Toolbar, the Bookmarks menu and Unfiled Bookmarks. If you allow storing bookmarks under All Bookmarks it will be (IMO) nontransparent for the user and the database will be scattered. All folders users need we already have.
(Assignee)

Comment 5

10 years ago
I think that if we allow it to be writable, "All Bookmarks" will become yet another "garbage dump" (as Faaborg has referred to the bookmarks menu in Fx2).
Are there potential problems interacting with other portions of the bookmarking code (in terms of searching for or otherwise referencing this as a location for bookmarks) or API here?

Blocking for now, since I agree with comment 5.

I think a nicer way to do this would be for us to redirect drags that go to the top level to end up in the "Unfiled Bookmarks" folder.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Blocks: 417228
related bug 415125

Comment 8

10 years ago
fyi - it's also possible to create a new sub-folder directly inside of the all bookmarks folder. It can't be done from the tree view but it can be done by selecting all bookmarks and then right-clicking in the right-pane to choose new folder.

Personally I like having this option available, however, I thought I should point that out somewhere.

Updated

10 years ago
Priority: P3 → P4

Comment 9

10 years ago
It would be useful to have an "Bookmark archive", where bookmarks are not shown in any toolbar and/or menu (this is different than unfiled bookmarks, which is for new bookmarks)
(Assignee)

Comment 10

10 years ago
All Bookmarks should be read-only.
Assignee: nobody → dietrich
Priority: P4 → P2
Target Milestone: --- → Firefox 3 beta5
(Assignee)

Comment 11

10 years ago
Created attachment 310129 [details] [diff] [review]
fix

make All Bookmarks read-only.
Attachment #310129 - Flags: review?(mano)
Comment on attachment 310129 [details] [diff] [review]
fix

r=mano
Attachment #310129 - Flags: review?(mano) → review+
(Assignee)

Comment 13

10 years ago
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.127; previous revision: 1.126
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

10 years ago
(In reply to comment #8)
> fyi - it's also possible to create a new sub-folder directly inside of the all
> bookmarks folder. It can't be done from the tree view but it can be done by
> selecting all bookmarks and then right-clicking in the right-pane to choose new
> folder.
> 
> Personally I like having this option available, however, I thought I should
> point that out somewhere.
> 

Yes, I agree it'd be nice to have a more customizable default view. However, that can't happen until we find a way to do that without allowing the "special" folders to get mucked up. Safest for now to resolve as we did in this bug. We can work in Fx3.next to get this more flexible.
This is not fixed. I run a test with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko Minefield/3.0b5pre ID:2008031923 and I'm still able to place bookmarks directly under "All Bookmarks".

Steps I did:
1. Open "Unfiled Bookmarks"
2. D&D a bookmark somewhere under the last bookmark (empty area)
=> Bookmark is placed at top level
3. D&D a bookmark between "Bookmarks Toolbar" and "Bookmarks Menu"
=> Bookmark is placed at top level
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED

Updated

10 years ago
Flags: in-litmus?
Target Milestone: Firefox 3 beta5 → Firefox 3
Summary: Drag and drop in Bookmarks Sidebar allows bookmarks to be place in top level ("/") folder above bookmarks menu → Drag and drop in Bookmarks Sidebar allows bookmarks to be placed in top level ("All Bookmarks") folder
(Assignee)

Comment 16

10 years ago
Did you try with a new or a pre-existing profile? This fix affects new profiles only. 
Ok, than this is working with Mozilla/5.0 (Macintosh; U; Intel Mac OS X
10.4; en-US; rv:1.9b5pre) Gecko Minefield/3.0b5pre ID:2008031923.

Lately there were a lot of checkins which only works with a new profile or places.sqlite. All testers already running a beta version will be affected even with a final release version of Firefox 3. All these will probably produce a lot of bug reports in the future.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 → Firefox 3 beta5
Status: RESOLVED → VERIFIED
(In reply to comment #16)
> Did you try with a new or a pre-existing profile? This fix affects new profiles
> only. 

David, should such things be added to SUMO? I think it would be helpful. At least it should be shown as notes for beta testers, and what they have to do to get places working properly.

Comment 19

10 years ago
I'm not sure if it's needed, as the only way to move a bookmark to the top folder is through the sidebar, and the bookmark doesn't disappear after the drop. Not sure how we would frame this. "Some bookmarks only accessible via the sidebar," with the solution of creating a new profile?
(Assignee)

Updated

10 years ago
Depends on: 425141
(In reply to comment #19)
> drop. Not sure how we would frame this. "Some bookmarks only accessible via the
> sidebar," with the solution of creating a new profile?

After talking with David on IRC there was made the decision to create a new page with information for beta users. All bugs which were fixed and require a fresh places.sqlite should be taken into account.
Depends on: 425161
Blocks: 426340
(Assignee)

Comment 21

10 years ago
Backed out in bug 425141.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 3 beta5 → Firefox 3
(Assignee)

Updated

10 years ago
Blocks: 426081
(Assignee)

Comment 22

10 years ago
(In reply to comment #9)
> It would be useful to have an "Bookmark archive", where bookmarks are not shown
> in any toolbar and/or menu (this is different than unfiled bookmarks, which is
> for new bookmarks)
> 

I think the workflow will be different depending on the user. I use Unsorted Bookmarks for this purpose.

You could easily make an extension to do this.
(Assignee)

Comment 23

10 years ago
Created attachment 313635 [details] [diff] [review]
fix v2

safer, saner.
Attachment #310129 - Attachment is obsolete: true
Attachment #313635 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano]

Comment 24

10 years ago
(In reply to comment #22)
> (In reply to comment #9)
> > It would be useful to have an "Bookmark archive", where bookmarks are not shown
> > in any toolbar and/or menu (this is different than unfiled bookmarks, which is
> > for new bookmarks)
> > 
> 
> I think the workflow will be different depending on the user. I use Unsorted
> Bookmarks for this purpose.

Is there negation in queries? If so, tacking on TAG=/!="archive" (or whatever) would seem a fairly intuitive, complete solution, without needing extra top-level folders. Save a few queries, as they reveal their usefulness, and Bob's your uncle, as a friend of mine delights in saying. User experience/interface awaits some attention ATM, o'course.

Sorry for any excessive off-topicality.
(Assignee)

Comment 25

10 years ago
Created attachment 313899 [details] [diff] [review]
wip

read-only nodes handled properly with this patch.

however, on drag-over, child-nodes of read-only nodes are not properly highlighted as drop-targets.
Attachment #313635 - Attachment is obsolete: true
Attachment #313635 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano] → [has wip patch]

Updated

10 years ago
Duplicate of this bug: 426081

Comment 27

10 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

I tried to re-organize my bookmarks by dragging and dropping two folders that appeared mixed in among accumulated bookmarks in the "Bookmarks Menu" folder and they ended up inserted as sub-folders of one of my existing folders already defined under the "Bookmarks Menu".  It took me a couple of days and reading some of this bug report to figure out where the folders had disappeared to.  This is very annoying.
(Assignee)

Comment 28

10 years ago
Created attachment 316619 [details] [diff] [review]
fix

ok, to sum up the changes:

- make All Bookmarks read-only
- use the correct drop target row in the canDrop chain (tree -> treeView -> draghelper)
- always call canDrop in onDragOver
- fix the treeview canDrop to allow dropping *on* non-read-only child containers of read-only containers (eg: the toolbar shortcut in All Bookmarks)
Attachment #313899 - Attachment is obsolete: true
Attachment #316619 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [has wip patch] → [has patch][needs review mano]
Comment on attachment 316619 [details] [diff] [review]
fix

r=mano
Attachment #316619 - Flags: review?(mano)
Attachment #316619 - Flags: review+
Attachment #316619 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano] → [has patch][needs approval]
Comment on attachment 316619 [details] [diff] [review]
fix

a1.9=beltzner
Attachment #316619 - Flags: approval1.9? → approval1.9+
i think you should bump up the left pane version since that patch has already been checked in (and we have users with not readonly AllBookmarks folder). unless you're not about doing other changes to those folders in another blocker.
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs approval] → [has patch]
(Assignee)

Comment 32

10 years ago
Created attachment 317056 [details] [diff] [review]
fix w/ bumped left pane version number

carrying over r+a
Attachment #316619 - Attachment is obsolete: true
Attachment #317056 - Flags: review+

Updated

10 years ago
Whiteboard: [has patch] → [has patch][has reviews][has approval]
(Assignee)

Comment 33

10 years ago
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.113; previous revision: 1.112
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.66; previous revision: 1.65
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.138; previous revision: 1.137
done
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][has approval]
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre)
Gecko/2008042408 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Test case https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=6080 has been updated to verify this test case for regression testing.
Flags: in-litmus? → in-litmus+
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.