Closed Bug 544047 Opened 15 years ago Closed 15 years ago

Places drop indicators are not dismissed on drop (revise code to get rid of nsDragAndDrop in Places)

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3.7a2

People

(Reporter: alice0775, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

patch v1.1
(deleted), patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2pre) Gecko/20100203 Firefox/3.6.2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100202 Minefield/3.7a1pre ID:20100202053418 After drop a link onto Bookmarks menu dropdown, menupopup-drop-indicator-bar is still present after closing and reopening the menu. After drop a link onto Bookmarks toolbar,toolbar-drop-indicator is still present. See https://bugzilla.mozilla.org/show_bug.cgi?id=541520#c21 Reproducible: Always Steps to Reproduce: 1.Drag a link onto Bookmarks menupoup 2. 3. OR 1.Drag a link onto Bookmarks toolbar 2. 3. Actual Results: drop-indicator-bar/toolbar-drop-indicator is still present. Expected Results: drop-indicator-bar/toolbar-drop-indicator should disappear.
regression window: works: http://hg.mozilla.org/mozilla-central/rev/f57b95afb57e Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100201 Minefield/3.7a1pre ID:20100201043011 broken: http://hg.mozilla.org/mozilla-central/rev/6e3003aeea75 Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100201 Minefield/3.7a1pre ID:20100201085418 pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f57b95afb57e&tochange=6e3003aeea75 edit: candidate regress bug: Bug 541520 - dragleave fired at successful drop
Blocks: 541520
Keywords: regression
Version: unspecified → Trunk
Summary: After drop a link, menupopup-drop-indicator-bar/toolbar-drop-indicator is still present after closing and reopening. → After drop a link, menupopup-drop-indicator-bar/toolbar-drop-indicator is still present
Now that dragleave does not fire on successful drop, I suspect that the bookmarks code just needs to do what the dragleave listener does on drop as well.
confirmed, places d&d code still needs a cleanup, hope to find some time in near future.
Status: UNCONFIRMED → NEW
Ever confirmed: true
let's see if i can bring back our d&d code in the 20th century.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Ugh there are actually 2 regression here. This one causes the drop indicator to not disappear, and i've fixed it locally. The other one is making it not appear. I've reduced the regrange between 9 and 13 of Jan, still working on a better regrange.
the regrange is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=120667a01fd2&tochange=a43e2f7eda8f in this range there are a tracemonkey merge and Roc's big rewrite. Excluding the TM merge (for now) the most plausible bugs are bug 526394 or bug 529670.
Attached patch wip v0.1 (obsolete) (deleted) — — Splinter Review
attaching current patch to avoid losing work. i'm actually investigating the other regression, filed as bug 545049.
Blocks: 545119
Summary: After drop a link, menupopup-drop-indicator-bar/toolbar-drop-indicator is still present → Places drop indicators are not dismissed on drop (revise code to get rid of nsDragAndDrop in Places)
Attached patch patch v1.0 (obsolete) (deleted) — — Splinter Review
afaict, this is working pretty well. I can't tell this is perfect, we could still move helpers and make code more elegant, but it's out of the scope of this bug and i want to avoid regressions as far as i can. This is a nice first step killing all nsDragAndDrop dependencies, after some baking time on nightlies we can further improve the code.
Attachment #425913 - Attachment is obsolete: true
Attachment #425985 - Flags: review?(mano)
PS: the move of nsDragAndDrop from PlacesOverlay.xuk to browser.xul looks correct to me (since Places is no more depending on it while browser.xul depends on it), but actually does not bring real improvements, since it's just a move. So i could avoid that change and just wait to be able to get rid of the inclusion completely.
Comment on attachment 425985 [details] [diff] [review] patch v1.0 Nice work! >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul >@@ -101,6 +101,10 @@ > > <script type="application/javascript" src="chrome://browser/content/places/editBookmarkOverlay.js"/> > >+# This is still used for dragDropSecurityCheck and transferUtils. Since we rely >+# on the new Drag and Drop API these dependencies should be removed. >+<script type="application/javascript" src="chrome://global/content/nsDragAndDrop.js"/> >+ Use a normal comment here. > <!-- Sub-menus should be opened when the mouse drags over them, and closed > when the mouse drags off. The overFolder object manages opening and > closing of folders when the mouse hovers. --> >@@ -490,6 +316,17 @@ > } > })]]></field> > >+ <method name="_cleanupDragDetails"> >+ <body><![CDATA[ >+ // Called on dragend or drop. nit: and, not or. >+ if (!event.target.node) >+ return; >+ >+ let draggedNode = event.target.node; >+ >+ // Force a copy action if parent node is a query or we are dragging a >+ // not-removable node >+ // activate the view and cache the dragged node UFirst and end with period (ditto for everything else you've moved). >+ <handler event="dragstart"><![CDATA[ >+ if (event.target.localName != "treechildren") >+ return; >+ >+ var nodes = this.getSelectionNodes(); let > <handler event="dragover"><![CDATA[ >- if (event.target.localName == "treechildren") >- nsDragAndDrop.dragOver(event, this); >+ if (event.target.localName != "treechildren") >+ return; >+ >+ var row = { }, col = { }, child = { }; >+ this.treeBoxObject.getCellAt(event.clientX, event.clientY, >+ row, col, child); >+ var node = row.value != -1 ? >+ this.view.nodeForTreeIndex(row.value) : >+ this.getResultNode(); Ditto for any other code you've moved. r=mano.
Attachment #425985 - Flags: review?(mano) → review+
Attached patch patch v1.1 (deleted) — — Splinter Review
Attachment #425985 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/8610d75c2d2e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a2
Notice there is still bug 545049 that is causing the drop indicator to not appear sometimes, but it is due to some kind of layout regression.
Depends on: 555474
Blocks: 469703
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: