ithinc Reporter
	Description • 13 years ago

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110503 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110503 Firefox/6.0a1

Is it necessary to keep an "orphan tab" concept? Why is it not a group with only one tab? If it is just for display reason, CSS could help. I'm sure many things could be simplified with the "orphan tab" concept removed.

Reproducible: Always

	Ian Gilman [:iangilman]
	Comment 1 • 13 years ago

Orphan tabs already act like groups of one anyway; I agree we should update the code to make that explicit. It remains to be seen whether we would want to update the visual display as well. I'm cc'ing Aza, as he may have some thoughts about what aspects of the orphan tab concept we may or may not want to hold on to.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 2 • 13 years ago

With the new internal API for global tab group storage we actually don't allow "real orphan tabs" anymore. They're implemented as tab groups with a special attribute and only one tab. So it's a Panorama-specific view for specific tab groups. We might as well change the UI to reflect those internal changes.

There really is a lot of code that could be much cleaner without the special handling and some of that will get at least a bit cleaner with the new storage.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 3 • 13 years ago

Limi, do we want this?

It's listed as "Tabs should not be able to exist without a group" in https://wiki.mozilla.org/Simplify_Panorama_UI.

This would be a great thing to have before we address the global storage because we wouldn't need to re-implement orphan tabs for the new storage.

	Raymond Lee [:raymondlee]
	Comment 4 • 13 years ago

If we remove the "orphan tab" concept, we would also fix bugs like bug 623440 - Orphaned tabs don't show app tab icons: can cause situation where app tabs are completely hidden.

Shall we do that?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 13 years ago

I already took a stab - here's the current progress.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 6 • 13 years ago

bugspam

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 7 • 13 years ago

bugspam

	Raymond Lee [:raymondlee]
	Comment 9 • 13 years ago

Comment on attachment 537352 [details] [diff] [review]
patch v2

Looks good! I've also tried the patch and it works great.  Thanks Tim!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 10 • 13 years ago

Comment on attachment 537352 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de

If anyone wants to try the new behavior, the builds are here:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tim.taubert@gmx.de-bc1e4cd0e616/

	(no longer active)
	Comment 11 • 13 years ago

Comment on attachment 537352 [details] [diff] [review]
patch v2

I'm not sure I know enough about the Panorama code to be able to review this...  Sorry!

	Raymond Lee [:raymondlee]
	Comment 12 • 13 years ago

See bug 628061 comment 21 abut removing the "zero" state of the tabview button image

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 13 • 13 years ago

Comment on attachment 537352 [details] [diff] [review]
patch v2

(In reply to comment #11) 
> I'm not sure I know enough about the Panorama code to be able to review
> this...  Sorry!

Np, that's a really big core patch. That might be a perfect job for Ian, hope he has the time to do it :)

	Ian Gilman [:iangilman]
	Comment 14 • 13 years ago

(In reply to comment #13)
> Np, that's a really big core patch. That might be a perfect job for Ian,
> hope he has the time to do it :)

I'll take a look at it; hopefully tomorrow.

	Ian Gilman [:iangilman]
	Comment 15 • 13 years ago

Comment on attachment 537352 [details] [diff] [review]
patch v2

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

Ok, I've reviewed everything but the tests so far... looking good! Thanks for making sure to clean up the comments and the CSS as you go.

UX definitely needs a chance to play with it, so I've flagged faaborg. Perhaps you can point him to a try build?

Hopefully I'll get to the tests tomorrow.

::: browser/base/content/tabview/items.js
@@ +165,5 @@
>        },
>        stop: function() {
> +        if (!this.isAGroupItem && !this.parent) {
> +          let groupItem = new GroupItem([drag.info.$el], {immediately: true});
> +          groupItem.pushAway();

new GroupItem() already calls pushAway (down at the bottom of that routine); you shouldn't need to do it here.

::: browser/base/content/tabview/tabitems.js
@@ +291,4 @@
>  
>        if (tabData.groupID) {
> +        groupItem = GroupItems.groupItem(tabData.groupID);
> +        self.setBounds(tabData.bounds, true);

It's possible we don't need to save the bounds for a tab any more, as tab layout is handled by the parent group. I believe there was some talk at some point about using the saved bounds as an optimization (so we didn't have to recalculate at start-up) but I don't think we got around to that, and at any rate, I think there are issues with that approach.

@@ +295,3 @@
>  
> +        if (Utils.isPoint(tabData.userSize))
> +          self.userSize = new Point(tabData.userSize);

I don't think TabItems need userSize any more, since the user can't resize them manually.

@@ +301,5 @@
> +        if (Utils.isPoint(tabData.userSize))
> +          groupItem.userSize = new Point(tabData.userSize);
> +      }
> +
> +      if (groupItem) {

groupItem should definitely exist at this point, so no need to check for it, right?

::: browser/base/content/tabview/ui.js
@@ +205,3 @@
>  
> +              let opts = {immediately: true, bounds: box};
> +              let groupItem = new GroupItem([], opts);

How does the tab get created? All I see is a groupItem getting created.

@@ +205,4 @@
>  
> +              let opts = {immediately: true, bounds: box};
> +              let groupItem = new GroupItem([], opts);
> +              groupItem.pushAway(true);

Do we need this pushAway or will new GroupItem take are of it?

@@ +421,5 @@
>    setActive: function UI_setActive(item, options) {
> +    Utils.assert(item, "item must be given");
> +
> +    if (item.isATabItem) {
> +      if (item.parent)

Will it be possible for an item not to have a parent?

::: browser/base/content/test/tabview/head.js
@@ +294,5 @@
>    let win = window.openDialog(getBrowserURL(), "_blank", opts);
>  
>    whenWindowLoaded(win, function () {
> +    let ss = Cc["@mozilla.org/browser/sessionstore;1"]
> +             .getService(Ci.nsISessionStore);

Good catch!

	Ian Gilman [:iangilman]
	Comment 16 • 13 years ago

One comment from a UX standpoint: maybe when you drag a tab out of a group, the new resulting group should be a bit bigger, so the tab doesn't appear to shrink so much.

	Ian Gilman [:iangilman]
	Comment 17 • 13 years ago

Comment on attachment 537352 [details] [diff] [review]
patch v2

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

Ok, now I've reviewed the tests. Definitely looks like you're on the right track, but I'll hold r+ until you've addressed my comments and UX has had a chance to chime in.

::: browser/base/content/test/tabview/browser_tabview_bug597399.js
@@ +40,5 @@
>      "tabviewsearchenabled", onSearchEnabled, false);
>    contentWindow.addEventListener(
>      "tabviewsearchdisabled", onSearchDisabled, false);
>  
>    onSearchDisabled();

Doesn't seem to be anything wrong with the clean up on this test, but it also doesn't seem to be necessary for this patch, which is already very long. 

Don't bother taking these changes out, but next time, please keep the patch focused.

::: browser/base/content/test/tabview/browser_tabview_bug654721.js
@@ +9,5 @@
> +      extData: {"tabview-tab": '{"url":"about:home","groupID":1,"bounds":{"left":20,"top":20,"width":20,"height":20}}'}
> +    },{
> +      entries: [{ url: "about:home" }],
> +      hidden: false,
> +      extData: {"tabview-tab": '{"url":"about:home","groupID":0,"bounds":{"left":300,"top":300,"width":200,"height":200}}'},

groupID 0? Is that a mistake, or testing to see what happens with people's existing orphan tabs? If the latter (which is a good idea), please comment as such.

@@ +49,5 @@
> +      EventUtils.synthesizeMouse(target, 10, 10, {type: 'mousedown'}, cw);
> +      EventUtils.synthesizeMouse(target, 20, -200, {type: 'mousemove'}, cw);
> +      EventUtils.synthesizeMouse(target, 10, 10, {type: 'mouseup'}, cw);
> +
> +      is(groupItems.length, 3, "two groupItems");

"two" should be "three"

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 18 • 13 years ago

(In reply to comment #17)
> Doesn't seem to be anything wrong with the clean up on this test, but it
> also doesn't seem to be necessary for this patch, which is already very
> long. 
> 
> Don't bother taking these changes out, but next time, please keep the patch
> focused.

These are changes I forgot to revert after some searching for the failing test. I took them out.

> groupID 0? Is that a mistake, or testing to see what happens with people's
> existing orphan tabs? If the latter (which is a good idea), please comment
> as such.

It's the latter, comment added.

> "two" should be "three"

Fixed.

I'll push it to the ux-branch and flag it then for ui-review again.

	Ian Gilman [:iangilman]
	Comment 20 • 13 years ago

Comment on attachment 542436 [details] [diff] [review]
patch v4

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

Looks good. I'm giving it an r+, but please don't land it on trunk until ux has had a chance to check it out.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 21 • 13 years ago

(In reply to comment #20)
> Looks good. I'm giving it an r+, but please don't land it on trunk until ux
> has had a chance to check it out.

Promise! :)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 22 • 13 years ago

Comment on attachment 542436 [details] [diff] [review]
patch v4

Pushed to ux-branch.

	Alex Limi (:limi) — Firefox UX Team
	Comment 23 • 13 years ago

Comment on attachment 542436 [details] [diff] [review]
patch v4

Works great!

There's one case that doesn't seem to be handled, though:

1. Have an existing group with a couple of tabs in it
2. Drag one of those to a new group, a new group gets created, name gets focused (all this is exactly as it should be!)
3. Avoid naming the group, and drag the tab to the previous group again, you are now left with an empty, unnamed group.

I think the rule should be that if you empty a group, and it has no name assigned to it, that group should go away.

I remember that we removed the "flush empty group on enter/exit of Panorama" — the difference here is that we're doing an explicit action to empty out the group ("cleaning up"), whereas if I created an empty group and just left it there, it should probably stay, since I'm planning to put something in it. I realize this difference is very slight!

(and I wouldn't hold the patch for this, just something to keep in mind if it's an easy fix)

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 24 • 13 years ago

(In reply to comment #23)
> There's one case that doesn't seem to be handled, though:
> 
> 1. Have an existing group with a couple of tabs in it
> 2. Drag one of those to a new group, a new group gets created, name gets
> focused (all this is exactly as it should be!)
> 3. Avoid naming the group, and drag the tab to the previous group again, you
> are now left with an empty, unnamed group.
> 
> I think the rule should be that if you empty a group, and it has no name
> assigned to it, that group should go away.
> 
> I remember that we removed the "flush empty group on enter/exit of Panorama"
> — the difference here is that we're doing an explicit action to empty out
> the group ("cleaning up"), whereas if I created an empty group and just left
> it there, it should probably stay, since I'm planning to put something in
> it. I realize this difference is very slight!

Will be fixed by 663421.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 25 • 13 years ago

Unrotted the patch.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 26 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/95a023903ae0

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 27 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/95a023903ae0

	George Carstoiu
	Comment 28 • 13 years ago

Mozilla/5.0 (X11; Linux i686; rv:7.0a2) Gecko/20110706 Firefox/7.0a2

Verified issue on Ubuntu 11.04 x86, WinXP, Win7 x86 and Mac OS X 10.6 using the following steps:
 1. Start Firefox and open several websites
 2. Enter Panorama
 3. Drag one of the tabs into the background

A new group is created containing the tab -> orphan tab concept no longer present. Setting status to Verified Fixed.