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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Tabbed Browser
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs)

Trunk
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 674925
(Assignee)

Comment 1

5 years ago
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).
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 652725 [details] [diff] [review]
patch v2

some cleanup and polish, probably won't solve the issue on Windows
Attachment #652457 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 654738 [details] [diff] [review]
patch v3

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-
Created attachment 654836 [details]
Video of patch v3
(Assignee)

Comment 9

5 years ago
> >          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.
(Assignee)

Comment 10

5 years ago
Created attachment 655040 [details] [diff] [review]
patch v4
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+
(Assignee)

Comment 12

5 years ago
(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?
(Assignee)

Comment 14

5 years ago
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.)
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/mozilla-central/rev/257e181b2a96
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(Assignee)

Comment 16

5 years ago
(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 ?
(Assignee)

Updated

5 years ago
Depends on: 786291

Updated

5 years ago
Depends on: 786397
(Assignee)

Updated

5 years ago
Depends on: 786406

Updated

5 years ago
Depends on: 786495

Updated

5 years ago
Depends on: 786593

Updated

5 years ago
Blocks: 347238
Depends on: 787897
(Assignee)

Comment 18

5 years ago
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.
Attachment #658457 - Flags: approval-mozilla-aurora?

Comment 19

5 years ago
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?
(Assignee)

Comment 21

5 years ago
They're all fixes to the new behavior implemented in this bug.
Depends on: 789105
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+
(Assignee)

Comment 23

5 years ago
Comment on attachment 658457 [details] [diff] [review]
combined followup and regression fixes

https://hg.mozilla.org/releases/mozilla-aurora/rev/be605a6e4ade
Attachment #658457 - Flags: checkin+
Depends on: 794474

Updated

5 years ago
status-firefox17: --- → fixed

Updated

5 years ago
Depends on: 685702

Updated

5 years ago
Depends on: 802934

Updated

5 years ago
Depends on: 815961

Updated

5 years ago
Blocks: 675438

Updated

4 years ago
Blocks: 886154
Depends on: 898772
No longer depends on: 898772
You need to log in before you can comment on or make changes to this bug.