619355 - Need to replace contentAreaDD.js with droppedLinkHandler

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

Bug 545714 effectively regressed browser drag-n-drop, because it was replaced with an internal handler that doesn't handle bookmark keywords and is being removed in bug 580710 anyway. We need to add our own dropped link handler.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Status bar drag'n'drop was implemented in bug 82965 (Mozilla 0.9.2/Netscape 6.1) and promptly broken 8 months later in bug 45605 (Mozilla 0.9.9/Netscape 7.0). I doubt we'll miss it. I think the sidebar drag probably went the same way.

I don't think the other two windows ever used content area drag'n'drop.

Comment 2

[jag (Peter Annema)]

Comment on attachment 497791 [details] [diff] [review]
Proposed patch

r=jag

Comment 3

[jag (Peter Annema)]

Hrm, Ratty made me think about this a bit more.

I'm not sure I like the getter/setter on tabbrowser. It implies the ability to dynamically change the handler, and since I'd expect the handler to be the same for all <browser>s, it should update it for all. I think rather than implement functionality we don't really need, how about instead you set |handleDroppedLink| as a field on <tabbrowser> and then from its constructor and |addTab()| method set the <browser>.droppedLinkHandler from the field?

Hrm, except I guess it's gonna be tricky to set the field before the constructor runs. Ideas for a solution along the lines outlined above? Other than just skipping the field and referring directly to a function in navigator.js ;-)

Comment 4

[[e-mail]]

Attachment: Alternative patch

This version only sets the dropped link handler on the current browser.

Comment 5

[jag (Peter Annema)]

Comment on attachment 497944 [details] [diff] [review]
Alternative patch

r=jag

I think I do like this version better.

Comment 6

[[e-mail]]

Attachment: Fixed patch [Checked in: See comment 10+11]

The previous patch failed to update the dropped link handler when closing the current tab. This is because this.mCurrentBrowser is set to null before calling updateCurrentBrowser as an optimisation. This patch fixes that bug by using this.mCurrentTab.linkedBrowser instead.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> Status bar drag'n'drop was implemented in bug 82965 (Mozilla 0.9.2/Netscape
> 6.1) and promptly broken 8 months later in bug 45605 (Mozilla 0.9.9/Netscape
> 7.0). I doubt we'll miss it. I think the sidebar drag probably went the same
> way.

Sorry I don't understand, what's with the sidebar? Are we losing d&d ability to or from the sidebar? Or dragging the sidebar itself? :-/

Comment 9

[[e-mail]]

(In reply to comment #8)
> (In reply to comment #1)
> > Status bar drag'n'drop was implemented in bug 82965 (Mozilla 0.9.2/Netscape
> > 6.1) and promptly broken 8 months later in bug 45605 (Mozilla 0.9.9/Netscape
> > 7.0). I doubt we'll miss it. I think the sidebar drag probably went the same
> > way.
> Sorry I don't understand, what's with the sidebar? Are we losing d&d ability to
> or from the sidebar? Or dragging the sidebar itself? :-/
Prior to bug 45605, nsContentAreaDD handled draggesture, dragover and dragdrop events. The main browser used all three events; the statusbar was only a drop target while the sidebar was only a drag source. After bug 45605 the draggesture and dragover code was moved to C++. This meant that the sidebar and main browser drag sources were now automatic. The dragdrop code remained in nsContentAreaDD which allowed the main browser to keep working but the statusbar no longer worked as a drop target because it no longer had any dragover processing.

Bug 545714 moved more of the dragdrop code to C++/browser.xml, bypassing nsContentAreaDD. This doesn't affect the use of the sidebar as a drag source.

Comment 10

[jag (Peter Annema)]

Comment on attachment 498074 [details] [diff] [review]
Fixed patch
[Checked in: See comment 10+11]

Looks good, but I'd add the comment about why you're using mCurrentTab.linkedBrowser instead of mCurrentBrowser (though the null check on the next line does kinda hint at it).

Comment 11

[[e-mail]]

Pushed changeset 85faf50deb16 to comm-central.