344044 - Dragging link to "new tab" icon in toolbar does not open link in new tab like in firefox.

Status: RESOLVED WONTFIX

(Camino Graveyard :: Drag & Drop, enhancement)

Description

[Harry]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2

I dragged a link to the "new tab" icon in the toolbar and was unable to drop it.  Firefox would have opened up the link in a new tab with drag and drop.

Reproducible: Always

Comment 1

[Chris Lawson (gone)]

In the interim, you can drag links to the empty space in the tab bar, but I agree with the reporter. We should fix this.

cl

Comment 2

[froodian (Ian Leue)]

This is similar to bug 298037 (and will have similar difficulties)

Comment 3

[Sean Murphy]

I'm working on a solution to bug 298037 which will allow us to easily add drop behavior to this button (and any other) as well.

Comment 4

[Sean Murphy]

Attachment: patch

I created a new object, DragDestinationButton, which provides a reusable way to add drop support to as many toolbar buttons (or any others as well) as we need.

from the comments in my patch:
"A subclass of NSButton which is able to receive drag operations and hands off behavior to its target when the drag should be performed. Made with reusability in mind, the class is not coupled to receive any specific drag types and instead relies on the target to provide that information, and stores the values locally for verification during a drag session."

"The button's dropOperationTarget should respond to any selector (previously supplied with setDropOperationSelector) which accepts two arguments: (id)currentDragPasteboard, (id)sender.  This selector is sent when a recognized type was dropped. The target is given a reference to the pasteboard so data can be retrieved using a technique appropriate for its use."

Some explanation for why I implemented the class this way:
Requiring the target to specify a custom selector allows that object to be the target for multiple DragDestinationButtons and not have to decipher which one is currently receiving the drag.  A reference to the active drag pasteboard is supplied as a parameter in the target's selector in an attempt to de-couple any specific behavior from the button class as to how the data should be obtained from the pasteboard.  The only object which should know how the data will be used is the target.

I did consider notifications, but that quickly became complicated since we can't assume NSDragPBoard always contains the drag session and this requires supplying it directly.  I also avoided explicitly utilizing a delegate, since only one action-type message needs to be called.

I am also providing here the patch for bug 344044, which was simple to implement with this fix in place.  It was easier for me to include both fixes together, sorry if that wasn't ok.
I wasn't sure whether to create new files for the class or not, so for now all code is inside of BrowserWindowController.mm.

Localization:
I chose to display an alert sheet to prevent accidental drops that set the home page unknowingly to the user.  There are three strings now required in Localizable.strings for this patch: (QA can feel free to change these if need be!)

/* Message for setting home page on toolbar drop */
"HomePageSetOnDropTitle" = "Set your home page?";
"HomePageSetOnDropInformativeFormat" = "Would you like to set your home page to: \n\"%@\"?";
"HomePageSetOnDropButton" = "Set Home Page";

One additional note: I had to patch the PageProxyIcon object to support NSDragOperationCopy in local drags.  Otherwise dragging this icon would not set the home page.

Comment 5

[Sean Murphy]

Ah man, I meant to submit the patch under bug 298037, cause that's how the dependency/blocking situation was set up.  Sorry for that..

As far as the new tabs are concerned, the drop behavior will obey the "load in background" preference and holding shift to override also works in this case.

I couldn't think of a way to determine if a referring URL should be supplied, though (in cases where a link is dragged off a page).

Comment 6

[Chris Lawson (gone)]

A few stylistic comments on the patch:

1) As we discussed on IRC, please put the new class in its own file so we don't have to make BWC grow any more unwieldy than it already is, and so we can re-use it outside a browser window if we decide to in the future.

2) While I realise our internal consistency leaves something to be desired, declaring

NSFoo* mFoo;

seems to be favoured over the Apple house style of

NSFoo *mFoo;

Likewise, we prefer (NSFoo*) to (NSFoo *) in method declarations.


Getting into actual discussion of code:

+// -createDropButtonWithImageNamed:

Wouldn't it make more sense to re-name this method |initWithImage| and make it the designated initializer for the subclass, especially since that's basically what it does anyway? :)

+- (void)setHomePageFromPasteboard:(id)pboard toolbarButton:(id)sender
+{
+  pboard = (NSPasteboard *)pboard;
+  NSString *droppedURL;

We had a discussion recently about variable names, and Wevah suggested something that I'm going to suggest here: instead of

NSString* fooURL;

we should prefer

NSString* fooURLString;

since NSURL is a valid type and causes a LOT of confusion if you have to use both NSURL and NSString in close proximity.

+  // Displaying an alert sheet to prevent accidental changes..

s/changes../changes

I'm not sure we really want to display a sheet; at some point we have to assume users are doing things on purpose and just let them do it, and this seems like it would get REALLY annoying really fast if you're using this method to set your homepage.

If we do, though:

NSLocalizedString(@"HomePageSetOnDropButton", @"The action button text")

Pass nil for the second argument there. We don't use it for anything.

One final question -- I haven't tested this, so I don't know the answer -- does this do the Right Thing™ for dragged bookmarks (from Camino, or from any other browser, but it definitely needs to do the right thing for our own) as well as .webloc/.url files from Finder? If you need testcases for the latter, I can help you track down the bugs where we've addressed previous drag issues with these sorts of files.

cl

Comment 7

[Chris Lawson (gone)]

Comment on attachment 248144 [details] [diff] [review]
patch

r- per comments above, largely because this should be in its own file and because I think the alert is going to piss people off.

Comment 8

[Gordon P. Hemsley [:GPHemsley]]

This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]