Last Comment Bug 783282 - When dragging a tab within the tab strip, move it directly instead of displaying a drop indicator
: When dragging a tab within the tab strip, move it directly instead of display...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: Firefox 17
Assigned To: Dão Gottwald [:dao]
:
: Dão Gottwald [:dao]
Mentors:
Depends on: 787897 789105 802934 685702 786291 786397 786406 786495 786593 794474 815961
Blocks: 674925 886154 347238 675438
  Show dependency treegraph
 
Reported: 2012-08-16 08:05 PDT by Dão Gottwald [:dao]
Modified: 2013-09-29 01:33 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (17.02 KB, patch)
2012-08-16 08:05 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v2 (16.80 KB, patch)
2012-08-17 03:45 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v3 (16.98 KB, patch)
2012-08-23 12:28 PDT, Dão Gottwald [:dao]
jaws: review-
Details | Diff | Splinter Review
Video of patch v3 (632.57 KB, video/ogg)
2012-08-23 16:28 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details
patch v4 (18.86 KB, patch)
2012-08-24 10:06 PDT, Dão Gottwald [:dao]
jaws: review+
Details | Diff | Splinter Review
combined followup and regression fixes (7.09 KB, patch)
2012-09-05 05:23 PDT, Dão Gottwald [:dao]
lukasblakk+bugs: approval‑mozilla‑aurora+
dao+bmo: checkin+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-08-16 08:05:30 PDT
Created attachment 652457 [details] [diff] [review]
patch

Spun off from bug 674925, since most of the issues blocking that bug have to do with detaching. This should be good as an incremental improvement over what we've shipped.
Comment 1 Dão Gottwald [:dao] 2012-08-16 10:26:02 PDT
Doesn't seem to work on Windows as well as it did on Linux. The dragged tab mysteriously gets stuck at seemingly random positions.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-08-16 15:57:00 PDT
The getting stuck at random positions seems to be a cause of the events getting too queued up. This is what I found with the Gecko profiler:
http://screencast.com/t/kOfEBhhhwH

The green parts show the places where _animateTabMove is being called, and the amount of red shows the level of jank (time since the last event queue movement).
Comment 3 Dão Gottwald [:dao] 2012-08-17 01:38:47 PDT
Are you sure you're seeing what I'm seeing? When I start dragging a tab to the left, that works flawlessly, but when I reverse the direction during that drag operation, I soon hit a point where I can't move the tab further to the right; reversing the direction again to the left still works at that point.
Comment 4 Dão Gottwald [:dao] 2012-08-17 03:45:41 PDT
Created attachment 652725 [details] [diff] [review]
patch v2

some cleanup and polish, probably won't solve the issue on Windows
Comment 5 Dão Gottwald [:dao] 2012-08-17 06:06:42 PDT
(In reply to Dão Gottwald [:dao] from comment #3)
> Are you sure you're seeing what I'm seeing? When I start dragging a tab to
> the left, that works flawlessly, but when I reverse the direction during
> that drag operation, I soon hit a point where I can't move the tab further
> to the right; reversing the direction again to the left still works at that
> point.

I see the same thing in the latest UX branch build.
Comment 6 Dão Gottwald [:dao] 2012-08-23 12:28:24 PDT
Created attachment 654738 [details] [diff] [review]
patch v3

shouldn't be locking on Windows anymore, I hope
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-08-23 16:28:20 PDT
Comment on attachment 654738 [details] [diff] [review]
patch v3

Review of attachment 654738 [details] [diff] [review]:
-----------------------------------------------------------------

I noticed an issue with the patch in that the background tab won't slide over to fill in the gap until the mouse cursor has passed the halfway point of the next tab. I'll upload a video showing what I'm seeing.

::: browser/base/content/tabbrowser.xml
@@ +3516,3 @@
>          canvas.mozOpaque = true;
> +        canvas.width = 160;
> +        canvas.height = 90;

This will probably be too small on very high resolutions (like retina display Macs).
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-08-23 16:28:44 PDT
Created attachment 654836 [details]
Video of patch v3
Comment 9 Dão Gottwald [:dao] 2012-08-24 09:37:38 PDT
> >          canvas.mozOpaque = true;
> > +        canvas.width = 160;
> > +        canvas.height = 90;
> 
> This will probably be too small on very high resolutions (like retina
> display Macs).

These are CSS pixels, so I don't think it should make a difference.
Comment 10 Dão Gottwald [:dao] 2012-08-24 10:06:47 PDT
Created attachment 655040 [details] [diff] [review]
patch v4
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-08-24 17:43:14 PDT
Comment on attachment 655040 [details] [diff] [review]
patch v4

Review of attachment 655040 [details] [diff] [review]:
-----------------------------------------------------------------

I tested this out and it works great.

::: browser/base/content/tabbrowser.xml
@@ +3175,5 @@
> +          // Determine what tab we're dragging over.
> +          // * Point of reference is the center of the dragged tab. If that
> +          //   point touches a background tab, the dragged tab would take that
> +          //   tab's position when dropped.
> +          // * We're doing a binary search in order to reduce the amount of

Thank you for doing this. In profiling the previous versions of this patch I noticed a significant amount of time was being spent in _getDropIndex. I am fine with leaving this here, but we should also make _getDropIndex use a binary search as well (in a follow up bug).

@@ +3187,5 @@
> +          let high = tabs.length - 1;
> +          while (low <= high) {
> +            let mid = Math.floor((low + high) / 2);
> +            if (tabs[mid] == draggedTab &&
> +                ++mid > high)

After this line is called, mid is no longer the halfway point. Is this intentional? In other words, should this be |if (tabs[mid] == draggedTab && (mid + 1) > high)| ?

@@ +3218,5 @@
> +          }
> +
> +          function getTabShift(tab, dropIndex) {
> +            if (tab._tPos < draggedTab._tPos && tab._tPos >= dropIndex)
> +              return rtl ? - tabWidth : tabWidth;

nit: please place the minus sign next to the variable name so negative is not confused with subtraction.
Comment 12 Dão Gottwald [:dao] 2012-08-25 04:24:17 PDT
(In reply to Jared Wein [:jaws] from comment #11)
> > +          let high = tabs.length - 1;
> > +          while (low <= high) {
> > +            let mid = Math.floor((low + high) / 2);
> > +            if (tabs[mid] == draggedTab &&
> > +                ++mid > high)
> 
> After this line is called, mid is no longer the halfway point. Is this
> intentional? In other words, should this be |if (tabs[mid] == draggedTab &&
> (mid + 1) > high)| ?

It's intentional. We must not use the dragged tab below, as we only want to test background tabs.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-08-26 18:22:11 PDT
Ok, that makes sense. Are you planning on landing this for 17 or waiting for tab detach?
Comment 14 Dão Gottwald [:dao] 2012-08-27 01:21:45 PDT
I would land this for 17 as a self-contained, incremental improvement, but dolske said he would prefer this to land in 18 along with other Australis stuff. (I don't think it has much to do with Australis, though.)
Comment 15 Dão Gottwald [:dao] 2012-08-27 10:44:55 PDT
https://hg.mozilla.org/mozilla-central/rev/257e181b2a96
Comment 16 Dão Gottwald [:dao] 2012-08-27 11:09:33 PDT
(20:04:52) RyanVM: dao: dumb question, but are you sure that landing on m-c is Fx-18 now?
(20:05:32) gavin: it's not, yet
(20:05:43) dao: RyanVM: assumed so based on "Next merge: 2012-10-08"
(20:05:54) Ms2ger: For m-i, that's the case
(20:06:02) bsmedberg: the merge has not happened yet
(20:06:06) bsmedberg: trunk is still 17
(20:06:10) Ms2ger: Nobody ever lands on m-c, so I gathered I'd update the topic
(20:06:27) RyanVM: Ms2ger: I was thinking about doing another merge in a bit - are you saying I shouldn't?
(20:06:36) dao: Ms2ger: that's not a sane assertion

We'll have to back this out from aurora if dolske feels strongly about it or other issues pop up.
Comment 17 Guillaume C. [:ge3k0s] 2012-08-28 06:52:38 PDT
Current implementation presents two major flaws :

-When you move a tab the partially visible thumbnail appears even if you don't distach the tab and for manipulation in the tab strip it feels odd.

-It's impossible to move a normal tab to the app tabs area.

I've also noticed that the tabs seem to respond slower than before since today's build.

And what are the current thoughts of the UX team on new Chrome implementation of tab manipulation ?
Comment 18 Dão Gottwald [:dao] 2012-09-05 05:23:33 PDT
Created attachment 658457 [details] [diff] [review]
combined followup and regression fixes

[Approval Request Comment]
This is a cumulative patch combining the followup and regression fixes from bug 786593, bug 786406, bug 786291, bug 786495 and bug 787895, all of which are small, relatively low-risk, limited to tabbrowser.xml and without interface or string changes.
Comment 19 annaeus 2012-09-05 05:50:26 PDT
Hey i would like give one small suggestion. The performance of dragging tabs is pretty good right now, however if you move the cursor off the tab-bar even slightly the animation stops. This results in a perceived "jerkiness". My suggestion is to increase the margin for which the animation still occurs. It's just a reminder for something you might want to do, please forgive me is this is meant to be fixed later in another bug.
Comment 20 Alex Keybl [:akeybl] 2012-09-05 15:49:02 PDT
(In reply to Dão Gottwald [:dao] from comment #18)
> [Approval Request Comment]
> This is a cumulative patch combining the followup and regression fixes from
> bug 786593, bug 786406, bug 786291, bug 786495 and bug 787895, all of which
> are small, relatively low-risk, limited to tabbrowser.xml and without
> interface or string changes.

Are all of these fixes for fallout from what landed in comment 15, or are we trying to fix longer standing issues?
Comment 21 Dão Gottwald [:dao] 2012-09-05 16:03:31 PDT
They're all fixes to the new behavior implemented in this bug.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-07 16:07:07 PDT
Comment on attachment 658457 [details] [diff] [review]
combined followup and regression fixes

Sounds like we need these fixes then, for this feature to run smoothly - approving.
Comment 23 Dão Gottwald [:dao] 2012-09-08 02:53:11 PDT
Comment on attachment 658457 [details] [diff] [review]
combined followup and regression fixes

https://hg.mozilla.org/releases/mozilla-aurora/rev/be605a6e4ade

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