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

RESOLVED FIXED in Firefox1.5

Status

()

Firefox
Toolbars and Customization
--
enhancement
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: Nathan Silva, Assigned: Ben Basson)

Tracking

unspecified
Firefox1.5
Points:
---
Bug Flags:
blocking-aviary1.0PR -
blocking-aviary1.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

14 years ago
Taking QA Contact
QA Contact: asa → bugzilla

Comment 3

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

Comment 4

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

Updated

14 years ago
OS: Windows XP → All
Hardware: PC → All

Comment 5

14 years ago
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).

Updated

14 years ago
Summary: Support dragging a link onto the "New Tab" toolbar button. → Support dragging a link onto the New Tab / New Window toolbar button.

Comment 6

13 years ago
*** Bug 240171 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

13 years ago
Created attachment 152515 [details] [diff] [review]
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.
(Assignee)

Comment 8

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

Can you check this out for me please Mike?
Attachment #152515 - Flags: review?(mconnor)
(Assignee)

Comment 9

13 years ago
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.
Flags: blocking-aviary1.0RC1?

Comment 10

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

Comment 11

13 years ago
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

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

Comment 13

13 years ago
Created attachment 152561 [details] [diff] [review]
Patch v0.1 - background tab

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

Comment 14

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

Here's another for you Mike... (don't kill me :D)
Attachment #152561 - Flags: review?(mconnor)

Comment 15

13 years ago
Not blocking, but we'll still take this patch.
Flags: blocking-aviary1.0RC1? → blocking-aviary1.0RC1-

Comment 16

13 years ago
*** Bug 252880 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

13 years ago
Attachment #152561 - Flags: approval-aviary?

Comment 17

13 years ago
(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.
Assignee: hyatt → cusser
(Assignee)

Comment 18

13 years ago
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

13 years ago
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

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

please don't request approval until you've got sufficient reviews.
Attachment #152561 - Flags: approval-aviary?
(Assignee)

Comment 21

13 years ago
Created attachment 156116 [details] [diff] [review]
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.
Attachment #152561 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #152515 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #156116 - Flags: review?(firefox)

Comment 22

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

Removing review request from obsolete patch.
Attachment #152515 - Flags: review?(mconnor)

Comment 23

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

Here as well.
Attachment #152561 - Flags: review?(mconnor)
(Assignee)

Comment 24

13 years ago
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?
Attachment #156116 - Flags: superreview?(bugs)
Attachment #156116 - Flags: review?(firefox)
Attachment #156116 - Flags: review?(dbaron)
Comment on attachment 156116 [details] [diff] [review]
Patch v0.2 - background tab

I'm not an appropriate reviewer for this patch.
Attachment #156116 - Flags: review?(dbaron) → review?
(Assignee)

Updated

13 years ago
Attachment #156116 - Flags: review? → review?(bugs)
*** Bug 269904 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
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.
Attachment #156116 - Flags: superreview?(bugs)

Comment 28

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

Updated

13 years ago
Attachment #156116 - Flags: review?(bugs)
(Assignee)

Comment 29

13 years ago
Created attachment 170039 [details] [diff] [review]
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.
Attachment #156116 - Attachment is obsolete: true
Attachment #170039 - Flags: review?(bugs)
(Assignee)

Comment 30

13 years ago
Nominating to invoke review. It's not critical for 1.1, but it'd be nice to
finally land this and prevent future bitrot. 
Flags: blocking-aviary1.1?
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.
Flags: blocking-aviary1.1? → blocking-aviary1.1+
(Assignee)

Comment 32

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

Done. Thanks Mike.
Attachment #170039 - Flags: review?(bugs) → review?(mconnor)

Comment 33

12 years ago
*** Bug 295068 has been marked as a duplicate of this bug. ***
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.
Attachment #170039 - Flags: review?(mconnor) → review+
(Assignee)

Comment 35

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

Thanks, Mike... and no worries :)

Requesting approval and checkin, please?
Attachment #170039 - Flags: approval-aviary1.1a2?

Updated

12 years ago
Attachment #170039 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1

Updated

12 years ago
Depends on: 297005
(Assignee)

Updated

12 years ago
No longer depends on: 297005

Comment 37

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

Updated

11 years ago
QA Contact: bugzilla → toolbars

Updated

9 years ago
Depends on: 477694
You need to log in before you can comment on or make changes to this bug.