Closed Bug 324591 Opened 18 years ago Closed 18 years ago

Allow me to drop URLs in the tab strip and get a new tab at the drop site (instead of replacing an existing tab)

Categories

(SeaMonkey :: UI Design, enhancement)

x86
Windows XP
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey1.1alpha

People

(Reporter: csthomas, Assigned: csthomas)

References

Details

(Keywords: fixed-seamonkey1.1a, relnote)

Attachments

(2 files, 9 obsolete files)

9.21 KB, patch
jag+mozilla
: review+
Details | Diff | Splinter Review
2.70 KB, patch
neil
: review+
Details | Diff | Splinter Review
See firefox bug 320638

It would be nice to be able to drop URLs between tabs to open a new tab at that location.

[very crude] patch in a minute...
Attached patch the patch (obsolete) — Splinter Review
No RTL support yet.
I suspect I could use my "getDropIndex2" in onDragOver to make that code simpler.
It works, though.
Attached patch diff -w (obsolete) — Splinter Review
(same thing, -w)
original summary was confusing ;)
Status: NEW → ASSIGNED
Summary: allow D&D in between tabs → Allow me to drop URLs in the tab strip and get a new tab at the drop site (instead of replacing an existing tab)
Attached patch the patch, cleaned up (obsolete) — Splinter Review
I messed up the RTL code, but I have a hard time figuring it out, so someone will have to help me with that.

Having two functions for drop indexes is probably not the best way to do this... I thought about returning values like "3.5" to indicate whether it's at a tab or between tabs, but that doesn't lead to pretty code.  The current method does make the code pretty simple.  Maybe it would be ok with a better name than "getDropIndex2".
Attachment #209537 - Attachment is obsolete: true
Attachment #209538 - Attachment is obsolete: true
Attachment #209542 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209542 - Flags: review?(jag)
When I saw bug 320638 I started thinking about how to fix this in SeaMonkey. I followed the same path. Use .5 to indicate in between, but that seemed like it'd get messy, so then I thought maybe getTabDropIndex() and getURLDropIndex(), which is pretty much what you have. Great work, few nits, e.g. s/\* .5/\/ 2/, a better name for the second method, as you already pointed out, and I'd probably replace |if (...) a = true; else a = false;| with |a = ...;|. And if we can make the ltr/rtl hit point detection and indicator placement code easier to read, that'd be a nice bonus. However:

The other solution I was thinking about was to add a second parameter to getDropIndex(), e.g. allowOnTab, and return a small object with .index and .between (boolean), where |between| can only be false if |allowOnTab| is true. That would let us walk the tabs only once.

The only thing I'd have done differently (sorry, no code to show for the above thought process, you beat me to it before I started writing) is when dragging a URL I would leave the "on tab" drag feedback the same as it is now instead of moving the arrow to the middle of the target tab.
(In reply to comment #7)
> URL I would leave the "on tab" drag feedback the same as it is now instead of
> moving the arrow to the middle of the target tab.

How would the user distinguish between drops that are on tabs and drops that are between tabs?  With my patch you still have the option of replacing a tab if you're dropping a URL (i.e., if the indicator shows up between tabs, you get a new tab there, and if the indicator shows up on a tab, that tab gets replaced).  The bug summary doesn't mention this, because it's long already and I couldn't come up with a concise wording :).  Or did I misunderstand what you mean?
For URL drops in between tabs we'd show the arrow, as you suggested, but drops on top of tabs would either give no feedback (other than the mouse cursor) like we currently do, or maybe some kind of drop ring, like we show when dropping on bookmark folders / bookmark groups.
Comment on attachment 209542 [details] [diff] [review]
the patch, cleaned up

>+            var betweenOnly;
>             if (aDragSession.sourceNode &&
>-                aDragSession.sourceNode.parentNode == this.mTabContainer) {
>-              var ib = document.getAnonymousElementByAttribute(this, "class", "tab-drop-indicator-bar");
>+                aDragSession.sourceNode.parentNode == this.mTabContainer)
>+              betweenOnly = true;
>+            else
>+              betweenOnly = false;
> 
>-              if (!aDragSession.canDrop)
>-              {
>-                ib.hidden = true;
>-                return;
>-              }
Here I'd probably not show the indicator for a drop on. But if you do, I'd code it as var dropOnIndex = -1; if (canDropOn) dropOnIndex = this.getDropOnIndex(event); ... if (dropOnIndex != -1) showIndicatorDropOn();

>-                // We're adding a new tab.
>-                // Load in an existing tab.
You lost these comments. Maybe reverse your test so that they don't need to be swapped?

>+      <method name="getDropIndex2">
getDropOnIndex?

>+              if (window.getComputedStyle(this, null).direction == "ltr") {
No rtl issues here, your drop area is the centre of the tab.

>+            return null;
I'd prefer -1 so that the return value is always an integer.
Attachment #209542 - Flags: superreview?(neil) → superreview-
(In reply to comment #10)
> (From update of attachment 209542 [details] [diff] [review] [edit])
> >+            var betweenOnly;
> Here I'd probably not show the indicator for a drop on. But if you do, I'd code
> it as var dropOnIndex = -1; if (canDropOn) dropOnIndex =
> this.getDropOnIndex(event); ... if (dropOnIndex != -1) showIndicatorDropOn();

Why break it out into a separate function?  If we do that, shouldn't we have showIndicatorDropOn() and showIndicatorDropBetween() both as functions?  Also, I think some indication that the drop is going onto a tab is useful - suggested alternatives to the arrow are welcome (though I think the arrow works pretty well).

> >-                // We're adding a new tab.
> >-                // Load in an existing tab.
> You lost these comments. Maybe reverse your test so that they don't need to be
> swapped?

Added comments.  I slightly prefer this order, let me know if you'd prefer flipping it.

> >+      <method name="getDropIndex2">
> getDropOnIndex?

Fixed

> >+              if (window.getComputedStyle(this, null).direction == "ltr") {
> No rtl issues here, your drop area is the centre of the tab.

Fixed

> >+            return null;
> I'd prefer -1 so that the return value is always an integer.

Fixed

Patch will be attached when questions are resolved.
Attachment #209542 - Attachment is obsolete: true
Attachment #209542 - Flags: review?(jag)
Attached patch revised patch (obsolete) — Splinter Review
Questions were answered over IRC.
Attachment #209543 - Attachment is obsolete: true
Attachment #210049 - Flags: superreview?(neil)
Attachment #210049 - Flags: review?(jag)
+                if (canDropOn && newIndexOn != null) {

I assume that was meant to be -1. Also, you're still multiplying by 0.5 instead of dividing by 2.

However, the arrow indenting code can be compressed to this (not tested for typos):

            var ltr = window.getComputedStyle(this, null).direction == "ltr";

            var arrowX;
            var tabBoxObject;
            if (canDropOn && newIndexOn != -1) {
              tabBoxObject = this.mTabs[newIndexOn].boxObject;
              arrowX = tabBoxObject.x + tabBoxObject.width / 2;
            }
            else if (newIndexBetween == this.mTabs.length) {
              tabBoxObject = this.mTabContainer.lastChild.boxObject;
              arrowX = tabBoxObject.x;
              if (ltr) // for LTR "after" is on the right-hand side of the tab
                arrowX += tabBoxObject.width;
            } else {
              tabBoxObject = this.mTabs[newIndexBetween].boxObject;
              arrowX = tabBoxObject.x;
              if (!ltr) // for RTL "before" is on the right-hand side of the tab
                arrowX += tabBoxObject.width;
            }

            if (ltr) {
              ind.style.marginLeft = (arrowX - this.boxObject.x) + "px";
            } else {
              ind.style.marginRight = (this.boxObject.x + this.boxObject.width - 
                                       arrowX) + "px";
            }

What do you think?

On for more nits:

+            for (var i = 0; i < this.mTabs.length; i++) {

Try to get into the habit of using ++i unless you really need to do something with the old value of |i|. It doesn't matter much here, but with non-trivial iterators you'll be glad to have formed the habit.

+              if (aEvent.clientX > this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width * .25
+                  && aEvent.clientX < this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width * .75)

While this isn't a big deal, I would put |this.mTabs[i].boxObject| in a var to avoid some look-ups.

General nit: try to keep the same curly-brace style as the file you're editing. Some of the ones visible in your patch were introduced earlier, probably by others. Could you clean up a bit in the areas you're touching?

I think I might be able to get used to having the arrow always indicate where the URL is gonna drop. Let's run with it for a while and see where we end up.
Attached patch patch (obsolete) — Splinter Review
I really like the new shorter code :)

+              tabBoxObject = this.mTabContainer.lastChild.boxObject;
I'd slightly prefer to use this.mTabs[] here just for consistency (and so you don't have to know that this.mTabs = this.mTabContainer.childNodes...
Attachment #210049 - Attachment is obsolete: true
Attachment #210493 - Flags: superreview?(neil)
Attachment #210493 - Flags: review?(jag)
Attachment #210049 - Flags: superreview?(neil)
Attachment #210049 - Flags: review?(jag)
Comment on attachment 210493 [details] [diff] [review]
patch

CTho, could you r= the arrowX bits?

>Index: mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml
>===================================================================
>@@ -1235,65 +1235,74 @@
>+            var canDropOn;
>             if (aDragSession.sourceNode &&
>+                aDragSession.sourceNode.parentNode == this.mTabContainer)
>+              canDropOn = false;
>+            else
>+              canDropOn = true;

Nit: prefer:

            var canDropOn = !aDragSession.sourceNode ||
                            aDragSession.sourceNode.parentNode != this.mTabContainer;


>+                this.moveTabTo(this.mTabs.length-1, newIndex);

Nit: spaces around '-'


>+              tabBoxObject = this.mTabContainer.lastChild.boxObject;

I guess for consistency we could use mTabs.


In method getDropIndex():

>                 if (aEvent.clientX < this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) 
>                 if (aEvent.clientX > this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) 

Would you mind pulling the right-hand side of both comparisons into a single variable?


In method getDropOnIndex():

>+              if (aEvent.clientX > tabBoxObject.x + tabBoxObject.width * .25
>+                  && aEvent.clientX < tabBoxObject.x + tabBoxObject.width * .75)

Nit: Put the && on the first line so the two lines line up nicely.

r=jag
Attachment #210493 - Flags: review?(jag) → review+
Comment on attachment 210493 [details] [diff] [review]
patch

r=me on the parts jag wrote
Attachment #210493 - Flags: review+
Whiteboard: [cst: r+nits sr?]
Actually, I'd prefer
var newIndexOn = aDragSession.sourceNode &&
                 aDragSession.sourceNode.parentNode == this.mTabContainer ?
                 -1 : this.getDropOnIndex(event);

Then later simply check if (newIndexOn == -1)
Forest, trees. Good suggestion, Neil!

New patch with all nits addressed and we'll slap on all the pretty flags?
Attached patch patchSplinter Review
Hopefully I got all of them...
Attachment #210493 - Attachment is obsolete: true
Attachment #210662 - Flags: superreview?(neil)
Attachment #210493 - Flags: superreview?(neil)
Comment on attachment 210662 [details] [diff] [review]
patch

r=jag, though I would've picked something like "tabMiddle" instead of "coord".

Nit: please put |else| on the same line as the } (I notice I didn't quite get that right in comment 13).

Actually, looking at the code, |newIndexBetween| and |ltr| are only needed in the else block of |if (newIndexOn != -1)|. That has the nice side effect that the 2nd and 3rd cases (drop between) will be more closely grouped, and offset vs. the 1st case (drop on).
Attachment #210662 - Flags: review+
Btw, here are two more ways of doing this:

1)
            var tabBoxObject;
            var arrowX;
            if (newIndexOn != -1) {
              tabBoxObject = this.mTabs[newIndexOn].boxObject;
              arrowX = tabBoxObject.x + tabBoxObject.width / 2;
            } else {
              var afterLast = false;
              var newIndexBetween = this.getDropIndex(aEvent);
              if (newIndexBetween == this.mTabs.length) {
                afterLast = true;
                --newIndexBetween;
              }

              tabBoxObject = this.mTabs[newIndexBetween].boxObject;
              arrowX = tabBoxObject.x;

              var ltr = window.getComputedStyle(this, null).direction == "ltr";
              if (ltr && afterLast || !ltr && !afterLast)
                arrowX += tabBoxObject.width; // right-hand side of the tab
            }

The above is a bit harder to document though, since we're merging the case where for LTR we get the (last + 1) tab's X by getting the last tab's X + its width, with the case where for RTL we get each tab's "RTL-X" by taking its X and adding its width, except for the (last + 1) tab, where we just take the last tab's X.

If we change the RTL case to always take the previous tab's X to get a tab's "RTL-X", your special case becomes the first tab, for which you'll still need to take its X and width to get its "RTL-X":

2)
            var tabBoxObject;
            var arrowX;
            if (newIndexOn != -1) {
              tabBoxObject = this.mTabs[newIndexOn].boxObject;
              arrowX = tabBoxObject.x + tabBoxObject.width / 2;
            } else {
              var moveOneToTheRight = false;
              var ltr = window.getComputedStyle(this, null).direction == "ltr";
              var newIndexBetween = this.getDropIndex(aEvent);
              if (ltr && newIndexBetween == this.mTabs.length ||
                  !ltr && newIndexBetween != 0) {
                --newIndexBetween;
                moveOneToTheRight = true;
              }

              tabBoxObject = this.mTabs[newIndexBetween].boxObject;
              arrowX = tabBoxObject.x;

              if (moveOneToTheRight)
                arrowX += tabBoxObject.width; // right-hand side of the tab
            }

While both of these reduce duplication I think they're harder to understand and not worth replacing my original suggestion with.
(In reply to comment #21)
>               var afterLast = false;
>               var newIndexBetween = this.getDropIndex(aEvent);
>               if (newIndexBetween == this.mTabs.length) {
>                 afterLast = true;
>                 --newIndexBetween;
>               }
I'd write this
var newIndexBetween = this.getDropIndex(aEvent);
var afterLast = newIndexBetween == this.mTabs.length;
if (afterLast)
  --newIndexBetween;
//I guess newIndexBetween -= afterLast; is harder to understand ;-)

>               if (ltr && afterLast || !ltr && !afterLast)
if (ltr == afterlast) // but again, harder to understand.

I like the idea of not computing newIndexBetween if it's not used though.
Comment on attachment 210662 [details] [diff] [review]
patch

>+            var arrowX;
>+            var tabBoxObject;
var arrowX, tabBoxObject;

>               var tabIndex = this.getTabIndex(aDragSession.sourceNode);

>+              var tabIndex = this.getDropOnIndex(aEvent);
Nit: redeclaration of var tabIndex (I know Brendan turned off the warnings but I still care).

>+                // We're adding a new tab
>+                tab = this.addTab(getShortcutOrURI(url));
>+                this.moveTabTo(this.mTabs.length - 1, newIndex);
Check for newIndex == this.mTabs.length - 1 first?
Attachment #210662 - Flags: superreview?(neil) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [cst: r+nits sr?] → [cst: baking on trunk]
(In reply to comment #22)
> I'd write this
> var newIndexBetween = this.getDropIndex(aEvent);
> var afterLast = newIndexBetween == this.mTabs.length;
> if (afterLast)
>   --newIndexBetween;

Yeah!

Though I kinda like my second alternative better, since it means on average adding tabBoxObject.width less often for RTL.

> //I guess newIndexBetween -= afterLast; is harder to understand ;-)

That, and I don't really like using that true evaluates to 1 in numeric context.

> >               if (ltr && afterLast || !ltr && !afterLast)
> if (ltr == afterlast) // but again, harder to understand.

Yeah, I considered that, but decided against it 'coz it'd warrent a comment longer than the test I ended up using ;-)
Attachment #210885 - Flags: superreview?(neil)
Attachment #210885 - Flags: review?
Comment on attachment 210885 [details] [diff] [review]
(diff -uw) Move newIndexBetween and ltr into the |else|

>+              var newIndexBetween = this.getDropIndex(aEvent);
>+              var ltr = window.getComputedStyle(this, null).direction == "ltr";
>+              if (newIndexBetween == this.mTabs.length) {
I'd swap the declarations because it looks more efficient to use newIndexBetween immediately after calculating it ;-)
Attachment #210885 - Flags: superreview?(neil) → superreview+
Yes, of course
Attachment #210885 - Attachment is obsolete: true
Attachment #210940 - Flags: superreview?(neil)
Attachment #210940 - Flags: review?(neil)
Attachment #210885 - Flags: review?
Attachment #210940 - Attachment is obsolete: true
Attachment #210962 - Flags: superreview?(neil)
Attachment #210962 - Flags: review?(neil)
Attachment #210940 - Flags: superreview?(neil)
Attachment #210940 - Flags: review?(neil)
Attachment #210962 - Flags: superreview?(neil)
Attachment #210962 - Flags: superreview+
Attachment #210962 - Flags: review?(neil)
Attachment #210962 - Flags: review+
Attachment #210662 - Flags: approval-branch-1.8.1?(neil)
Attachment #210962 - Flags: approval-branch-1.8.1?(neil)
Attachment #210662 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
Attachment #210962 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
Checking in mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.126.2.16; previous revision: 1.126.2.15
done
Whiteboard: [cst: baking on trunk]
'approval-branch-1.8.1=?': (SeaMonkey only)
Attachment 210662 [details] [diff] landed on (Trunk and) MOZILLA_1_8_BRANCH,
but these simple enhanced bits never made it to the branch: synchronizing.
Attachment #222621 - Flags: superreview?(neil)
Attachment #222621 - Flags: review?(neil)
Attachment #222621 - Flags: approval-branch-1.8.1?(neil)
Attachment #222621 - Flags: superreview?(neil)
Attachment #222621 - Flags: superreview+
Attachment #222621 - Flags: review?(neil)
Attachment #222621 - Flags: review+
Attachment #222621 - Flags: approval-branch-1.8.1?(neil)
Attachment #222621 - Flags: approval-branch-1.8.1+
Comment on attachment 222621 [details] [diff] [review]
(Cv1-181) <tabbrowser.xml>
[Checkin: Comment 32]


Checkin: {
2006-05-19 08:05	neil%parkwaycc.co.uk 	mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml 	1.126.2.21 	MOZILLA_1_8_BRANCH
}
Attachment #222621 - Attachment description: (Cv1-181) <tabbrowser.xml> → (Cv1-181) <tabbrowser.xml> [Checkin: Comment 32]
Attachment #222621 - Attachment is obsolete: true
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.