Closed Bug 179845 Opened 18 years ago Closed 16 years ago
Support dragging a link onto the New Tab / New Window toolbar button
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".
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
Taking QA Contact
QA Contact: asa → bugzilla
*** Bug 234203 has been marked as a duplicate of this bug. ***
Can we add "New Window" button to the subject as well?
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).
Summary: Support dragging a link onto the "New Tab" toolbar button. → Support dragging a link onto the New Tab / New Window toolbar button.
*** Bug 240171 has been marked as a duplicate of this bug. ***
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 on attachment 152515 [details] [diff] [review] Patch v0.1 Can you check this out for me please Mike?
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.
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.
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().
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.
Same patch, but without changes to contentAreaUtils.js and tab opens in the background. Whoever reviews this can decide.
Comment on attachment 152561 [details] [diff] [review] Patch v0.1 - background tab Here's another for you Mike... (don't kill me :D)
Not blocking, but we'll still take this patch.
Flags: blocking-aviary1.0RC1? → blocking-aviary1.0RC1-
*** Bug 252880 has been marked as a duplicate of this bug. ***
(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
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.
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 on attachment 152561 [details] [diff] [review] Patch v0.1 - background tab please don't request approval until you've got sufficient reviews.
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
Comment on attachment 152515 [details] [diff] [review] Patch v0.1 Removing review request from obsolete patch.
Comment on attachment 152561 [details] [diff] [review] Patch v0.1 - background tab Here as well.
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 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?
*** Bug 269904 has been marked as a duplicate of this bug. ***
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.
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;
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.
Nominating to invoke review. It's not critical for 1.1, but it'd be nice to finally land this and prevent future bitrot.
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+
Comment on attachment 170039 [details] [diff] [review] Patch v0.3 Done. Thanks Mike.
Attachment #170039 - Flags: review?(bugs) → review?(mconnor)
*** 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+
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?
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
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1
You need to log in before you can comment on or make changes to this bug.