Closed
Bug 456048
Opened 16 years ago
Closed 16 years ago
Use the new D&D API in tabbrowser
Categories
(Firefox :: Tabbed Browser, defect, P1)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 3.1b1
People
(Reporter: asaf, Assigned: asaf)
References
Details
Attachments
(1 file, 3 obsolete files)
12.73 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•16 years ago
|
Assignee: nobody → mano
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 3.1
Assignee | ||
Updated•16 years ago
|
OS: Mac OS X → All
Hardware: PC → All
Comment 1•16 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
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #339468 -
Attachment is obsolete: true
Attachment #341075 -
Flags: review?(enndeakin)
Attachment #339468 -
Flags: review?(enndeakin)
Assignee | ||
Comment 3•16 years ago
|
||
Attachment #341075 -
Attachment is obsolete: true
Attachment #341078 -
Flags: review?(mconnor)
Attachment #341075 -
Flags: review?(enndeakin)
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #341078 -
Attachment is obsolete: true
Attachment #341083 -
Flags: review?(mconnor)
Attachment #341078 -
Flags: review?(mconnor)
Comment 5•16 years ago
|
||
Comment on attachment 341083 [details] [diff] [review]
patch
ok, let's get this done
Attachment #341083 -
Flags: review?(mconnor) → review+
Comment 6•16 years ago
|
||
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
Comment 7•16 years ago
|
||
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
Updated•16 years ago
|
Flags: blocking-firefox3.1?
Keywords: crash,
regression
Comment 8•16 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
Comment 9•16 years ago
|
||
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..
Comment 10•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1b1
Comment 11•16 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?
Comment 12•16 years ago
|
||
one more ping in here, looking for testcases. thanks.
Assignee | ||
Comment 13•16 years ago
|
||
You cannot test this any other way you didn't test the old tab d&d code with.
You need to log in
before you can comment on or make changes to this bug.
Description
•