Closed Bug 68547 Opened 24 years ago Closed 24 years ago

D&D of bookmarks to the top of their folder sometimes deletes them

Categories

(SeaMonkey :: Bookmarks & History, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9

People

(Reporter: fabian, Assigned: bugs)

References

Details

(Keywords: dataloss)

Attachments

(1 file)

From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Win95; en-US; 0.8) Gecko/20010210 BuildID: 2001021020 This is BAD dataloss. In the bookmarks manager, dragging and dropping bookmarks to the top of their own folder sometimes deletes them. I have not yet found a way to reproduce this 100% but it's happened pretty often. Boris Zbarsky also could reproduce it on linux. Reproducible: Always Steps to Reproduce: 1) Launch Mozilla 2) Bookmarks|Bookmarks Manager 3) Expand Mozilla Project folder 4) Expand Developer Information subfolder 5) Drag the items in that subfolder to the top of the "Developer Information" folder (their own folder). If you do this a number of times (sometimes it takes another folder to reproduce), the folder or bookmarks disappears. Actual Results: The bookmark/folder is deleted Expected Results: successfully moved to the top of the folder Happens on linux and win at least, suspect xp
Blocks: 68550
*** Bug 69797 has been marked as a duplicate of this bug. ***
The problem seems to occur when we go through the code marked // XXX magical fix for bug # 33546: handle dropping after open container in bookmarksDD.js.
JavaScript error: uncaught exception ... NS_ERROR_ILLEGAL_VALUE ... nsIRDFContainer.InsertElementAt ... location ... bookmarksDD.js :: anonymous :: line 256 data:no Lines 255/256 of bookmarksDD.js are var dropIx = RDFC.IndexOf(rTarget); RDFC.InsertElementAt(rSource, dropAction == "after" ? ++dropIx : dropIx, true); When the error occurs dropIx is -1, i.e. rTarget is not in the RDFC container. If I add a line to set dropIx to 0 if it is -1, then the bookmark moves to the first item of the parent folder. Hope that helps.
Attached patch Proposed patch (deleted) — Splinter Review
The above patch fixes the dataloss problem, I think. I'm now seeing a problem where the bookmark disappears in the Bookmarks Manager window, but is shown as moving correctly in the Sidebar. If I close and reopen Bookmarks Manager, it's OK again. Tree display problem?
There's a band-aid patch here to fix some serious dataloss, but the underlying problem needs looking into. Gerv
The problem of the tree not refreshing seems not to occur on WinNT.
What is the effect of moving the rContainer line? I'm curious because the patch you attached is not what you describe in your previous comment.
Status: NEW → ASSIGNED
I think this is what happens: In the original code, we set ContainerItem, then set rContainer based on the value of ContainerItem, and then possibly change ContainerItem (in the bug 33546 fix section of the code). In this case rContainer does not really point to the container we are trying to put the item in, so the IndexOf fails, and then the paste fails.
Marking nsbeta1+, p1, mozilla0.9
Keywords: nsbeta1+
Priority: -- → P1
Target Milestone: --- → mozilla0.9
Richard: I'm CCing you on this because of your patch for bug 33546. Please verify that Matthew's patch to your bugfix makes it do what you intended all along. ... Matthew's patch (attachment 02 [details] [diff] [review]/22/01 14:51) is correct. It is not a band-aid patch. Richard's fix for bug 33546 was meant to handle the case where the drop target is the bottom quarter or so of a folder. In this case, the drop action variables are calculated as: dropAction = "after" dropItem = target folder containerItem = parent of container folder Normally, this would insert the dropped item after the folder at the same level as the folder. The "magical fix" changes this to: dropAction = "before" dropItem = first item of target folder containerItem = target folder if the target folder is open, so the dragged item gets stuck at the beginning of the target folder, as expected. Now, "dropItem" and "containerItem" become "rTarget" and "rContainer" like so: var rContainer = this.getResource(NODE_ID(containerItem)); var rTarget = this.getResource(NODE_ID(dropItem)); HOWEVER, there's a bug (either in the original fix to introduced later). "rContainer" is assigned before the magical fix changes the value of "containerItem" whose value is not used again. Therefore, the algorithm proceeds with the corrected values of "dropItem" and "rTarget" and the wrong value of "rContainer". Chaos ensues! (That is, the dropped bookmark is lost forever.) This patch is obviously more correct than what's there. Is it too late to keyword this with mozilla0.8.1? Note that the redraw problem (where items disappear but return when you close and reopen the bookmark manager) is a separate issue. It happens in other contexts, too, and shouldn't stop us from getting this fix in.
r=fabian tested on windows95 on multiple mozilla versions, makes sense, and comments by kevin were very useful :-)
Keywords: approval
This bug is still there in 2001041104 (win98). In sidebar D&D of a subfolder from one folder to another resulted in losing the whole subfolder.
nerp
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
aside from the aforementioned separate redraw issues this works. VERIFIED Fixed with 2001042608 builds on all platforms.
Status: RESOLVED → VERIFIED
Reposting my comments from bug 56349, as it is more appropriate to this bug. I'd hate for one to slip through the cracks... This might be caused by the redraw issues, except I don't have to close Bookmark Manager to fix it. Can I get a bug number for that bug? I either need to mark this as depending on it, or re-open this one. Win98 build 2001080603: 1)Open Bookmark Manager 2)Create a Folder 3)Create or Move 3 bookmarks into Folder 4)Drag and drop second bookmark onto gap between Folder and first bookmark (after it highlights) Actual Results: Bookmark disappears until Folder is closed, then reopened. Expected Results: Bookmark moves to first position within Folder, no refresh needed.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: