Last Comment Bug 324591 - Allow me to drop URLs in the tab strip and get a new tab at the drop site (instead of replacing an existing tab)
: Allow me to drop URLs in the tab strip and get a new tab at the drop site (in...
Status: RESOLVED FIXED
: fixed-seamonkey1.1a, relnote
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: x86 Windows XP
: -- enhancement with 1 vote (vote)
: seamonkey1.1alpha
Assigned To: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
:
Mentors:
Depends on:
Blocks: sm1.1
  Show dependency treegraph
 
Reported: 2006-01-24 18:32 PST by Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
Modified: 2008-07-31 04:23 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
the patch (9.43 KB, patch)
2006-01-24 18:36 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Review
diff -w (7.81 KB, patch)
2006-01-24 18:36 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Review
the patch, cleaned up (9.17 KB, patch)
2006-01-24 19:34 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: superreview-
Details | Diff | Review
diff -w (8.03 KB, patch)
2006-01-24 19:34 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Review
revised patch (9.11 KB, patch)
2006-01-29 11:22 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Review
patch (9.09 KB, patch)
2006-02-02 10:11 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
jag-mozilla: review+
csthomas: review+
Details | Diff | Review
patch (9.21 KB, patch)
2006-02-03 17:45 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
jag-mozilla: review+
neil: superreview+
neil: approval‑branch‑1.8.1+
Details | Diff | Review
(diff -uw) Move newIndexBetween and ltr into the |else| (1.74 KB, patch)
2006-02-06 08:51 PST, jag (Peter Annema)
neil: superreview+
Details | Diff | Review
(diff -u) Swapped those two lines (2.68 KB, patch)
2006-02-06 16:08 PST, jag (Peter Annema)
no flags Details | Diff | Review
Can't move ltr, it's used later (thanks Neil!). I could've sworn I had checked that. (2.70 KB, patch)
2006-02-06 18:04 PST, jag (Peter Annema)
neil: review+
neil: superreview+
neil: approval‑branch‑1.8.1+
Details | Diff | Review
(Cv1-181) <tabbrowser.xml> [Checkin: Comment 32] (2.26 KB, patch)
2006-05-19 07:42 PDT, Serge Gautherie (:sgautherie)
neil: review+
neil: superreview+
neil: approval‑branch‑1.8.1+
Details | Diff | Review

Description Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 18:32:29 PST
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...
Comment 1 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 18:36:02 PST
Created attachment 209537 [details] [diff] [review]
the patch

No RTL support yet.
I suspect I could use my "getDropIndex2" in onDragOver to make that code simpler.
It works, though.
Comment 2 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 18:36:44 PST
Created attachment 209538 [details] [diff] [review]
diff -w

(same thing, -w)
Comment 3 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 18:37:40 PST
This also covers firefox bug 320639.
Comment 4 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 19:01:29 PST
original summary was confusing ;)
Comment 5 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 19:34:09 PST
Created attachment 209542 [details] [diff] [review]
the patch, cleaned up

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".
Comment 6 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 19:34:36 PST
Created attachment 209543 [details] [diff] [review]
diff -w
Comment 7 jag (Peter Annema) 2006-01-24 22:45:24 PST
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.
Comment 8 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-25 05:12:34 PST
(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?
Comment 9 jag (Peter Annema) 2006-01-25 07:07:04 PST
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 10 neil@parkwaycc.co.uk 2006-01-29 08:00:43 PST
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.
Comment 11 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-29 10:42:55 PST
(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.
Comment 12 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-29 11:22:37 PST
Created attachment 210049 [details] [diff] [review]
revised patch

Questions were answered over IRC.
Comment 13 jag (Peter Annema) 2006-01-30 12:37:13 PST
+                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.
Comment 14 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-02-02 10:11:43 PST
Created attachment 210493 [details] [diff] [review]
patch

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...
Comment 15 jag (Peter Annema) 2006-02-02 15:49:58 PST
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
Comment 16 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-02-02 19:17:24 PST
Comment on attachment 210493 [details] [diff] [review]
patch

r=me on the parts jag wrote
Comment 17 neil@parkwaycc.co.uk 2006-02-03 02:29:20 PST
Actually, I'd prefer
var newIndexOn = aDragSession.sourceNode &&
                 aDragSession.sourceNode.parentNode == this.mTabContainer ?
                 -1 : this.getDropOnIndex(event);

Then later simply check if (newIndexOn == -1)
Comment 18 jag (Peter Annema) 2006-02-03 03:31:04 PST
Forest, trees. Good suggestion, Neil!

New patch with all nits addressed and we'll slap on all the pretty flags?
Comment 19 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-02-03 17:45:05 PST
Created attachment 210662 [details] [diff] [review]
patch

Hopefully I got all of them...
Comment 20 jag (Peter Annema) 2006-02-03 18:52:08 PST
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).
Comment 21 jag (Peter Annema) 2006-02-03 19:32:33 PST
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.
Comment 22 neil@parkwaycc.co.uk 2006-02-04 13:41:21 PST
(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 23 neil@parkwaycc.co.uk 2006-02-04 13:50:06 PST
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?
Comment 24 jag (Peter Annema) 2006-02-05 19:47:51 PST
(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 ;-)
Comment 25 jag (Peter Annema) 2006-02-06 08:51:01 PST
Created attachment 210885 [details] [diff] [review]
(diff -uw) Move newIndexBetween and ltr into the |else|
Comment 26 neil@parkwaycc.co.uk 2006-02-06 09:33:02 PST
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 ;-)
Comment 27 jag (Peter Annema) 2006-02-06 15:11:26 PST
Yes, of course
Comment 28 jag (Peter Annema) 2006-02-06 16:08:05 PST
Created attachment 210940 [details] [diff] [review]
(diff -u) Swapped those two lines
Comment 29 jag (Peter Annema) 2006-02-06 18:04:19 PST
Created attachment 210962 [details] [diff] [review]
Can't move ltr, it's used later (thanks Neil!). I could've sworn I had checked that.
Comment 30 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-03-04 06:08:42 PST
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
Comment 31 Serge Gautherie (:sgautherie) 2006-05-19 07:42:05 PDT
Created attachment 222621 [details] [diff] [review]
(Cv1-181) <tabbrowser.xml>
[Checkin: Comment 32]

'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.
Comment 32 Serge Gautherie (:sgautherie) 2006-05-19 08:40:29 PDT
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
}

Note You need to log in before you can comment on or make changes to this bug.