The default bug view has changed. See this FAQ.

Need to replace contentAreaDD.js with droppedLinkHandler

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
UI Design
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

Trunk
seamonkey2.1b2
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 497791 [details] [diff] [review]
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #497791 - Flags: review?(philip.chee)

Comment 2

6 years ago
Comment on attachment 497791 [details] [diff] [review]
Proposed patch

r=jag
Attachment #497791 - Flags: review?(philip.chee) → review+

Comment 3

6 years ago
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 ;-)
(Assignee)

Comment 4

6 years ago
Created attachment 497944 [details] [diff] [review]
Alternative patch

This version only sets the dropped link handler on the current browser.
Attachment #497944 - Flags: review?(jag-mozilla)

Comment 5

6 years ago
Comment on attachment 497944 [details] [diff] [review]
Alternative patch

r=jag

I think I do like this version better.
Attachment #497944 - Flags: review?(jag-mozilla) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 498074 [details] [diff] [review]
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.
Attachment #497944 - Attachment is obsolete: true
Attachment #498074 - Flags: review?(jag-mozilla)

Updated

6 years ago
Duplicate of this bug: 560889

Updated

6 years ago
Blocks: 467530
(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? :-/
(Assignee)

Comment 9

6 years ago
(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

6 years ago
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).
Attachment #498074 - Flags: review?(jag-mozilla) → review+
(Assignee)

Comment 11

6 years ago
Pushed changeset 85faf50deb16 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
Attachment #497791 - Attachment is obsolete: true
Attachment #498074 - Attachment description: Fixed patch → Fixed patch [Checked in: See comment 10+11]
Depends on: 560443
No longer blocks: 467530
You need to log in before you can comment on or make changes to this bug.