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)
Firefox
Tabbed Browser
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)
18.86 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
7.09 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
dao
:
checkin+
|
Details | Diff | 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.
Assignee | ||
Comment 1•12 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.
Comment 2•12 years ago
|
||
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•12 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•12 years ago
|
||
some cleanup and polish, probably won't solve the issue on Windows
Attachment #652457 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 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•12 years ago
|
||
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 7•12 years ago
|
||
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-
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 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•12 years ago
|
||
Attachment #654738 -
Attachment is obsolete: true
Attachment #654836 -
Attachment is obsolete: true
Attachment #655040 -
Flags: review?(jaws)
Comment 11•12 years ago
|
||
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•12 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.
Comment 13•12 years ago
|
||
Ok, that makes sense. Are you planning on landing this for 17 or waiting for tab detach?
Assignee | ||
Comment 14•12 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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/257e181b2a96
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 16•12 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
Comment 17•12 years ago
|
||
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 | ||
Comment 18•12 years ago
|
||
[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•12 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.
Comment 20•12 years ago
|
||
(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•12 years ago
|
||
They're all fixes to the new behavior implemented in this bug.
Comment 22•12 years ago
|
||
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•12 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+
Updated•12 years ago
|
status-firefox17:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•