Closed Bug 783282 Opened 12 years ago Closed 12 years ago

When dragging a tab within the tab strip, move it directly instead of displaying a drop indicator

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17
Tracking Status
firefox17 --- fixed

People

(Reporter: dao, Assigned: dao)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Blocks: 674925
Doesn't seem to work on Windows as well as it did on Linux. The dragged tab mysteriously gets stuck at seemingly random positions.
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).
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.
Attached patch patch v2 (obsolete) — Splinter Review
some cleanup and polish, probably won't solve the issue on Windows
Attachment #652457 - Attachment is obsolete: true
(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.
Attached patch patch v3 (obsolete) — Splinter Review
shouldn't be locking on Windows anymore, I hope
Assignee: nobody → dao
Attachment #652725 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #654738 - Flags: review?(jaws)
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).
Attachment #654738 - Flags: review?(jaws) → review-
Attached video Video of patch v3 (obsolete) —
> >          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.
Attached patch patch v4Splinter Review
Attachment #654738 - Attachment is obsolete: true
Attachment #654836 - Attachment is obsolete: true
Attachment #655040 - Flags: review?(jaws)
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.
Attachment #655040 - Flags: review?(jaws) → review+
(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.
Ok, that makes sense. Are you planning on landing this for 17 or waiting for tab detach?
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.)
https://hg.mozilla.org/mozilla-central/rev/257e181b2a96
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(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.
Target Milestone: Firefox 18 → Firefox 17
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 ?
Depends on: 786291
Depends on: 786397
Depends on: 786406
Depends on: 786495
Depends on: 786593
Blocks: 347238
[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.
Attachment #658457 - Flags: approval-mozilla-aurora?
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.
(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?
They're all fixes to the new behavior implemented in this bug.
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.
Attachment #658457 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 685702
Depends on: 802934
Depends on: 815961
Blocks: 675438
Blocks: 886154
No longer depends on: 898772
You need to log in before you can comment on or make changes to this bug.