Last Comment Bug 674925 - Improve UI of detaching tabs
: Improve UI of detaching tabs
Status: NEW
p=0 [qx:spec]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 85 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 455694 700099 794474 841817 962551 (view as bug list)
Depends on: 666864 1019736 1019738 1019742 1019746 1019758 1019760 1019763 1019767 1019778 533460 685702 712184 782811 783282 982990
Blocks: 485105 590297 597989 682559 1019646 674487 674723 674789 674823 674831 674913 675340 675393 675860 675863 676304 676686 689575 690227
  Show dependency treegraph
 
Reported: 2011-07-28 08:39 PDT by Dão Gottwald [:dao]
Modified: 2016-08-07 12:38 PDT (History)
134 users (show)
mmucci: firefox‑backlog+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch - ~90% done (40.86 KB, patch)
2011-11-14 09:27 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
WIP patch 2 - ~90% done (42.65 KB, patch)
2011-11-14 10:39 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
WIP patch 3 (44.17 KB, patch)
2012-02-13 15:57 PST, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch (39.58 KB, patch)
2012-03-30 09:03 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v2 (38.67 KB, patch)
2012-04-03 07:01 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v3 (40.15 KB, patch)
2012-06-21 16:45 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v3 w/ fixed ifdef (40.20 KB, patch)
2012-06-21 20:45 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
Tab Detach Mockup (92.40 KB, image/png)
2012-07-05 18:30 PDT, Chris Lee (:cleer)
no flags Details
patch v3 merged to mozilla-central tip (40.53 KB, patch)
2012-08-16 02:32 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v3 merged to mozilla-central tip (40.37 KB, patch)
2012-08-16 02:54 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2011-07-28 08:39:06 PDT
Use drag & drop API for tab detaching in order to fix various glitches with the mouse-event based implementation.
Comment 1 Brandon Cheng 2011-07-31 13:30:02 PDT
Wasn't the drag & drop API ditched for the reasons mentioned by Frank Yan at bug 455694, comment 184?

https://bugzilla.mozilla.org/show_bug.cgi?id=455694#c184
Comment 2 Jonathan Haas 2011-08-01 12:44:27 PDT
(In reply to comment #1)
> Wasn't the drag & drop API ditched for the reasons mentioned by Frank Yan at
> bug 455694, comment 184?

We have to use the drag&drop API to properly support dragging tabs to different windows in the taskbar for example. Using simple mouse events sounds like a bad hack IMO.
Comment 3 ithinc 2011-08-09 23:20:06 PDT
Should bug 455694 be backed out to use d&d api?
Comment 4 Jonathan Haas 2011-08-10 00:20:25 PDT
I don't think we want to completely back out bug 455694. We could just modify the code to use the drag&drop API for detached tabs.
Comment 5 Sean Newman 2011-10-02 04:35:28 PDT
This bug has so many dependencies... Frank, could you, please, start working on fixing this bug?
Comment 6 Asa Dotzler [:asa] 2011-10-02 11:02:35 PDT
(In reply to Sean Newman from comment #5)
> This bug has so many dependencies... Frank, could you, please, start working
> on fixing this bug?

Sean, this kind of comment isn't helpful. Unless you're adding new information to a bug, please don't comment. https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Comment 7 Guillaume C. [:ge3k0s] 2011-10-28 02:16:56 PDT
These two bugs [url=https://bugzilla.mozilla.org/show_bug.cgi?id=672085]672085[/url] and [url=https://bugzilla.mozilla.org/show_bug.cgi?id=682559]682559[/url] should be blocking the actual release of this bug.
Comment 8 Dão Gottwald [:dao] 2011-11-01 23:48:57 PDT
*** Bug 455694 has been marked as a duplicate of this bug. ***
Comment 9 u420019 2011-11-05 11:52:21 PDT
Please add an option in "about:config" to activate the implementation of bug 455694 optional, I like this feature and I want to use it. It's too long to wait until Firefox 10/11(oder later?) Is it possible to realize?
Comment 10 Dão Gottwald [:dao] 2011-11-06 02:09:25 PST
*** Bug 700099 has been marked as a duplicate of this bug. ***
Comment 11 Marat Tanalin | tanalin.com 2011-11-06 14:52:35 PST
(In reply to Jukens from comment #9)
> Please add an option in "about:config" to activate the implementation of bug
> 455694 optional, I like this feature and I want to use it.

Indeed, it would be nice to have an about:config option to enable current tab-animation implementation for users who don't use any tab-related add-ons.
Comment 12 Erunno 2011-11-06 21:54:49 PST
(In reply to Marat Tanalin | tanalin.com from comment #11)
> (In reply to Jukens from comment #9)
> > Please add an option in "about:config" to activate the implementation of bug
> > 455694 optional, I like this feature and I want to use it.
> 
> Indeed, it would be nice to have an about:config option to enable current
> tab-animation implementation for users who don't use any tab-related add-ons.

The code for the deficient tab animation implementation has been removed from beta, aurora and nightly builds. There is nothing there that an option could turn on or off.
Comment 13 Marat Tanalin | tanalin.com 2011-11-07 09:42:37 PST
(In reply to Erunno from comment #12)
> The code for the deficient tab animation implementation has been removed
> from beta, aurora and nightly builds.

Thanks, it's quite clear, but does not prevent us from desire for such option (this means that, instead of full backout, existing implementation could be just hidden behind the option as a more appropriate and flexible solution for advanced users).
Comment 14 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-09 10:30:08 PST
(In reply to Marat Tanalin | tanalin.com from comment #13)
> (In reply to Erunno from comment #12)
> > The code for the deficient tab animation implementation has been removed
> > from beta, aurora and nightly builds.
> 
> Thanks, it's quite clear, but does not prevent us from desire for such
> option (this means that, instead of full backout, existing implementation
> could be just hidden behind the option as a more appropriate and flexible
> solution for advanced users).

To add such an option, we would need to (a) re-add the problematic code, (b) add a preference and code to handle turning it on/off (c) maintain that code. So we will not be doing that.
Comment 15 SpeciesX 2011-11-11 11:52:46 PST
(In reply to Jukens from comment #9)
> Please add an option in "about:config" to activate the implementation of bug
> 455694 optional, I like this feature and I want to use it. It's too long to
> wait until Firefox 10/11(oder later?) Is it possible to realize?

Firefox 20+ is more realistic.
Comment 16 Justin Dolske [:Dolske] 2011-11-11 18:50:45 PST
(In reply to speciesx from comment #15)
 
> Firefox 20+ is more realistic.

Comments like this are not helpful, and only serve to further delay work.

This isn't a forum -- Bugzilla is for the people doing the work. https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Comment 17 Frank Yan (:fryn) 2011-11-14 09:27:29 PST
Created attachment 574321 [details] [diff] [review]
WIP patch - ~90% done

Dão, I'd appreciate your feedback on this patch. It's mostly done. The incomplete parts are:
- _getDropIndex() needs a separate code path for tab dragging.
- _animateTabMove() handles some cases poorly.
- dragend/drop should animate tab movement within the tab strip.
- the drag image should be horizontally centered under the cursor.

In particular, I'm not sure what the best approach is for handling all the cases of dragging normal/app tabs in underflow/overflow modes in _animateTabMove() without it getting messy. If you have ideas, please let me know, or even feel free to take the bug and build on my patch.
Comment 18 Frank Yan (:fryn) 2011-11-14 09:35:00 PST
Also, bug 666864 is still a problem. It causes a non-dragged, non-selected tab occasionally to flicker during or immediately before/after drags. I'm not sure how best to fix this besides setting pointer-events: none on non-selected tabs during drags.

---

>  /* WIP: We have to offset the drag image like this, or the panel will
>          capture all the events, and the scroll buttons will not work.
>          Setting allowevents="false" doesn't help at all here. */
>  dt.setDragImage(panel, -1, -1);

Neil, could you help me with this? We'd like the panel to be horizontally centered under the cursor, but once the panel is under the cursor, it captures all the events, and the tab strip scroll buttons no longer work during drags, since they rely on receiving dragover events. A last-resort workaround would be to calculate manually whether the cursor is inside each button's rect, but that is gross and unmaintainable.
Comment 19 Neil Deakin (away until Oct 3) 2011-11-14 09:38:14 PST
(In reply to Frank Yan (:fryn) from comment #18)
> Neil, could you help me with this? We'd like the panel to be horizontally
> centered under the cursor, but once the panel is under the cursor, it
> captures all the events, and the tab strip scroll buttons no longer work
> during drags, since they rely on receiving dragover events. A last-resort
> workaround would be to calculate manually whether the cursor is inside each
> button's rect, but that is gross and unmaintainable.

Did you set <panel type="drag"> ?
Comment 20 Marco Bonardo [::mak] 2011-11-14 09:52:30 PST
This still disables dropping to the bookmarks toolbar, right? That was one of the major complaints of the new approach, which issues should still we fix to allow at least the toolbar as a drop target with this version of the patch?
Comment 21 Frank Yan (:fryn) 2011-11-14 10:39:53 PST
Created attachment 574340 [details] [diff] [review]
WIP patch 2 - ~90% done

(In reply to Neil Deakin from comment #19)
> Did you set <panel type="drag"> ?

No, I forgot that. I added it to this iteration and then tried changing the offsets in setDragImage to positive values, but it appeared to break in the same way that it did without type="drag".

(In reply to Marco Bonardo [:mak] from comment #20)
> This still disables dropping to the bookmarks toolbar, right? That was one
> of the major complaints of the new approach, which issues should still we
> fix to allow at least the toolbar as a drop target with this version of the
> patch?

It doesn't make sense to allow the toolbar as a drop target for drags that didn't begin with a modifier key. You can try out this patch, and you'll see why. When you drag the tab downwards, you appear to pull its entire contents into a panel, so then it's no longer intuitive to drop it as a bookmark, since you're directly manipulating the tab itself.
Comment 22 Marco Bonardo [::mak] 2011-11-14 10:55:44 PST
(In reply to Frank Yan (:fryn) from comment #21)
> (In reply to Marco Bonardo [:mak] from comment #20)
> > This still disables dropping to the bookmarks toolbar, right? That was one
> > of the major complaints of the new approach, which issues should still we
> > fix to allow at least the toolbar as a drop target with this version of the
> > patch?
> 
> It doesn't make sense to allow the toolbar as a drop target for drags that
> didn't begin with a modifier key.

I already got this in the previous version but still, from the newsgroup discussion, Shaver said we may accept drops to the bookmarks toolbar. As this is still the change that got more negative feedback in input.m.o on the previous version, I think we should re-evaluate the decision. Consistency is a good value, but is it worth to replace functionality with an undiscoverable one?
Comment 23 Mikko Rantalainen 2011-11-14 22:56:33 PST
(In reply to Frank Yan (:fryn) from comment #21)
> It doesn't make sense to allow the toolbar as a drop target for drags that
> didn't begin with a modifier key. You can try out this patch, and you'll see
> why. When you drag the tab downwards, you appear to pull its entire contents
> into a panel, so then it's no longer intuitive to drop it as a bookmark,
> since you're directly manipulating the tab itself.

How about the drag-and-dropped "tab" morphs itself into a dragged bookmark when hovered above bookmarks menu? It could then change back to its "tab" display/behavior if the user drags it away from the bookmarks menu. I think that wouldn't be too far from usual file manager drap-and-drop actions (where dragging a file from a folder executes move if the target is on the same volume/media and copy if the target is on another volume/media).
Comment 24 Notlost 2011-11-15 08:39:49 PST
@Mikko
I hadn't noticed it was volume dependant for copy/move, but I did know it changed back and forth. (The reason is moving a file on the same volume is just a change in the $msft, moving between volumes requires you to move the file on the disk)
Comment 25 Frank Yan (:fryn) 2011-11-18 15:46:32 PST
Dão and Neil, you both observed in bug 685702 that the drag&drop testcase was as fast as the mouse-events testcase for you. Is this patch just as fast for you as the mouse events one? It's clearly not the case for me and Limi, especially when dragging around the detach preview panel.

Neil, what can I do to address the panel events issue, or is that something you are looking into?
Comment 26 Dão Gottwald [:dao] 2011-11-26 05:22:56 PST
(In reply to Frank Yan (:fryn) from comment #17)
> Created attachment 574321 [details] [diff] [review] [diff] [details] [review]
> WIP patch - ~90% done
> 
> Dão, I'd appreciate your feedback on this patch. It's mostly done. The
> incomplete parts are:

Is this supposed to be functional yet? Dragging seems completely broken to me on Linux.

> - _getDropIndex() needs a separate code path for tab dragging.

Is this different from what dolske reviewed in bug 455694? Is it just that you're not happy with that code?
Comment 27 Wyatt Childers 2011-11-26 22:38:27 PST
(In reply to Marco Bonardo [:mak] from comment #22)
> (In reply to Frank Yan (:fryn) from comment #21)
> > (In reply to Marco Bonardo [:mak] from comment #20)
> > > This still disables dropping to the bookmarks toolbar, right? That was one
> > > of the major complaints of the new approach, which issues should still we
> > > fix to allow at least the toolbar as a drop target with this version of the
> > > patch?
> > 
> > It doesn't make sense to allow the toolbar as a drop target for drags that
> > didn't begin with a modifier key.
> 
> I already got this in the previous version but still, from the newsgroup
> discussion, Shaver said we may accept drops to the bookmarks toolbar. As
> this is still the change that got more negative feedback in input.m.o on the
> previous version, I think we should re-evaluate the decision. Consistency is
> a good value, but is it worth to replace functionality with an
> undiscoverable one?

If it's going to be capable to drag a tab to the bookmarks bar then a modifier key makes very little sense to me. A modifier emplies that it has something to do with that particular tab in my mind. Why would u hold down a button and then try and drag it? This seems almost boggy in my mind. 

What would be more fluid is if it recondensed when over the tab bar as well as pushed away the other bookmarks for the space. Then uncondensed when moved away. This would show that you could drop it there and that it wouldn't open a new window. 

However dragging a tab inside itself makes no real sense. Personal Opinion is panorama should show when you drag a tab putting your windows intro groups, and you tab groups into sub groups of that window. Then the bookmarks bar could show up at the top by search, and you could daily move windows and tabs around.
Comment 28 Wyatt Childers 2011-11-26 22:39:55 PST
Ignore daily in the last paragraph, typo.
Comment 29 Frank Yan (:fryn) 2011-11-28 15:07:53 PST
(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Frank Yan (:fryn) from comment #17)
> > Dão, I'd appreciate your feedback on this patch. It's mostly done. The
> > incomplete parts are:
> 
> Is this supposed to be functional yet? Dragging seems completely broken to
> me on Linux.

Is it really? It works rather well on OS X. I will test it on Linux then.

> > - _getDropIndex() needs a separate code path for tab dragging.
> 
> Is this different from what dolske reviewed in bug 455694? Is it just that
> you're not happy with that code?

I haven't written any new code for that yet. If you think that the _getDropIndex code that dolske reviewed looks okay, I'll just add that in. If not, please let me know what you think would work better.
Comment 30 Dão Gottwald [:dao] 2011-12-09 06:16:58 PST
(In reply to Frank Yan (:fryn) from comment #29)
> > > - _getDropIndex() needs a separate code path for tab dragging.
> > 
> > Is this different from what dolske reviewed in bug 455694? Is it just that
> > you're not happy with that code?
> 
> I haven't written any new code for that yet.

Is what _getDropIndex *needs* to do different from what dolske reviewed in bug 455694?
I.e. why would you write mew code? I assumed you'd mostly build on what you did in bug 455694. I'm missing the motivation for rewriting it.

> If you think that the
> _getDropIndex code that dolske reviewed looks okay, I'll just add that in.
> If not, please let me know what you think would work better.

I haven't closely reviewed it, but apparently you and dolske thought it was okay at that time. Have you changed your mind?

(In reply to Frank Yan (:fryn) from comment #21)
> (In reply to Marco Bonardo [:mak] from comment #20)
> > This still disables dropping to the bookmarks toolbar, right? That was one
> > of the major complaints of the new approach, which issues should still we
> > fix to allow at least the toolbar as a drop target with this version of the
> > patch?
> 
> It doesn't make sense to allow the toolbar as a drop target for drags that
> didn't begin with a modifier key. You can try out this patch, and you'll see
> why. When you drag the tab downwards, you appear to pull its entire contents
> into a panel, so then it's no longer intuitive to drop it as a bookmark,
> since you're directly manipulating the tab itself.

Fun story: My mom, aunt and grandparents (all bloody novices) somehow managed to populate the bookmarks toolbar with their own stuff. They must have quickly learned from me when I set up webmail for them, or maybe the had the kids do it for them, but those aren't experts either. I'm certain that they didn't use the folder picker for this, and they shouldn't have to. I'm also certain that they wouldn't think of holding Ctrl while dragging. (By the way, they wouldn't rearrange or detach tabs either, so it's not even like we'd give them something in return.)

Combined with the desire to remove the favicon from the location bar, I think we need to keep supporting this. Frankly, I don't care if it looks a little funky, the basic functionality should be there.
Comment 31 Jared Wein [:jaws] (please needinfo? me) 2012-01-08 16:02:38 PST
(In reply to Dão Gottwald [:dao] from comment #30)
> Combined with the desire to remove the favicon from the location bar, I
> think we need to keep supporting this. Frankly, I don't care if it looks a
> little funky, the basic functionality should be there.

Just to note here, removing the favicon from the location bar is still going to keep the behavior of dragging to the bookmarks bar, desktop, tab strip, etc. See bug 588270 for more information.
Comment 32 bogas04 2012-01-19 06:12:59 PST
Any chance of this being fixed for Firefox 12?
Comment 33 Adam [:hobophobe] 2012-01-26 14:37:38 PST
Tried the patch on Linux here.  It's a problem with the drag image, more precisely with displaying the content of the panel used as the drag image.  A ctrl+drag (copy operation), which uses the favicon as the drag image, works fine.  So does an empty panel.  

Tried to use an xul:image as the content, which also failed, so it seems likely that it's not the specific content that's a problem.
Comment 34 Adam [:hobophobe] 2012-01-27 14:49:33 PST
So when the panel shows on Linux, it is grabbed, which cancels the drag.  

When the panel is built, it triggers gtk2/nsWindow.cpp's CaptureRollupEvents(), which happens before the first "drag_motion" signal (which causes nsWindow::sLastDragMotionWindow to be set, which in turn would stop CaptureRollupEvents() from making a grab).  

I found that setting the panel attribute 'noautohide="true"' fixes that, as it prevents the CaptureRollupEvents() from being triggered.  That should be sufficient in this case, as it shouldn't be possible to click while dragging?

Not sure the best remedy if the underlying cause is a bug in its own right (that the nsWindow::sLastDragMotionWindow isn't set fast enough).

I also noticed the content of the panel only gets set if and when the drag enters the content area.  Until then it contains either whatever was beneath the panel when it became visible or it is gray.
Comment 35 Karl Tomlinson (:karlt) 2012-02-01 18:34:16 PST
Do you know why CaptureRollupEvents is called?
Is it necessary for anything or should it not be called for drag panels?

You may find that the implementation of DragInProgress() from attachment 512375 [details] [diff] [review] works better.
Comment 36 Neil Deakin (away until Oct 3) 2012-02-02 01:05:09 PST
No, CaptureRollupEvents isn't needed for drag panels.
Comment 37 Frank Yan (:fryn) 2012-02-13 15:57:36 PST
Created attachment 596834 [details] [diff] [review]
WIP patch 3
Comment 38 Tiger.711 2012-02-14 00:50:50 PST
How soon we'll see try-builds with animation?
Comment 39 Guillaume C. [:ge3k0s] 2012-02-25 07:46:23 PST
There are great ideas that have came (should come) to Chrome : http://www.chromium.org/developers/design-documents/tabtastic-2-requirements
Comment 40 Guillaume C. [:ge3k0s] 2012-03-16 11:28:44 PDT
The UI animations were a P1 priority and since regression it seems that they are completely forgotten. Firefox is the only browser not to provide tab animations...
Comment 41 Asa Dotzler [:asa] 2012-03-16 11:37:33 PDT
(In reply to Guillaume C. [:GE3K0S] from comment #40)
> The UI animations were a P1 priority and since regression it seems that they
> are completely forgotten. Firefox is the only browser not to provide tab
> animations...

This enhancement is no longer a top priority. It will come when we've got other higher priority changes finished.
Comment 42 Frank Yan (:fryn) 2012-03-30 09:03:18 PDT
Created attachment 610902 [details] [diff] [review]
patch

Things that this patch leaves to followups due to complexity:
- Dragging tabs between the app tab and non-app tabs regions
- Animating a tab to its final position upon its release
- Animating a detached tab into a window
- Hiding the window frame when a lone tab is dragged out of it
Comment 43 Marco Bonardo [::mak] 2012-03-30 09:11:19 PDT
hm, I thought we decided to keep accepting drops to bookmarks since the favicon is going aways from the locationbar.
Comment 44 Frank Yan (:fryn) 2012-03-30 09:20:31 PDT
(In reply to Marco Bonardo [:mak] from comment #43)
> hm, I thought we decided to keep accepting drops to bookmarks since the
> favicon is going aways from the locationbar.

We should accept dropping the contents of the URL bar to any Places drop target.
This patch also enables copying the tab (including history) to another location in the tab bar or to a Places drop target by beginning a drag with a modifier.
I really don't want to implement anything that confuses the conceptual model.
Comment 45 Guillaume C. [:ge3k0s] 2012-03-30 09:49:39 PDT
(In reply to Frank Yan (:fryn) from comment #42)
> - Animating a detached tab into a window
This concerns stealing focus from moved tab as Google chrome does ?
>- Dragging tabs between the app tab and non-app tabs regions
The animation should also be present with the contextual menu action.

Will this patch be pushed to UX branch ?
Comment 46 Marco Bonardo [::mak] 2012-03-30 10:02:39 PDT
(In reply to Frank Yan (:fryn) from comment #44)
> We should accept dropping the contents of the URL bar to any Places drop
> target.

we do only if the whole address is selected, not much helpful. Maybe we should also allow to do that if there is no selection, at least.

> This patch also enables copying the tab (including history) to another
> location in the tab bar or to a Places drop target by beginning a drag with
> a modifier.

that was already discussed as a bad solution cause it's undiscoverable

We are basically removing all discoverable ways to bookmark with drag&drop
Comment 47 Guillaume C. [:ge3k0s] 2012-03-30 10:06:19 PDT
(In reply to Marco Bonardo [:mak] from comment #46)
> (In reply to Frank Yan (:fryn) from comment #44)
> > This patch also enables copying the tab (including history) to another
> > location in the tab bar or to a Places drop target by beginning a drag with
> > a modifier.
> 
> that was already discussed as a bad solution cause it's undiscoverable
> 
> We are basically removing all discoverable ways to bookmark with drag&drop
Trade-off are to be made. People should use the star instead.
Comment 48 Thomas Ahlblom 2012-03-30 11:11:50 PDT
(In reply to Guillaume C. [:GE3K0S] from comment #47)
> > We are basically removing all discoverable ways to bookmark with drag&drop
> Trade-off are to be made. People should use the star instead.

I do hope I misunderstand this.

The star doesn't even let people select where the bookmark should appear. It just lets people pop up a tunnel vision-dialog and select a folder in which they later on have try to clean up the mess. Instead of one single click using drag & drop the star may require like 10 clicks to do the same thing.
Comment 49 Dão Gottwald [:dao] 2012-03-30 11:13:01 PDT
(In reply to Guillaume C. [:GE3K0S] from comment #47)
> (In reply to Marco Bonardo [:mak] from comment #46)
> > (In reply to Frank Yan (:fryn) from comment #44)
> > > This patch also enables copying the tab (including history) to another
> > > location in the tab bar or to a Places drop target by beginning a drag with
> > > a modifier.
> > 
> > that was already discussed as a bad solution cause it's undiscoverable
> > 
> > We are basically removing all discoverable ways to bookmark with drag&drop
> Trade-off are to be made.

Well, sure. I'm not convinced that tab reordering and detaching is (or is going to be) more popular than dragging tabs to the bookmarks toolbar, though.
Comment 50 Jared Wein [:jaws] (please needinfo? me) 2012-03-30 12:01:35 PDT
(In reply to Marco Bonardo [:mak] from comment #43)
> hm, I thought we decided to keep accepting drops to bookmarks since the
> favicon is going aways from the locationbar.

Not until bug 588270 is fixed. Dragging the site-identity block is much more prominent and pushback on removing that feature should take place in bug 588270.

(In reply to Marco Bonardo [:mak] from comment #46)
> We are basically removing all discoverable ways to bookmark with drag&drop

(In reply to Dão Gottwald [:dao] from comment #49)
> Well, sure. I'm not convinced that tab reordering and detaching is (or is
> going to be) more popular than dragging tabs to the bookmarks toolbar,
> though.

We should defer to UX here as they should own all the interactions of the browser. If the UX team decides that this patch is the direction that we want to go in, then we should follow that.
Comment 51 Marco Bonardo [::mak] 2012-03-30 12:27:53 PDT
(In reply to Jared Wein [:jaws] from comment #50)
> Not until bug 588270 is fixed. Dragging the site-identity block is much more
> prominent

Are we going to have a site identity block on any uri, even common http ones? The example there is a secure page.

(In reply to Jared Wein [:jaws] from comment #50)
> We should defer to UX here as they should own all the interactions of the
> browser

No doubt, though last time we pushed this, we got a ton of negative feedback on the bookmarks regression, we should also listen to that.  I absolutely love tabs animations, but the price/gain balance is still functionality/animations.
Comment 52 Alex Limi (:limi) — Firefox UX Team 2012-03-30 13:49:43 PDT
(In reply to Jared Wein [:jaws] from comment #50)
> (In reply to Marco Bonardo [:mak] from comment #43)
> > hm, I thought we decided to keep accepting drops to bookmarks since the
> > favicon is going aways from the locationbar.
> 
> Not until bug 588270 is fixed. Dragging the site-identity block is much more
> prominent and pushback on removing that feature should take place in bug
> 588270.

URLs will be drag/drop-capable, and site identity block too.

> (In reply to Dão Gottwald [:dao] from comment #49)
> > Well, sure. I'm not convinced that tab reordering and detaching is (or is
> > going to be) more popular than dragging tabs to the bookmarks toolbar,
> > though.
> 
> We should defer to UX here as they should own all the interactions of the
> browser. If the UX team decides that this patch is the direction that we
> want to go in, then we should follow that.

Reordering tabs is a lot more common than bookmarking via drag & drop. We also don't ship the bookmarks toolbar enabled by default since Firefox 4, so we're already in a tiny percentage of the population here. 

Reordering takes precedence over bookmarking.
Comment 53 Alex Limi (:limi) — Firefox UX Team 2012-03-30 13:52:47 PDT
In other words, patch as described in comment 42 is what we want.
Comment 54 Dão Gottwald [:dao] 2012-03-30 14:46:29 PDT
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #52)
> Reordering tabs is a lot more common than bookmarking via drag & drop.

Do we actually know that? (I have no doubt that it's executed more often. I have doubts that more users do it.)

> We
> also don't ship the bookmarks toolbar enabled by default since Firefox 4, so
> we're already in a tiny percentage of the population here. 

Good point about the bookmarks toolbar not being enabled by default. This means that we're not even talking about what the more popular action for all users is, but only for those who have the bookmarks toolbar enabled.
Comment 55 Frank Yan (:fryn) 2012-03-30 15:28:08 PDT
Dão, I understand your design & usability concerns, but are you willing/planning to review the patch on its technical merits despite these? (Not expecting an immediate review of course, and there almost certainly are things I need to fix technically after this first pass.) If not, I can ask another Fx peer for feedback.
Comment 56 Guillaume C. [:ge3k0s] 2012-03-30 16:04:59 PDT
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #53)
> In other words, patch as described in comment 42 is what we want.

Make perfect sense. If I don't misunderstood it's exactly the same that Chrome does (in fact the best approach in current browsers).
Comment 57 Josh Matthews [:jdm] 2012-03-30 16:12:40 PDT
(In reply to Dão Gottwald [:dao] from comment #54)
> (In reply to Alex Limi (:limi) — Firefox UX Team from comment #52)
> > Reordering tabs is a lot more common than bookmarking via drag & drop.
> 
> Do we actually know that? (I have no doubt that it's executed more often. I
> have doubts that more users do it.)
> 
> > We
> > also don't ship the bookmarks toolbar enabled by default since Firefox 4, so
> > we're already in a tiny percentage of the population here. 
> 
> Good point about the bookmarks toolbar not being enabled by default. This
> means that we're not even talking about what the more popular action for all
> users is, but only for those who have the bookmarks toolbar enabled.

I presume we haven't actively disabled the toolbar for existing users with non-default bookmarks inside of it. It's entirely possible we have a crowd of users for whom the change in defaults did not make a difference at all.
Comment 58 rob64rock 2012-03-30 18:07:22 PDT
You might want to take this into consideration.

On prior Fx versions before the New Bookmarks Menu was place on the nav-bar, users fell into 3 categories:
Those that used the bookmarks toolbar, those that used the bookmarks sidebar, and those that didn't use either.

With the Bookmarks SideBar being removed off the nav-bar and the New Bookmarks Menu Button placed on the nav-bar.

New or average Fx users mistaken the new Bookmarks Menu button for Bookmarks Sidebar button, and those that don't know about the "Customize Toolbar" options or don't ask where the Bookmark Sidebar button went, settle for using the option in the New Bookmarks Menu button for using the Bookmarks Toolbar. So, I would think that with this happening of more new or average Fx users using the Bookmarks Toolbar more than the Bookmarks Sidebar you would reconsider this bug effects it will have on bookmark users.

Without another Test Pilot study being done regarding Bookmark toolbar's and Bookmark Sidebar usage since the landing of the New Bookmarks Menu, you inadvertently may do more harm with landing Bug 674925.

Note: From Fx 1.5 to Fx 4.0 I used the Bookmarks Sidebar, but I have now converted to using the Bookmarks Toolbar, I figure I'm not the only above average Fx user that has done so. So with more average and above average Fx users using the Bookmarks Toolbar you may need do a new Test Pilot study to know for certain.
Comment 59 avada 2012-03-30 23:23:10 PDT
I always used drag and drop for bookmarking. Because that's the fastest way to get the bookmark where I want it. I suspect a significant amount of people also do. Why is the desperation to make bookmarking cumbersome?

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #52)
> Reordering takes precedence over bookmarking.
If you can have both why axe one feature?

(In reply to Marco Bonardo [:mak] from comment #46)

> We are basically removing all discoverable ways to bookmark with drag&drop

True. Having only the url d&d is as good as nothing. You have to select the whole thing and it uses the url as the bookmark name instead of the page title, so useless.
(In reply to Marco Bonardo [:mak] from comment #51)
> No doubt, though last time we pushed this, we got a ton of negative feedback
> on the bookmarks regression, we should also listen to that.  I absolutely
> love tabs animations, but the price/gain balance is still
> functionality/animations.

I'm sure you will get a ton again and then some, if you remove the other ways to drag too.
Comment 60 Eriatile 2012-03-30 23:53:54 PDT
What does it hurt to let people be able to drag and drop into bookmark toolbar ?
From what I read people against it are people saying bookmark toolbar is not display for a lot of user. It doesn't make a difference for most people to implement it but make a lot for those who do. Does it really add great complexity ?

Beside is there away to know which percentage of users :

- are actually reordering tabs ? I would think it's a minority ?
If users on average have only a few tab (7/8 ?) won't it be surprising if they needed reordering tabs ?

- have bookmark toolbar displayed and reorder tabs ?
- have bookmark toolbar displayed and drag and drop tabs into it ?
Comment 61 [not reading bugmail] 2012-03-31 01:20:42 PDT
I don't see why we have want to drop the favicon from location bar (while also moving the star to more logical place to the bookmarks toolbar menu button), but I for sure would not expect that the average user will understand the identity block will be D&D discoverable and able to D&D to the bookmarks toolbar and more so frustrating users of that functionality if you do.  If you remove that, well then I would hate having to lose that functionality just so I can reorder my tabs which has nothing to do with reordering bookmarks or dragging my favicon to the bookmarks toolbar which is super fast compared with having to star/edit/select folder/ok.
Comment 62 annaeus 2012-03-31 02:41:15 PDT
My thoughts : I've always used D&D tabs to bookmark tabs even thought i've never used the bookmark toolbar. I've been using this feature ever since i first started using the internet with fx2, so i'm clearly against removing it. 

Animations/design should never outweigh features especially, such as these which used very often and apparently by a lot of users.
Comment 63 Marco Castelluccio [:marco] 2012-03-31 03:16:16 PDT
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #52)
> Reordering tabs is a lot more common than bookmarking via drag & drop. We
> also don't ship the bookmarks toolbar enabled by default since Firefox 4, so
> we're already in a tiny percentage of the population here. 

Having the bookmark toolbar enabled isn't necessary to use bookmarking via drag & drop. I haven't the toolbar enabled but I drag & drop tabs in the bookmark menu button, that is enabled by default.
Comment 64 Lozzy 2012-03-31 03:30:42 PDT
Just curious, would it be acceptable in the eyes of Fryn and Limi to simply have hover delay? If a user wants to drop a tab onto the bookmarks toolbar they would be required to hover over the toolbar for a brief moment before the tab transforms into a drop indicator.

That would mean that people who want to drag it elsewhere won't see the transformation and people who are expecting to be able to drop it on the toolbar will probably hover long enough to learn the new interaction. I think it's a compromise worth thinking about.
Comment 65 Constantine 2012-03-31 04:02:23 PDT
(In reply to Lozzy from comment #64)
> Just curious, would it be acceptable in the eyes of Fryn and Limi to simply
> have hover delay? If a user wants to drop a tab onto the bookmarks toolbar
> they would be required to hover over the toolbar for a brief moment before
> the tab transforms into a drop indicator.
> 
> That would mean that people who want to drag it elsewhere won't see the
> transformation and people who are expecting to be able to drop it on the
> toolbar will probably hover long enough to learn the new interaction. I
> think it's a compromise worth thinking about.

It may be a good solution. But why not simply add ability to D&D to the bookmarks toolbar and other places which used to work before? Isn't there enough space to D&D onto for creating new windows (I mean a whole content area)? What are the limitations?
I also don't use the bookmarks panel. I usually open bookmarks in the sidebar via a shortcut or a mouse gesture and D&D onto it. Some extensions (Scrapbook+) behave the same. In any case breaking it looks like a step backwards in usability.
Comment 66 Emanuele Alimonda 2012-03-31 05:45:20 PDT
(In reply to avada from comment #59)
> (In reply to Alex Limi (:limi) — Firefox UX Team from comment #52)
> > Reordering takes precedence over bookmarking.
> If you can have both why axe one feature?

I, for one, am annoyed when a tab I'm reordering gets bookmarked.  The tab bar (as a long-time user, I'm still using tabs on bottom) is really close to the bookmarks toolbar, and it's really easy for your tabs to become bookmarks without noticing.  When I really want to bookmark something, I drag the identity box into the bookmarks toolbar (I'm aware the identity box is going away, even though I wish it could stay - I've been using that for quite a while - but that's handled elsewhere so I will say no more).  I was happy when this was pushed last time, as I finally could reorder tabs without worrying about them getting into my bookmarks toolbar (and getting a visual feedback of the operation I was about to do.

Perhaps I represent a minority of the Fx user base, but it's just to say not everyone agrees to some of the comments above.

(In reply to Lozzy from comment #64)
> Just curious, would it be acceptable in the eyes of Fryn and Limi to simply
> have hover delay? If a user wants to drop a tab onto the bookmarks toolbar
> they would be required to hover over the toolbar for a brief moment before
> the tab transforms into a drop indicator.
> 
> That would mean that people who want to drag it elsewhere won't see the
> transformation and people who are expecting to be able to drop it on the
> toolbar will probably hover long enough to learn the new interaction. I
> think it's a compromise worth thinking about.

That would be great to me.  It would be okay for my tabs being dragged around to become bookmarks, if there was a delay (say 1 second) and a visual feedback of what would happen.  And by visual feedback, I don't mean the barely visible drop indicator we currently have, I'd rather prefer seeing the tab taking the shape of a bookmark bar entry AND seeing the "+" indicator next to the mouse pointer, as it happens in Safari and in Chrome (not sure about the Windows version, I've only ever tried the Mac one) when you drag the favicon to the bookmarks toolbar for example. (on a side note, Chrome no longer seems to have a favicon, but it has a "globe" icon that behaves like the Fx identity box, that you can still drag.)
Comment 67 Guillaume C. [:ge3k0s] 2012-04-01 09:10:21 PDT
Some thoughts about the implementation as seen in the 31 march UX build.
-The dragged tab appears as a miniature when you click on the tab. It should appears only when dragged out of the tab strip.
-The thumbnail was better in the previous implementation. Partial thumbnail is a bit weird to see. Have the whole window represented was better. A trade off - complete site thumbnail with the tab strip represented (exactly like in Google Chrome)- could be the solution.
-Bug when a plugin has stolen page focus (Flash for example). The dragged tab isn't visible.
- Drag and drop between windows has the same issue. The dragged tab disappears.

I also had a strange bug with dragged text (or even objects like in the new tab page) that always tried to load a URL when drag and drop on the page. But I don't if that was related to this bug.
Comment 68 Girish Sharma [:Optimizer] 2012-04-02 23:25:31 PDT
Can you post the changes in the patch that just got pushed to UX branch ?
Comment 69 Frank Yan (:fryn) 2012-04-03 07:01:51 PDT
Created attachment 611795 [details] [diff] [review]
patch v2

Changed detach threshold to cover the entire #navigator-toolbox rect, so that tabs can be dropped onto the bookmarks toolbar if it exists. The reattach threshold is still the tabstrip rect itself.

This patch is causing two test timeouts on Linux, due to GTK asserting and probably actually breaking, but I won't have time to debug this until next week.
Comment 70 Marco Bonardo [::mak] 2012-04-03 07:16:17 PDT
To clarify (if further needed), the problem is not just the toolbar.
there are many use-cases we are breaking:
1. drop to the toolbar
2. drop to the toolbar overflow area
3. drop to the menubar
4. drop to the bookmarks button
5. drop to the library button
6. drop to the bookmarks sidebar (quick organize into folders)
7. drop to the Library
the patch is just supporting (1), right?
Comment 71 Marco Bonardo [::mak] 2012-04-03 07:19:05 PDT
hm or maybe even (2), iirc the parent is still the toolbar for the chevron menu.
Comment 72 Guillaume C. [:ge3k0s] 2012-04-03 11:04:05 PDT
(In reply to Guillaume C. [:GE3K0S] from comment #67)
> Some thoughts about the implementation as seen in the 31 march UX build.
> -The dragged tab appears as a miniature when you click on the tab. It should
> appears only when dragged out of the tab strip.
> -The thumbnail was better in the previous implementation. Partial thumbnail
> is a bit weird to see. Have the whole window represented was better. A trade
> off - complete site thumbnail with the tab strip represented (exactly like
> in Google Chrome)- could be the solution.
> -Bug when a plugin has stolen page focus (Flash for example). The dragged
> tab isn't visible.
> - Drag and drop between windows has the same issue. The dragged tab
> disappears.
> 
> I also had a strange bug with dragged text (or even objects like in the new
> tab page) that always tried to load a URL when drag and drop on the page.
> But I don't if that was related to this bug.

Still a problem in the last UX build.
Comment 73 Guillaume C. [:ge3k0s] 2012-04-03 13:20:00 PDT
There could also be nice enhancements : in Chrome when you drag a tab to the border of the window you have a nice animation and you can drop the tab here to have the two side to side.
Comment 74 Tiger.711 2012-05-03 23:25:48 PDT
Can you land the patch back to the UX, please?
Comment 75 Justin Dolske [:Dolske] 2012-05-06 19:51:40 PDT
Dao: is there anything still holding up beginning review of this patch?
Comment 76 Dão Gottwald [:dao] 2012-05-07 01:54:02 PDT
Comment on attachment 611795 [details] [diff] [review]
patch v2

I was told that Frank is working on an updated patch addressing some feedback.
Comment 77 Frank Yan (:fryn) 2012-06-21 16:45:40 PDT
Created attachment 635529 [details] [diff] [review]
patch v3
Comment 78 Jared Wein [:jaws] (please needinfo? me) 2012-06-21 18:21:31 PDT
On Windows Aero:

Open two windows with a single tab
Make window A maximized
Make window B restored
Tear off the tab from window B
Window B is now only glass
Release the tab on top of window A.
window B appears beneath window A (and therefore cannot be seen without altering window order)

Expected results:
window B does not turn in to all glass (drag should probably not be allowed in this case)
window B stays on top of window A
Comment 79 Jared Wein [:jaws] (please needinfo? me) 2012-06-21 18:39:41 PDT
I talked with Felipe about this and he told me that dragging a standalone tab to another window is necessary for merging that tab in to another tabstrip. The other issue I found should just be pushed to a followup bug so that it doesn't make this patch larger/more complicated.
Comment 80 Frank Yan (:fryn) 2012-06-21 20:45:12 PDT
Created attachment 635593 [details] [diff] [review]
patch v3 w/ fixed ifdef

fixed ifdef logic
Comment 81 Guillaume C. [:ge3k0s] 2012-06-22 02:42:42 PDT
Nice but there is still a bug with this implementation : when you drag and drop content on the page itself (texte, link, etc.) the browser try to go to the page or search a corresponding address for the text.
Comment 82 Michał 2012-06-22 03:09:40 PDT
I just have downloaded the latest-ux build, and I must say that this "Tab Animation" performance is still laggy and this state is unacceptable.
Even IE9 has better Tab Animation - it feels smooth and easy to drap tabs, while in Firefox it's only pain in the ass.

I don't know if it's based on the same Drag&Drop mechanism as New Tab Page, but if it is, devs should first fix performance problems with that mechanism.
Comment 83 Csaba Kozák [:WonderCsabo] 2012-06-22 03:39:54 PDT
I found an interesting problem:
Close Fx w/ more than one tab, and save the session. Then reopen the browser. Try to move a tab (not the actual). When you click on it to move, the tab will load, and you cannot move it, because the ui locks. That's very bad ux. :(

The other thing, when you want to move the last tab from a window to another, you cannot, because the window wont be hidden, just a full glass thing. But i guess that's a known problem.
Comment 84 Jared Wein [:jaws] (please needinfo? me) 2012-06-22 10:37:49 PDT
(In reply to Michał from comment #82)
> I just have downloaded the latest-ux build, and I must say that this "Tab
> Animation" performance is still laggy and this state is unacceptable.

The performance issues aren't due to the patch here, but need to be fixed in a lower level area. Getting this bug fixed will elevate the priority of fixing the performance issues :)
Comment 85 guillaume.goessel 2012-06-25 03:01:23 PDT
May the other tabs move following the same accelerate-decelerate-pattern that the smooth scrolling ?
also the tab when picked out of the bar doesn't look the same as before (no fade)

Sorry for my bad english
Comment 86 Guillaume C. [:ge3k0s] 2012-07-02 05:16:50 PDT
Another useful thing : tabs dragged from the tab strip to create a new window shouldn't appear in maximized mode, even if the parent window is maximized. Currently only Internet Explorer does it that way, but I think it is an important feature.
Comment 87 Jared Wein [:jaws] (please needinfo? me) 2012-07-02 10:21:26 PDT
Frank, have you had a chance to fix some of the issues that have been mentioned in the previous comments?
Comment 88 Frank Yan (:fryn) 2012-07-02 10:46:29 PDT
(In reply to Jared Wein [:jaws] from comment #87)
> Frank, have you had a chance to fix some of the issues that have been
> mentioned in the previous comments?

Which of the issues?
I don't think most of them need to be fixed in this bug, and I haven't reproduced Csaba's first issue.
Comment 89 Guillaume C. [:ge3k0s] 2012-07-02 15:54:47 PDT
(In reply to Guillaume C. [:ge3k0s] from comment #81)
> Nice but there is still a bug with this implementation : when you drag and
> drop content on the page itself (texte, link, etc.) the browser try to go to
> the page or search a corresponding address for the text.

I still see this one with a new profile and all.
Comment 90 Hugh Nougher [:Hughman] 2012-07-02 17:58:24 PDT
(In reply to Guillaume C. [:ge3k0s] from comment #81)
> Nice but there is still a bug with this implementation : when you drag and
> drop content on the page itself (texte, link, etc.) the browser try to go to
> the page or search a corresponding address for the text.

This issue is unrelated to tab detaching. As far as I can remember the browser would navigate to the address of the text which has been dropped onto it, unless the page itself explicitly handles the drop.

If you want this to change I suggest filing a new bug with how you think it should work, if there is not already a bug about it.
Comment 91 Jared Wein [:jaws] (please needinfo? me) 2012-07-02 18:10:11 PDT
(In reply to Hugh Nougher [:Hughman] from comment #90)
> (In reply to Guillaume C. [:ge3k0s] from comment #81)
> > Nice but there is still a bug with this implementation : when you drag and
> > drop content on the page itself (texte, link, etc.) the browser try to go to
> > the page or search a corresponding address for the text.
> 
> This issue is unrelated to tab detaching. As far as I can remember the
> browser would navigate to the address of the text which has been dropped
> onto it, unless the page itself explicitly handles the drop.
> 
> If you want this to change I suggest filing a new bug with how you think it
> should work, if there is not already a bug about it.

This is a regression introduced by this patch. To reproduce, perform the following stpes:

1) open a webpage with an image on it <https://msujaws.wordpress.com/2012/06/18/get-a-sneak-preview-at-the-new-australis-tabs-windows-only/>
2) drag the image located in the blog post and release it on the web content

Expected:
the user should see a circle with line through it saying that it can't be dropped there

Actual:
dropping it on the page navigates to the image. this was introduced by this patch and should be removed.
Comment 92 Jared Wein [:jaws] (please needinfo? me) 2012-07-02 18:35:06 PDT
(In reply to Jared Wein [:jaws] from comment #91) 
> This is a regression introduced by this patch. To reproduce, perform the
> following stpes:
> 
> 1) open a webpage with an image on it
> <https://msujaws.wordpress.com/2012/06/18/get-a-sneak-preview-at-the-new-
> australis-tabs-windows-only/>
> 2) drag the image located in the blog post and release it on the web content
> 
> Expected:
> the user should see a circle with line through it saying that it can't be
> dropped there
> 
> Actual:
> dropping it on the page navigates to the image. this was introduced by this
> patch and should be removed.

There is a step zero in that you must first rearrange tabs. The event handlers don't seem to be cleaned up properly in this case.
Comment 93 Csaba Kozák [:WonderCsabo] 2012-07-03 03:12:45 PDT
(In reply to Frank Yan (:fryn) from comment #88)
> (In reply to Jared Wein [:jaws] from comment #87)
> > Frank, have you had a chance to fix some of the issues that have been
> > mentioned in the previous comments?
> 
> Which of the issues?
> I don't think most of them need to be fixed in this bug, and I haven't
> reproduced Csaba's first issue.

I just tried again w/ the latest UX build. I can still reproduce my issue. When clicking on the other tab to move, it'll start to load, and makes the browser irresponsible, so you cannot move the tab/moving is very-very slow. I'm on Windows 7. A possible solution is to load the tab only after it was moved and released by the user.
Comment 94 Guillaume C. [:ge3k0s] 2012-07-04 10:28:35 PDT
There is also a lack of consistency between OSX and Windows implementation. The dragged thumbnail on mac has an header containing the title of the tab. This doesn't appear on Windows and so the thumbnail looks weird. OSX implementation should be the reference in this case.
Comment 95 Chris Lee (:cleer) 2012-07-05 18:30:43 PDT
Created attachment 639550 [details]
Tab Detach Mockup

What about something like this?
Comment 96 Guillaume C. [:ge3k0s] 2012-07-06 00:18:10 PDT
(In reply to Chris Lee (:cleer) from comment #95)
> Created attachment 639550 [details]
> Tab Distach Mockup
> 
> What about something like this?

This looks good and current OSX implementation looks like this (without the favicon), but I keep thinking that a more talking tab representation could be better (current Chrome implementation or first interaction mockup : http://www.stephenhorlander.com/images/blog-posts/tab-animation/animation-tab-tear-off.html )
Comment 97 Girish Sharma [:Optimizer] 2012-08-09 04:56:16 PDT
The latest patch in the UX is very good. It has one bug though.
If you have 2 tabs (or more) on tab strip. Say [0][1] .
You start dragging 0 to the right of 1, it works perfectly and the tab moves with your mouse, but in that draging, without leaving the drag if you tryo to bring back the tab 0 to the left of 1, the tab stop following the mouse as soon as your mouse comes over tab 1.

That is if you started grabbing tab 0 from the middle of the tab (horizontally middle), move it to the right of tab 1, then try to move it back to its original position, it will stop at its position as soon as its half over tab 1.
Comment 98 Guillaume C. [:ge3k0s] 2012-08-11 15:02:56 PDT
Patch is broken for me on the last UX branch. Detaching the tab from the tab strip doesn't work anymore : the thumbnail doesn't appear.
Comment 99 Notlost 2012-08-11 16:53:09 PDT
Can reproduce Comment 98. Detach doesn't work for me either. Undetached animatin is the same as in Comment 97.
Comment 100 Dão Gottwald [:dao] 2012-08-13 06:04:15 PDT
Comment on attachment 635593 [details] [diff] [review]
patch v3 w/ fixed ifdef

I can reproduce the issues mentioned in comment 97 and comment 98 in the UX branch nightly on Windows. Not sure if this is due to a real bug or a messed up merge. Anyway, trying to reproduce this by applying the patch myself brought my attention to the panel being hidden intentionally on Linux, which doesn't seem like an acceptable user experience for a tier-1 platform.
Comment 101 Frank Yan (:fryn) 2012-08-14 15:16:42 PDT
(In reply to Dão Gottwald [:dao] from comment #100)
> I can reproduce the issues mentioned in comment 97 and comment 98 in the UX
> branch nightly on Windows. Not sure if this is due to a real bug or a messed
> up merge.

My guess is merge, because I haven't changed the patch since I posted it.

> Anyway, trying to reproduce this by applying the patch myself
> brought my attention to the panel being hidden intentionally on Linux, which
> doesn't seem like an acceptable user experience for a tier-1 platform.

If that's not acceptable, we'll need a platform fix, before we can move forward here.
Comment 102 Justin Dolske [:Dolske] 2012-08-14 21:02:17 PDT
Comment on attachment 635593 [details] [diff] [review]
patch v3 w/ fixed ifdef

Dao: Actually, I think it would be most helpful if you made a full review pass on this, even in the current state.  This is a priority for Australis, and I want to shake out any remaining review issues ASAP. This bug has unfortunately languished for far too long, so it's worth some extra effort to get it back on track. Getting a full review pass will help clarify what other changes may be needed -- I don't want to do that one slow step at a time.

If a bad merge or other change makes this not work right on current-trunk, let's at least get a pass going with the Hg changeset this patch was based on.
Comment 103 Justin Dolske [:Dolske] 2012-08-14 21:02:56 PDT
Comment on attachment 635593 [details] [diff] [review]
patch v3 w/ fixed ifdef

(grr, I fail at flags)
Comment 104 Dão Gottwald [:dao] 2012-08-15 00:59:59 PDT
Comment on attachment 635593 [details] [diff] [review]
patch v3 w/ fixed ifdef

Frank, please merge this to mozilla-central tip to rule out or confirm that this only stopped working on Windows due to the patch having been misapplied when landing on the UX branch.

Secondly, while I can make comments on the code, I won't be able to grant this review since I don't think it ever worked on Linux.

Finally, I'd generally appreciate if the issues pointed out by various testers over the last few months would get more attention.
Comment 105 Adam [:hobophobe] 2012-08-15 18:35:03 PDT
While looking at bug 782811 using the patch here, I ran into a separate (related?) bug.  Even when working around that bug, the panel wasn't showing, except a brief flicker when a drag began.  Tracked it down to the style:

(patched version) browser/base/content/browser.css:91:

.tab-drag-panel:not([drag=detach]):not([drag=drop]) {
  opacity: 0;
}

Not sure if this is a problem with |drag| attribute inheritance or something else yet.
Comment 106 Justin Dolske [:Dolske] 2012-08-15 21:47:59 PDT
Comment on attachment 635593 [details] [diff] [review]
patch v3 w/ fixed ifdef

As I noted before, this should be ready for review on the changeset it was originally built upon: e240d6e43c9a675ecbaea450d96cf55601ea0119

My expectation is that as a large patch, you should be able to identify any other significant issues with it, so that when we produce an updated patch (rebased to current-trunk and with those comments addressed), there will be a minimum of further changes to be done before landing. [With the usual caveat that an r+ is hard to predict as the number of currently needed changes goes up.]

In other words, we know the current patch is an r- based on (1) linux issues and (2) the UX-branch merge possibly being wrong.  But is that it? What is the total best-effort list of issues (large and small) that this patch needs to address before landing?

I know this is is an unusual path to be taking, but I think it's what we need to do to help get this out of the months-long rut this patch has been stuck in awaiting review. There's no easy path here, alas.

Frank is going to be focusing on some other tasks now, but I'll find a new owner for this ASAP. Having a list of to-dos from a reviewer will make that hand-off more successful.

Additionally, if you'd rather have someone else take this review that's fine. I've asked this go to your queue simply because you know tab-related code and had concerns about the previously landed-but-now-backed-out patch. We have other competent reviewers to share the burden -- just let me know. :)

Let's chat if there are any further concerns.
Comment 107 Dão Gottwald [:dao] 2012-08-16 00:12:56 PDT
(In reply to Justin Dolske [:Dolske] from comment #106)
> In other words, we know the current patch is an r- based on (1) linux issues
> and (2) the UX-branch merge possibly being wrong.  But is that it?

The problem is not that the UX branch merge is possibly wrong, but that the patch was possibly applied /correctly/, meaning that there's a deeper issue with it regardless of whether it worked against an old revision. And then, skimming through the last bunch of comments, I can identify issues (3) and (4) in comment 83 and (5) in comment 91. Any of these points may require invasive changes.
Comment 108 Dão Gottwald [:dao] 2012-08-16 02:32:03 PDT
Created attachment 652374 [details] [diff] [review]
patch v3 merged to mozilla-central tip

This may or may not suffer from the same breakage that's currently on the UX branch. I haven't tested this yet.
Comment 109 Dão Gottwald [:dao] 2012-08-16 02:54:11 PDT
Created attachment 652378 [details] [diff] [review]
patch v3 merged to mozilla-central tip
Comment 110 Dão Gottwald [:dao] 2012-08-16 08:07:18 PDT
Comment 97 is definitely not due to the merge but an issue with this patch. I ran into it again and solved it in bug 783282.
Comment 111 Guillaume C. [:ge3k0s] 2012-08-19 05:02:02 PDT
Just as info there is a new flag in the last Chrome dev build to enable a complete view (as a window) of distached tab instead of thumbnail. I think it should be considered for Firefox too.
Comment 112 Frank Yan (:fryn) 2012-08-30 16:25:43 PDT
Comment on attachment 639550 [details]
Tab Detach Mockup

In bug 455694, I had already implemented this kind of interaction with UX approval, so there doesn't need to be any more UX review regarding this mockup.

I no longer have a screencast of that particular interaction available, but here are some related links:
a prototype from two years ago:
https://bug455694.bugzilla.mozilla.org/attachment.cgi?id=463283
a screencast of an early patch showing tab detach on Windows:
https://bug455694.bugzilla.mozilla.org/attachment.cgi?id=545132
Comment 113 Guillaume C. [:ge3k0s] 2012-08-31 01:43:01 PDT
Currently this (http://www.chromium.org/developers/design-documents/tabtastic-2-requirements) is being implemented to Chrome on Windows (I think it's already done on OSX). It's a more modern way to handle drag and drop.
Comment 114 Jared Wein [:jaws] (please needinfo? me) 2012-09-26 22:16:20 PDT
*** Bug 794474 has been marked as a duplicate of this bug. ***
Comment 115 Dão Gottwald [:dao] 2013-02-15 10:36:13 PST
*** Bug 841817 has been marked as a duplicate of this bug. ***
Comment 116 Josiah Bruner [:JosiahOne] (needinfo for responses) 2013-02-15 10:51:31 PST
So, this is essentially the behavior we are trying to achieve. https://www.dropbox.com/s/e321ubedrr67t96/Safari.mov

That is what Safari does on OS X, and looks quite nice. We should shoot for a similar experience.
Comment 117 Guillaume C. [:ge3k0s] 2013-02-15 11:39:20 PST
(In reply to Josiah [:JosiahOne] from comment #116)
> So, this is essentially the behavior we are trying to achieve.
> https://www.dropbox.com/s/e321ubedrr67t96/Safari.mov
> 
> That is what Safari does on OS X, and looks quite nice. We should shoot for
> a similar experience.

I personnaly prefer IE and Chrome behaviour (especially IE since the moved content stays in restored mode even if the browser is maximized).
Comment 118 Josiah Bruner [:JosiahOne] (needinfo for responses) 2013-02-15 11:48:54 PST
(In reply to Guillaume C. [:ge3k0s] from comment #117)
> (In reply to Josiah [:JosiahOne] from comment #116)
> > So, this is essentially the behavior we are trying to achieve.
> > https://www.dropbox.com/s/e321ubedrr67t96/Safari.mov
> > 
> > That is what Safari does on OS X, and looks quite nice. We should shoot for
> > a similar experience.
> 
> I personnaly prefer IE and Chrome behaviour (especially IE since the moved
> content stays in restored mode even if the browser is maximized).

After looking at this, I agree that IE's behavior is the probably ideal.
Comment 119 Philipp Sackl [:phlsa] (Firefox UX) please use needinfo 2014-01-22 06:14:56 PST
*** Bug 962551 has been marked as a duplicate of this bug. ***
Comment 120 Johan C 2015-01-18 09:33:28 PST
Adding bug 563540 to "see also", wasn't able to add it as a dependency due to the fact that it would create a circular dependency.

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