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

RESOLVED FIXED in Firefox 3.7a2

Status

()

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: alice0775, Assigned: mak)

Tracking

({regression})

Trunk
Firefox 3.7a2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

10 years ago
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.
Reporter

Comment 1

10 years ago
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
Reporter

Updated

10 years ago
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

Comment 2

10 years ago
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.
Assignee

Comment 3

10 years ago
confirmed, places d&d code still needs a cleanup, hope to find some time in near future.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 4

10 years ago
let's see if i can bring back our d&d code in the 20th century.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Duplicate of this bug: 544786
Assignee

Comment 6

10 years ago
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.
Assignee

Comment 7

10 years ago
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.
Assignee

Comment 8

10 years ago
Posted patch wip v0.1 (obsolete) — Splinter Review
attaching current patch to avoid losing work.
i'm actually investigating the other regression, filed as bug 545049.
Assignee

Updated

10 years ago
Blocks: 545119
Assignee

Updated

10 years ago
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)
Assignee

Comment 9

10 years ago
Posted patch patch v1.0 (obsolete) — 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)
Assignee

Comment 10

10 years ago
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.
Assignee

Updated

10 years ago
Duplicate of this bug: 545528
Duplicate of this bug: 546374
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+
Assignee

Comment 14

9 years ago
Posted patch patch v1.1Splinter Review
Attachment #425985 - Attachment is obsolete: true
Assignee

Comment 15

9 years ago
http://hg.mozilla.org/mozilla-central/rev/8610d75c2d2e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a2
Assignee

Comment 16

9 years ago
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.
Reporter

Updated

9 years ago
Depends on: 555474
Assignee

Updated

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