Use the new D&D API in tabbrowser

RESOLVED FIXED in Firefox 3.1b1

Status

()

Firefox
Tabbed Browser
P1
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

unspecified
Firefox 3.1b1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 339468 [details] [diff] [review]
patch

Use the new D&D API in tabbrowser

Next -  places; next -  extend drop target; next - actual work on detach tabs!
Attachment #339468 - Flags: review?(enndeakin)
Assignee: nobody → mano
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 3.1
OS: Mac OS X → All
Hardware: PC → All

Comment 1

10 years ago
Comment on attachment 339468 [details] [diff] [review]
patch

-                  ondraggesture="nsDragAndDrop.startDrag(event, this.parentNode.parentNode); event.stopPropagation();"
-                  ondragover="nsDragAndDrop.dragOver(event, this.parentNode.parentNode); event.stopPropagation();"
-                  ondragdrop="nsDragAndDrop.drop(event, this.parentNode.parentNode); event.stopPropagation();"
-                  ondragexit="nsDragAndDrop.dragExit(event, this.parentNode.parentNode); event.stopPropagation();">
+                  ondragstart="this.parentNode.parentNode._onDragStart(event); event.stopPropagation();"
+                  ondragover="this.parentNode.parentNode._onDragOver(event); event.stopPropagation();"
+                  ondragdrop="this.parentNode.parentNode._onDragDrop(event); event.stopPropagation();"
+                  ondragleave="this.parentNode.parentNode._onDragLeave(event); event.stopPropagation();">

The new api uses 'ondrop', not 'ondragdrop'.

-              aXferData.data.addDataForFlavour("text/x-moz-url", URI.spec + "\n" + aEvent.target.label);
-              aXferData.data.addDataForFlavour("text/unicode", URI.spec);
-              aXferData.data.addDataForFlavour("text/html", '<a href="' + URI.spec + '">' + aEvent.target.label + '</a>');
+              dt.mozSetDataAt("text/x-moz-url", URI.spec + "\n" + aEvent.target.label, 0);
+              dt.mozSetDataAt("text/unicode", URI.spec, 0);
+              dt.mozSetDataAt("text/html", '<a href="' + URI.spec + '">' + aEvent.target.label + '</a>', 0);
             } else {
-              aXferData.data.addDataForFlavour("text/unicode", "about:blank");
+              dt.mozSetDataAt("text/unicode", "about:blank", 0);

Use text/plain instead of text/unicode. We also want to be able to support the spec type for urls: text/uri-list, but not a big deal for now.

+      <method name="_setEffectAllowedForDataTransfer">
...
-              return false;
-            return true;
+            var dt = aEvent.dataTransfer

add semicolon here in case the line gets moved.

+            // tabs are alwasy added as the first type
 
spelling: always

+            if (types[0] == "application/x-moz-node") {
+              var sourceNode = dt.mozGetDataAt("application/x-moz-node", 0);
+              if (sourceNode instanceof Ci.nsIDOMXULElement &&
+                  sourceNode.localName == "tab" &&
+                  (sourceNode.parentNode == this.mTabContainer ||
+                   (sourceNode.ownerDocument.defaultView instanceof ChromeWindow &&
+                    sourceNode.ownerDocument.documentElement.getAttribute("windowtype") == "navigator:browser"))) {

You could just call mozGetDataAt without the earlier condition, as it will return null if the data isn't present rather than even checking the type first.

Also 'sourceNode instanceof XULElement' is sufficient.

+              var url;
+              var types = dt.mozTypesAt(0);
+              for (var i=0; i < this._supportedLinkDropTypes.length; i++) {
+                if (types.contains(this._supportedLinkDropTypes[i])) {
+                  var urlData = dt.mozGetDataAt(this._supportedLinkDropTypes[i], 0);
+                  url = transferUtils.retrieveURLFromData(urlData, this._supportedLinkDropTypes[i]);
+                  break;

I'd cache this._supportedLinkDropTypes[i] in another variable, as you use it three times.

-              if (document.getBindingParent(aEvent.originalTarget).localName != "tab" || accelKeyPressed) {
+              if (document.getBindingParent(aEvent.originalTarget).localName != "tab" || dtopEffect == "copy") {

spelling here: dropEffect
Created attachment 341075 [details] [diff] [review]
patch
Attachment #339468 - Attachment is obsolete: true
Attachment #341075 - Flags: review?(enndeakin)
Attachment #339468 - Flags: review?(enndeakin)
Created attachment 341078 [details] [diff] [review]
patch
Attachment #341075 - Attachment is obsolete: true
Attachment #341078 - Flags: review?(mconnor)
Attachment #341075 - Flags: review?(enndeakin)
Created attachment 341083 [details] [diff] [review]
patch
Attachment #341078 - Attachment is obsolete: true
Attachment #341083 - Flags: review?(mconnor)
Attachment #341078 - Flags: review?(mconnor)
Comment on attachment 341083 [details] [diff] [review]
patch

ok, let's get this done
Attachment #341083 - Flags: review?(mconnor) → review+
Did this break d&d causing crashes:
http://crash-stats.mozilla.com/report/index/987427ae-8fc2-11dd-a81c-001cc4e2bf68?p=1

Also reported in the Builds forums.  

Also seeing an Assert dragging a link to the tab-bar:
ASSERT: copy or move action without a tab
Stack Trace: 
0:_onDrop([object DragEvent])
1:ondrop([object DragEvent])

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20081001 Minefield/3.1b1pre ID:20081001053411

Vista HP SP1
The Assert seems to be caused by TabMix Plus add-on - the d&d crash is still valid on a virgin profile.

http://crash-stats.mozilla.com/report/index/2a6b1bfd-8fc5-11dd-87c2-001cc4e2bf68
Flags: blocking-firefox3.1?
Keywords: crash, regression

Comment 8

10 years ago
(In reply to comment #6)
Why do you add keywords and request blocking here? I suggest you file a new bug.
Flags: blocking-firefox3.1?
Keywords: crash, regression
because this bug is still open and landed yesterday ?  

If this bug was 'closed/fixed' I would have filed a new bug...Obviously its not 'fixed' - and has broken drag&drop - a bad thing since we are frozen for B1..
http://hg.mozilla.org/mozilla-central/rev/fa472fbd399e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1b1

Updated

10 years ago
Depends on: 458070

Comment 11

10 years ago
hi, can we get testcases for this patch?  Also, what can be done manually to verify it, other than simple tab browsing tests?

Updated

10 years ago
Depends on: 458696

Comment 12

10 years ago
one more ping in here, looking for testcases.  thanks.
You cannot test this any other way you didn't test the old tab d&d code with.
Depends on: 481904
You need to log in before you can comment on or make changes to this bug.