179845 - Support dragging a link onto the New Tab / New Window toolbar button.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement)

Description

[Nathan Silva]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021112 Phoenix/0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021112 Phoenix/0.4

You should be able to drag a link onto the New Tab toolbar button and have it
open in a tab.

Reproducible: Always

Steps to Reproduce:




Since you can drag a URL onto Home or Go you should be able to drag a link onto
the New Tab button.

This would be convenient if you use the pref "Hide the tab bar when only one tab
is open".

Comment 1

[David P James]

We're in danger of getting into feature bloat here, but since the other drag &
drop operations exist this one should be considered too.

-->Confirming as New for developer review

Comment 2

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 3

[Jesse Ruderman]

*** Bug 234203 has been marked as a duplicate of this bug. ***

Comment 4

[Kevin Ar18]

Can we add "New Window" button to the subject as well?

Comment 5

[Kevin Ar18]

BTW, I think it would be appropriate if the buttons would appear "depressed"
whenever you hover over it with your mouse (and you have a URL you are dragging).

Comment 6

[Asa Dotzler [:asa]]

*** Bug 240171 has been marked as a duplicate of this bug. ***

Comment 7

[Ben Basson]

Attachment: Patch v0.1

This should do the trick, however it could do with reviewing and, most likely,
tidying up. Can someone please point out to me a better way of opening an
active (selected) tab, if there is one?

I'm surprised that there was no function for doing so, or a flag for
openNewTabWith(), which would be the most logical way. I assume that would be a
seperate bug.

Anyway, this appears to work ok. I'll flag for review.

Comment 8

[Ben Basson]

Comment on attachment 152515 [details] [diff] [review]
Patch v0.1

Can you check this out for me please Mike?

Comment 9

[Ben Basson]

Nominating 1.0RC1 due to localisation impacts and the fact that it's exactly a
simple patch. Also security issues to consider. 

If it's going to miss 1.0, that's ok, but if someone wants to get this in, it
needs to be done before RC1. It's polish, but logical polish adding to the
precedent set by the Home and Go buttons.

Comment 10

[Steffen Wilberg]

Why you don't use openNewTabWith()? That function checks the pref
"browser.tabs.loadInBackground" and the shift key to determine whether to select
the new tab. The same pref is checked when you drop a link on the tabbar, see
the onDrop method in tabbrowser.xml:
http://lxr.mozilla.org/aviarybranch/source/toolkit/content/widgets/tabbrowser.xml#1080

Dropping a link on the new tab button should have the same behaviour as dropping
on the tabbar: create a new tab, and select that tab if the pref is set to
false. Your patch always selects the new tab.

Comment 11

[Ben Basson]

Clicking the "New Tab" button opens a focussed "about:blank" tab, ideally
dragging a link should perform the same action, in my opinion, which is why I
wrote my own version of openNewTabWith().

Comment 12

[Steffen Wilberg]

The blank tab is selected so that you can enter an url in the urlbar. Opening a
blank tab in the background wouldn't make much sense.
But opening links in the background makes a lot of sense: you can open multiple
links from the same page. I do a middleclick for that, but there are people
without a third mousebutton; Mac users only have one.

Comment 13

[Ben Basson]

Attachment: Patch v0.1 - background tab

Same patch, but without changes to contentAreaUtils.js and tab opens in the
background. Whoever reviews this can decide.

Comment 14

[Ben Basson]

Comment on attachment 152561 [details] [diff] [review]
Patch v0.1 - background tab

Here's another for you Mike... (don't kill me :D)

Comment 15

[Blake Ross]

Not blocking, but we'll still take this patch.

Comment 16

[Jesse Ruderman]

*** Bug 252880 has been marked as a duplicate of this bug. ***

Comment 17

[Steffen Wilberg]

(In reply to bug 250974 comment #9)
> Steffen, sorry to bring this up here, but since it's related, any chance of you
> taking a look at my patch for Bug 179845 and getting it in? Apologies if I'm
> just being a pain!
The second patch looks good to me at a glance, but I'm not able to give reviews.
browser.properties has been moved to
mozilla/browser/locales/en-US/chrome/browser/browser.properties by the way.

-> cusser.

Comment 18

[Ben Basson]

Will CVS take care of that or do I need to update the patch? If an update is
needed, I'll do it tomorrow afternoon.

Comment 19

[Steffen Wilberg]

The person who wants to check in a patch first needs to patch his local tree by
using the patch command. That complains that the file doesn't exist, and one has
to enter it manually.

So it's not absolutely necessary for you to update the patch, but it's common
practice, and it helps, especially when time is running out.
And please run |cvs diff (file1) (file2) (file3)| from above mozilla/.

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 152561 [details] [diff] [review]
Patch v0.1 - background tab

please don't request approval until you've got sufficient reviews.

Comment 21

[Ben Basson]

Attachment: Patch v0.2 - background tab

Thanks for your help Steffen, I've taken note of your comments, here's the
updated patch. Sorry asa, my bad. Will wait until this is reviewed, as
directed.

Comment 22

[Steffen Wilberg]

Comment on attachment 152515 [details] [diff] [review]
Patch v0.1

Removing review request from obsolete patch.

Comment 23

[Steffen Wilberg]

Comment on attachment 152561 [details] [diff] [review]
Patch v0.1 - background tab

Here as well.

Comment 24

[Ben Basson]

Comment on attachment 156116 [details] [diff] [review]
Patch v0.2 - background tab

If you want this for 1.0, it needs to be reviewed and checked in before 1.0PR
due to localisation.

Since it's a simple patch, any chance of this happening?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 156116 [details] [diff] [review]
Patch v0.2 - background tab

I'm not an appropriate reviewer for this patch.

Comment 26

[Phil Ringnalda (:philor)]

*** Bug 269904 has been marked as a duplicate of this bug. ***

Comment 27

[Simon Paquet [:sipaq]]

Comment on attachment 156116 [details] [diff] [review]
Patch v0.2 - background tab

You don't need superreview for Firefox patches. You might try to ask Ben via
IRC to give a review, to get this in faster.

Comment 28

[nrlz]

I found some weird behavior with "Patch v0.2".

I can drag and drop links from within the browser onto those icons fine. But I
cannot drag and drop files from the desktop onto those icons. They won't open.

However, if I modify the order of the flavor sets, with text/unicode as the
first and x-moz-file as the last, then I can drag and drop desktop icons as
well. I don't understand why it works though but it just does. The new order
doesn't seem to impact the other features.

i.e.,

var flavourSet = new FlavourSet();
flavourSet.appendFlavour("text/unicode");
flavourSet.appendFlavour("text/x-moz-url");
flavourSet.appendFlavour("application/x-moz-file", "nsIFile");
return flavourSet;

Comment 29

[Ben Basson]

Attachment: Patch v0.3

Patch v0.3 - Same as the last patch, with newer files (to prevent bitrot) and
reordering of the flavoursets to allow file drag/dropping. Also reordered
flavoursets for the Go button while I was there.

Comment 30

[Ben Basson]

Nominating to invoke review. It's not critical for 1.1, but it'd be nice to
finally land this and prevent future bitrot. 

Comment 31

[Mike Connor [:mconnor]]

plussing to get on radar, we're getting pretty good with drag and drop support,
we can be better though.

Please stick this in my review queue, I will get to it this weekend/next week. 
Looks good, though I want to grok the flavourset changes better than my sick
head can do right now.

Comment 32

[Ben Basson]

Comment on attachment 170039 [details] [diff] [review]
Patch v0.3

Done. Thanks Mike.

Comment 33

[Tristor]

*** Bug 295068 has been marked as a duplicate of this bug. ***

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 170039 [details] [diff] [review]
Patch v0.3

sometimes, prevailing style is evil... but I'd rather not reindent 5000+ lines
of JS :)

sorry about the delay, fixing my review queue now.

Comment 35

[Ben Basson]

Comment on attachment 170039 [details] [diff] [review]
Patch v0.3

Thanks, Mike... and no worries :)

Requesting approval and checkin, please?

Comment 36

[Asaf Romano (gone)]

Checking in base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.427; previous revision: 1.426
done
Checking in base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.262; previous revision: 1.261
done
Checking in locales/en-US/chrome/browser/browser.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v  <--
 browser.properties
new revision: 1.12; previous revision: 1.11
done

Comment 37

[mv_van_rantwijk]

Why two observers? Why not one? Oh, and more importantly, what about that
security hole (javascript: and data: links) ?